TWIM improvements. #242
Merged
TWIM improvements. #242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Code for setting DMA buffers is deduplicated into fns set_tx_buffer, set_rx_buffer. Fixes nrf-rs#208
Previously only AddressNack was handled, and the transfer would hang if another error ocurred.
The previous implementation would send a TWI start condition for each chunk, so the slave chip would see each as a separate transaction. This is not equivalent to `write_then_read`, which was the original goal of adding chunking. It seems TWIM has no way to send multiple buffers in a single transaction, therefore the ability to chunk is removed.
|
All nits addressed (commits amended). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
copy_write_then_read. The previous implementation would send a TWI start condition for each chunk, so the slave chip would see each as a separate transaction. This is not equivalent towrite_then_read, which was the original goal of adding chunking. It seems TWIM has no way to send multiple buffers in a single transaction, therefore the ability to chunk is removed.There are some issues left with TWIM:
There's no way for the user use SUSPEND manually if they want to do a more complex repeated start sequence. Nordic's lib has a "hold" flag that suspends instead of stopping so you can do any combination of reads and writes.
If we do this it could be used for implementing
write_then_readandcopy_write_then_readto reduce code duplicationThis is probably left for a future PR :)
old, no longer apply
- `copy_write_then_read` splits written data into 1024 byte chunks and txs each individually. This actually causes a repeated start condition for each chunk, instead of a single start condition at the beginning like it would happen with `write_then_read`.
This should be at least documented, but I think in practice the chunking is useless because devices will take data after a repeated start as a new transaction (expecting you to send command/address bytes again).
Maybe we should remove the chunking?
copy_write_then_readshould use SUSPEND. It seems that once LASTRX/LASTTX happens it's not OK to leave TWIM hanging, you must immediately STOP, SUSPEND, or STARTTX/STARTRX. This can be a problem if using TWIM from low priority code. Nordic says here:This was a problem before with
write_then_readtoo, but this PR changes it to use SHORTS.