embedded-hal
embedded-hal copied to clipboard
rust-embedded
ADC OneShot: Clarify asynchronous behavior
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?
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.
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.
I agree. Real errors should always result in an Err() != Err(nb:WouldBlock).
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.)
(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).
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.
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.
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.
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).
(Also apologies if I'm being overly affected by my knowledge of the stm32F0 ADC, which supports active stopping of in progress conversions)
@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.
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.
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?
Also I have the feeling this is going to cause me to write the non-blocking version of the F0 ADC. sighs
@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.
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.
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.
@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.
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) -> ();
}
@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...
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.
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.
@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.
@chrysn Why do you think that making cancel blocking would prevent implementation?
@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.)
I suppose, might be worth doing a fn cancel(&mut self) -> nb::Result<(), !>; or with an appropriate Err value.