cpu/nrf5x i2c: Set up correct interrupts after NOSTOP transmissions

[chrysn]

Contribution description

On nrf5x chips (tested on nrf52840), I2C_NOSTOP is not implemented correctly: It sets up the interrupts for the ERROR and STOPPED events, but the STOPPED condition is only reached when the "short" (nRF5x ling for triggering actions after state transitions) LASTTX_STOP causes the STOP condition to be set after the last transmission (eventually winding up in STOPPED). But when setting I2C_NOSTOP, the only interrupt that triggers is LASTTX, and that is not enabled.

Testing procedure

In the periph_i2c test (with DEBUG enabled for full output, but you'll see the hang anyway):

> i2c_acquire 0
2024-01-22 01:55:00,900 # i2c_acquire 0
2024-01-22 01:55:00,902 # Command: i2c_acquire(0)
2024-01-22 01:55:00,903 # [i2c] acquired dev 0
2024-01-22 01:55:00,905 # Success: i2c_0 acquired
> i2c_write_byte 0 30 0 4
2024-01-22 01:55:18,049 # i2c_write_byte 0 30 0 4
2024-01-22 01:55:18,053 # Command: i2c_write_byte(0, 0x1e, 0x04, [0x00])
2024-01-22 01:55:18,057 # [i2c] write_bytes: 1 byte(s) to addr 0x1e
2024-01-22 01:55:18,061 # [i2c] waiting for STOPPED or ERROR event

After that, the program hangs; with this patch, it reports

2024-01-22 02:00:11,915 # Success: i2c_0 wrote 1 byte to the bus

I've tested this with a microbit-v2, which has an accelerometer/magnetometer at address 30.

Issues/PRs references

Found this while implementing Rust embedded-hal 1.0, which is strict about not sending stop conditions, and also allows scatter-gather I2C that is implemented with I2C_NOSTART.

[riot-ci]

Murdock results

✔️ PASSED

fce82de cpu/nrf5x i2c: Forbid receive NOSTOP, document rationale

Success	Failures	Total	Runtime
8634	0	8634	11m:22s

Artifacts

• Documentation preview

[maribu]

Why not just always wait on all three success IRQs? Or do you suspect spurious IRQs?

If the finish would be the same code in the stop case, the nostop+RX, and the nostop+TX case a bit of ROM is likely to be safed.

[chrysn]

I suspect (but didn't test) that the LASTTX would fire a bit before STOPPED and thus we need precision. Unifying last TX and RX sounds viable -- will need to test what's smaller.

[chrysn]

Hm, this might be introducing a race condition: Researching whether doing LASTTX | LASTRX unconditionally might be viable, I found around https://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.nrf52832.ps.v1.1%2Ftwim.html figure 5 that "The TWI master will generate a LASTRX event when it is ready to receive the last byte" (i.e., when it is signalled it hasn't been read yet).

From more docs reading I think that the correct thing to do is to set a LASTTX -> SUSPEND shortcut when our application requests NOSTOP, and then to unblock the device on SUSPENDED. As I don't expect rogue interrupts, we can once more wait for anything (ERROR | STOPPED | SUSPENDED), and just need to set some shortcut unconditionally.

[chrysn]

After lengthy discussion I summarize that it's a low quality I2C implementation -- discussion on Matrix (linked in the commit) confirms that multiple chained operations of the same kind (read or write) can not be done without sending a ST(art), and while not sending a S(to)P is supported in theory, there is no good interrupt that can be caught after the read has completed. But it'd be of little use anyway as no continued read can be performed. (It might be used in a write-read-write pattern, but that's uncommon).

I've thus abandoned the announced SUSPEND route (those shortcuts are not available consistently across the nRF5x devices, and I don't think any more that they'd have done the desired thing in the first place), stuck with the original patch and forbade the race condition by making read requests with I2C_NOSTOP fail with an error. (Before this PR, those would stall the thread, so it's still a compatible change).

I've tested this by running the GPIO/I2C tests on the nrf52dk and the RIOT Peripheral Selftest Shield v0.3 (thanks maribu!) with a pinout that'll follow in a separate PR.