From 16fc26c2a64e3ffbf9cde659f39ac276401caa81 Mon Sep 17 00:00:00 2001 From: Jan Niehusmann Date: Thu, 15 Aug 2024 19:45:55 +0000 Subject: [PATCH 1/4] Remove code setting IC_DATA_CMD.RESTART bit According to the datasheet, a restart is automatically triggered "if the transfer direction is changing from the previous command". The RESTART bit is only needed to trigger an unconditional RESTART sequence. According to the contract of I2c::transaction, "Data from adjacent operations of the same type are sent after each other without an SP or SR" and "Between adjacent operations of a different type an SR and SAD+R/W is sent". This looks like it is exactly what the hardware provides out of the box. (See: https://docs.rs/embedded-hal/1.0.0/ebedded_hal/i2c/trait.I2c.html#tymethod.transaction) --- rp2040-hal/src/i2c/controller.rs | 15 --------------- rp2040-hal/src/i2c/controller/non_blocking.rs | 15 --------------- 2 files changed, 30 deletions(-) diff --git a/rp2040-hal/src/i2c/controller.rs b/rp2040-hal/src/i2c/controller.rs index 92653f686..7b3590425 100644 --- a/rp2040-hal/src/i2c/controller.rs +++ b/rp2040-hal/src/i2c/controller.rs @@ -227,7 +227,6 @@ impl, PINS> I2C { )?; let lastindex = buffer.len() - 1; - let mut first_byte = true; for (i, byte) in buffer.iter_mut().enumerate() { let last_byte = i == lastindex; @@ -235,13 +234,6 @@ impl, PINS> I2C { while self.i2c.ic_status().read().tfnf().bit_is_clear() {} self.i2c.ic_data_cmd().write(|w| { - if first_byte { - if !first_transaction { - w.restart().enable(); - } - first_byte = false; - } - w.stop().bit(do_stop && last_byte); w.cmd().read() }); @@ -270,7 +262,6 @@ impl, PINS> I2C { )?; let mut abort_reason = Ok(()); - let mut first_byte = true; 'outer: while let Some(byte) = peekable.next() { if self.tx_fifo_full() { // wait for more room in the fifo @@ -289,12 +280,6 @@ impl, PINS> I2C { // else enqueue let last = peekable.peek().is_none(); self.i2c.ic_data_cmd().write(|w| { - if first_byte { - if !first_transaction { - w.restart().enable(); - } - first_byte = false; - } w.stop().bit(do_stop && last); unsafe { w.dat().bits(byte) } }); diff --git a/rp2040-hal/src/i2c/controller/non_blocking.rs b/rp2040-hal/src/i2c/controller/non_blocking.rs index c9a7f3a52..ea154f024 100644 --- a/rp2040-hal/src/i2c/controller/non_blocking.rs +++ b/rp2040-hal/src/i2c/controller/non_blocking.rs @@ -120,7 +120,6 @@ where )?; let lastindex = buffer.len() - 1; - let mut first_byte = true; for (i, byte) in buffer.iter_mut().enumerate() { let last_byte = i == lastindex; @@ -135,13 +134,6 @@ where .await; self.i2c.ic_data_cmd().write(|w| { - if first_byte { - if !first_transaction { - w.restart().enable(); - } - first_byte = false; - } - w.stop().bit(do_stop && last_byte); w.cmd().read() }); @@ -174,7 +166,6 @@ where )?; let mut abort_reason = Ok(()); - let mut first_byte = true; while let Some(byte) = peekable.next() { if self.tx_fifo_full() { // wait for more room in the fifo @@ -193,12 +184,6 @@ where // else enqueue let last = peekable.peek().is_none(); self.i2c.ic_data_cmd().write(|w| { - if first_byte { - if !first_transaction { - w.restart().enable(); - } - first_byte = false; - } w.stop().bit(do_stop && last); unsafe { w.dat().bits(byte) } }); From 82f076ea3031e1e2fe8fd33dc70be6b5c9566549 Mon Sep 17 00:00:00 2001 From: Jan Niehusmann Date: Thu, 15 Aug 2024 21:13:21 +0000 Subject: [PATCH 2/4] Increase rtt buffer size for on-target tests Some of the panic messages are longer than the default buffer, causing a lockup. --- on-target-tests/run_tests.bat | 1 + on-target-tests/run_tests.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/on-target-tests/run_tests.bat b/on-target-tests/run_tests.bat index f8fcfa61d..26bb5d5a3 100755 --- a/on-target-tests/run_tests.bat +++ b/on-target-tests/run_tests.bat @@ -2,5 +2,6 @@ @rem We need to specify environment variables here to control build since we aren't able to override them in Cargo.toml @SET "CARGO_TARGET_THUMBV6M_NONE_EABI_RUNNER=probe-rs run" +@SET "DEFMT_RTT_BUFFER_SIZE=4096" cargo test --no-fail-fast -- --chip rp2040 diff --git a/on-target-tests/run_tests.sh b/on-target-tests/run_tests.sh index 326e7992b..416215af6 100755 --- a/on-target-tests/run_tests.sh +++ b/on-target-tests/run_tests.sh @@ -3,4 +3,4 @@ # Keep running tests even if one of them fails # We need to specify probe-rs as our runner via environment variables here # to control build since we aren't able to override them in config.toml -CARGO_TARGET_THUMBV6M_NONE_EABI_RUNNER="probe-rs run" cargo test --no-fail-fast -- --chip rp2040 +DEFMT_RTT_BUFFER_SIZE=4096 CARGO_TARGET_THUMBV6M_NONE_EABI_RUNNER="probe-rs run" cargo test --no-fail-fast -- --chip rp2040 From 5046f14fb2400e7e63fbbc0ed686d4fb2711e8a2 Mon Sep 17 00:00:00 2001 From: Jan Niehusmann Date: Sun, 25 Aug 2024 19:11:39 +0000 Subject: [PATCH 3/4] Fix on-target tests --- on-target-tests/tests/i2c_loopback.rs | 13 ++++++----- on-target-tests/tests/i2c_loopback_async.rs | 4 ++-- on-target-tests/tests/i2c_tests/blocking.rs | 23 +++++++++++-------- on-target-tests/tests/i2c_tests/mod.rs | 5 +++- .../tests/i2c_tests/non_blocking.rs | 8 +++---- 5 files changed, 30 insertions(+), 23 deletions(-) diff --git a/on-target-tests/tests/i2c_loopback.rs b/on-target-tests/tests/i2c_loopback.rs index bad66a8ea..a907843c7 100644 --- a/on-target-tests/tests/i2c_loopback.rs +++ b/on-target-tests/tests/i2c_loopback.rs @@ -59,13 +59,13 @@ mod tests { #[test] fn write_iter_read(state: &mut State) { i2c_tests::blocking::write_iter_read(state, ADDR_7BIT, 1..=1); - i2c_tests::blocking::write_iter_read(state, ADDR_10BIT, 2..=2); + i2c_tests::blocking::write_iter_read(state, ADDR_10BIT, 1..=1); } #[test] fn write_read(state: &mut State) { i2c_tests::blocking::write_read(state, ADDR_7BIT, 1..=1); - i2c_tests::blocking::write_read(state, ADDR_10BIT, 2..=2); + i2c_tests::blocking::write_read(state, ADDR_10BIT, 1..=1); } #[test] @@ -89,25 +89,26 @@ mod tests { #[test] fn transactions_read_write(state: &mut State) { i2c_tests::blocking::transactions_read_write(state, ADDR_7BIT, 1..=1); + // An initial read in 10 bit mode contains an implicit restart condition i2c_tests::blocking::transactions_read_write(state, ADDR_10BIT, 2..=2); } #[test] fn transactions_write_read(state: &mut State) { i2c_tests::blocking::transactions_write_read(state, ADDR_7BIT, 1..=1); - i2c_tests::blocking::transactions_write_read(state, ADDR_10BIT, 2..=2); + i2c_tests::blocking::transactions_write_read(state, ADDR_10BIT, 1..=1); } #[test] fn transaction(state: &mut State) { - i2c_tests::blocking::transaction(state, ADDR_7BIT, 7..=9); - i2c_tests::blocking::transaction(state, ADDR_10BIT, 7..=9); + i2c_tests::blocking::transaction(state, ADDR_7BIT, 5..=5); + i2c_tests::blocking::transaction(state, ADDR_10BIT, 5..=5); } #[test] fn transactions_iter(state: &mut State) { i2c_tests::blocking::transactions_iter(state, ADDR_7BIT, 1..=1); - i2c_tests::blocking::transactions_iter(state, ADDR_10BIT, 2..=2); + i2c_tests::blocking::transactions_iter(state, ADDR_10BIT, 1..=1); } #[test] diff --git a/on-target-tests/tests/i2c_loopback_async.rs b/on-target-tests/tests/i2c_loopback_async.rs index b8d4543e0..e604cf3bf 100644 --- a/on-target-tests/tests/i2c_loopback_async.rs +++ b/on-target-tests/tests/i2c_loopback_async.rs @@ -60,8 +60,8 @@ mod tests { #[test] fn transactions_iter(state: &mut State) { - run_test(non_blocking::transaction(state, ADDR_7BIT, 7..=9)); - run_test(non_blocking::transaction(state, ADDR_10BIT, 7..=14)); + run_test(non_blocking::transaction(state, ADDR_7BIT, 5..=5)); + run_test(non_blocking::transaction(state, ADDR_10BIT, 5..=5)); } #[test] diff --git a/on-target-tests/tests/i2c_tests/blocking.rs b/on-target-tests/tests/i2c_tests/blocking.rs index 13d731c0c..58eda5219 100644 --- a/on-target-tests/tests/i2c_tests/blocking.rs +++ b/on-target-tests/tests/i2c_tests/blocking.rs @@ -330,7 +330,7 @@ pub fn transactions_read_write( restart_count: RangeInclusive, ) { use embedded_hal::i2c::{I2c, Operation}; - let controller = reset(state, addr, true); + let controller = reset(state, addr, false); let samples_seq: FIFOBuffer = Generator::seq().take(25).collect(); let samples_fib: FIFOBuffer = Generator::fib().take(25).collect(); @@ -381,9 +381,9 @@ pub fn transaction( // does not "waste" bytes that would be discarded otherwise. // // One down side of this is that the Target implementation is unable to detect restarts - // between consicutive write operations + // between consecutive write operations use embedded_hal::i2c::{I2c, Operation}; - let controller = reset(state, addr, true); + let controller = reset(state, addr, false); let mut v = ([0u8; 14], [0u8; 25], [0u8; 25], [0u8; 14], [0u8; 14]); let samples: FIFOBuffer = Generator::seq().take(25).collect(); @@ -391,15 +391,15 @@ pub fn transaction( .transaction( addr, &mut [ - Operation::Write(&samples), // goes to v2 + Operation::Write(&samples), Operation::Read(&mut v.0), Operation::Read(&mut v.1), Operation::Read(&mut v.2), - Operation::Write(&samples), // goes to v3 + Operation::Write(&samples), Operation::Read(&mut v.3), - Operation::Write(&samples), // goes to v4 - Operation::Write(&samples), // remains in buffer - Operation::Write(&samples), // remains in buffer + Operation::Write(&samples), + Operation::Write(&samples), + Operation::Write(&samples), Operation::Read(&mut v.4), ], ) @@ -423,7 +423,12 @@ pub fn transaction( assert_vec_eq!(e); // assert reads - let g: FIFOBuffer = Generator::seq().take(92).collect(); + let g: FIFOBuffer = itertools::chain!( + Generator::seq().take(14 + 25 + 25), + Generator::fib().take(14), + Generator::seq().take(14), + ) + .collect(); let h: FIFOBuffer = itertools::chain!( v.0.into_iter(), v.1.into_iter(), diff --git a/on-target-tests/tests/i2c_tests/mod.rs b/on-target-tests/tests/i2c_tests/mod.rs index d186e3a69..40f1973b6 100644 --- a/on-target-tests/tests/i2c_tests/mod.rs +++ b/on-target-tests/tests/i2c_tests/mod.rs @@ -100,7 +100,10 @@ fn target_handler( } = payload; match evt { Event::Start => *first = true, - Event::Restart => *restart_cnt += 1, + Event::Restart => { + *first = true; + *restart_cnt += 1; + } Event::TransferRead => { let n = throttle.then_some(1).unwrap_or(target.tx_fifo_available()); let v: FIFOBuffer = gen.take(n.into()).collect(); diff --git a/on-target-tests/tests/i2c_tests/non_blocking.rs b/on-target-tests/tests/i2c_tests/non_blocking.rs index 43923fd36..033cfabc5 100644 --- a/on-target-tests/tests/i2c_tests/non_blocking.rs +++ b/on-target-tests/tests/i2c_tests/non_blocking.rs @@ -342,11 +342,9 @@ pub async fn transaction( assert_eq!(e, state.payload.borrow().vec); // assert reads let g: FIFOBuffer = itertools::chain!( - Generator::fib().take(25), - Generator::fib().skip(32).take(25), - Generator::fib().skip(64).take(25), - Generator::fib().skip(96).take(14), - Generator::fib().skip(112).take(14), + Generator::fib().take(25 * 3), + Generator::seq().take(14), + Generator::fib().take(14), ) .collect(); let h: FIFOBuffer = itertools::chain!( From e7c6d144d1c70b9e5f1504cebf0140b562409cb0 Mon Sep 17 00:00:00 2001 From: Jan Niehusmann Date: Sun, 25 Aug 2024 21:47:02 +0000 Subject: [PATCH 4/4] Also apply changes to rp235x-hal --- rp235x-hal/src/i2c/controller.rs | 15 --------------- rp235x-hal/src/i2c/controller/non_blocking.rs | 15 --------------- 2 files changed, 30 deletions(-) diff --git a/rp235x-hal/src/i2c/controller.rs b/rp235x-hal/src/i2c/controller.rs index bc244ab4a..c034ae8b1 100644 --- a/rp235x-hal/src/i2c/controller.rs +++ b/rp235x-hal/src/i2c/controller.rs @@ -227,7 +227,6 @@ impl, PINS> I2C { )?; let lastindex = buffer.len() - 1; - let mut first_byte = true; for (i, byte) in buffer.iter_mut().enumerate() { let last_byte = i == lastindex; @@ -235,13 +234,6 @@ impl, PINS> I2C { while self.i2c.ic_status().read().tfnf().bit_is_clear() {} self.i2c.ic_data_cmd().write(|w| { - if first_byte { - if !first_transaction { - w.restart().enable(); - } - first_byte = false; - } - w.stop().bit(do_stop && last_byte); w.cmd().read() }); @@ -270,7 +262,6 @@ impl, PINS> I2C { )?; let mut abort_reason = Ok(()); - let mut first_byte = true; 'outer: while let Some(byte) = peekable.next() { if self.tx_fifo_full() { // wait for more room in the fifo @@ -289,12 +280,6 @@ impl, PINS> I2C { // else enqueue let last = peekable.peek().is_none(); self.i2c.ic_data_cmd().write(|w| { - if first_byte { - if !first_transaction { - w.restart().enable(); - } - first_byte = false; - } w.stop().bit(do_stop && last); unsafe { w.dat().bits(byte) } }); diff --git a/rp235x-hal/src/i2c/controller/non_blocking.rs b/rp235x-hal/src/i2c/controller/non_blocking.rs index b3bd6933f..7e501fe5e 100644 --- a/rp235x-hal/src/i2c/controller/non_blocking.rs +++ b/rp235x-hal/src/i2c/controller/non_blocking.rs @@ -120,7 +120,6 @@ where )?; let lastindex = buffer.len() - 1; - let mut first_byte = true; for (i, byte) in buffer.iter_mut().enumerate() { let last_byte = i == lastindex; @@ -135,13 +134,6 @@ where .await; self.i2c.ic_data_cmd().write(|w| { - if first_byte { - if !first_transaction { - w.restart().enable(); - } - first_byte = false; - } - w.stop().bit(do_stop && last_byte); w.cmd().read() }); @@ -174,7 +166,6 @@ where )?; let mut abort_reason = Ok(()); - let mut first_byte = true; while let Some(byte) = peekable.next() { if self.tx_fifo_full() { // wait for more room in the fifo @@ -193,12 +184,6 @@ where // else enqueue let last = peekable.peek().is_none(); self.i2c.ic_data_cmd().write(|w| { - if first_byte { - if !first_transaction { - w.restart().enable(); - } - first_byte = false; - } w.stop().bit(do_stop && last); unsafe { w.dat().bits(byte) } });