Add NoDma I2C master async bus implementation #333
Conversation
tullom
requested review from
felipebalbi,
jerrysxie,
JamesHuard,
madeleyneVaca and
RobertZ2011
as code owners
src/i2c/master.rs
Outdated
| /// which can lead to bus timeouts if a byte is not handled fast enough. | ||
| /// | ||
| /// use flexcomm fc with Pins scl, sda as an I2C Master bus, configuring to speed and pull | ||
| pub fn new_async_nodma<T: Instance>( |
There was a problem hiding this comment.
Instead of implementing this for all the instances, can we also only implement this for the I2C instance corresponding to FC15?
There was a problem hiding this comment.
Why? User can decide to not use DMA on any instance, right? Should we make the choice for them? I'm not convinced, IMO.
In any case, we shouldn't add a new method here. Just patch new_async() to take an Option<impl Peripheral...> or something like that. Could you give that a go?
There was a problem hiding this comment.
So my issue with patching new_async() is that that would be a breaking change to our I2C driver. If we are ok with that, I can do it (and would prefer it if we aren't going the FC15 only route)
There was a problem hiding this comment.
FYI @jerrysxie @felipebalbi
I was able to create a NoDma structure. I had to minimally modify the DMA api, specifically reserve_channel() now returns an option that will return None if NoDma is used. Needed to slightly modify the UART driver and hashcrypt driver as well to return errors if NoDma is used since it doesn't make sense if NoDma is used.
Tested all touched drivers via examples on hardware and it works flawlessly.
src/i2c/master.rs
Outdated
| /// which can lead to bus timeouts if a byte is not handled fast enough. | ||
| /// | ||
| /// use flexcomm fc with Pins scl, sda as an I2C Master bus, configuring to speed and pull | ||
| pub fn new_async_nodma<T: Instance>( |
There was a problem hiding this comment.
Why? User can decide to not use DMA on any instance, right? Should we make the choice for them? I'm not convinced, IMO.
In any case, we shouldn't add a new method here. Just patch new_async() to take an Option<impl Peripheral...> or something like that. Could you give that a go?
tullom
force-pushed
the
i2c-master-async-nodma
branch
4 times, most recently
from
65a1e3d to
76988a3
Compare
tullom
force-pushed
the
i2c-master-async-nodma
branch
from
76988a3 to
a9ffcd8
Compare
|
FYI @jerrysxie @felipebalbi Tested all touched drivers via examples on hardware and it works flawlessly. |
Closes #327
Some kicking off questions:
new_async_nodma()fn instead of hijackingnew_async()is becausenew_async()requires a dma_ch object that impls Peripheral and impls an interrupt withMasterDma. I tried creating structs to impl all that but I hit a wall.new_async(), except in the cases of the Flexcomm not having DMA support. Is this enough or do you think this should be gated behind some sort of feature flag to reduce the possibility of misuse?EDIT:
I was able to create a NoDma structure. I had to minimally modify the DMA api, specifically reserve_channel() now returns an option that will return None if NoDma is used. Needed to slightly modify the UART driver and hashcrypt driver as well to return errors if NoDma is used since it doesn't make sense if NoDma is used.