Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add support for transactions to Twim in embassy-nrf #3257

Merged
merged 3 commits into from
Oct 27, 2024

Conversation

alexmoon
Copy link
Contributor

Adds support for the embedded_hal(_async)::i2c::I2c::transaction() method to the Twim peripheral in embassy_nrf.

The inherent transaction methods also take an additional stop parameter to allow the user to suspend the bus following a transaction. This allows for operations like reading a register from a peripheral, modifying it, then writing it back all in a single I2C transaction.

A few nRF chips (specifically the nrf52832, nrf5340, and nrf9120) lack the LASTRX_SUSPEND short. This means they can't automatically enter the suspend state after a read operation, which is needed for certain (uncommon) operation sequences. Therefore, a manual implementation of the short has been added to the interrupt handler for those chips.

Lastly, I discovered a case of UB related to copying write buffers into RAM. setup_write and setup_write_read would allocate a buffer on the stack if the initial setup failed with a BufferNotInRAM error. However, they would then immediately return after setting up the I2C transaction and DMA, meaning the DMA engine would be pointing to stack memory that was no longer owned. This PR fixes that issue by providing an optional buffer parameter to set_tx_buffer so that the buffer can be passed in from an outer scope that is valid for the entire transaction.

@alexmoon alexmoon marked this pull request as draft August 15, 2024 22:55
@alexmoon alexmoon marked this pull request as ready for review August 19, 2024 20:39
@alexmoon alexmoon marked this pull request as draft August 20, 2024 00:23
@alexmoon
Copy link
Contributor Author

alexmoon commented Aug 21, 2024

Ok, I think this is ready to merge. I've tested all supported combinations of operations on an nrf52840 and everything works as expected.

The main limitation is that consecutive Read operations are not supported (and will panic). The issue is that the Twim hardware does not support suspending at the end of a read operation. If you set the SUSPEND task in response to the LASTRX event it will ACK the last byte of the current operation. When you then resume it will read one more byte from the peripheral before starting a new operation.

For example, what we would want on the bus for the transaction [Operation::Read(buf1), Operation::Read(buf2)] would be:

(S = Start, AR = Address+Read, A = ACK, N = NAK, P = Stop)
S AR A buf1[0] A buf1[1] A ... buf1[n-1] N <SUSPEND> S AR A buf2[0] A buf2[1] A ... buf2[n-1] N P

However this does not appear to be possible. The sequence of events and tasks: LASTRX -> SUSPEND; SUSPENDED -> STARTRX + RESUME results in the following data on the bus:

S AR A buf1[0] A buf1[1] A ... buf1[n-1] A <SUSPEND> buf2[0] N S AR A buf2[1] A buf2[2] A ... buf2[n-1] N P

Note that the first byte of buf2 is read before the repeated start condition on the bus.

The STARTRX task is required to load the DMA engine with the new pointer and buffer length, so it's not possible to just skip the repeated start condition and do one long read into the two buffers.

This behavior is sufficiently non-standard that I think it's better to simply not support consecutive read operations. Instead I've written the transaction support to avoid ever suspending after a read operation.

@alexmoon alexmoon marked this pull request as ready for review August 21, 2024 18:22
@alexmoon alexmoon requested a review from Dirbaio August 21, 2024 18:22
Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@Dirbaio Dirbaio added this pull request to the merge queue Oct 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 27, 2024
@Dirbaio Dirbaio added this pull request to the merge queue Oct 27, 2024
Merged via the queue into embassy-rs:main with commit cd9e581 Oct 27, 2024
6 checks passed
@alexmoon alexmoon deleted the nrf-twim-transactions branch October 28, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants