into_async
#2430
into_async
#2430
Conversation
bugadani
force-pushed
the
asyncify
branch
3 times, most recently
from
cf609a1
to
58034a8
Compare
bugadani
force-pushed
the
asyncify
branch
5 times, most recently
from
c78da49
to
53438f4
Compare
esp-hal/src/rsa/mod.rs
Outdated
| crate::interrupt::disable(Cpu::ProCpu, Interrupt::RSA); | ||
| #[cfg(multi_core)] | ||
| crate::interrupt::disable(Cpu::AppCpu, Interrupt::RSA); |
There was a problem hiding this comment.
Why disable on both cores? Just the current one should suffice, no?
There was a problem hiding this comment.
The drivers are currently Send, so it's possible to drop one on the other core. We don't assume where a driver was created (except to bind its interrupts in the first place, which is problematic as it is).
There was a problem hiding this comment.
Considering the interrupt handlers are not written to handle cross core setups, the async drivers should be marked at !Send. If folks wants to move the drivers, they should do so in blocking mode.
There was a problem hiding this comment.
Considering the interrupt handlers are not written to handle cross core setups, the async drivers should be marked at !Send. If folks wants to move the drivers, they should do so in blocking mode.
This is a nice idea and I think resolves #1525 properly.
There was a problem hiding this comment.
I'll do this separately, there are weird edge cases with split peripherals.
bugadani
force-pushed
the
asyncify
branch
3 times, most recently
from
377cb06
to
9815f07
Compare
esp-hal/src/rsa/mod.rs
Outdated
| crate::interrupt::disable(Cpu::ProCpu, Interrupt::RSA); | ||
| #[cfg(multi_core)] | ||
| crate::interrupt::disable(Cpu::AppCpu, Interrupt::RSA); |
There was a problem hiding this comment.
Considering the interrupt handlers are not written to handle cross core setups, the async drivers should be marked at !Send. If folks wants to move the drivers, they should do so in blocking mode.
This is a nice idea and I think resolves #1525 properly.
| { | ||
| const CHANNEL: u8; | ||
| #[doc(hidden)] | ||
| pub trait TxChannelInternal<M> |
There was a problem hiding this comment.
now a user could implement TxChannelInternal (and TxChannel) etc. - when removing the private mod we need to seal the internal traits, no?
There was a problem hiding this comment.
These traits shouldn't even exist in the first place, so I don't think this is a big deal.
|
|
||
| /// Converts a blocking channel to an async channel. | ||
| pub fn into_async(mut self) -> Channel<'d, C, Async> { | ||
| self.set_interrupt_handler(C::async_handler(&self)); |
There was a problem hiding this comment.
In general, there should probably be more clean up work when converting to async, since who knows which core the user setup the interrupt handlers on, or if they still have some interrupt pending.
There was a problem hiding this comment.
Okay I've done some of it but I guess we'll have more edge cases. Since users were able to shoot themselves in the foot previously anyway, I'm fine with the current state - which mostly just neglects peripheral interrupts bits.
bugadani
force-pushed
the
asyncify
branch
6 times, most recently
from
83ea4b4
to
67394ce
Compare
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Closes (almost) #2321. Not closing the issue because of timers.
I'm ignoring timers because they aren't in our core peripheral set and I don't want to deal with them just now.