Add ADC proxy

[geomatsi]

Hi,

This PR adds new proxy type for sharing access to ADC hardware block. The AdcProxy implements OneShot trait so it can be passed to drivers instead of ADC instance. PR also includes some tests for the new proxy type based on embedded-hal-mock.

Regards,
Sergey

[Rahix]

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...

[geomatsi]

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

[geomatsi]

Hi @Rahix

Do you plan to merge this PR in its current form ? Let me know if you have any other concerns.

Regards,
Sergey

[Rahix]

Hi,

yes, sorry for the silence. One note for the future: The place where you documented the implementation details (on the read() method inside the trait impl) makes them almost invisible in the documentation. I'll move it to the AdcProxy struct documentation, so you don't have to worry about it :)

Thanks again for the PR!

[eflukx]

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! 😉

[Rahix]

Done!

[eflukx]

You're da man! 🎉