drivers/adc_ng: Added common ADC API

[maribu]

Contribution description

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
• Support for burst ADC conversions (e.g. via DMA)
• Convenience functions to convert into meaningful physical units

It is written to allow coexistence with the existing periph ADC API.

Design decisions

• Do not use a single "ADC line" identifier, but an "ADC device + ADC channel" identifier
  • This maps very naturally and intuitively with to the use of multiple ADCs
• Explicit initialization and de-initialization of the ADCs, rather than implicitly turning the ADC on and off for each call as done in the periph ADC API in adc_sample()
  • This is more efficient when performing multiple conversions in fast succession
  • This is more extensible: The actual implementation of triggering a conversion and retrieving the result is expected to be independent of the configuration. Adding an alternative init function (e.g. for differential mode to measure the difference of two inputs) would be easy to add, and the existing API could still be used to trigger a conversion and retrieve the result
• Add an internal API with an adc_ng_driver_t struct containing function pointers
  • This design has proven to work fine in the context of network drivers
  • This allows to only implement the actual hardware dependent stuff when adding drivers

Testing procedure

#13248 provides a proof of concept implementation of the ATmega ADC as well as a test application.

Proof that the API is feasible

• PoC implementation of the basic API on an MCU internal (peripheral) ADC (see [PoC] drivers/adc_ng: Common ADC API and implementation #13248)
• PoC implementation of the basic API on an external ADC (see drivers/ads101x: Changes for Common ADC API #10619)
• PoC implementation of utility features
  • Access to internal thermistors (see [PoC] drivers/adc_ng: Common ADC API and implementation #13248)
  • Using constant voltage reference as input (to allow measurement of the supply voltage or calibration of reference voltages) (see [PoC] drivers/adc_ng: Common ADC API and implementation #13248)
  • Accelerated burst access; e.g. performing conversions in background using DMA, or using ADC conversion completed interrupts
  • "Entropy" ADC channel
• Application using the API (the test application in [PoC] drivers/adc_ng: Common ADC API and implementation #13248)

Issues/PRs references

• periph/adc: support for ADC extension API  #10527 tries to extend the existing periph ADC API to also support external ADCs. This PR instead provides a completely new API to address other limitations of the existing API (choice of reference, "burst" readings) as well
• [POC/RFC/WIP] periph/gpio: Proposal for the change of the GPIO API #12877 has similar goals but targets GPIO access rather than ADC access

[maribu]

@bergzand: Could you verify that the issue of using the ADC API from MicroPython without knowing the resolution supported by the hardware in use in advance is possible with this API? E.g. I expect adc_ng_quick() to be usable without any background knowledge knowing hardware details in advance.

---

Update: Rephrased the poor choice of words in the last sentence.

[maribu]

@gschorcht: You're interested in provided access to both external and internal ADCs using a common API. Could you have a look if this API would fit your goals?

[maribu]

@benpicco: As you're currently working on the ADC driver of the LPC2387, maybe you might be interested in this as well?

[gschorcht]

It is written to allow coexistence with the existing periph ADC API.

Do you plan to replace the legacy API in future or will it coexist forever?

• PoC implementation of the basic API on an external ADC

I could offer to modify PR #10619 for the new API as proof for external ADCs.

[maribu]

Do you plan to replace the legacy API in future or will it coexist forever?

At least until the proposed API here has matured and proven, it is nice to have stable API as alternative to point users to. When this point it reached, a well informed discussion of the pros and cons of keeping the existing API can be done.

There is at least one advantage of the old API: By exposing fewer features and not having a adc_ng_driver_t or similar lying around, it will be more resource efficient. But by how much is hard to see until a couple of actual implementations are provided.

[maribu]

I could offer to modify PR #10619 for the new API as proof for external ADCs.

Sounds good, thanks!

I suggest to wait a bit first, so that I can address the first round of feedback first. It would be a pity if you have to constantly rebase and adapt to whatever changes are made to this API based on the reviews.

[gschorcht]

Suggested change

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

could be used instead since an undefined macro yields 0 in expressions by definition. It's just a comment, no change request.

[maribu]

Sure? It wasn't like this a month ago. There was a heated discussion for adding IS_USED(MODULE_FOO) to get 0 in case either MODULE_FOO is 0 or not defined.

[kaspar030]

Sure? It wasn't like this a month ago. There was a heated discussion for adding IS_USED(MODULE_FOO) to get 0 in case either MODULE_FOO is 0 or not defined.

yeah, I figured that out just during the mentioned discussion. An undefined macro resolves to "0" only in preprocessor expressions. With IS_USED() it can be used in C expressions.

[gschorcht]

An undefined macro resolves to "0" only in preprocessor expressions.

Exactly. So if there are no design rules against it (???), #if MODULE_FOO could be used especially in long expressions to keep them shorter and avoid cascades of #if defined() && !defined() && defined().

[gschorcht]

[x] Application using the API (the test application in #13248)

Did you forget to commit the application?

[maribu]

Did you forget to commit the application?

No, the commit is there: 9500294

[gschorcht]

Did you forget to commit the application?

No, the commit is there: 9500294

Ah sorry, I didn't realize the different PR 😎

[bergzand]

Thanks for picking this up.

First thing I noticed is that you're using uint8_t for most selectors. Is there a reason for not typedeffing something like an adc_device_t and an adc_line_t for these? If only to not have developers assume that they are plain numbers but should be treated as opaque identifiers.

@bergzand: Could you verify that the issue of using the ADC API from MicroPython without knowing the resolution supported by the hardware in use in advance is possible with this API? E.g. I expect adc_ng_quick() to be usable without any background knowledge knowing hardware details in advance.

Taking a quick look, I think the glue will be fairly simple. Only issue I see is that MicroPython expects a single number as ADC line (so without the adc dev). An initial implementation might assume that Micropython will always use the internal ADC lines or maybe a conversion function is possible to convert a single number into a combination of device and ADC line.

[maribu]

First thing I noticed is that you're using uint8_t for most selectors. Is there a reason for not typedeffing something like an adc_device_t and an adc_line_t for these? If only to not have developers assume that they are plain numbers but should be treated as opaque identifiers.

Well, actually this is the reason for this: Those are plain numbers and should be treated like that. Unlike e.g. spi_t etc. where we can have an SPI_DEV(x) macro that converts this number into an internal representation (e.g. to an base address or whatever), this is no longer possible here: There would have to be one ADC_DEV_<DRIVER_FOO>(x) macro per driver needed to perform the conversion. As a result the user would have to know which specific driver is used where. But the goal of the API is to be fully transparent in regard to the actual driver. So people implementing the drivers should be aware that those identifiers are to be treated as plain numbers.

[maribu]

I just did some major changes to the proposed API:

• split of the utility functions into a submodule
• changed the entropy interface, as it has proven to be quite useless in the original definition during implementing this
  • Now the ADC driver can provide less bits containing entropy than the resolution specified. (E.g. the current ATmega ADC implementation can only provide 1 bit containing only weak entropy. It turned out that poor measurement quality (which is quite easy to achieve) is hard to turn into actually usable entropy; at least when performing many conversions in fast succession

As this major changes invalidated any reviews anyway, I squashed and rebased to fix a merge conflict.

[maribu]

And updated some leftovers in the doc. (I stop squashing now ;-))

[maribu]

I hope I can allocate some time for this this week. But I guess the realistic path forward is to mark this API as unstable and get it upstream with only one implementation (the ATmega one already implemented). Likely, the API will have to be adapted to meet the requirements of various ADCs well. But I guess when marked as unstable, there is not that much harm in letting in mature upstream.

Note that @gschorcht has ported an external ADC driver to this new API, though. Once that is rebased, there would be two implementations of this API :)

[maribu]

This received some love:

1. I migrated to use XFA, so that multiple backends can easily be integrated
2. I dropped the burst() function.
   • IMO an API to expose DMA based sampling should allow continuous sampling via a ring buffer and should be added later on with an example implementation.
3. I also included the ATmega example implementation as well as a couple of test apps here, so that one can actually test the code.

The requested feature to change the ADC MUX config without re-initializing the ADC is not yet provided. At least in the case of the ATmega implementation, there would be really few CPU cycles be gained by this, especially since the sample directly after changing the MUX config should be thrown away anyway due to high noise. I guess it might make sense to add this later on, combined with a PoC implementation that shows that there is actually something to gain. At least of the ATmega backend it is likely not worth the ROM.

[maribu]

@kfessel convinced me that adding a way to change the mux settings while the ADC is up an running is a sensible thing to do. I'll extend the API accordingly.

[jue89]

Nice one! I'm still missing burst conversions - but as you mentioned - that could be a topic for a follow-up.

Some points that make me wonder:

• Why do you implement temperature measurement into this driver? I'd rather implement a dedicated ntc module that makes use of the adc_ng (or maybe also periph_adc as a fallback) driver. I'd rather stick with raw ADC values and voltages being the output of this driver.
• Why do you implement entropy collection into this driver? We already have the entropy_source and adc_noise driver.
• adc_ng_mux() / adc_ng_select_res() / adc_ng_single() looks racy. Especially the muxing will result into multiple threads accessing the ADC. I'd rather go the route of filling a configuration struct which is then given to adc_ng_single(). This method then makes sure that it carries out the sampling atomically.

Some pseudo code:

adc_ng_req_t request = ADC_NG_SAMPLE_SINGLE(0); /* 0 is the ADC backend */
adc_ng_req_channel(&request, 1);
adc_ng_req_resolution(&request, 16); /* 16 bit, might return an errno if the requested resolution can't be acchieved */
adc_ng_req_reference_mV(&request, 2500); /* tries to find the best reference */
adc_ng_req_reference_VDD(&request); /* compares to (A)VDD, might call adc_ng_req_reference_mV(&request, 0) internally?  */
int sample_raw;
adc_ng_sample(&request, &sample_raw); /* working with pointers allows to hand over arrays for burst conversions */
int sample_mV;
adc_ng_convert_mV(&request, &sample_raw, &sample_mV);
/* `request` can be used for further adc_ng_sample calls - it might be const */

For conversions adc_ng_sample() will block until it's finished the job.
It might be useful to have a adc_ng_sample_async(&request, &sample_raw) and provide a callback thru adc_ng_req_t. In this case the user must make sure that sample_raw stays available until the given callback is called.

Sorry for the bike shedding this late in the review process 🙈

[benpicco]

What resolution will the sample have?

[maribu]

This function is intended to be lightweight and does not support any configuration of the ADC. It depends on a call to the init() function to be issued first, which configures and enables the ADC. A call to off() is needed after one is done with sampling the ADC.

It would be e.g. totally valid to implement the ADC to just constantly sample and atomically write the sample into a buffer (e.g. via DMA). And this would atomically read form this buffer to fetch the most recent sample. To allow this level of optimization for speed, we cannot allow switching the resolution anymore here.

Note that many ADCs have something along the line of "if you change the resolution, reference,e etc. make sure to throw away the first three samples". So any chance in configuration should ideally result in three ADC samples to be taken and tossed away first. So this really is not only about shaving of CPU overhead, but also ADC stabilization overhead.

For ATmega, this is mutliple orders of magnitude faster than the old ADC API but not having to wait for the ADC to get only and stable before the next sample is taken when sampling as fast as possible.

I eventually hope to flash an STM32F1 based DSO (I think this one to be similar to the one I bought years ago) with RIOT. But for that I need fast samples :)

[maribu]

• Why do you implement temperature measurement into this driver? I'd rather implement a dedicated ntc module that makes use of the adc_ng (or maybe also periph_adc as a fallback) driver. I'd rather stick with raw ADC values and voltages being the output of this driver.

Note that the architecture here is fundamentally different to the other periph drivers. The drivers consists of a single upper part, and the multiple lower part implementations. The lower part needs to be written for every different hardware ADC and is a low-level hardware abstraction only, which is not intended to be used by the "typical" application developer. The upper part does the impedance matching between an API that adds little to what the bare hardware provides (hence, it is easy to implement for a driver developer) and an API that is easy to use for an end user.

All the interesting NTC stuff is implemented in a single place (drivers/adc_ng/util.c) as part of the ADC NG utility functions. This is NOT to be implemented by each and every ADC NG driver. Other than naming of the module, this basically is separate from the ADC NG API.

There is one exception, though: The NTC parameters of an ADC internal NTC need to be provided by the the ADC driver as a struct with two parameters. As many hardware ADC implementations just toss in an NTC anyway, it is convenient to have it here: We only need to provide the NTC parameters for each different ADC hardware and all boards using the same ADC hardware will have them readily available.

Also note: The NTC utility function works fine with a board or application defined adc_ng_ntc_t, e.g. for when an external NTC is connected to the ADC.

[benpicco]

Let's get the ball rolling on this again!

[benpicco]

I really don't like this auto-magic.
Usually the hardware designer has thought of a specific supported reference voltage that is to be used with an ADC.
In the case of external voltage reference, there is no way the driver could know that value.
IMHO this should be part of periph_conf.h

[maribu]

Could you provide more details on how this would look? Especially for the use case of two internal ADCs with a different set of reference voltages and one external with a third, distinct set of reference voltages.

[benpicco]

I would imaging the reference voltage to be a property of the ADC, so you'd have an ADC config struct in periph_conf.h

static const adc_conf_t adc_conf[] = {
    {
        .dev  = ADC0,
        .gclk = SAM0_GCLK_8MHZ,
        .ref  = ADC_REFCTRL_REFSEL_INTVCC1,
    },
};

[kfessel]

adc_common:
typedef struct adc_ref { 
int32_t voltage_mv; 
union{uint32 refconfv, void* refconfs} internal;
} adc_ref_t; 

MCU_adc:
const struct MCU_adc_dev {
 adc_ref_t references[NUMBER_OF_REFS];
} mcu_adc;

int adc_ng_init(uint8_t adc, uint8_t chan, uint8_t res, const adc_ref_t *ref)

Initialisation will will be made with a pointer to the ref you want to use

but i would actually go with

adc_init( *adc_dev, *adc_config)

the adc_config can be const and device specific and be prepared for the board / application.

and adc_mux could just get the same config.

[maribu]

We can have just an "index" to identity a reference voltage. But we still need to add something to allow users to find what is the most suitable index. Sometimes looking it up for a specific ADC driver would be enough, but if the app is intended to compile and work on multiple boards whe need some abstraction.

Also, just exposing an array of the reference voltage values per ADC isn't sufficient. E.g. for the supply voltage as reference this may depend on the battary charge left ans the temperature, or the Seeduino UNI can run from 5V or 3.3V depending on a sliding button. The driver mostly can figure out the reference voltage at run time (e.g. by selecting the reference voltage of unkown value as reference and a stable internal reference voltage as input).

I don't mind having the fancy reference selection split out. But I really want to be able to write a "multimeter app" that works on any board with an ADC and a display supported by RIOT.

[benpicco]

But that's the thing, the reference voltage is usually not something you'd chose at run-time. ADC pins are usually hardwired to a certain functionality, so we can as well specify their voltage reference at the board level and don't have to make it part of the API.

[maribu]

For the multimeter or the DSO use cases the run-time selection is crucial.

It can also be helpful for reading analog sensor. By swichting to a lower reference voltage, it is possible to pick up smaller changes. But going for a larger reference voltage, high sensor outputs won't oversample.

[benpicco]

What format is result?
e.g. with a 12 bit ADC, will the max value be 4095 or 4294967295?

[kfessel]

anything but left alignment wouldn't make sense :)

so max value == int32 max and min value == int32 min, if not the value would depend on the chose resolution.

[kfessel]

but why is the return value int wont this get us into trouble for avr (int == int16)

[maribu]

This intents to also work for external ADCs, e.g. 24 bit ADCs on 8 bit MCUs.

[kfessel]

oops i thought return is the returned value, but the return value is just an error code

[kfessel]

does not sound quick but simple or easy to me

[maribu]

indeed, those names are better. Any preference?

[kfessel]

not really (just a little in favor of simple) or all_in_one

[kfessel]

i am also in favor of return values being return values and not error codes