Skip to content

cpu/sam0_common/periph/dma: add ADC sampling via DMA - #22626

Open
fabian18 wants to merge 26 commits into
RIOT-OS:masterfrom
fabian18:pr/periph/adc_dma
Open

cpu/sam0_common/periph/dma: add ADC sampling via DMA#22626
fabian18 wants to merge 26 commits into
RIOT-OS:masterfrom
fabian18:pr/periph/adc_dma

Conversation

@fabian18

Copy link
Copy Markdown
Contributor

Contribution description

Implement ADC sampling via DMA, using circular DMA descriptors as in #22261.

Testing procedure

BOARD=same54-xpro make -C test/periph/adc_dma
The test application samples ADC at most possible speed for a given timespan.

Issues/PRs references

Declaration of AI-Tools / LLMs usage:

AI-Tools / LLMs that were used are:

  • Gemini added the DMAC_BTCTRL_BLOCKACT_INT

benpicco and others added 18 commits May 8, 2026 18:15
The `dac_set()` API says

> The value is always given as 16-bit value and is internally scaled to the
> actual resolution that the DAC unit provides (e.g. 12-bit).

We didn't do that scaling before - enable it even though it breaks existing
users.
@github-actions github-actions Bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: tests Area: tests and testing framework Area: build system Area: Build system Area: drivers Area: Device drivers Area: cpu Area: CPU/MCU ports labels Aug 26, 2026
@crasbe crasbe added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR AI: Co-Authored PR/Issue relies on AI. Documentation / Code was partly generated by AI. labels Aug 26, 2026
@crasbe

crasbe commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

This includes the previous two PRs, right?

@fabian18

Copy link
Copy Markdown
Contributor Author

does only depend on #22261

@crasbe crasbe added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 26, 2026
@riot-ci

riot-ci commented Aug 26, 2026

Copy link
Copy Markdown

Murdock results

FAILED

f7f6d3d fixup! test/periph/adc_dma: add test for ADC DMA based sampling

Success Failures Total Runtime
87 1 10268 01m:16s
Build failures (1)
Application Target Toolchain Runtime (s) Worker
tests/build_system/cpp_exclude samr21-xpro llvm 1.45 mobi6

Artifacts

Comment thread tests/periph/adc_dma/main.c Outdated

int main(void)
{
adc_continuous_begin(ADC_RES_12BIT);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to move this into adc_dma_setup()?

You already forgot the adc_continuous_stop() after adc_dma_release() - if those always go together, just reduce them to one function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adc_continuous_begin is handy because of _adc_configure which is in the non continuous variant tied to direct sampling. I would assume adc_continuous_begin/stop is used already by people, so using DMA is like on top of that. Before I call adc_continuous_, I would rather reuse _adc_configure(). I dont know/remember, why I did not do this. That way, there would be no dependency to adc_continuous.
But using both could be in conflict, because adc_continuous set up ADC already and the DMA could reconfigure it.

adc_dma_setup(adc_arg.adc, _adc_dma_cb, &adc_arg);
adc_dma_start(adc_arg.adc, _buf, adc_arg.avg_samples_numof);

xtimer_msleep(CONFIG_TEST_ADC_SLEEP_SEC * MS_PER_SEC);

@benpicco benpicco Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no way to get a notification / wait until the sample buffer is full?

@fabian18 fabian18 Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sample buffer (2048) is full every time when a DMA interrupt happens. The interrupt triggers an event to copy the samples to a separate buffer and process them. The second buffer exists so that sampling can continue while processing is ongoing. The xtimer_msleep was for me a cheap alternative to a shell command to stop sampling. The shell is flooded with DMA samples anyway.

@benpicco benpicco Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the DMA is always looping around?
Since you only have a single buffer, how do you know your memcpy is faster than the DMA already writing the next sample to the beginning again?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the DMA ISR, samples are copied form sampling to processing buffer, so there are 2 buffers. When processing was too slow the event would still be queued and "ADC: processing too slow\n" prints.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that DMA does not continue when I am in the ISR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapping buffers is favorable. By using properly, you mean calling dma_prepare from ISR to swap buffers?
I think still the BLOCKACT_BOTH is required, because the DMA could continue until buffers are swapped, so a few newer samples could still override the sampling buffer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would wrap this in adc_dma_continue(), calling dma_prepare and continuing execution from ISR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can chain the buffers together, so no additional call to dma_prepare() is needed.

It would still be good to also support single shot operation if one want to only get one buffer full of samples.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by chaining them together?

One shot or n-shot could be realized with calling adc_dma_stop instead of adc_dma_continue from ISR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With dma_append_dst() you can specify the next buffer that should be used when the first buffer is full.

If you then chain the last buffer to the first, you get a loop (this is what dma_enable_loop() does)

Comment thread cpu/sam0_common/periph/adc.c Outdated
@@ -477,3 +526,89 @@ int32_t adc_sample(adc_t line, adc_res_t res)

return val;
}

int adc_dma_setup(adc_t line, dma_cb_t cb, void *arg)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #20622 I could specify the sampling frequency - here we just use whatever was configured on the board level for all ADC channels?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes whatever is configured with ADC_GCLK_SRC and ADC_PRESCALER. Do I have to do it like in in your PR, or can it stay like this now?

/**
* @brief Update the DMA Completion callback context
*
* @param dma[in] DMA channel to release

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param dma[in] DMA channel to release
* @param[in] dma DMA channel to release

* @brief Update the DMA Completion callback context
*
* @param dma[in] DMA channel to release
* @param ctx[in] DMA complete callback context

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param ctx[in] DMA complete callback context
* @param[in] ctx DMA complete callback context

@kfessel

kfessel commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

be sure to adjust the PR description to match the actual amount of changes

  • does a makeover of the sam0/dma
  • removes dma_wait in for a callback based implementation
  • adds a DMA based DAC play
  • adjust DMA based SPI implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Co-Authored PR/Issue relies on AI. Documentation / Code was partly generated by AI. Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms State: waiting for other PR State: The PR requires another PR to be merged first Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants