diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ffc1fa8..271568e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ **BREAKING** `LpspiError::{Busy, NoData}` are removed as possible LPSPI errors. +Correct LPSPI receive operations. Previously, `u8` and `u16` elements received +by the driver were returned out of order to the user. This release fixes the +ordering. If you were correcting this behavior in application code, you'll need +to remove that behavior before adopting this fix. + ## [0.5.8] 2024-11-01 Fix LPUART baud rate computation, ensuring updates to SBR when evaluating other diff --git a/examples/rtic_spi_blocking.rs b/examples/rtic_spi_blocking.rs index 85fffb23..81b81c2f 100644 --- a/examples/rtic_spi_blocking.rs +++ b/examples/rtic_spi_blocking.rs @@ -71,13 +71,13 @@ mod app { spi.write(&U32_WORDS).unwrap(); } - const U8_WORDS: [u8; 7] = [0xDEu8, 0xAD, 0xBE, 0xEF, 0xA5, 0x00, 0x1D]; + const U8_WORDS: [u8; 7] = [0xDEu8, 0xAD, 0xBE, 0xEF, 0x12, 0x34, 0x56]; for bit_order in BIT_ORDERS { spi.set_bit_order(bit_order); spi.write(&U8_WORDS).unwrap(); } - const U16_WORDS: [u16; 3] = [0xDEADu16, 0xBEEF, 0xA5A5]; + const U16_WORDS: [u16; 3] = [0xDEADu16, 0xBEEF, 0x1234]; for bit_order in BIT_ORDERS { spi.set_bit_order(bit_order); spi.write(&U16_WORDS).unwrap(); @@ -91,22 +91,53 @@ mod app { spi.set_bit_order(hal::lpspi::BitOrder::Msb); // Make sure concatenated elements look correct on the wire. + // Make sure we can read those elements. { - use eh02::blocking::spi::Write; + use eh02::blocking::spi::Transfer; + use hal::lpspi::BitOrder; + + macro_rules! transfer_test { + ($arr:expr, $bit_order:expr) => { + let bit_order_name = match $bit_order { + BitOrder::Msb => "MSB", + BitOrder::Lsb => "LSB", + }; + + spi.set_bit_order($bit_order); + let mut buffer = $arr; + spi.transfer(&mut buffer).unwrap(); + defmt::assert_eq!(buffer, $arr, "Bit Order {}", bit_order_name); + }; + } + + transfer_test!([1u8, 2, 3], BitOrder::Msb); + transfer_test!([1u8, 2, 3], BitOrder::Lsb); + + transfer_test!([1u8, 2, 3, 4], BitOrder::Msb); + transfer_test!([1u8, 2, 3, 4], BitOrder::Lsb); + + transfer_test!([1u8, 2, 3, 4, 5], BitOrder::Msb); + transfer_test!([1u8, 2, 3, 4, 5], BitOrder::Lsb); + + transfer_test!([1u8, 2, 3, 4, 5, 6], BitOrder::Msb); + transfer_test!([1u8, 2, 3, 4, 5, 6], BitOrder::Lsb); + + transfer_test!([1u8, 2, 3, 4, 5, 6, 7], BitOrder::Msb); + transfer_test!([1u8, 2, 3, 4, 5, 6, 7], BitOrder::Lsb); + + transfer_test!([0x0102u16, 0x0304, 0x0506], BitOrder::Msb); + transfer_test!([0x0102u16, 0x0304, 0x0506], BitOrder::Lsb); - spi.write(&[1u8, 2, 3]).unwrap(); - spi.write(&[1u8, 2, 3, 4]).unwrap(); - spi.write(&[1u8, 2, 3, 4, 5]).unwrap(); - spi.write(&[1u8, 2, 3, 4, 5, 6]).unwrap(); - spi.write(&[1u8, 2, 3, 4, 5, 6, 7]).unwrap(); + transfer_test!([0x0102u16, 0x0304, 0x0506, 0x0708], BitOrder::Msb); + transfer_test!([0x0102u16, 0x0304, 0x0506, 0x0708], BitOrder::Lsb); - spi.write(&[0x0102u16, 0x0304, 0x0506]).unwrap(); - spi.write(&[0x0102u16, 0x0304, 0x0506, 0x0708]).unwrap(); - spi.write(&[0x0102u16, 0x0304, 0x0506, 0x0708, 0x090A]) - .unwrap(); + transfer_test!([0x0102u16, 0x0304, 0x0506, 0x0708, 0x090A], BitOrder::Msb); + transfer_test!([0x0102u16, 0x0304, 0x0506, 0x0708, 0x090A], BitOrder::Lsb); - spi.write(&[0x01020304u32, 0x05060708, 0x090A0B0C]).unwrap(); + transfer_test!([0x01020304u32, 0x05060708, 0x090A0B0C], BitOrder::Msb); + transfer_test!([0x01020304u32, 0x05060708, 0x090A0B0C], BitOrder::Lsb); + spi.set_bit_order(BitOrder::Msb); delay(); } diff --git a/src/common/lpspi.rs b/src/common/lpspi.rs index 7b25d78a..8342bf83 100644 --- a/src/common/lpspi.rs +++ b/src/common/lpspi.rs @@ -716,7 +716,7 @@ impl Lpspi { async fn spin_receive(&self, mut data: impl ReceiveData, len: usize) -> Result<(), LpspiError> { for _ in 0..len { let word = self.spin_for_word().await?; - data.next_word(word); + data.next_word(self.bit_order, word); } Ok(()) } @@ -1256,8 +1256,10 @@ trait Word: Copy + Into + TryFrom { fn pack_word(bit_order: BitOrder, provider: impl FnMut() -> Option) -> u32; /// Given a word, deconstruct the word and call the - /// `sink` with those components. - fn unpack_word(word: u32, sink: impl FnMut(Self)); + /// `sink` with those components. `valid_bytes` conveys + /// how many bytes in `word` are valid. It's never more + /// than four, and it's never zero. + fn unpack_word(word: u32, bit_order: BitOrder, valid_bytes: usize, sink: impl FnMut(Self)); } impl Word for u8 { @@ -1283,9 +1285,14 @@ impl Word for u8 { word } - fn unpack_word(word: u32, mut sink: impl FnMut(Self)) { - for offset in [0, 8, 16, 24] { - sink((word >> offset) as u8); + fn unpack_word(word: u32, bit_order: BitOrder, valid_bytes: usize, mut sink: impl FnMut(Self)) { + let mut offsets = [0usize, 8, 16, 24]; + let valid = &mut offsets[..valid_bytes]; + if matches!(bit_order, BitOrder::Msb) { + valid.reverse(); + } + for offset in valid { + sink((word >> *offset) as u8); } } } @@ -1313,9 +1320,16 @@ impl Word for u16 { word } - fn unpack_word(word: u32, mut sink: impl FnMut(Self)) { - for offset in [0, 16] { - sink((word >> offset) as u16); + fn unpack_word(word: u32, bit_order: BitOrder, valid_bytes: usize, mut sink: impl FnMut(Self)) { + let mut offsets = [0usize, 16]; + let valid = &mut offsets[..valid_bytes / 2]; + + if matches!(bit_order, BitOrder::Msb) { + valid.reverse(); + } + + for offset in valid { + sink((word >> *offset) as u16); } } } @@ -1324,7 +1338,7 @@ impl Word for u32 { fn pack_word(_: BitOrder, mut provider: impl FnMut() -> Option) -> u32 { provider().unwrap_or(0) } - fn unpack_word(word: u32, mut sink: impl FnMut(Self)) { + fn unpack_word(word: u32, _: BitOrder, _: usize, mut sink: impl FnMut(Self)) { sink(word) } } @@ -1340,7 +1354,7 @@ trait TransmitData { /// Generalizes how we save LPSPI data into memory. trait ReceiveData { /// Invoked each time we read data from the queue. - fn next_word(&mut self, word: u32); + fn next_word(&mut self, bit_order: BitOrder, word: u32); } /// Transmit data from a buffer. @@ -1442,14 +1456,22 @@ where } } } + + fn array_len(&self) -> usize { + // Safety: end and ptr derive from the same allocation. + // We always update ptr in multiples of it's object type. + // The end pointer is always at a higher address in memory. + unsafe { self.end.byte_offset_from(self.ptr) as _ } + } } impl ReceiveData for ReceiveBuffer<'_, W> where W: Word, { - fn next_word(&mut self, word: u32) { - W::unpack_word(word, |elem| self.next_write(elem)); + fn next_word(&mut self, bit_order: BitOrder, word: u32) { + let valid_bytes = self.array_len().min(size_of_val(&word)); + W::unpack_word(word, bit_order, valid_bytes, |elem| self.next_write(elem)); } } @@ -1645,7 +1667,10 @@ mod tests { #[test] fn receive_buffer() { - use super::{ReceiveBuffer, ReceiveData}; + // See notes in transmit_buffer test to understand MSB and LSB + // transformations. + + use super::{BitOrder::*, ReceiveBuffer, ReceiveData}; // // u8 @@ -1653,10 +1678,21 @@ mod tests { let mut buffer = [0u8; 9]; let mut rx = ReceiveBuffer::new(&mut buffer); - rx.next_word(0xDEADBEEF); - rx.next_word(0xAD1CAC1D); - rx.next_word(0x04030201); - rx.next_word(0x55555555); + rx.next_word(Msb, 0xDEADBEEF); + rx.next_word(Msb, 0xAD1CAC1D); + rx.next_word(Msb, 0x04030201); + rx.next_word(Msb, 0x55555555); + assert_eq!( + buffer, + [0xDE, 0xAD, 0xBE, 0xEF, 0xAD, 0x1C, 0xAC, 0x1D, 0x01] + ); + + let mut buffer = [0u8; 9]; + let mut rx = ReceiveBuffer::new(&mut buffer); + rx.next_word(Lsb, 0xDEADBEEF); + rx.next_word(Lsb, 0xAD1CAC1D); + rx.next_word(Lsb, 0x04030201); + rx.next_word(Lsb, 0x55555555); assert_eq!( buffer, [0xEF, 0xBE, 0xAD, 0xDE, 0x1D, 0xAC, 0x1C, 0xAD, 0x01] @@ -1668,10 +1704,18 @@ mod tests { let mut buffer = [0u16; 5]; let mut rx = ReceiveBuffer::new(&mut buffer); - rx.next_word(0xDEADBEEF); - rx.next_word(0xAD1CAC1D); - rx.next_word(0x04030201); - rx.next_word(0x55555555); + rx.next_word(Msb, 0xDEADBEEF); + rx.next_word(Msb, 0xAD1CAC1D); + rx.next_word(Msb, 0x04030201); + rx.next_word(Msb, 0x55555555); + assert_eq!(buffer, [0xDEAD, 0xBEEF, 0xAD1C, 0xAC1D, 0x0201]); + + let mut buffer = [0u16; 5]; + let mut rx = ReceiveBuffer::new(&mut buffer); + rx.next_word(Lsb, 0xDEADBEEF); + rx.next_word(Lsb, 0xAD1CAC1D); + rx.next_word(Lsb, 0x04030201); + rx.next_word(Lsb, 0x55555555); assert_eq!(buffer, [0xBEEF, 0xDEAD, 0xAC1D, 0xAD1C, 0x0201]); // @@ -1680,10 +1724,18 @@ mod tests { let mut buffer = [0u32; 3]; let mut rx = ReceiveBuffer::new(&mut buffer); - rx.next_word(0xDEADBEEF); - rx.next_word(0xAD1CAC1D); - rx.next_word(0x77777777); - rx.next_word(0x55555555); + rx.next_word(Msb, 0xDEADBEEF); + rx.next_word(Msb, 0xAD1CAC1D); + rx.next_word(Msb, 0x77777777); + rx.next_word(Msb, 0x55555555); + assert_eq!(buffer, [0xDEADBEEF, 0xAD1CAC1D, 0x77777777]); + + let mut buffer = [0u32; 3]; + let mut rx = ReceiveBuffer::new(&mut buffer); + rx.next_word(Lsb, 0xDEADBEEF); + rx.next_word(Lsb, 0xAD1CAC1D); + rx.next_word(Lsb, 0x77777777); + rx.next_word(Lsb, 0x55555555); assert_eq!(buffer, [0xDEADBEEF, 0xAD1CAC1D, 0x77777777]); }