ADC free-running mode & FIFO #626

nilclass · 2023-05-31T18:11:03Z

An attempt to come up with an API for the ADC in free-running mode.

Inspired by #326.

Overview of changes:

Adds two new structs to the adc module: FreeRunning and Fifo (EDIT: naming changed to AdcFifoBuilder and AdcFifo)
FreeRunning represents the Adc configuration for free-running mode:
- it is obtained by calling adc.free_running()
- it provides methods to configure things that only make sense in this mode: set clock_divider (DIV.INT, DIV.FRAC), set round_robin (CS.RROBIN), ...
- and methods to start enable the FIFO and start the capture (CS.START_MANY): start_fifo, start_fifo_with_dma(thresh)
the Fifo struct represents the ADC Fifo while capture is running:
- it is returned by the start_* methods from FreeRunning
- fifo.len() and fifo.read() give access to the FCS.LEVEL and FIFO register values
- fifo.is_over() and fifo.is_under() expose the FCS.OVER and FCS.UNDER flags (and reset them on read)
- fifo.stop() stops the capture and disables the fifo

Brief example:

    let mut adc = Adc::new(dp.ADC, &mut resets);
    let mut adc_pin_0 = pins.gpio26.into_floating_input();
    let mut adc_pin_1 = pins.gpio27.into_floating_input();

    let mut fifo = adc.free_running()
        .clock_divider(0, 0)
        // first conversion is from pin 26
        .initial_input(&mut adc_pin_0)
        // cycle between pin 26 and 27
        .round_robin((&mut adc_pin_0, &mut adc_pin_1))
        // enable fifo and starts capture
        .start_fifo();

    // alternative to .start_fifo(), which also enables FCS.DREQ (with given FCS.THRESH of 1):
    //  .start_fifo_with_dma(1)

    let mut i = 0;

    while i < 100 {
        if fifo.len() >= 2 {
            // a comes from pin 26, b from pin 27
            let (a, b) = (fifo.read(), fifo.read());

            // ... do something with `a` and `b`

            i += 1;
        }
    }

    // Stop capturing (CS.START_MANY = 0).
    // Returns the Adc, so it can be reused for single capture or another free-running capture.
    let adc = fifo.stop();

rp2040-hal/Cargo.toml

jannic

Hi @nilclass, thanks for this pull request. Overall I like the API, but there are a few details left to do.

rp2040-hal/src/adc.rs

jannic · 2023-06-03T08:23:05Z

rp2040-hal/src/adc.rs

+    ///
+    /// In addition to being usable the same way as a `Fifo` returned from `start_fifo`,
+    /// the Fifo returned by this function can also be used as a source for DMA transfers.
+    pub fn start_fifo_with_dma(self, thresh: u8) -> Fifo<'a, true> {


The datasheet says: "The threshold for DREQ assertion (FCS.THRESH) should be set to 1, so that the DMA transfers as soon as a single sample is present in the FIFO. Note this is also the threshold used for IRQ assertion, so non-DMA use cases might prefer a higher value for less frequent interrupts."

Therefore I'd say for the DMA use case, the threshold should not be configurable, but fixed to 1.
Also, it probably (but see my other comment) doesn't harm to always enable DREQ. So start_fifo_with_dma and start_fifo may not need to be separate functions? And the DmaEnabled flag could be removed from the Fifo type signature?

hm, yeah, those are some good points.

If DREQ is always enabled, we can't enforce FCS.THRESH=1 for the DMA usecase (or else not have the threshold configurable).

We could move this issue to the user (at least until it's clear how to handle it nicely API-wise), by having builder methods set_threshold(...), enable_dma(true | false), enable_interrupt(true | false), and have a single start_fifo function as you suggest.
That would make these features usable without messing with registers manually, but it would leave some caveats:

either there needs to be some note, that when enabling DMA, the threshold should be set to 1

or enable_dma would force the threshold to 1, possibly overriding the user choice (or not, if the user calls set_threshold after enable_dma)

Not the best options, imho.

So, also in light of your other comments: maybe the DMA part can be skipped for now, and handled in a future PR, when more is known about the usecases.
Then my goal would be to make the API nice for the interrupt usecase.

Which makes me wonder: do you think there is a usecase where neither the interrupt, nor DMA is used?
From what I can tell when capturing in free-running mode, CS.READY does not change, so even though the samples can be read from CS.RESULT there is no way to tell when a new sample was captured.

You might have code that's only interested in the most recent measurement (ie. it doesn't matter if you skip some measurements). For example, with ADC in free-running mode, you could do such a thing:

loop { do_something_timing_critical(); if adc.read_single() > THRESHOLD { led.set_high().unwrap(); } else { led.set_low().unwrap(); } }

That way, the CPU doesn't need to wait 96 cycles for a new ADC value, but still always gets a (relatively) recent sample.

I agree, that sounds reasonable.

Since AdcFifo currently holds a &mut Adc calling adc.read_single() directly wouldn't work though (unless the AdcFifo is dropped first, which makes it impossible to stop the conversion later on).
So I've added a read_single method to AdcFifo now, which just calls the underlying adc.read_single to enable this use case.

rp2040-hal/src/adc.rs

nilclass · 2023-06-04T18:42:10Z

@jannic thanks for taking the time to review this.

I realize this PR is not in a good state to merge, it's more of a draft, to see if there is interest in such an API 🙂

I'll respond to your feedback inline.

nilclass · 2023-06-06T09:57:42Z

I've done a bit of cleaning up:

removed the DMA related things for now, as mentioned above
renamed the two types & some of the methods
added method to enable the FIFO interrupt explicitly (with given threshold)

I've also added a working example: adc_fifo_poll. It polls fifo.len() to wait for a sample, then calls fifo.read(). I figured this is the easiest way to demonstrate the feature.
I'm planning to add another example which demonstrates the interrupt usecase.

Some things that are still missing in my opinion (though they don't block usage of the features that are there):

Error handling: when setting FCS.ERR = 1, the FIFO will also contain an error flag. AdcFifo::read could return a Result or Option to expose these errors, but it only really makes sense when that flag is set.
Shifting (FCS.SHIFT = 1): it would be nice if AdcFifo::read returned u8 instead of u16 when the result is shifted.

- Rename `Fifo` struct to `AdcFifo` - Rename `FreeRunning` struct to `AdcFifoBuilder` (since that's what it is) - Remove DMA related features from `Fifo`: it's not clear to me how a good API for this will look

When this is called, the resulting AdcFifo returns a u8 from it's `read` method.

jannic · 2023-06-07T06:01:36Z

I'll try to do the review soon but I'm quite busy atm so I can't promise much. May take a few days, sorry!

nilclass · 2023-06-21T11:58:10Z

@jannic did you get a chance to take a look? 🫣

Is there anything I can do to make things easier to review?

In case there is no capacity currently for this review, or there are general concerns about the API, I could look into putting this functionality in a separate crate for now. That way it can be used, without locking rp-hal into this particular approach. Wdyt?

jannic · 2023-06-21T13:35:46Z

@jannic did you get a chance to take a look? 🫣

Is there anything I can do to make things easier to review?

In case there is no capacity currently for this review, or there are general concerns about the API, I could look into putting this functionality in a separate crate for now. That way it can be used, without locking rp-hal into this particular approach. Wdyt?

Sorry, the delay was entirely caused by my lack of time, and not by anything wrong with your work. The code is nicely readable and any difficulty reviewing it is caused by the inherent complexity of the hardware.

I added a few minor remarks, but in general the pull request looks good. I won't worry too much about locking rp-hal into one specific API. After all, the top-level readme still states "These packages are under active development. As such, it is likely to remain volatile until a 1.0.0 release."

rp2040-hal/src/adc.rs

jannic · 2023-06-21T13:21:11Z

rp2040-hal/src/adc.rs

+    /// Manually set clock divider integral and fractional parts
+    ///
+    /// Default: 0, 0
+    pub fn clock_divider(self, int: u16, frac: u8) -> Self {


You could add an explanation to those parameters.

Also, in the examples (eg. https://github.com/rp-rs/rp-hal/pull/626/files#diff-54b5bbce718eaba891383cd18b03aa2b4533ef7b314c7584ec31bf880712a18bR114) it is difficult to understand why one needs such an odd divisor value.

Fair enough :)

Added a bit of explanation, and a few example values

"A bit"? That doc comment is great!

jannic · 2023-06-21T13:29:35Z

rp2040-hal/src/adc.rs

+    ///
+    /// In addition to being usable the same way as a `Fifo` returned from `start_fifo`,
+    /// the Fifo returned by this function can also be used as a source for DMA transfers.
+    pub fn start_fifo_with_dma(self, thresh: u8) -> Fifo<'a, true> {


You might have code that's only interested in the most recent measurement (ie. it doesn't matter if you skip some measurements). For example, with ADC in free-running mode, you could do such a thing:

loop { do_something_timing_critical(); if adc.read_single() > THRESHOLD { led.set_high().unwrap(); } else { led.set_low().unwrap(); } }

That way, the CPU doesn't need to wait 96 cycles for a new ADC value, but still always gets a (relatively) recent sample.

jannic

Both code and comments are of high quality. Very readable, and correct as far as I can tell. While I didn't have time to try it out on real hardware, I think it's ready to be merged.

Thanks, @nilclass, for this great contribution, and sorry again for the delay reviewing it.

(And I should have waited for the pipeline results. It would be nice if you could fix those warnings / errors. Some of the code comments could be marked as ignore or text, eg. the examples in AdcFifoBuilder, as they are obviously not meant to be working code.)

jannic · 2023-06-24T07:21:12Z

rp2040-hal/src/adc.rs

+    /// Manually set clock divider integral and fractional parts
+    ///
+    /// Default: 0, 0
+    pub fn clock_divider(self, int: u16, frac: u8) -> Self {


"A bit"? That doc comment is great!

nilclass · 2023-06-24T08:25:00Z

Thanks for the review @jannic, I'm happy to hear the changes are appreciated.

I believe the pipeline will be green now. I've just run the two examples one more time on real hardware, still working fine :)

nilclass mentioned this pull request May 31, 2023

WIP: ADC FIFO #326

Closed

jannic reviewed Jun 3, 2023

View reviewed changes

rp2040-hal/Cargo.toml Outdated Show resolved Hide resolved

jannic requested changes Jun 3, 2023

View reviewed changes

nilclass marked this pull request as ready for review June 6, 2023 09:57

nilclass changed the title ~~ADC: free-running mode experiment~~ ADC free-running mode & FIFO Jun 6, 2023

nilclass added 9 commits June 6, 2023 17:44

ADC: free-running mode experiments

63c761d

A bit of renaming; remove DMA related code

9a96526

- Rename `Fifo` struct to `AdcFifo` - Rename `FreeRunning` struct to `AdcFifoBuilder` (since that's what it is) - Remove DMA related features from `Fifo`: it's not clear to me how a good API for this will look

Add example: adc_fifo_poll

c45dac0

Remove unused "num-traits" dependency

a2541fe

doc fixes

2cf81e9

AdcFifo.stop: drain remaining values from FIFO, to prevent interrupts

0129221

Add example: adc_fifo_irq

85a5c4b

Add AdcFifoBuilder::shift_8bit method

89d50f9

When this is called, the resulting AdcFifo returns a u8 from it's `read` method.

Update examples after rebase

bc53742

nilclass force-pushed the free-running-adc branch from 97239a4 to bc53742 Compare June 6, 2023 16:03

nilclass requested a review from jannic June 6, 2023 16:03

jannic reviewed Jun 21, 2023

View reviewed changes

nilclass added 3 commits June 22, 2023 09:40

AdcFifo.stop: Wait for CS.READY before draining fifo

70ea42c

Document AdcFifoBuilder.clock_divider

0ed047c

Add read_single to AdcFifo

ba68bfe

nilclass requested a review from jannic June 22, 2023 10:18

jannic approved these changes Jun 24, 2023

View reviewed changes

nilclass added 3 commits June 24, 2023 09:50

Formatting & clippy

a272084

Adjust documentation, mark code blocks properly

708a511

use ignore for doc comments that can't compile

fe5023f

Clippy adjustments for examples

dc5901c

jannic merged commit 08f5e0f into rp-rs:main Jun 24, 2023

nilclass deleted the free-running-adc branch June 24, 2023 08:41

9names mentioned this pull request Jul 12, 2023

Add other ADC modes: free-running, FIFO #322

Closed

ADC free-running mode & FIFO #626

ADC free-running mode & FIFO #626

Uh oh!

Conversation

nilclass commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jannic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nilclass commented Jun 4, 2023

Uh oh!

nilclass commented Jun 6, 2023

Uh oh!

jannic commented Jun 7, 2023

Uh oh!

nilclass commented Jun 21, 2023

Uh oh!

jannic commented Jun 21, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilclass commented Jun 24, 2023

Uh oh!

Uh oh!

nilclass commented May 31, 2023 •

edited

Loading

jannic left a comment •

edited

Loading