ADC-related traits #101

thenewwazoo · 2018-10-06T05:08:13Z

Opening the discussion! One implementation currently exists here, and I'll be adding another for the LPC177x/8x HAL soonish.

I feel like these traits encompass nearly all the useful "modes" that an ADC operates in; n.b. that IME "continuous" sampling of a single pin looks just like a "scan" with one pin enabled, so I didn't distinguish between the two. I'm not sure if I should, but it's a known t.b.d.

(obviously the final PR would gate this behind unproven, or however is appropriate)

austinbes · 2018-10-08T00:35:03Z

src/analog.rs

+}
+
+/// ADCs that sample on single channels per request, and do so at the time of the request.
+pub trait SingleMode<Word, Pin: AdcChannel> {


Minor bikeshedding: analog::SingleMode doesn't seem like the a great name here -- and it doesn't really line up with analog::Continuous.

Some off-the-cuff ideas:

analog::OnDemand

analog::Once

I like both suggestions. Another one: OneShot, which I've seen used in this context.

Yeah, OneShot sampling is the typical term here. I think it's also important to recognise what is being described here: Is this the acquisition or the trigger mode? At the moment it sounds more like a mix of the two.

Continuous: Continuous sampling with single value capture (Immediate trigger)

Once: Single sampling with single value capture (Immediate trigger)

Continuous is only mildly useful as it only saves the reinitialisation cost for the ADC but running an ADC continuously to just capture a value every now and then is rather energy intensive.

Continuous mode is far more interesting if you can do automatic sampling of many values at a specific rate into a fixed buffer (e.g. audio signal), rolling values into a circular buffer or automatically trigger a recording of a a single or burst of samples at some event (edge/level triggering on the signal).

I think it's fair to say that I'm trying to describe the trigger mode here since it's closest to how the hardware configuration is realized. I do agree that Continuous gets more interesting when you use it in the ways you describe. I don't see a clear path there, and I feel like there's kind of an implied DMA activity too, traits for which are not yet very well-defined.

austinbes · 2018-10-08T00:43:30Z

src/analog.rs

+    fn stop(&mut self) -> nb::Result<(), Self::Error>;
+
+    /// Read the specified channel
+    fn read(&mut self, chan: Pin::ID) -> nb::Result<Word, Self::Error>;


I'm confused as to how this API ought to work -- maybe I'm missing something.

It seems as though the intended flow would be something like:

fn get_samples() { let pin = Channel::new(); // actually get a channel though let adc = Adc::init(); // actually get an ADC handle adc.add_channel(pin); adc.start(); for _ in 0..=10 { let _ = adc.read(pin); // fails borrow check } adc.stop(); let rpin = adc.release_channel().unwrap(); }

I'm omitting error checking, etc, and might have made a mistake with this hasty example. The fact remains, however, that unless I'm misinterpreting when and how add_channel() and read() must interact, this interface cannot work.

I'm also confused as to how this would work with multiple continuously-sampled channels.

Sorry for the earlier deleted comments - I misunderstood your question.

Note that read takes a Pin::ID argument, not the Pin itself. So the code would look something like:

let pin: gpio::PA0<Analog> = gpioa.PA0.into_analog(); // PA0<Analog> impls AdcChannel<ADC1> let adc: Adc1<Continuous<...>> = adc::init(...).into_continuous_mode(); // Adc1<Continuous> impls adc::Continuous<...> adc.add_channel(pin); adc.start(); for _ in 0..=10 { let _ = adc.read(PA0::CHANNEL); } adc.stop(); let rpin = adc.release_channel(PA0::CHANNEL).unwrap();

hannobraun · 2018-10-08T12:15:24Z

src/lib.rs

@@ -689,6 +689,7 @@
 extern crate nb;
 extern crate void;

+pub mod analog;


(Please note that I haven't worked with a DAC so far, so the following concerns may be unfounded. It would be great if someone with more experience in this area could pitch in.)

I'm concerned about the naming, and how well it will suit our needs once we add DAC traits. While analog lines up nicely with digital, having a single analog module means both ADC and DAC will have to share the same namespace, and you're already reserving some attractive names for ADC.

Digital I/O is usually implemented in the same peripheral, and some drivers need to concern themselves with both input and output on the same pin. For that reason, I think it's a good idea to have a single digital module. The question is, is the same true for ADC and DAC?

If the answer is "no", then I suggest renaming the module to adc.

My experience lines up with your intuition here -- a rename to adc would be preferable IMO.

It's even what I started typing when bikeshedding the trait name, before I double checked.

adc and Once works for me.

hannobraun · 2018-10-08T12:16:32Z

src/analog.rs

+}
+
+/// ADCs that sample on single channels per request, and do so at the time of the request.
+pub trait SingleMode<Word, Pin: AdcChannel> {


I like both suggestions. Another one: OneShot, which I've seen used in this context.

hannobraun · 2018-10-08T12:17:19Z

src/analog.rs

+}
+
+/// ADCs that sample on single channels per request, and do so at the time of the request.
+pub trait SingleMode<Word, Pin: AdcChannel> {


Not sure how I feel about the name Pin. Maybe it Channel, or just C would be simpler.

And have you considered moving Pin to read_channel? That would make it more convenient to use the trait (one less argument to specify in parameter lists), but I see two potential problems:

I'm not sure if that would allow an implementation to specify what Pin is in a where clause.

It would be inconsistent with Continuous.

So yeah, not sure if this is a good idea, but I'm interested in knowing if you tried/considered it.

I mostly kept it there for consistency with the other modes. Nothing's preventing moving Pin into the various functions. If it's cleaner/easier, I'm not opposed.

I'm not opposed to keeping it where it is. Just wondering out loud.

hannobraun · 2018-10-08T12:24:55Z

src/analog.rs

+    fn add_channel(&mut self, pin: Pin) -> nb::Result<(), Self::Error>;
+
+    /// Release the ADC channel/pin
+    fn release_channel(&mut self) -> nb::Result<Pin, Self::Error>;


Presumably you can call add_channel multiple times. At least the trait won't prevent that. What will release_channel return then?

Sorry, mistake here. Continuous::release_channel should indeed take a Pin::ID argument.

hannobraun · 2018-10-08T12:27:30Z

src/analog.rs

+    fn stop(&mut self) -> nb::Result<(), Self::Error>;
+
+    /// Read the specified channel
+    fn read(&mut self, chan: Pin::ID) -> nb::Result<Word, Self::Error>;


In addition to @austinbes's concerns, I'm confused about the overall purpose of this API. What's the point of starting a continuous conversion, if I then have to poll the read method anyway?

On such point might be to get a sample rate that's known, and regulated by hardware.

With that said, it would probably be more ergonomic (and more in-line with standard embedded paradigms) to somehow provide a buffer of samples rather than each individual one. I'm not sure the right way to do that in a memory-safe manner, though...

The Continuous::read method here would likely be non-blocking; For example, in the (iirc) LPC178x, each channel has a specific register and the results of the most recent conversion for that specific channel are stored there. (There's also a register for last conversion result for any channel.)

Also, I contemplated a way to provide access to a buffer, but I couldn't see a way until const generics land because the number of channels would need to be constant in order to know the size of the buffer provided.

I don't know if it's appropriate here, but I think the canonical way to work around the lack of const generics is generic-array.

thenewwazoo · 2018-10-08T20:18:46Z

It occurs to me that some ADC operations would be blocking (Once-mode reads), while some would not (Continuous-mode reads). There's a place in the library for blocking traits; should this go there? Can some part of it go there?

hannobraun · 2018-10-09T11:52:08Z

@thenewwazoo

It occurs to me that some ADC operations would be blocking (Once-mode reads), while some would not (Continuous-mode reads). There's a place in the library for blocking traits; should this go there? Can some part of it go there?

Why would Once reads be blocking? As currently proposed, Once::read_channel returns nb::Result.

thejpster

LGTM

thenewwazoo · 2018-10-09T19:50:41Z

@hannobraun

Why would Once reads be blocking? As currently proposed, Once::read_channel returns nb::Result.

I don't suppose they'd have to be, but I imagined them as such because that's how I've used single-ended conversions in the past: request a conversion, wait until done, return the result. I don't think there's anything about these traits that would require one behavior or another - that could be up to the implementor.

I also think I should gate this behind unproven, which I haven't done yet, so another commit will be incoming soon.

hannobraun · 2018-10-10T13:10:50Z

@thenewwazoo

I don't suppose they'd have to be, but I imagined them as such because that's how I've used single-ended conversions in the past: request a conversion, wait until done, return the result. I don't think there's anything about these traits that would require one behavior or another - that could be up to the implementor.

I think they absolutely should be non-blocking then. The precedent in this crate so far has been to provide blocking traits only in two cases:

As a convenience, implemented on top of a non-blocking trait.
When a non-blocking implementation would not be possible.

I've had some time to reflect on this pull request now, and I have the following concerns and suggestions:

Everything needs to go behind unproven, as already noted.
I'd prefer OneShot to Once, as one-shot seems to me the common term for this (as confirmed by @therealprof). I don't feel strongly about this point.
The code talks about "pins" and "channels", which to me seem like two words for the same thing. I'd just call it "channel" in every case.
I'm unsure if Continuous is the right approach. I'm not specifically saying it's wrong, but I'm concerned that it's too specific to be broadly implemented and of limited use. I'd feel more comfortable if we had a few implementations for different platforms, or someone with solid experience with the matter telling me my concern is unfounded. (My own experience with ADC HAL API implementations is limited and not recent.)
To my question what the purpose of Continuous is, @austinbes answered that one purpose might be to have a hardware-regulated sample rate. Should the sample rate be an argument of the start method?
Could Once and Continuous be traits that are implemented for Channels instead? That could cut down on a lot of complexity (no dealing with pin/channel ids).

@thejpster You've already approved this PR without otherwise taking part in the discussion. What's your take on my list of concerns? I'm very interested in more input.

thenewwazoo · 2018-10-10T17:36:38Z

@hannobraun all good points, thanks for the feedback.

I split the concepts of pins and channels because the pin (the physical, mux-able component of the MCU) is distinct from the channel (the voltage source for the ADC conversion). In some cases (e.g. LPC1788) the mapping is 1:1, but in others, you can select between multiple pins. Much as unsafe impl RxPin<$USART> for PX<AFn>, I'm imagining unsafe impl AdcChannel<$ADC> for PX<Analog>. The ID type needs to be generic because in some cases you can have banks of channels (so your ID might be (bank, channel)) or some other thing I've not seen, which is a possibility I want to permit. If the conceptual split is unclear, then there's definitely room to improve the docs (which is true anyway), or rename them to make it more obvious. I don't think the two concepts should be combined or conflated, though.

It seems Continuous mode isn't entirely clear. The two chips I'm working with on-hand both have a mode whereby you specify one-or-many channels to sample and then start conversions. The results of the conversions are stored in registers that can be accessed immediately, and interrupts can be generated based on single conversion completion or completion of all selected channels. (Burst mode is similar, except it will not continue conversion after all channels have been sampled).

I'll call out separately that I feel very strongly that sample rates should not be part of this API. In every ADC I've ever seen, sample rate is a property of the ADC's core and is determined by some clock input and a prescaler. IOW, I think sample_rate should be an argument to whatever init function (or perhaps new method) the ADC struct impls. If you need to adjust the sample rate (if it's even possible, which it may not be!) that's outside of the scope of these traits.

thenewwazoo · 2018-10-10T17:38:50Z

I'll rename Once to OneShot.

Another possibility - move forward with the OneShot trait since that seems to be well-understood and straightforward, and open a separate PR for more complex ADC modes?

thenewwazoo · 2018-10-10T17:40:32Z

Sorry for the comment spam - one last note. I think implementing these as non-blocking (that is, returning nb::Result) would be easy. In every case I've seen there's a way to check if the result of a conversion is complete. I'm starting to think that makes the most sense anyway.

hannobraun · 2018-10-10T18:30:12Z

Thank you the explanation, @thenewwazoo. Your comments have cleared things up a lot for me. My objections have been reduced to a vague feeling that better traits are possible, but until I dive into some ADCs and try to implement, and improve on, your traits (which, for the foreseeable future, I won't have time for) that's a moot point.

I'm still interested in hearing more feedback from others, but as far as I'm concerned, I'm not opposed to merging this (at least for now; can't guarantee I won't come up with more feedback, given enough time :) ).

Another possibility - move forward with the OneShot trait since that seems to be well-understood and straightforward, and open a separate PR for more complex ADC modes?

We can do that, but unless someone comes up with new objections, I don't think it's necessary.

There's one more thing I thought about. I think it's part of a larger discussion and shouldn't hold back this pull request, but at least I'd like to note it, while we're all here.

Continuous has the methods start, stop and read. Presumably calling read is only valid between start and stop. Calling it before start or after stop would be an error or a no-op, I assume. This constraint, read only being valid between start and stop, could be encoded in the traits, to enforce it at compile time.

There's precedent for keeping things simple and not enforcing this stuff at compile time (see timer::CountDown, for example. But I think at some point we should have the discussion whether we want to do this.

hannobraun

Thanks for the changes, @thenewwazoo. The added documentation improves things a lot!

I found some more stuff to nitpick about. Let me know what you think. Otherwise this is looking really good!

src/adc.rs

hannobraun · 2018-10-10T18:33:54Z

src/adc.rs

+    ///
+    /// This method takes a `Pin` reference, as it is expected that the ADC will be able to sample
+    /// whatever channel underlies the pin.
+    fn read_channel(&mut self, pin: &mut Pin) -> nb::Result<Word, Self::Error>;


This is called read_channel, the method on Continuous is called read. I think both are fine names, but it may be better to keep it consistent and call both of them the same.

src/adc.rs

thenewwazoo · 2018-10-10T19:47:01Z

Given @hannobraun's excellent points regarding the design of Continuous, plus my realization that the API can't work in the way I'd proposed, I'm going to cut it (and Burst) out of this PR, and revisit the design of more ADC operation modes later on.

ryankurte

Thanks for the effort! Looks reasonable to me. I wonder whether it would be useful to have examples of consuming these traits (ie. into structs for drivers) in the documentation too? (on the basis that eventually consumers >> implementers).

thenewwazoo · 2018-10-15T16:04:58Z

@ryankurte I'm happy to drop a link in the doc pointing to my own crate which impls this if that's acceptable; I don't think I'll have time for a few days to work on a minimal-esque example.

ryankurte · 2018-10-16T19:09:34Z

@thenewwazoo even just here for us to look at would be neat ^_^

thenewwazoo · 2018-10-16T19:33:17Z

Sure! You can see my impl here for the STM32L0x1. I can't publish yet (it's pending this merge) so you'll have to generate docs yourself if you want to see them rendered.

ryankurte

looks excellent, thanks!

hannobraun

Thank you, @thenewwazoo, looks good to me!

One last wish: Please squash everything into one commit, then I'll merge.

jonwingfield · 2018-10-18T13:58:28Z

Waiting anxiously for this!

Also worth noting:
https://gitlab.henriktjader.com/pln/nucleo-64/commit/23d83bac3e26b5301bc54227a4bc2249150e43a8
I don't know if @japaric still prefers the DMA/ring-buffer method, but I like the idea!

thenewwazoo · 2018-10-23T15:07:38Z

Do I need to do anything more on this? I'm eagerly anticipating its merge. :)

hannobraun · 2018-10-23T15:18:09Z

Sorry, @thenewwazoo, I didn't notice that you pushed a new commit.

bors r+

101: ADC-related traits r=hannobraun a=thenewwazoo Opening the discussion! One implementation currently exists [here](https://github.com/thenewwazoo/stm32l0x1-context-hal/blob/master/src/adc.rs), and I'll be adding another for the LPC177x/8x HAL soonish. I feel like these traits encompass nearly all the useful "modes" that an ADC operates in; n.b. that IME "continuous" sampling of a single pin looks just like a "scan" with one pin enabled, so I didn't distinguish between the two. I'm not sure if I _should_, but it's a known t.b.d. (obviously the final PR would gate this behind `unproven`, or however is appropriate) Co-authored-by: Brandon Matthews <bmatthews@zipcar.com>

hannobraun · 2018-10-23T15:18:46Z

Oh, and thank you for your patience and working with us on this!

bors · 2018-10-23T15:23:25Z

Build succeeded

continuous-integration/travis-ci/push

thenewwazoo · 2018-10-23T15:35:19Z

Huzzah!

thenewwazoo · 2018-10-23T15:36:44Z

Now the next question is: when can this be released (i.e. cargo published)? :)

hannobraun · 2018-10-23T15:57:47Z

@thenewwazoo

Now the next question is: when can this be released (i.e. cargo published)? :)

Pretty much right now. Consensus in #92 was to release a new patch version right after landing this. Just requires someone from the team finding the time to do it.

thenewwazoo requested review from ilya-epifanov, thejpster and a team as code owners October 6, 2018 05:08

austinbes reviewed Oct 8, 2018

View reviewed changes

hannobraun reviewed Oct 8, 2018

View reviewed changes

thejpster previously approved these changes Oct 9, 2018

View reviewed changes

thenewwazoo dismissed thejpster’s stale review via 413a33e October 10, 2018 18:19

hannobraun reviewed Oct 10, 2018

View reviewed changes

thenewwazoo commented Oct 10, 2018

View reviewed changes

src/adc.rs Outdated Show resolved Hide resolved

hannobraun added the S-waiting-on-review Review is incomplete label Oct 11, 2018

ryankurte previously approved these changes Oct 15, 2018

View reviewed changes

thenewwazoo dismissed ryankurte’s stale review via 40c121c October 15, 2018 16:02

ryankurte mentioned this pull request Oct 16, 2018

New version? #92

Closed

ryankurte previously approved these changes Oct 16, 2018

View reviewed changes

hannobraun suggested changes Oct 17, 2018

View reviewed changes

hannobraun added S-waiting-on-author Author needs to make changes to address reviewer comments and removed S-waiting-on-review Review is incomplete labels Oct 17, 2018

ADC-related traits

ca3adee

thenewwazoo dismissed ryankurte’s stale review via ca3adee October 18, 2018 04:21

thenewwazoo force-pushed the adc branch from 1a9cd22 to ca3adee Compare October 18, 2018 04:21

ryankurte approved these changes Oct 19, 2018

View reviewed changes

burrbull mentioned this pull request Oct 23, 2018

Add support for the ADC peripheral japaric/stm32f103xx-hal#114

Open

hannobraun approved these changes Oct 23, 2018

View reviewed changes

bors bot merged commit ca3adee into rust-embedded:master Oct 23, 2018

thenewwazoo deleted the adc branch October 23, 2018 15:35

burrbull mentioned this pull request Nov 2, 2018

Adc API #10

Open

ryankurte mentioned this pull request Nov 12, 2018

ADC API does not allow non-unit channels #110

Closed

eldruin mentioned this pull request Sep 23, 2020

added dac #247

Closed

eldruin mentioned this pull request Apr 13, 2022

Tracking issue for ADC trait #377

Open

ADC-related traits #101

ADC-related traits #101

Conversation

thenewwazoo commented Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thenewwazoo Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannobraun Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thenewwazoo Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thenewwazoo commented Oct 8, 2018

hannobraun commented Oct 9, 2018

thejpster left a comment

Choose a reason for hiding this comment

thenewwazoo commented Oct 9, 2018

hannobraun commented Oct 10, 2018

thenewwazoo commented Oct 10, 2018 • edited Loading

thenewwazoo commented Oct 10, 2018

thenewwazoo commented Oct 10, 2018

hannobraun commented Oct 10, 2018

hannobraun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thenewwazoo commented Oct 10, 2018 • edited Loading

ryankurte left a comment • edited Loading

Choose a reason for hiding this comment

thenewwazoo commented Oct 15, 2018

ryankurte commented Oct 16, 2018

thenewwazoo commented Oct 16, 2018

ryankurte left a comment

Choose a reason for hiding this comment

hannobraun left a comment

Choose a reason for hiding this comment

jonwingfield commented Oct 18, 2018

thenewwazoo commented Oct 23, 2018

hannobraun commented Oct 23, 2018

hannobraun commented Oct 23, 2018

bors bot commented Oct 23, 2018

Build succeeded

thenewwazoo commented Oct 23, 2018

thenewwazoo commented Oct 23, 2018

hannobraun commented Oct 23, 2018

thenewwazoo commented Oct 6, 2018 •

edited

Loading

thenewwazoo Oct 8, 2018 •

edited

Loading

hannobraun Oct 8, 2018 •

edited

Loading

thenewwazoo Oct 8, 2018 •

edited

Loading

thenewwazoo commented Oct 10, 2018 •

edited

Loading

thenewwazoo commented Oct 10, 2018 •

edited

Loading

ryankurte left a comment •

edited

Loading