-
Notifications
You must be signed in to change notification settings - Fork 272
pio: enable async.await support #913
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
base: main
Are you sure you want to change the base?
Conversation
62be79e
to
c3f222c
Compare
($t:ident, $doc:expr, $offset:expr) => { | ||
impl<S: ValidStateMachine, W: TransferSize> Wakeable for $t<S, W> { | ||
fn waker() -> &'static IrqWaker { | ||
static WAKER: IrqWaker = IrqWaker::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First: I may be completely wrong, feel free to just tell me that I'm writing nonsense. I don't understand the internals of async rust well enough to confidently state that there is an issue with this code. I just don't understand how it is supposed to work.
If I understand this correctly, there is an issue with this static
: It is shared with all instances of Rx
(or Tx
). There are no separate instances per monomorphization (https://doc.rust-lang.org/reference/items/static-items.html#r-items.static.generics). Therefore, if there are multiple state machines running in parallel, and one tries to await
the Rx
(or Tx
) of more than one state machine at the same time, the second one to be polled will overwrite the waker of the first one.
Of course, this only matters if there actually are multiple wakers. With the trivial mock-executor used in the on-target-tests, where a single futures is called to completion from a single task, this wouldn't make a difference.
But if, for example, the two Rx
are awaited from different CPU cores, this code may wake the wrong one, and the other one might stall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you got it right >< I thought there would be 1 instance per monomorphisation. I'll fix this !!
This change saves us from reading ic_raw_intr_stat twice in the same loop.
One case is on read operations for 10bit I2C. First a write is issued with the address, then a restart and a the msb of the address is resent with this time the read flag. Because there is no data phase, the peripheral entered the active state but never switched to either Read or Write.
4fff5fb
to
8039ffb
Compare
I didn’t get time to test this on rp235x yet. If anyone has the bandwidth please try it and let me know <3
An example use case: