Implement the SpiBus trait from embedded-hal 1.0. #503

ghismary · 2024-01-30T18:05:55Z

Here is the implementation of the embedded-hal 1.0 traits for SPI.
I need to do further testing, but from what I have seen up to now, it seems to work quite well.

Rahix

Hi, thanks a lot for picking up this work! I have two preliminary comments, see below.

avr-hal-generic/src/spi.rs

ghismary · 2024-01-30T20:59:21Z

Hi, thanks for your comments.
I had not seen this stuff about hardware CS management, and had not taken a look to embedded-hal-bus yet.
It makes sense, I'll rework on that.

ghismary · 2024-01-31T12:14:44Z

I made the changes for the problems reported in the previous comments.
I also adapted an arduino example to the usage of this SpiBus trait implementation via the ExclusiveDevice from embedded-hal-bus.

Rahix

Took a closer look at the changes now. Please check my comments below :)

Rahix · 2024-02-11T09:35:04Z

avr-hal-generic/src/spi.rs

+            /// Read n bytes from the data register, n being to the size of
+            /// the given buffer.
+            fn raw_read_buf(&self, buf: &mut [u8]) {
+                buf.iter_mut().for_each(|b| *b = self.spdr.read().bits())


This doesn't look right to me: You can't just read from a SPI bus without somehow triggering a transfer. You'll also have to wait for the transfer to have completed somewhere.

The documentation from embedded-hal says this:

The word value sent on MOSI during reading is implementation-defined, typically 0x00, 0xFF, or configurable.

So something like this is needed:

fn raw_read_buf(&self, buf: &mut [u8]) { for b in buf.iter_mut() { // We send 0x00 on MOSI during "pure" reading self.spdr.write(|w| unsafe { w.bits(0x00) }); while self.spsr.read().spif().bit_is_clear() {} *b = self.spdr.read().bits(); } }

Rahix · 2024-02-11T09:36:32Z

avr-hal-generic/src/spi.rs

+                buf.iter().for_each(|b| {
+                    self.spdr.write(|w| unsafe { w.bits(*b) });
+                    while self.spsr.read().spif().bit_is_clear() {}
+                })


I prefer a plain for loop over .for_each() when the .for_each isn't strictly required for some reason:

Suggested change

buf.iter().for_each(|b| {

self.spdr.write(|w| unsafe { w.bits(*b) });

while self.spsr.read().spif().bit_is_clear() {}

})

for b in buf.iter() {

self.spdr.write(|w| unsafe { w.bits(*b) });

while self.spsr.read().spif().bit_is_clear() {}

}

Rahix · 2024-02-11T09:41:48Z

avr-hal-generic/src/spi.rs

+        let write = unsafe {
+            let p = buffer.as_ptr();
+            core::slice::from_raw_parts(p, buffer.len())
+        };
+        self.p.raw_read_write_buf(buffer, write);


To my knowledge, this is illegal: You are constructing an immutable reference to the same data that is referenced mutably by buffer. This violates aliasing rules.

I think we have to provide a separate low-level raw_read_write_in_place() here - there is no way to reuse raw_read_write_buf().

Rahix · 2024-02-11T09:47:33Z

avr-hal-generic/src/spi.rs

+        match read.len().cmp(&write.len()) {
+            Ordering::Less => {
+                let (write1, write2) = write.split_at(read.len());
+                self.p.raw_read_write_buf(read, write1);
+                self.p.raw_write_buf(write2);
+            }
+            Ordering::Equal => self.p.raw_read_write_buf(read, write),
+            Ordering::Greater => {
+                let (read1, read2) = read.split_at_mut(write.len());
+                self.p.raw_read_write_buf(read1, write);
+                self.p.raw_read_buf(read2);
+            }
+        }


Just a thought: The following implementation would be equivalent. But I am not sure which one is more elegant. I think yours might still be more explicit about the intent and thus easier to understand. But I'll let you decide which one to use.

let common_length = read.len().min(write.len()); let (write1, write2) = write.split_at(common_length); let (read1, read2) = read.split_at(common_length); self.p.raw_read_write_buf(read1, write1); debug_assert!(write2.len() == 0 || read2.len() == 0); self.p.raw_write_buf(write2); self.p.raw_read_buf(read2);

Rahix · 2024-02-11T10:18:43Z

avr-hal-generic/src/spi.rs

+    }
+
+    fn flush(&mut self) -> Result<(), Self::Error> {
+        if self.write_in_progress {


Just to make sure I understand this correctly:

You are only making use of the write_in_progress flag here to ensure correct behavior when both e-h0.2::FullDuplex and e-h1.0::SpiBus are used at the same time, right?

All the other methods of e-h1.0::SpiBus synchronize with the end of the transfer internally and thus they don't need to set write_in_progress. Is that correct?

And related: What if write_in_progress is set when calling e.g. SpiBus::write()? Shouldn't SpiBus::write() call <Self as SpiBus>::flush() before operating on the bus to ensure any FullDuplex transfer from before has actually finished?

Rahix · 2024-02-11T10:21:24Z

avr-hal-generic/src/spi.rs

+                let mut buffer_it = buffer.iter_mut();
+                let mut bytes_it = bytes.iter();
+                while let Some(byte) = bytes_it.next() {
+                    self.raw_write(*byte);
+                    while self.spsr.read().spif().bit_is_clear() {}
+                    let mut data = buffer_it.next().unwrap();
+                    *data = self.raw_read();
+                }


Another option would be to zip the two iterators:

for (byte, data) in bytes.iter().zip(buffer.iter_mut()) { self.raw_write(*byte); while self.spsr.read().spif().bit_is_clear() {} *data = self.raw_read(); }

Rahix · 2024-02-11T10:28:39Z

mcu/atmega-hal/src/spi.rs

-
-#[cfg(any(feature = "atmega8"))]
-avr_hal_generic::impl_spi! {
-    hal: crate::Atmega,
-    peripheral: crate::pac::SPI,
-    sclk: port::PB5,
-    mosi: port::PB3,
-    miso: port::PB4,
-    cs: port::PB2,
-}


Nice catch! I took the liberty to merge this change already in commit b2be3f9 ("atmega-hal: Drop redundant atmega8 macro invocation"). Please rebase your PR on latest main when you send the next iteration :)

Rahix · 2024-02-11T10:34:41Z

avr-hal-generic/src/spi.rs

+/// This can be used both with the embedded-hal 0.2 spi::FullDuplex trait, and
+/// with the embedded-hal 1.0 spi::SpiBus trait.


You can link the traits like this:

Suggested change

/// This can be used both with the embedded-hal 0.2 spi::FullDuplex trait, and

/// with the embedded-hal 1.0 spi::SpiBus trait.

/// This can be used both with the embedded-hal 0.2 [`spi::FullDuplex`] trait, and

/// with the embedded-hal 1.0 [`spi::SpiBus`] trait.

///

/// [`spi::FullDuplex`]: `embedded_hal_v0::spi::FullDuplex`

/// [`spi::SpiBus`]: `embedded_hal::spi::SpiBus`

Rahix · 2024-03-10T12:46:31Z

@ghismary, thanks a lot for your work on this. @rubend056 picked up your changeset and incorporated my review comments over in #517. We'll continue work there - but I'll make sure you will keep appropriate credit for your work in the merged changes.

Rahix reviewed Jan 30, 2024

View reviewed changes

avr-hal-generic/src/spi.rs Outdated Show resolved Hide resolved

avr-hal-generic/src/spi.rs Outdated Show resolved Hide resolved

ghismary force-pushed the feature/implement-embedded-hal-1.0-spi-traits branch from 208d4a3 to d061a7c Compare January 30, 2024 20:50

Implement the SpiBus trait from embedded-hal 1.0.

0461fa4

ghismary force-pushed the feature/implement-embedded-hal-1.0-spi-traits branch from d061a7c to 0461fa4 Compare January 31, 2024 12:08

ghismary changed the title ~~Implement SPI traits from embedded-hal 1.0.~~ Implement the SpiBus trait from embedded-hal 1.0. Jan 31, 2024

Rahix requested changes Feb 11, 2024

View reviewed changes

rubend056 mentioned this pull request Mar 8, 2024

Upgrading spi to embedded_hal 1.0 #517

Merged

Rahix closed this Mar 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the SpiBus trait from embedded-hal 1.0. #503

Implement the SpiBus trait from embedded-hal 1.0. #503

ghismary commented Jan 30, 2024

Rahix left a comment

ghismary commented Jan 30, 2024

ghismary commented Jan 31, 2024

Rahix left a comment

Rahix Feb 11, 2024

Rahix Feb 11, 2024

Rahix Feb 11, 2024

Rahix Feb 11, 2024

Rahix Feb 11, 2024

Rahix Feb 11, 2024

Rahix Feb 11, 2024

Rahix Feb 11, 2024

Rahix commented Mar 10, 2024

		/// This can be used both with the embedded-hal 0.2 spi::FullDuplex trait, and
		/// with the embedded-hal 1.0 spi::SpiBus trait.

-/// This can be used both with the embedded-hal 0.2 spi::FullDuplex trait, and
-/// with the embedded-hal 1.0 spi::SpiBus trait.
+/// This can be used both with the embedded-hal 0.2 [`spi::FullDuplex`] trait, and
+/// with the embedded-hal 1.0 [`spi::SpiBus`] trait.
+///
+/// [`spi::FullDuplex`]: `embedded_hal_v0::spi::FullDuplex`
+/// [`spi::SpiBus`]: `embedded_hal::spi::SpiBus`

Implement the SpiBus trait from embedded-hal 1.0. #503

Implement the SpiBus trait from embedded-hal 1.0. #503

Conversation

ghismary commented Jan 30, 2024

Rahix left a comment

Choose a reason for hiding this comment

ghismary commented Jan 30, 2024

ghismary commented Jan 31, 2024

Rahix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rahix commented Mar 10, 2024