ADC OneShot: Clarify asynchronous behavior

[chrysn]

The OneShot trait defines returning a nb::Result, but it's not fully clear to me what a a WouldBlock means; could be either of

• Can't do, there's another ADC conversion in progress (should really not happen, b/c it takes a &mut self, but maybe it's on a shared bus, or maybe there was a channel switch and some calibration time needs to be accounted for or whatever)
• Yes I started it, but the result is not ready yet.

If it can be the latter, that begs the question of whether calling .read() incurs the risk of not actually getting a fresh value but the result an earlier read attempt that was aborted – and a caller can't rely on an instant return to mean "this has been requested earlier" either, for some ADCs might give results instantly.

My gut feeling is that nb::Result functions should be "Retry means I ignored you (and when I do something, I return something)" and not "Retry means I heard you (but I'll ignore any future requests until I've once returned something)" – but I might be wrong there and it doesn't need to be so strict to still give reliable oneshot results. What are other peripherals doing there? SPI doesn't have that issue as it has a send and a read method, maybe OneShot should too?

[HarkonenBade]

My general opinion is that an ADC implementation should reject attempts to 'read' different channels if there is an in progress conversion. I believe the use of nb is because ADC conversions can take a very long time and it may be advantageous to perform them asynchronously. It might be worth adding an explicit 'cancel' call that allows you to end the current conversion. So that in the case you need to force a new read through at the expense of an older one, you can call the cancel method to force the end of the previous read.

[HarkonenBade]

I think the risk of returning previous values from cancelled conversions should be avoidable by proper state machine use in the OneShot implementor code. I think that the latter 'conversion started but not yet done' would be the most common reason for returning would block, and I think the former should probably be represented by a separate error.

[therealprof]

I agree. Real errors should always result in an Err() != Err(nb:WouldBlock).

[chrysn]

I don't think that a proper state machine can avoid the risk of having stale values, for that state machine doesn't get told when an attempt to read is aborted, and when a new read is started.

Having a .cancel() would help a lot; whoever cares about having a fresh read would cancel and then read. (And the analogous .start()/.read() could be implemented in terms of it.)

[chrysn]

(Thinking on this a bit longer, other errors than "still waiting" are really not the point here, and the first bullet on my original post which @therealprof probably commented on is rather moot).

[HarkonenBade]

I don't think that a proper state machine can avoid the risk of having stale values, for that state machine doesn't get told when an attempt to read is aborted, and when a new read is started.

Having a .cancel() would help a lot; whoever cares about having a fresh read would cancel and then read. (And the analogous .start()/.read() could be implemented in terms of it.)

My assumption would be that the state machine would get informed about cancels, as it would be part of the struct implementing OneShot, and would also then be the interface through which reads are cancelled. I highly agree though that making that explicit in the interface would be very useful.

[therealprof]

cancel() might not always be possible to implement though. A state machine to prevent reading stale values is usually not even necessary because the hardware usually has internally bits indicating whether the sampling has finished, is in progress or resulted in an error.

[HarkonenBade]

cancel() might not always be possible to implement though. A state machine to prevent reading stale values is usually not even necessary because the hardware usually has internally bits indicating whether the sampling has finished, is in progress or resulted in an error.

Out of interest, when is cancel not possible to implement?
As from what I can see, if your 'read' method is blocking, then cancel is always a no-op. Otherwise cancel either stops the ADC conversion properly, and halts any active state machines, or it just cuts off the state machine and the next read tramples the existing one.

[chrysn]

A cancel would, with the ADCs I'm aware of, either set the "start a new sampling now" bit, or WouldBlock until the current reading is completed and then clear the result value. (However we call the method: it doesn't necessarily cancel the ongoing sampling, but ensures that when I .read() next I get a value sampled no earlier than the cancel call).

[HarkonenBade]

(Also apologies if I'm being overly affected by my knowledge of the stm32F0 ADC, which supports active stopping of in progress conversions)

[therealprof]

@HarkonenBade Some hardware does not allow you to cancel ongoing sampling. The only option in that case is to wait as @chrysn said, which is fine but should be documented accordingly.

[HarkonenBade]

Fair, the other take would be to just cut off your internal state machine and thus delay the block to the next read call (by making it wait until it can start a conversion again).

This is given that cancel is only really relevant in implementations with non-blocking read implementations.

[chrysn]

Sounds like a "let's have a concrete sketch of such an extension" to me. What are the procedures here in terms of unproven and adding a method to a trait? Could I add a method to the trait that, unless unproven is enabled, has a default no-op implementation? Or would all of this need to go into a separate trait?

[HarkonenBade]

You could add a method that only exists in unproven builds I believe.

[HarkonenBade]

Also I have the feeling this is going to cause me to write the non-blocking version of the F0 ADC. sighs

[therealprof]

@chrysn I disagree about the semantics of cancel as laid out. A blocking cancel implementation should always make sure it the operation is cancelled when it returns. I don't think a non-blocking cancel makes a lot of sense because their's no way to guarantee proper implementation and usage.

[HarkonenBade]

Yeah, my take would be that cancel always ensures that the next read can be invoked and returns a fresh value. I'm not sure if we want to force it to always block there and then or not, but thats just me.

[HarkonenBade]

That said I don't think cancel should be an 'nb' call. I just think that either it should block, or it should setup state such that the blocking happens at the start of the next read call.

[therealprof]

@HarkonenBade I'd rather we don't break existing traits without a good reason.
@chrysn The wait to go IMHO is to compose a new trait CancellableOneShot or something like that which extends the OneShot trait.

[HarkonenBade]

So I'd probably go for

fn cancel(&mut self) -> ();

[HarkonenBade]

I'd be down for an extension trait, so something like:

trait CancellableOneShot<ADC, Word, Pin: Channel<ADC>>: OneShot<ADC, Word, Pin> {
    fn cancel(&mut self) -> ();
}

[therealprof]

@HarkonenBade I don't expect a lot of hardware not being able to cancel ongoing sampling, can't remember where I've seen that. It should be clear what to expect though, maybe the application is not even interested in a new reading but wants to disable the ADC instead...

[chrysn]

ad CancellableOneShot, will do.

ad blocking: If cancel does not return a nb::Result, I'd fear that some ADCs might not be able to implement CancellableOneShot – but worst case they can't implement CancellableOneShot or need to block in there.

[therealprof]

@chrysn That should be fine, though, no?

[chrysn]

Yeah, as it won't affect me ;-) – more seriously though: Yes, I'd assume so.

The most exotic ADC that comes to my mind right now is an SPI-based reader (implementation pending), and that has enough state that a .cancel() can flag the pending transfer as stale or something like that.

[therealprof]

@chrysn If the new trait cannot be implemented then it cannot be implemented. That's why it is going to be a new trait. ;) And even if it could be implemented, the implementer has the options to not do it, e.g. if it is overly expensive or complicated to do.

[HarkonenBade]

@chrysn Why do you think that making cancel blocking would prevent implementation?

[chrysn]

@HarkonenBade Because in my ideal world, no HAL function will block for longer than its CPU / memory access time … no further reasons beyond that. (But given the ADCs at hand, no blocking will happen anyway.)

[HarkonenBade]

I suppose, might be worth doing a fn cancel(&mut self) -> nb::Result<(), !>; or with an appropriate Err value.

[therealprof]

Ewww. :-D

[chrysn]

Either way that'd need a bit of an update on the read documentation side.

What's the currently intended behavior of (not that'd be a particularly clever thing to do given OneShot's documentation, but typewise it's legal)

loop {
    let left = adc.read(left_pin);
    let right = adc.read(right_pin);
    if let Ok(x) = left && x < 128 {
        ...
    }
}

on a typical MCU ADC? read(left_pin) would start a conversion; assume that returns WouldBlock but the ADC result is available immediately after. read(right_pin) would see that there's a ready result, but would to verify that the pin selected in hardware is left_pin, decide that this result is not something to validly return, configure the ADC to sample right_pin and start – is that what the current interface demands? (Because that doesn't sound too great to me, checking all the configuration that was set before).

[HarkonenBade]

Either way that'd need a bit of an update on the read documentation side.

What's the currently intended behavior of (not that'd be a particularly clever thing to do given OneShot's documentation, but typewise it's legal)

loop {
    let left = adc.read(left_pin);
    let right = adc.read(right_pin);
    if let Ok(x) = left && x < 128 {
        ...
    }
}

on a typical MCU ADC? read(left_pin) would start a conversion; assume that returns WouldBlock but the ADC result is available immediately after. read(right_pin) would see that there's a ready result, but would to verify that the pin selected in hardware is left_pin, decide that this result is not something to validly return, configure the ADC to sample right_pin and start – is that what the current interface demands? (Because that doesn't sound too great to me, checking all the configuration that was set before).

My expectation would be that the intended behaviour of that would either cause both left and right to contain 'WouldBlock' or in the best possible case, left would contain the first read and right contain the second value. I'd hope that it would never result in the second read pulling the value that was read from the first.

[chrysn]

WouldBlock on both (or a read if it actually is available immediately) would certainly be the desireable outcomes, but that's not actually trivial to implement. I've been looking around for implementations, but the only one I've found so far is [stm32f0xx]https://github.com/stm32-rs/stm32f0xx-hal/blob/master/src/adc.rs), and that's plainly blocking.

[HarkonenBade]

Basically i'd assume that a non blocking implementation would either refuse the second call with an error, or steamroller the existing call. I'm a biggest fan of the first case where would refuse other conversions until the data had been read for the first read request (or until a cancel was issued).

[HarkonenBade]

(that implementation is mine)

[HarkonenBade]

My intent would be to make a state machine system that is aware of which pin it is converting, and that will refuse conversions on other pins until the active conversion is completed and the data read, or until a cancel is issued.

[eldruin]

If the OneShot trait implementation in my ads1x1x driver crate can serve as example here, both read() calls would return Error::WouldBlock and would continue to do so until the first conversion finished (no cancel supported). At this point you can either read the value for the first channel or a new conversion will be started, depending on the arguments passed to read(). Forcing data to be read before starting a new conversion on a different channel seems overly stringent to me.

[eldruin]

Alternatively, this implementation offers a different error reporting and would return Error::AlreadyInProgress as in already-in-progress-but-for-different-channel.

[chrysn]

Mh, I'd have hoped to avoid any such state machines or checking back at the registers. But while that's what it is, then there is #124 to have a proposal to talk about, along with the best example I could come up with that doesn't involve two timers and an interrupt.

As for the behavior when switching channels (whether it's an error or silently switches), is there an intended behavior we agree on? (For the best text to add to .read() on this right now would be that "When a read returns WouldBlock, it has to be called to exhaustion with the same channel by the caller; otherwise, the implementation is free to either discard the old request, or to report an error indefinitely.").

(Personally I'd have expected something type-state-ish along the lines of

trait OneShot<...> {
    fn read<'a>(&mut self, channel: &Self::Channel) -> nb::Result<ReadResult<'a, Self>>;
    /// Read the latest value of any read operation on this channel
    fn fetch(&self) -> nb::Result<...>; // maybe requiring an argument that only a ReadResult could provide
}

struct ReadResult<'a, A>(&'a mut A);
impl<'a, A: OneShot> ReadResult<'a, A> {
    fn fetch(&mut self) -> nb::Result<...> {
        self.0.fetch()
    }
}

but that's probably something for much later.)

[therealprof]

@chrysn Don't forget that a type implementing such a trait is not conjured out of thin air but requires target specific initialisation so the implementer has every possibility of ensuring that sampling requests are mutually exclusive or allow (hardware assisted) parallel use. If useful in any form it would also be possible to implement the trait for a collection of values to be sampled in parallel.

[eldruin]

@chrysn Thanks for #124!

Some thoughts:

1. If I did not miss anything in the datasheet, ADS1x1x devices are an example of devices that do not allow cancelling an ongoing conversion other than by doing a general I2C reset call or power reset. Therefore at least for the time being I would implement the cancel as a no-op, which would already ensure the trait guarantees, albeit not shortening the waiting time if a conversion is ongoing.

2. Implementations that actually need to do some I2C communication to cancel an ongoing conversion would need a way to report I2C communication errors.

3. I think the fetch below does not bring much benefit and I would say it would be overly complicated.

   1. I would see remembering the last value read as a user responsibility and not something that a driver must do. Furthermore, nothing would stop you from writing something like that on top of the OneShot trait just by remembering the channel in ReadResult and calling read(the_channel) again on ReadResult::fetch().

   2. In my eyes a fetch() method in OneShot kind of deceives the point of OneShot and resembles more the continuous mode.

trait OneShot<...> {
   fn read<'a>(&mut self, channel: &Self::Channel) -> nb::Result<ReadResult<'a, Self>>;

    /// Read the latest value of any read operation on this channel
    fn fetch(&self) -> nb::Result<...>; // maybe requiring an argument that only a ReadResult could provide
}

struct ReadResult<'a, A>(&'a mut A);
impl<'a, A: OneShot> ReadResult<'a, A> {
    fn fetch(&mut self) -> nb::Result<...> {
        self.0.fetch()
    }
}

[therealprof]

If I did not miss anything in the datasheet, ADS1x1x devices are an example of devices that do not allow cancelling an ongoing conversion other than by doing a general I2C reset call or power reset. Therefore at least for the time being I would implement the cancel as a no-op, which would already ensure the trait guarantees, albeit not shortening the waiting time if a conversion is ongoing.

Why would you implement the CancellableOneShot trait at all for that device?

[eldruin]

Why would you implement the CancellableOneShot trait at all for that device?

Sure, I could just not implement it. The only inconvenience would be if some other library requires a CancellableOneShot-capable device and the driver does not provide it although it is technically possible. The user could still implement it himself I guess.

[therealprof]

Well, you said it is technically not possible. ;) One could always create a blanket implementation which simply waits for the result...

[eldruin]

Sorry my response was not clear enough.
It is technically possible to implement the CancellableOneShot trait meeting its guarantees as currently documented by implementing a no-op given the read() implementation behavior.
The hardware will not actually stop the current measurement, though.
I would not favor a blocking cancel() implementation because the same is already possible just with read() plus no unnecessary busy-wait:

dev.read(Channel::A0);
dev.cancel();
block!(dev.read(Channel::A1));

In the code above, with or without the call to cancel() however this is implemented (blocking or no-op), the result and running time are the same.

[therealprof]

For me cancel has two uses:

1. to ensure that the next read will yield a clean result obtained only after called cancel
2. to ensure that the hardware is in a consistent and safe state because it will be shutdown or the system is supposed to go into sleep mode

The latter implies that CancellableOneShot is implemented iff the hardware is actually capable of cancelling the operation and that when cancel returns the operation is concluded.

[eldruin]

I see. I did not think of 2.
We should definitely document this in #124. Then I could implement a blocking cancel() and document why it blocks and why you may not need it.

[chrysn]

1. to ensure that the next read will yield a clean result obtained only after called cancel

That's the purpose I'm interested in.

2. to ensure that the hardware is in a consistent and safe state because it will be shutdown or the system is supposed to go into sleep mode

Is that a) something we have a use case for, and b) is CancellableOneShot the right trait for it? After all, that'd be something *any* peripheral that can have "running" nb operations could face that issue, or even be reluctant to be shut down when not "inside" an nb operation for whatever that'd mean.

[therealprof]

Is that a) something we have a use case for,

Sane interfaces you mean? I always have a usecase for that. :-D

is CancellableOneShot the right trait for it?

Yes.

After all, that'd be something any peripheral that can have "running" nb operations could face that issue,

True, and we're gonna get to that. Timers already have cancel and other (potentially long running) operations will eventually get it too.

or even be reluctant to be shut down when not "inside" an nb operation for whatever that'd mean.

I don't understand, I'm afraid.