Skip to content

Commit

Permalink
Fix LPSPI word unpacking in receive operations
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@mciantyre
mciantyre committed Nov 18, 2024
1 parent 9ec1e55 commit 56f91a1
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 39 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
57 changes: 44 additions & 13 deletions examples/rtic_spi_blocking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
}

Expand Down
104 changes: 78 additions & 26 deletions src/common/lpspi.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ impl<P, const N: u8> Lpspi<P, N> {
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(())
}
Expand Down Expand Up @@ -1256,8 +1256,10 @@ trait Word: Copy + Into<u32> + TryFrom<u32> {
fn pack_word(bit_order: BitOrder, provider: impl FnMut() -> Option<Self>) -> 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 {
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -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);
}
}
}
Expand All @@ -1324,7 +1338,7 @@ impl Word for u32 {
fn pack_word(_: BitOrder, mut provider: impl FnMut() -> Option<Self>) -> 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)
}
}
Expand All @@ -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.
Expand Down Expand Up @@ -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<W> 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));
}
}

Expand Down Expand Up @@ -1645,18 +1667,32 @@ 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
//

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]
Expand All @@ -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]);

//
Expand All @@ -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]);
}

Expand Down

0 comments on commit 56f91a1

Please sign in to comment.