From dee9e1b33e4a32118334bafc94445f5ed549647c Mon Sep 17 00:00:00 2001 From: Ian McIntyre Date: Mon, 18 Nov 2024 06:38:20 -0500 Subject: [PATCH] Fix LPSPI word unpacking in receive operations LPSPI receive operations are reversing byte and half-word elements when they place data in the caller's buffer. We can show this using the revised loopback tests on an older LPSPI driver. The unpacking routines need to consider the frame boundary and the bit order when they deconstruct the 32 bit LPSPI word. --- CHANGELOG.md | 5 ++ examples/rtic_spi_blocking.rs | 57 ++++++++++++++----- src/common/lpspi.rs | 104 +++++++++++++++++++++++++--------- 3 files changed, 127 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21307d9f..fff40810 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## [Unreleased] +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 9b787c90..3629616a 100644 --- a/src/common/lpspi.rs +++ b/src/common/lpspi.rs @@ -720,7 +720,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(()) } @@ -1289,8 +1289,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 { @@ -1316,9 +1318,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); } } } @@ -1346,9 +1353,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); } } } @@ -1357,7 +1371,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) } } @@ -1373,7 +1387,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. @@ -1471,14 +1485,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)); } } @@ -1674,7 +1696,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 @@ -1682,10 +1707,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] @@ -1697,10 +1733,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]); // @@ -1709,10 +1753,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]); }