Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix LPSPI word unpacking in receive operations #176

Merged
merged 1 commit into from
Nov 25, 2024
+127 −39
Merged

Fix LPSPI word unpacking in receive operations #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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