Closed geomatsi closed 3 years ago
Hi, thanks a lot for the PR! In general this is a good and useful idea. However I see a problem with the OneShot
trait, making this not quite as straightforward:
The read()
method returns an nb::Result<Word, Self::Error>
which means that it should be non-blocking. Users will poll this method repeatedly until it returns the ADC value instead of WouldBlock
.
In the context of shared-bus
this means we will only lock the mutex for the duration of one non-blocking poll call, not from beginning to end of a conversion. This can lead to very nasty situations: Consider two tasks both trying to read a channel, then yielding to the rest of the system before attempting the poll again. It could end up like this:
TASK A TASK B Comment
| |
adc_proxy1.read(Ch1) | The ADC HAL starts a conversion on Ch1.
-> WouldBlock |
| |
| adc_proxy2.read(Ch2) A read() for a different channel means the ADC
| -> WouldBlock HAL will cancel the ongoing conversion and
| | restart converting for Ch2 now.
| |
adc_proxy1.read(Ch1) | Again, a read() for a channel which is not the
-> WouldBlock | one we're currently converting leads to
| | cancelling the ongoing conversion and
| | restarting for Ch1...
| |
| adc_proxy2.read(Ch2) This will now continue forever...
| -> WouldBlock
. .
. .
. .
I think shared-bus
absolutely must prevent such situations from happening. After all, its entire job is to arbitrate "bus" access between multiple competing users.
Now, all hope is not lost, I see a few possible solutions. First of all, I want to observe that actually a lot of HALs don't even implement OneShot
in a non-blocking way! Take a look at the stm32f3xx-hal
ADC for example (not that convert_one
blocks until the conversion is done) or the stm32f4xx-hal
ADC which also blockingly converts a sample.
So what we can do is, just like the HALs, we break the non-blocking contract of the trait and actually just busy-spin until a sample is returned. In practice this would mean changing the proxy implementation to the following:
fn read(&mut self, pin: &mut Pin) -> nb::Result<Word, Self::Error> {
self.mutex.lock(|bus| nb::block!(bus.read(pin)))
}
For the above-mentioned "blocking HALs", this would behave exactly the same, but for the non-blocking HALs which implement OneShot
"correctly", it would ensure race-conditions like demonstrated above are impossible because we will keep the ADC mutex locked until a conversion has completed.
Of course this is kind of a hack - I think the better approach here would be to change embedded-hal
to have an explicitly blocking OneShot
trait instead and then make shared-bus
work ontop of that. But as that's not going to happen quickly (and HAL adoption of the new trait is also not going to be fast), I think going with the hacky solution for now is acceptable. We should, however, explicitly document this behavior to at least not negatively surprise anyone...
Hello @Rahix
Thanks for careful review. I force-pushed PR after introducing the changes according to your review comments. One minor change was proper error type mapping of the blocking wait result. ADC blocking behavior has been documented in both documentation block to read
function and in commit message.
Regards, Sergey
Hi @Rahix
Do you plan to merge this PR in its current form ? Let me know if you have any other concerns.
Regards, Sergey
Great to see this is already implemented here. Needed this feat, and rust analyzer pointed me out that shared bus is already implementing the OneShot
trait! Saves me the hassle implementing 👍
One remark though: maybe call out in this projects' readme.md
that an ADC proxy actually exists within this crate, so other people don't have to stumble upon this functionality! 😉
Done!
You're da man! 🎉
Hi,
This PR adds new proxy type for sharing access to ADC hardware block. The
AdcProxy
implementsOneShot
trait so it can be passed to drivers instead of ADC instance. PR also includes some tests for the new proxy type based onembedded-hal-mock
.Regards, Sergey