[PoC] drivers/adc_ng: Common ADC API and implementation by maribu · Pull Request #13248 · RIOT-OS/RIOT

maribu · 2020-01-31T12:00:07Z

Contribution description

This PR depends on and includes #13247. It adds:

An implementation of the existing ADC periph API on top of the new, so that new ADC-NG drivers can be used with existing code. (This also allows to use external ADCs with the existing API.)
An ADC NG driver for the ATmega ADC
A test application

Testing procedure

The test application can be used with an ATmega powered board. It drops you in a shell that allows you to test all of the functions of the ADC NG API exposed as separate shell commands. (Currently almost 3KiB RAM are used due to constant data being placed in RAM on AVR. Using PROGMEM could improve the situation to allow it running e.g. on the Arduino UNO; but that is out of scope.)

Issues/PRs references

#13247

gschorcht · 2020-02-12T12:57:55Z

@maribu Commit 2acba62 does not belong to this PR, right?

maribu · 2020-02-12T13:29:48Z

@maribu Commit 2acba62 does not belong to this PR, right?

After I rebase against current master, it will no longer be needed here :-)

gschorcht · 2020-02-13T07:05:58Z

cpu/atmega_common/Makefile.features

 FEATURES_PROVIDED += arch_8bit
 FEATURES_PROVIDED += arch_avr8
 FEATURES_PROVIDED += atmega_pcint0
+FEATURES_PROVIDED += periph_adc_ng


Should be defined in board definitions. Otherwise, a board is not able to use only an external ADC if the feature is provided by the MCU because of the following condition in adc_ng.c:

RIOT/drivers/adc_ng/adc_ng.c

Line 24 in 4661b92

#if (ADC_NG_NUMOF == 1) && defined(MODULE_PERIPH_ADC_NG)

See also #13247 (comment).

Features and modules are conceptional not the same, even though for most (all?) features with periph_% a module with the same name exists and is automatically used, if the corresponding feature is used.

So: Just because the feature is provided, it is not automatically used. The FEATURES_OPTIONAL += periph_adc_ng in drivers/Makefile.dep that is done when adc_ng is used would indeed pull that feature (and therefore) the corresponding module in. But that could be prevented by FEATURES_BLACKLIST += periph_adc_ng.

I'm not sure if this is super elegant. Maybe it would be better to provide an adc_ng_default pseudo-module that would pull in all ADC-NG backends supported by a board.

tests/adc_ng/main.c

maribu · 2020-02-13T21:46:14Z

@maribu Commit 2acba62 does not belong to this PR, right?

After I rebase against current master, it will no longer be needed here :-)

I did the rebase, so that commit is no longer part of this PR.

maribu · 2020-02-13T21:47:48Z

The PoC was also updated to reflect the current state of the API. But I didn't manage to apply all feedback yet; I will do so tomorrow (hopefully)

stale · 2020-08-21T20:30:53Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

Enoch247 · 2022-01-24T17:22:18Z

One thing I noticed is that there is no provision to sample more than one channel per ADC without re-initializing it. The channel is selected in adc_ng_init(...) rather than as a parameter in the ADC reading functions. Alternatively, a channel struct could be added with a handle back to the ADC peripheral.

maribu · 2022-01-24T18:48:39Z

That's a valid point. There could be a footgun in that the ADC might not be able to switch between channels for a given configuration it is in. E.g. many ADCs have a channel that is wired to an internal NTC for temperature measurements. I read at least in one datasheet that stated one "must use" a particular reference voltage for it. Likely the result of none-compliance are just a high level of noise or so, but I'm not sure.

I guess a driver could have assert()s to ensure no channel that is invalid in the context of the current configuration is selected. I hope I can find some time to work on this PR this week. I will likely just drop the adc_ng_burst() function and let a better interface that allows for truly continuous sampling be added in a subsequent PR - possibly by someone else.

maribu · 2022-01-25T09:14:26Z

I spend some time thinking about this. I do not want to drop the channel argument from the initialization function. Some ADCs enable the ADC MUX when they are powered up no matter what, and some ADC MUXs can only select a valid input. In this case, the driver would have to blindly guess what channel to connect to. But not every channel is compatible with all settings, so this "default channel selection" can be a bit involved. If however the caller has to directly pass the channel it want to get the first sample from as argument, things are easy for the driver.

I also don't like adding a channel argument to the sample function. I would like to keep the function to retrieve a sample as trivial as possible. For ADCs that do not support DMA (or drivers that have not implemented it) requesting samples in a busy loop is the only option to get samples as fast as possible. Keeping this function trivial can increase the sample rate a bit. The sample rate will be particularly affected if assert()s are added to enforce only valid configurations - which I believe is the right thing to do.

How about this instead: Adding a dedicated function to swap the channel while keeping the ADC running? This should still be faster than re-initializing the ADC when a channel needs to be changed, but we can afford to pay for an assert() here, as often the level of noise is higher directly after switching the ADC channel and one often want to wait for that to stabilize again anyway.

Enoch247 · 2022-03-17T01:27:48Z

How about this instead: Adding a dedicated function to swap the channel while keeping the ADC running? This should still be faster than re-initializing the ADC when a channel needs to be changed, but we can afford to pay for an assert() here, as often the level of noise is higher directly after switching the ADC channel and one often want to wait for that to stabilize again anyway.

I think this could be a good idea. I was playing around with the SPI driver and noticed it has a use pattern where you must call spi_init to initialize the driver and then call spi_acquire and spi_release anytime you wish to use the bus. The bus speed is passed into the acquire function since a bus might use different speeds for each device on the bus. What do you think about doing something similar with the ADC?

This API aims to provided an alternative to the periph ADC API with the following improvements: - Support for multiple ADC devices (both external and peripheral ADCs) - Support for selecting the reference voltage - Convenience functions to convert into meaningful physical units It is written to allow coexistence with the existing periph ADC API.

Implemented the periph/adc API on top of the adc_ng API. This allows application using the periph ADC API to also use external ADCs.

Otherwise waiting for IRQs during periph_init() results in a deadlock.

Teufelchen1 · 2024-03-18T13:39:56Z

Can this be a draft?

maribu added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet State: waiting for other PR State: The PR requires another PR to be merged first Area: drivers Area: Device drivers labels Jan 31, 2020

maribu mentioned this pull request Jan 31, 2020

drivers/adc_ng: Added common ADC API #13247

Draft

8 tasks

maribu force-pushed the adc branch 2 times, most recently from 4b40748 to f046143 Compare February 6, 2020 21:01

gschorcht reviewed Feb 13, 2020

View reviewed changes

tests/adc_ng/main.c Outdated Show resolved Hide resolved

tests/adc_ng/main.c Outdated Show resolved Hide resolved

tests/adc_ng/main.c Outdated Show resolved Hide resolved

tests/adc_ng/main.c Outdated Show resolved Hide resolved

tests/adc_ng/main.c Outdated Show resolved Hide resolved

gschorcht reviewed Feb 13, 2020

View reviewed changes

tests/adc_ng/main.c Outdated Show resolved Hide resolved

maribu force-pushed the adc branch from f046143 to 546eaef Compare February 13, 2020 21:14

maribu force-pushed the adc branch from 579e8c5 to 1a8e803 Compare February 15, 2020 22:06

gschorcht mentioned this pull request Jun 25, 2020

periph/dac: support for DAC extension API #10512

Closed

maribu mentioned this pull request Jul 6, 2020

sys/entropy_source: add new module for entropy sources #14324

Merged

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 21, 2020

maribu added the State: don't stale State: Tell state-bot to ignore this issue label Aug 21, 2020

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 21, 2020

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

maribu added 3 commits April 26, 2022 22:03

drivers/periph/adc: Add compat layer

9946bc1

Implemented the periph/adc API on top of the adc_ng API. This allows application using the periph ADC API to also use external ADCs.

tests/adc_ng: Add test for ADC NG API

2109e6c

maribu added 4 commits April 26, 2022 22:04

tests/adc_ng_ntc: Small test for ADC-NG NTC driver

d79aa15

tests/adc_ng_entropy: Small test for the ADC-NG entropy collection

44b6db7

cpu/avr8_common: Enable IRQs prior to periph_init()

c883df0

Otherwise waiting for IRQs during periph_init() results in a deadlock.

cpu/atmega_common: Add periph_adc_ng

9aadca5

maribu force-pushed the adc branch from cde74ed to 9aadca5 Compare April 26, 2022 20:10

maribu requested review from MichelRottleuthner, MrKevinWeiss, aabadie, fjmolinas, kYc0o, leandrolanzieri and smlng as code owners April 26, 2022 20:10

github-actions bot added Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: tests Area: tests and testing framework Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Apr 26, 2022

Conversation

maribu commented Jan 31, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

gschorcht commented Feb 12, 2020

Uh oh!

maribu commented Feb 12, 2020

Uh oh!

gschorcht Feb 13, 2020 • 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!

maribu Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Feb 13, 2020

Uh oh!

maribu commented Feb 13, 2020

Uh oh!

stale bot commented Aug 21, 2020

Uh oh!

Enoch247 commented Jan 24, 2022

Uh oh!

maribu commented Jan 24, 2022

Uh oh!

maribu commented Jan 25, 2022

Uh oh!

Enoch247 commented Mar 17, 2022

Uh oh!

Teufelchen1 commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gschorcht Feb 13, 2020 •

edited

Loading