embedded_hal_async::digital::Wait implementation for Pin#903
embedded_hal_async::digital::Wait implementation for Pin#903martinsp wants to merge 4 commits intorp-rs:mainfrom
Conversation
| { | ||
| fn on_interrupt() { | ||
| let pin_id = I::ID; | ||
| let mut pin = unsafe { new_pin(pin_id) }; |
There was a problem hiding this comment.
Please add a comment why this is safe.
There was a problem hiding this comment.
The on_interrupt() implementation has changed and this is not needed anymore
| if pin.interrupt_status(Interrupt::LevelLow) | ||
| || pin.interrupt_status(Interrupt::LevelHigh) | ||
| || pin.interrupt_status(Interrupt::EdgeLow) | ||
| || pin.interrupt_status(Interrupt::EdgeHigh) |
There was a problem hiding this comment.
interrupt_status reads PROCx_INTS and then returns the bit selected by the parameter. As the read is volatile, the compiler can't aggregate the 4 reads to a single one. Therefore, this is quite inefficient.
There was a problem hiding this comment.
Changed implementation to read register using proc_ints
| pin.clear_interrupt(Interrupt::LevelLow); | ||
| pin.clear_interrupt(Interrupt::LevelHigh); |
There was a problem hiding this comment.
The level interrupts can't be cleared. From the datasheet (section 2.19.3):
The level interrupts are not latched. This means that if the pin is a logical 1 and the level high interrupt is active, it will become inactive as soon as the pin changes to a logical 0. The edge interrupts are stored in the INTR register and can be cleared by writing to the INTR register.
There was a problem hiding this comment.
Thanks for pointing this out, changed the implementation
|
|
||
| let _ = CPFn::new( | ||
| self, | ||
| Self::poll_is_high, |
There was a problem hiding this comment.
I don't think "wait for rising edge" can use poll_is_high as its poll function. A high level does not imply that an edge has occurred.
Better read the INTR register. (This may require that you don't clear the INTR bit in on_interrupt, but only do it in the poll function. I don't remember the details of the waker interactions.)
There was a problem hiding this comment.
Initialy before this call there was a code to ensure pin is low.
I updated the implementation to not clear interrupt inside on_interrupt and use a poll function specific to the relevant edge interrupt
|
Thanks for taking a look, I'll work on addressing the comments |
martinsp
force-pushed
the
pin-async-wait
branch
from
cf5011c to
c5c50ad
Compare
3 participants
Currently this implementation is only for rp2040.
I plan to implement also the rp235x version, but firstly I would like to fix any issues in the rp2040 implementation to not waste review time if there are soundness issues or other changes needed.