-
Notifications
You must be signed in to change notification settings - Fork 193
Async drivers #279
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
Async drivers #279
Conversation
111e209
to
03c8cf7
Compare
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.
I've only looked at SPI. (I'll look at gpio at some point since I know I'll have some thoughts about the interrupt handling 🙂)
Besides the inability to queue multiple transactions I don't think your implementation is lacking any features that mine had.
I do think this feature is quite important though as it means the next ESP-IDF transactions can still happen even when the runtime is too busy polling other futures.
The reason my implementation was so complex is because "async in traits" weren't a thing a the time so I had to make an SpiFuture
struct to use a GAT return type.
With "async in traits" being available now, I'd have just written a much simpler for loop with all the state being implicit on the stack.
Also, the reason I went with a separate async driver was to avoid mixing polling and interrupt transactions but &mut self
should be enough to prevent this. The other reason was because the e-hal async trait has the same method names as the sync trait, which will make the compiler unhappy. I still think it's best to separate them but this is up to you.
This PR is good timing though, as I had just thought about how to take advantage of "async in traits" but I hadn't written any code yet, so I can put my ideas here.
I have more comments but they're sort of contingent on how you response to my comment about the Operation
trait, so I'll just wait until we resolve that.
src/spi.rs
Outdated
@@ -300,10 +308,11 @@ pub mod config { | |||
} | |||
|
|||
pub struct SpiBusDriver<T> { | |||
_lock: Lock, | |||
lock: Option<Lock>, |
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.
Why has this become optional? I saw transaction_async
takes is_locked: bool
but why would users want to not have a lock? That would lead to some non-transactional behavior, which users can already achieve by individually calling the operations themselves.
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.
Yeah. Think about multiple devices on a single bus. This case is impossible to do asynchronously (assuming no changes are made to the ESP IDF itself and no horrible hacks). Reason: ESP IDF's lock the bus
functions are blocking/synchronous and unfortunately we have to use these so as to implement proper embedded-hal
(or if you wish - "Rust") transactions, as the native ESP IDF transactions are constrained in terms of buffer size. Therefore:
- Either you have just one device on the bus, and then locking the bus always completes without waiting
- Or you need to lift the "lock the bus" requirement - which - of course - means you are not doing "real" transactions, from e-hal POV, but at least the stuff runs without blocking.
The esp-idf-hal
SPI public *_async
APIs do take a lock_bus: bool
extra parameter precisely for that reason. I mean, the user - in the current state of things, has to pick the lesser evil. Which one is the lesser evil (try to lock the bus and block if there are multiple devices on it or don't lock the bus but then no real transactions - no big deal for stuff like LCD I guess) only the user knows.
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.
Ah, I hadn't considered the blocking aspect of this.
I see your point. I think it'd be better if users just make the individual calls to read
, write
, etc. if they what lock_bus = false
behaviour.
spi_device_acquire_bus
takes a timeout though, so you could make it poll like you've done with spi_device_get_trans_result
I think.
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.
spi_device_acquire_bus
takes a timeout though, so you could make it poll like you've done withspi_device_get_trans_result
I think.
Timeout is of no use if I don't get notified when to try again. Hmmm...
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, don't you have that problem already with spi_device_queue_trans
? It internally locks the bus, which is why it takes the timeout parameter as well.
loop {
match esp!(unsafe {
spi_device_queue_trans(handle, &mut transaction as *mut _, delay::NON_BLOCK)
}) {
Ok(_) => break,
Err(e) if e.code() != ESP_ERR_TIMEOUT => return Err(e),
_ => NOTIFIER[*host as usize].wait().await,
}
}
Ahhh the shared notifier makes this work. Pesky pesky. Idk if this is too far but maybe some shared state per driver to keep a bag of all wakers waiting for transactions to end maybe?
There's also the option of calling the waker immediately but I'll get scolded for that suggestion haha.
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.
Yes I agree with you. Now that you've pointed out the native esp-idf driver problem, the issue I brought up still stands.
How do you solve this problem for spi_device_queue_trans
? Since you don't know when the bus lock is released. (spi_device_queue_trans
and spi_device_polling_transmit
locks the bus)
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.
We pre-lock it. Which we anyway have to do if the keep-cs-active
flag is raised (and it always is by us, as the ESP IDF SPI transactions don't fit the notion of E-HAL transactions). Otherwise the SPI driver returns an error.
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.
... but as the table at the top of the driver says - you can't currently use async when you have multiple devices on the bus. Or actually you can, but it will not be "async".
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.
Ah fair enough, it all makes sense now.
At least folks who want multiple devices can create an SpiBusDriver
, share it with an async mutex and use software CS.
In a future, it'd be interesting to consider injecting some kinda thread to perform the blocking.
A fun thing you can do now to enforce the documentation is to only implement the async methods on SpiDevice
when T
is BorrowMut
instead of just Borrow
.
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.
Ah fair enough, it all makes sense now. At least folks who want multiple devices can create an
SpiBusDriver
, share it with an async mutex and use software CS. In a future, it'd be interesting to consider injecting some kinda thread to perform the blocking.
Coincidentally, I just implemented something similar to ^^^ in Spi*DeviceDriver
(including the "SoftCs" one): they all now use a hidden shared async mutex which is centralized in SpiDriver
before hitting the blocking call to "lock the bus". Thus, if all devices on the bus are used in an async way, the blocking "lock the bus" call is no longer an issue.
src/spi.rs
Outdated
pub fn fetch<F: Future>(mut fut: F) -> F::Output { | ||
// safety: we don't move the future after this line. | ||
let mut fut = unsafe { Pin::new_unchecked(&mut fut) }; | ||
|
||
let raw_waker = RawWaker::new(core::ptr::null(), &VTABLE); | ||
let waker = unsafe { Waker::from_raw(raw_waker) }; | ||
let mut cx = Context::from_waker(&waker); | ||
|
||
match fut.as_mut().poll(&mut cx) { | ||
Poll::Ready(output) => output, | ||
Poll::Pending => panic!("Future is not ready yet"), | ||
} | ||
} |
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.
This seems like a complicated way to re-use the do_read
between read
and read_async
. It's a bit awkward to generate a Future
for synchronous code.
There's a way to implement the reuse without messing with a waker, context and panic.
See the Operation
trait I had in my async SPI PR (but ignore the CsRule
stuff I did, using peekable
like @Vollbrecht did, is better).
trait Operation {
fn fill(&mut self, transaction: &mut spi_transaction_t) -> bool;
}
The trait just gives a way to generate the ESP-IDF transactions, then you can write a function to send operations synchronously and one to send operations asynchronously.
Of course I didn't do this for e-hal transactions yet but it should be doable, see the TransferTrail
enum I wrote. Worse case scenario the method can be duplicated for e-hal transactions. The duplication will be needed for transaction delays anyway.
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.
This seems like a complicated way to re-use the
do_read
betweenread
andread_async
. It's a bit awkward to generate aFuture
for synchronous code.
I'm in a good company though! See what I found just the other day.
Seriously though - good point, I'll look into it. If we get a net win in terms of code size by adopting Operation
+ ChunksMut
- I'll switch.
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.
Yeah. I'm not really sure that the Operation
approach resulted in net savings in terms of LOCs, but I think it was a win in terms of simplicity when I applied it consistently throughout the codebase... as I was able to centralize the actual execution of transactions in like 3-4 key methods. Everything else now in this (large) driver is just delegating boilerplate.
I've not followed literally your approach with the Operation
trait - it is more of a switch to monadic combinators (map
, flat_map
and so on) to build an iterator of spi_transaction_t
instances out of everything possible. This iterator is then executed either synchronously, or asynchronously. But same idea, I think.
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.
Did a quick skim and this iterator approach is much cleaner than the Operation
trait. I'll take a proper look in a bit.
Will try to get a look at it this evening (US Pacific, -0700). |
Queueing multiple transactions across several devices is still possible, and will naturally happen. For multiple transactions on a single device... I have to think a bit if that's possible to implement without complicating the code too much, which is a key requirement for me to have a first pass at async. I can't escape the "YAGNI" feeling though. If the CPU is constantly busy by polling too many other futures, one is mixing an IO bound async execution (SPI and other peripherals) with a CPU-bound execution ("async" futures which are not really async as they do computationally intensive tasks). This is an anti-pattern IMO. One should offload the CPU-bound computations to a thread pool (or on embedded - to a separate thread-local executor which runs within a lower priority thread). This way, the critical IO-bound workloads (as in scheduling new SPI transactions) will always preempt the lower prio computations and run without delays.
Let me set some context here. From my POV, "async fn in trait" (AFIT) is not a thing yet. :-( It is (a) in nightly only and (b) still even marked as "incomplete". Worse, they blew all deadlines outlined in their May Issue of the Inside Rust blog. As in - none of the deadlines were met, so AFIT won't hit stable this year, in my opinion. Another context: for this reason (and others),
My patch does not even implement yet the AFIT traits of
As per above, AFIT - for now - would only be a thing when the "nightly" feature is enabled, and it would only be used as much as to implement the With that said, aren't you confusing stuff? Why do you need AFIT? It is only related to implementing traits. These - in turn - are necessary only when you want to abstract concrete implementations and hide them behind a layer of trait-based generics. This is only strictly necessary when you implement the
Let me see the operation trait... |
I think you misunderstand me or missed the point I was making with AFIT.
Yes, you achieve this by queuing. The peripheral would then be able to execute transactions without CPU needing to spend time setting up the next IO bound transaction. Though I see what you mean, at the end of the day this is an optimisation.
Doesn't building Rust for esp already require nightly or am I mistaken?
What do you mean? At the end of the day don't you want to implement the e-hal traits? You'll still have to take them into consideration when designing the API. |
But then again: you don't have any of these restrictions to methods in regular structs. These can be trivially async, and can return
Both do queueing, just on a different level. Each executor has a hidden task queue. By (trivially) using two async executors, you get two such hidden queues, where the tasks in the queue of the first executor are prioritized over the tasks in the queue of the second executor, if the second executor runs in a lower priority thread. But then and as I said - if not too much trouble - I'll look into supporting the native queueing caps of the ESP IDF SPI driver.
Yes and no. the ESP-RS toolchain is - strictly speaking - a nightly toolchain, so you can enable and use nightly features. Yet, my understanding is that it is always branched from the same commit which becomes "the next stable Rust", so to say. So it is also stable. The one and only reason why the
I want to implement them, but behind a feature flag.
What is the lesser evil: (a) having two drivers, just because of this (and like a common "core" ugggh, more complexity and annoying delegation boilerplate code to the common "core"), or (b) a single driver and the |
The nightly feature doesn't help. The removal of the restriction of having to return a named type helps.
Well, if you're asking me, for what I think, I'll have to chose (a). It depends how the code looks after you look into the user context and |
Again - and if that's an argument - Embassy also chooses (b). Except that the ugliness goes to the blocking part - for obvious reasons in their case, as they don't have a traditional task scheduler ala FreeRtos. |
I really need an example as to what you mean. Sorry - I feel lost in this particular item of the debate . :-) |
Any particular reason to follow embassy so closely?
Let's just drop this, no point wasting energy on it. In my initial comment, I was only explaining why my implementation was complex. It doesn't really change anything about this PR. |
I trust Dario's judgement on embedded design - particularly w.r.t. async - as I've seen a lot of their stuff in Embassy stand the test of time. (Not that we are always in agreement - look at the discussion around the TCP async traits in Also I personally view the Embassy HAL crates as a modernization on the original ones. I've mostly copied from there the
Sure, np. |
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.
I think there's one case where an OutputPin
was flipped to an InputPin
by mistake (see comments); feel free to change this and submit.
I have some comments about the removal of Option<>
on the pins; there was an intent there, but I've since forgotten it (probably related to ADC/DAC support). I doubt that will come to fruition, as it seems to be poorly supported in ESP-IDF (requires fiddling with the GPIO MUX, which isn't supported).
The core driver changes look good. Undoes some of the earlier review comments about splitting the driver, but that's alright. :-)
I have not studied the async
workflow throughly by any means, and am definitely not read up enough to comment on the Waker bits. I looks correct, and I'm certainly looking forward to being able to use it (looks far more ergonomic).
@ivmarkov, do you have a rough estimate as to when this will land? I have a few fixes for remmy (esp-rs@matrix) on the I2S side that I'd like him to test; if it'll be awhile, I'll go ahead and have him test against a branch over on my side. Thanks! |
Hopefully next weekend (I'm on travel right now, and there was quite some activity on the What I would strongly suggest is to PR the changes agains this branch ( |
No need to be sorry! Just wanted to coordinate. Safe travels! |
Just FYI, @ivmarkov, you'll probably need to add a couple of Clippy dacut@b247e94 has the changes. Just a
|
Sorry about that, @ivmarkov, but glad you got it working! |
if let Some(notification) = | ||
unsafe { (transaction.user as *mut Notification as *const Notification).as_ref() } |
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.
Any reason not to split this into multiple lines?
if let Some(notification) = | |
unsafe { (transaction.user as *mut Notification as *const Notification).as_ref() } | |
let condition = transaction.user as *mut Notification as *const Notification; | |
if let Some(notification) = unsafe { condition.as_ref() } |
self.notified.store(false, Ordering::SeqCst); | ||
} | ||
|
||
#[allow(unused)] |
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.
This macro still needed?
Co-authored-by: Dominic Fischer <14130965+Dominaezzz@users.noreply.github.com>
@ivmarkov I realize this change was a little while ago, but thank you for adding I was using I'm trying to push for replacing C code with Rust at my company and this will be very helpful to continue to do so. So thank you (and your fellow contributors) for all your hard work! :) |
ps. thank you also @MabezDev, @Dominaezzz , and @dacut! |
Overview
This PR is introducing/changing
async
support to four drivers: GPIO, ADC, I2S, and SPI:read_async
andwrite_async
async methods; removed unsafe subscription to ISR eventsread_async
andwrite_async
async methodsThere is a certain commonality between the async support for all the drivers from above and the reason why I did one PR for all these drivers - as I wanted to see if this simple approach would work everywhere:
async
functions next to the blocking ones. No blocking code was damaged in this implementation :-) Semantics of blocking code stays the sameNotification
object is set to "triggered", which awakes the async executor and schedules any taskawait
-ing (polling) theNotification
object - see belowread_async
/write_async
methods in each driver are simply wrappers, that - in a loop - call the blockingread
/write
methods with a timeout 0 (so they - in fact - don't block then) and if these methods returnESP_ERR_TIMEOUT
(meaning, the read/write was unsuccessful because the driver's receive FreeRtos queue was empty or the driver's send FreeRtos queue was full), these methods justawait
the Notification which is triggered from the ISRNext steps (short term)
read/write
blocking API was defined by you in a trait. Unfortunately, I couldn't just addasync read_async
/async write_async
method declarations to your traits, because async methods in traits are not yet supported in stable Rust. This forced me to actually retire your traits and instead implement theread
/write
methods directly on to theI2sDriver
structure. This - in turn - and to avoid enormous code repetition - meant that I needed to fold all of theI2sStdDriver
,I2sPdmDriver
andI2sTdmDriver
into theI2sDriver
itself. As a result, all constructors have astd
/tdm
/pdm
suffix now, as inI2sStdDriver::new_rx()
becameI2sDriver::new_std_rx()
. But... the outcome (IMO) is a simpler API, as the only I2S driver we have now is theI2sDriver
struct, and the pdm vs std vs tdm selection is just solved by using the appropriate constructor.InputFuture
in favor ofasync
/await
(why do we need it? as if writingpoll
implementations is fun), removed the need foralloc
, and made the async support a tad more flexible in that the GPIO driver does not do ISR subscribe/unsubscribe every time you await a pin input state, which means the chance to miss state changes is lowerNext steps (mid term)
If the above approach (trigger notification from the ISR and then just wrap around the existing driver's blocking read/write methods with a timeout of 0) proves successful, we need to "operate" - in a similar fashion - all of UART, I2C, CAN, PWM and LEDC. UART, I2C and CAN specifically will need small PRs to the ESP IDF itself, as they don't currently offer a way for the user to subscribe and be called back from the driver's ISR handling routine. But these drivers have mostly everything else in place (i.e. FreeRtos queues and read/write methods with a timeout) so the changes to the ESP IDF should be minimal.