(embassy-rp): Add I2C master implementation #914
Conversation
MathiasKoch
force-pushed
the
embassy-rp/i2c
branch
from
c5a5698 to
aa8f8ef
Compare
MathiasKoch
changed the title
MathiasKoch
force-pushed
the
embassy-rp/i2c
branch
from
1e96e19 to
2e1baf6
Compare
I dunno the hardware details. It's possible that if you wait just for rxing the data, the bus takes a bit more time to become idle (for example to do the stop condition). The current embedded-hal trait says nothing about how to handle that, the new trait rust-embedded/embedded-hal#392 has a
I think that's to workaround i2c devices on the bus getting stuck, not the i2c master peripheral itself getting stuck, right? In that case I'd say no. nrf and stm32 doesn't have that either. Also the user can implement that on their own with GPIO (I've done it for the nRF once).
probably best not to do it for now. I'd wait until the new i2c trait is in rust-embedded/embedded-hal#392, that'll allow transferring multiple buffers in a single transaction without |
embassy-rp/src/i2c.rs
Outdated
| pub struct I2c<'d, T: Instance, M: Mode> { | ||
| tx_dma: Option<PeripheralRef<'d, AnyChannel>>, | ||
| rx_dma: Option<PeripheralRef<'d, AnyChannel>>, | ||
| dma_buf: [u16; 256], |
There was a problem hiding this comment.
I don't think DMA is winning much here, if we need to statically allocate this buffer and copy the payload data to it. Given that i2c is pretty slow, it wouldn't be the end of the world to take an interrupt for each byte each time the fifo drains, at least for low speed devices.
DMA might be useful for demanding applications, but it seems to me that the default impl would be better simple and low overhead. I imagine we'd want a standard i2c implemented with pio as well (and maybe pio+dma) so in principle we'd want to support multiple impls anyway.
There was a problem hiding this comment.
I think I would agree with this.
Thoughts @Dirbaio
There was a problem hiding this comment.
It's very unfortunate the i2c design doesn't allow DMAing directly from the buffer, yes... Whether that makes using DMA not worth it.. I dunno! I'm OK with whatever you guys decide.
Re pio-i2c, I'd imagine that'd be a different struct.
|
Given that i have no hardware to test async I2C with, I am inclined to split the blocking and async parts into two PR's here, so we can merge the blocking part & continue working on the async part? |
|
Moved the async part into #978. This PR with just the blocking implementation should be good to go now 👍 |
|
Merge conflict. |
|
bah |
Co-authored-by: Jacob Gonzalez <[email protected]>
…plementation faster
Dirbaio
force-pushed
the
embassy-rp/i2c
branch
from
2b6f343 to
bf1da04
Compare
|
rebased bors r+ |
|
Build succeeded: |
984: rp pico async i2c implementation r=Dirbaio a=jsgf This implements an interrupt-driven async i2c master. It is based on #914, a bit of #978 and `@ithinuel's` https://github.com/ithinuel/rp2040-async-i2c.git This is still work-in-progress, and is currently untested. 1006: Removes some of the code duplication for UarteWithIdle r=Dirbaio a=huntc This PR removes some of the code duplications for `UarteWithIdle` at the slight expense of requiring a split when using idle processing. As the nRF example illustrates though given the LoC removed, this expense seems worth the benefit in terms of maintenance, and the avoidance of copying over methods. My main motivation for this PR was actually due to the `event_endtx` method not having been copied across to the idle-related code. Tested the uart_idle example on my nRF52840-dk, and from within my app. Both appear to work fine. Co-authored-by: Jeremy Fitzhardinge <[email protected]> Co-authored-by: huntc <[email protected]>
This PR adds both blocking and DMA based async implementations of I2C master.
Both E-H 0.2 & E-H 1.0 abstractions are implemented as well.
Questions & concerns:
vectored_{read, write}implementations?