Skip to content

Commit 6c2997f

Browse files
jannicjannic
authored
Remove code setting IC_DATA_CMD.RESTART bit (#827)
* 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) * Increase rtt buffer size for on-target tests Some of the panic messages are longer than the default buffer, causing a lockup. * Fix on-target tests * Also apply changes to rp235x-hal
1 parent eea14eb commit 6c2997f

File tree

11 files changed

+32
-84
lines changed

11 files changed

+32
-84
lines changed

‎on-target-tests/run_tests.bat‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,6 @@
22
@rem We need to specify environment variables here to control build since we aren't able to override them in Cargo.toml
33

44
@SET "CARGO_TARGET_THUMBV6M_NONE_EABI_RUNNER=probe-rs run"
5+
@SET "DEFMT_RTT_BUFFER_SIZE=4096"
56

67
cargo test --no-fail-fast --features rp2040 -- --chip rp2040

‎on-target-tests/run_tests.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@
33
# Keep running tests even if one of them fails
44
# We need to specify probe-rs as our runner via environment variables here
55
# to control build since we aren't able to override them in config.toml
6-
CARGO_TARGET_THUMBV6M_NONE_EABI_RUNNER="probe-rs run" cargo test --no-fail-fast --features rp2040 -- --chip rp2040
6+
DEFMT_RTT_BUFFER_SIZE=4096 CARGO_TARGET_THUMBV6M_NONE_EABI_RUNNER="probe-rs run" cargo test --no-fail-fast --features rp2040 -- --chip rp2040

‎on-target-tests/tests/i2c_loopback.rs‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,13 +71,13 @@ mod tests {
7171
#[test]
7272
fn write_iter_read(state: &mut State) {
7373
i2c_tests::blocking::write_iter_read(state, ADDR_7BIT, 1..=1);
74-
i2c_tests::blocking::write_iter_read(state, ADDR_10BIT, 2..=2);
74+
i2c_tests::blocking::write_iter_read(state, ADDR_10BIT, 1..=1);
7575
}
7676

7777
#[test]
7878
fn write_read(state: &mut State) {
7979
i2c_tests::blocking::write_read(state, ADDR_7BIT, 1..=1);
80-
i2c_tests::blocking::write_read(state, ADDR_10BIT, 2..=2);
80+
i2c_tests::blocking::write_read(state, ADDR_10BIT, 1..=1);
8181
}
8282

8383
#[test]
@@ -101,25 +101,26 @@ mod tests {
101101
#[test]
102102
fn transactions_read_write(state: &mut State) {
103103
i2c_tests::blocking::transactions_read_write(state, ADDR_7BIT, 1..=1);
104+
// An initial read in 10 bit mode contains an implicit restart condition
104105
i2c_tests::blocking::transactions_read_write(state, ADDR_10BIT, 2..=2);
105106
}
106107

107108
#[test]
108109
fn transactions_write_read(state: &mut State) {
109110
i2c_tests::blocking::transactions_write_read(state, ADDR_7BIT, 1..=1);
110-
i2c_tests::blocking::transactions_write_read(state, ADDR_10BIT, 2..=2);
111+
i2c_tests::blocking::transactions_write_read(state, ADDR_10BIT, 1..=1);
111112
}
112113

113114
#[test]
114115
fn transaction(state: &mut State) {
115-
i2c_tests::blocking::transaction(state, ADDR_7BIT, 7..=9);
116-
i2c_tests::blocking::transaction(state, ADDR_10BIT, 7..=9);
116+
i2c_tests::blocking::transaction(state, ADDR_7BIT, 5..=5);
117+
i2c_tests::blocking::transaction(state, ADDR_10BIT, 5..=5);
117118
}
118119

119120
#[test]
120121
fn transactions_iter(state: &mut State) {
121122
i2c_tests::blocking::transactions_iter(state, ADDR_7BIT, 1..=1);
122-
i2c_tests::blocking::transactions_iter(state, ADDR_10BIT, 2..=2);
123+
i2c_tests::blocking::transactions_iter(state, ADDR_10BIT, 1..=1);
123124
}
124125

125126
#[test]

‎on-target-tests/tests/i2c_loopback_async.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,8 @@ mod tests {
7272

7373
#[test]
7474
fn transactions_iter(state: &mut State) {
75-
run_test(non_blocking::transaction(state, ADDR_7BIT, 7..=9));
76-
run_test(non_blocking::transaction(state, ADDR_10BIT, 7..=14));
75+
run_test(non_blocking::transaction(state, ADDR_7BIT, 5..=5));
76+
run_test(non_blocking::transaction(state, ADDR_10BIT, 5..=5));
7777
}
7878

7979
#[test]

‎on-target-tests/tests/i2c_tests/blocking.rs‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ pub fn transactions_read_write<T: ValidAddress>(
339339
restart_count: RangeInclusive<u32>,
340340
) {
341341
use embedded_hal::i2c::{I2c, Operation};
342-
let controller = reset(state, addr, true);
342+
let controller = reset(state, addr, false);
343343

344344
let samples_seq: FIFOBuffer = Generator::seq().take(25).collect();
345345
let samples_fib: FIFOBuffer = Generator::fib().take(25).collect();
@@ -390,25 +390,25 @@ pub fn transaction<T: ValidAddress>(
390390
// does not "waste" bytes that would be discarded otherwise.
391391
//
392392
// One down side of this is that the Target implementation is unable to detect restarts
393-
// between consicutive write operations
393+
// between consecutive write operations
394394
use embedded_hal::i2c::{I2c, Operation};
395-
let controller = reset(state, addr, true);
395+
let controller = reset(state, addr, false);
396396

397397
let mut v = ([0u8; 14], [0u8; 25], [0u8; 25], [0u8; 14], [0u8; 14]);
398398
let samples: FIFOBuffer = Generator::seq().take(25).collect();
399399
controller
400400
.transaction(
401401
addr,
402402
&mut [
403-
Operation::Write(&samples), // goes to v2
403+
Operation::Write(&samples),
404404
Operation::Read(&mut v.0),
405405
Operation::Read(&mut v.1),
406406
Operation::Read(&mut v.2),
407-
Operation::Write(&samples), // goes to v3
407+
Operation::Write(&samples),
408408
Operation::Read(&mut v.3),
409-
Operation::Write(&samples), // goes to v4
410-
Operation::Write(&samples), // remains in buffer
411-
Operation::Write(&samples), // remains in buffer
409+
Operation::Write(&samples),
410+
Operation::Write(&samples),
411+
Operation::Write(&samples),
412412
Operation::Read(&mut v.4),
413413
],
414414
)
@@ -432,7 +432,12 @@ pub fn transaction<T: ValidAddress>(
432432
assert_vec_eq!(e);
433433

434434
// assert reads
435-
let g: FIFOBuffer = Generator::seq().take(92).collect();
435+
let g: FIFOBuffer = itertools::chain!(
436+
Generator::seq().take(14 + 25 + 25),
437+
Generator::fib().take(14),
438+
Generator::seq().take(14),
439+
)
440+
.collect();
436441
let h: FIFOBuffer = itertools::chain!(
437442
v.0.into_iter(),
438443
v.1.into_iter(),

‎on-target-tests/tests/i2c_tests/mod.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,10 @@ fn target_handler(
104104
} = payload;
105105
match evt {
106106
Event::Start => *first = true,
107-
Event::Restart => *restart_cnt += 1,
107+
Event::Restart => {
108+
*first = true;
109+
*restart_cnt += 1;
110+
}
108111
Event::TransferRead => {
109112
let n = throttle.then_some(1).unwrap_or(target.tx_fifo_available());
110113
let v: FIFOBuffer = gen.take(n.into()).collect();

‎on-target-tests/tests/i2c_tests/non_blocking.rs‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -341,11 +341,9 @@ pub async fn transaction<A: ValidAddress>(
341341
assert_eq!(e, state.payload.borrow().vec);
342342
// assert reads
343343
let g: FIFOBuffer = itertools::chain!(
344-
Generator::fib().take(25),
345-
Generator::fib().skip(32).take(25),
346-
Generator::fib().skip(64).take(25),
347-
Generator::fib().skip(96).take(14),
348-
Generator::fib().skip(112).take(14),
344+
Generator::fib().take(25 * 3),
345+
Generator::seq().take(14),
346+
Generator::fib().take(14),
349347
)
350348
.collect();
351349
let h: FIFOBuffer = itertools::chain!(

‎rp2040-hal/src/i2c/controller.rs‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -227,21 +227,13 @@ impl<T: Deref<Target = Block>, PINS> I2C<T, PINS, Controller> {
227227
)?;
228228

229229
let lastindex = buffer.len() - 1;
230-
let mut first_byte = true;
231230
for (i, byte) in buffer.iter_mut().enumerate() {
232231
let last_byte = i == lastindex;
233232

234233
// wait until there is space in the FIFO to write the next byte
235234
while self.i2c.ic_status().read().tfnf().bit_is_clear() {}
236235

237236
self.i2c.ic_data_cmd().write(|w| {
238-
if first_byte {
239-
if !first_transaction {
240-
w.restart().enable();
241-
}
242-
first_byte = false;
243-
}
244-
245237
w.stop().bit(do_stop && last_byte);
246238
w.cmd().read()
247239
});
@@ -270,7 +262,6 @@ impl<T: Deref<Target = Block>, PINS> I2C<T, PINS, Controller> {
270262
)?;
271263

272264
let mut abort_reason = Ok(());
273-
let mut first_byte = true;
274265
'outer: while let Some(byte) = peekable.next() {
275266
if self.tx_fifo_full() {
276267
// wait for more room in the fifo
@@ -289,12 +280,6 @@ impl<T: Deref<Target = Block>, PINS> I2C<T, PINS, Controller> {
289280
// else enqueue
290281
let last = peekable.peek().is_none();
291282
self.i2c.ic_data_cmd().write(|w| {
292-
if first_byte {
293-
if !first_transaction {
294-
w.restart().enable();
295-
}
296-
first_byte = false;
297-
}
298283
w.stop().bit(do_stop && last);
299284
unsafe { w.dat().bits(byte) }
300285
});

‎rp2040-hal/src/i2c/controller/non_blocking.rs‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,6 @@ where
120120
)?;
121121

122122
let lastindex = buffer.len() - 1;
123-
let mut first_byte = true;
124123
for (i, byte) in buffer.iter_mut().enumerate() {
125124
let last_byte = i == lastindex;
126125

@@ -135,13 +134,6 @@ where
135134
.await;
136135

137136
self.i2c.ic_data_cmd().write(|w| {
138-
if first_byte {
139-
if !first_transaction {
140-
w.restart().enable();
141-
}
142-
first_byte = false;
143-
}
144-
145137
w.stop().bit(do_stop && last_byte);
146138
w.cmd().read()
147139
});
@@ -174,7 +166,6 @@ where
174166
)?;
175167

176168
let mut abort_reason = Ok(());
177-
let mut first_byte = true;
178169
while let Some(byte) = peekable.next() {
179170
if self.tx_fifo_full() {
180171
// wait for more room in the fifo
@@ -193,12 +184,6 @@ where
193184
// else enqueue
194185
let last = peekable.peek().is_none();
195186
self.i2c.ic_data_cmd().write(|w| {
196-
if first_byte {
197-
if !first_transaction {
198-
w.restart().enable();
199-
}
200-
first_byte = false;
201-
}
202187
w.stop().bit(do_stop && last);
203188
unsafe { w.dat().bits(byte) }
204189
});

‎rp235x-hal/src/i2c/controller.rs‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -227,21 +227,13 @@ impl<T: Deref<Target = Block>, PINS> I2C<T, PINS, Controller> {
227227
)?;
228228

229229
let lastindex = buffer.len() - 1;
230-
let mut first_byte = true;
231230
for (i, byte) in buffer.iter_mut().enumerate() {
232231
let last_byte = i == lastindex;
233232

234233
// wait until there is space in the FIFO to write the next byte
235234
while self.i2c.ic_status().read().tfnf().bit_is_clear() {}
236235

237236
self.i2c.ic_data_cmd().write(|w| {
238-
if first_byte {
239-
if !first_transaction {
240-
w.restart().enable();
241-
}
242-
first_byte = false;
243-
}
244-
245237
w.stop().bit(do_stop && last_byte);
246238
w.cmd().read()
247239
});
@@ -270,7 +262,6 @@ impl<T: Deref<Target = Block>, PINS> I2C<T, PINS, Controller> {
270262
)?;
271263

272264
let mut abort_reason = Ok(());
273-
let mut first_byte = true;
274265
'outer: while let Some(byte) = peekable.next() {
275266
if self.tx_fifo_full() {
276267
// wait for more room in the fifo
@@ -289,12 +280,6 @@ impl<T: Deref<Target = Block>, PINS> I2C<T, PINS, Controller> {
289280
// else enqueue
290281
let last = peekable.peek().is_none();
291282
self.i2c.ic_data_cmd().write(|w| {
292-
if first_byte {
293-
if !first_transaction {
294-
w.restart().enable();
295-
}
296-
first_byte = false;
297-
}
298283
w.stop().bit(do_stop && last);
299284
unsafe { w.dat().bits(byte) }
300285
});

0 commit comments

Comments
 (0)