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

https://github.com/RIOT-OS/RIOT/pull/13248 provides a proof of concept implementation of the ATmega ADC as well as a test application.

Proof that the API is feasible

• [x] PoC implementation of the basic API on an MCU internal (peripheral) ADC (see https://github.com/RIOT-OS/RIOT/pull/13248)
• [x] PoC implementation of the basic API on an external ADC (see https://github.com/RIOT-OS/RIOT/pull/10619)
• [ ] PoC implementation of utility features
  • [x] Access to internal thermistors (see https://github.com/RIOT-OS/RIOT/pull/13248)
  • [x] Using constant voltage reference as input (to allow measurement of the supply voltage or calibration of reference voltages) (see https://github.com/RIOT-OS/RIOT/pull/13248)
  • [ ] Accelerated burst access; e.g. performing conversions in background using DMA, or using ADC conversion completed interrupts
  • [x] "Entropy" ADC channel
• [x] Application using the API (the test application in https://github.com/RIOT-OS/RIOT/pull/13248)

Issues/PRs references

• https://github.com/RIOT-OS/RIOT/pull/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
• https://github.com/RIOT-OS/RIOT/pull/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]

[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: https://github.com/RIOT-OS/RIOT/pull/13248/commits/9500294be62a718497075eac53c9113e991e2aca

[gschorcht]

Did you forget to commit the application?

No, the commit is there: 9500294

Ah sorry, I didn't realize the different PR :sunglasses:

[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 ;-))

[gschorcht]

When trying to adapt the ads101x driver for adc_ng, the following additional questions regarding design decisions arose:

• There are a number of different arrays with the same number of elements

  const adc_ng_driver_t * const adc_ng_drivers[ADC_NG_NUMOF];
  extern void * const adc_ng_handles[ADC_NG_NUMOF];
  extern uint16_t adc_ng_refs[ADC_NG_NUMOF];
  extern uint8_t adc_ng_res[ADC_NG_NUMOF];
  

  where all together describe the parameters and the state of one device. What are the reasons that we don't have
  • a structure which describes completely one device consisting of a driver, a handle, the selected reference voltage and the selected resolution, for example adc_ng_dev_t
  • only one array of this structure extern adc_ng_dev_t adc_ng_devs[ADC_NG_NUMOF]?
• Why are res_supported, refs a.s.o. part of the driver? If there is a driver that can be used for different variants of an ADC, each instance of the driver contains the references to same functions.

[gschorcht]

@maribu BTW, I have ads101x driver running with adc_ng (for the moment only for single positive samples and with wrong conversion to voltage because resolution is 32 bit and the sample range goes from -32768 to 32767 with 12.288 V full scale range):

RIOT ADC NG driver test
=======================

Driver #0:
Resolutions supported: 16 bit
Supported references: 0.256 V, 0.512 V, 1.024 V, 2.048 V, 4.096 V, 6.144 V
Reference selectable as input: None
Accelerated burst mode: Module "adc_ng_burst" not used!
> init 0 0 16 5000
adc_ng_init(0, 0, 16, &5000)
Chosen reference voltage: 6.144V
> 
> single 0
adc_ng_single(0): Success
Sample: 10352
>
> volt 0
adc_ng_voltage(0, &value)
value = 0.970V

[gschorcht]

@maribu You might have a look on the ads101x test application in PR #10619.

[maribu]

TODO: Add (optional) access to conversion time.

[gschorcht]

@maribu Maybe you missed my comment https://github.com/RIOT-OS/RIOT/pull/13247#discussion_r379307616 between all the others. The following part still needs to be removed. It's now in adc_ng_global.h.

https://github.com/RIOT-OS/RIOT/blob/ff157d74d0bfb7a77d7026349b23e6a5a281f860/drivers/adc_ng/include/adc_ng_internal.h#L68-L78

[gschorcht]

@maribu There is a further general question. https://github.com/RIOT-OS/RIOT/blob/ff157d74d0bfb7a77d7026349b23e6a5a281f860/drivers/include/adc_ng.h#L261-L262 [Update]~This does not apply to ADS1x1x. These ADCs also allow negative values for single-ended inputs.~ Negative values are only possible for differential channels. They have 12- or 16-bit resolutions, the raw values are in the range ~-1024 ... 1023~ -2048 ... 2047 or -32768 ... 32767. There is no reference voltage in the strict sense, but they define a full scale range using a programmable gain amplifier, for example ±0.256 V, ±0.512 V, ..., ±6.144 V.

This brings me back to my initial question of how we handle this. [Update]~Would this be an 11- or 15-bit ADC with only differential channels even if they are used single-ended~ Would these 12- or 16-bit ADCs then be treated as 11- and 15-bit ADCs because their values for single-ended channels are in the range ~-1024 ... 1023~ -2048 ... 2047 or -32768 ... 32767?

[maribu]

They have 12- or 16-bit resolutions, the raw values are in the range -1024 ... 1023 or -32768 ... 32767.

Hmm, that's odd. A 12 bit signed integer should have the range -2048 ... 2047. A 16 bit integer indeed should have range of -32768 ... 32767.

So I'd say the first is 11 bit, the second is 16 bit.

[gschorcht]

Hmm, that's odd. A 12 bit signed integer should have the range -2048 ... 2047. A 16 bit integer indeed should have range of -32768 ... 32767.

Sorry, my fault. Of course the 12 bit ADC gives samples in the range of -2048 ... 2047.

So I'd say the first is 11 bit, the second is 16 bit.

The problem is that the values for single ended channels are in the range 0 ... 32767 which corresponds to a resolution of only 15 bit. In fact, if I define the ADCs with 15 bit resolution, the converted value from adc_ng_voltage is correct. So I would say, that these ADC have to be defined as 15 bit or 11 bit ADCs. This should be also correct for differential channels where we can also have negative values.

[kaspar030]

I once had a project that needed to sample multiple ADCs, in parallel, fast (IIRC with one MHz or higher), simultaneously, and synchronized. probably timer synced DMA would've been the technically correct solution, but it also needed to be done fast and portable.

plain adc_sample() was too slow, because it re-setup the adc line every time, and waited for the result before returning. I solved this by splitting the target's adc_sample() (it was an stm32) into something like adc_configure(), adc_start() and adc_read(). Then I called adc_configure() once at the beginning, then issued adc_start() for both ADCs, then called adc_read() for both. Before that, I could not remotely achieve the target freq. After, there was plenty of time left.

If I understand correctly, adc_ng already splits configuration and sampling. Would it maybe make sense to also split "start conversion" and "wait until conversion is finished, return result"? I remember for stm32, this was easily done by splitting the function in two, with the original function just calling both in sequence. I don't know if this can be done on all adc's...

[maribu]

If I understand correctly, adc_ng already splits configuration and sampling.

Yep. And for the proof of concept implementation (ATmega ADC) it resulted in a dramatic increase of sample rate. I assume it can be even better for ADCs that support DMA.

Would it maybe make sense to also split "start conversion" and "wait until conversion is finished, return result"?

Good idea. That would at least be possible to implement for all ADCs I have looked into (which is the vast number of cough three). The actual implementation could still choose to make the "start conversion" function a non-op. (Or with the current design of having a low level ADC driver struct, the function pointer could be NULL and the high level adc_ng_start_conversion() would check for that.) That way ADCs not allowing this could still comply with the API.

[kaspar030]

The actual implementation could still choose to make the "start conversion" function a non-op.

that makes total sense. code would still work, just maybe a bit slower.

[kfessel]

Deducing from its usage within the ATMega implementation this is still a "Analog Input/Analog Pin" abstraction (it is configuring pins) (I think it should be named like this). Since it is configuring pins, i think it should use the GPIO abstraction for this, but i also think this needs to be devided in an ADC and an Analog Input Part.

This is the most sophisticated implementation of the ATMega ADC featureset I have ever seen (deducing clock configuration and CPU stop mode from the resolution chosen, more than supporting niche features like measuring the voltage reference and the internal temperature sensor) while still being pretty generic.

But while this was developed especially the edge features got some problems since they may differ even within one line of MCU. E.g.: adc_ng_measure_ref takes for granted that there is a Vref (and therefor a value in the refarray) that can be Messured, but it is not the ATMegas measure the band gab and this is not always a Vref (see ATMega32U4), the band gap is from 1.0 to 1.2V I am not sure but there might be a configuration value available at least for the temperature sensor there are (in the EEPROM until someone deletes it (bad (but cheap) design choice by Atmel):) ) For the stm32l0 the band gap is not available as a Vref either (therefor the method will not work no value in the table of Vrefs), but is has the much more accurate value of the bandgap (at avcc of 3V) written into its ROM in production. So you can measure it and modify the value that is adc_ng_refs[MCUADCindex] which would make adc_ng_convert much more accurate but atm this will be overwritten as soon as the next adc_ng_init is called with some constant value.

What do i think a ADC Driver should do: -manage the access to the ADC (as we see there are multiple use-cases) and make the lock available to wait on from the outside -support configuration (the clock may be chosen either by the user or a automatic) -startsample on channel x (i would also like to chose the refrence at this point by configuration bits (a helper function to find the appropriate ones will be nice) (this should lock the ADC and is should only be unlocked if the sample is retrived or stored away by some mechanism for later retrieval (interrupt) -retrieve the sampled value from channel x -extra features can use the lock capabilities of the ADC

an Analog pin or input: -configure the pin -lock and configure and start adc -may convert sample value to mV (or temperature) (provide a conversion function)

[maribu]

E.g.: adc_ng_measure_ref takes for granted that there is a Vref (and therefor a value in the refarray) that can be Messured, but it is not the ATMegas measure the band gab and this is not always a Vref (see ATMega32U4)

The API allows to return -ENOTSUP, if this is not supported. (Are you sure about the ATMega32U4? I think I successfully tested this on an Arduino Leonardo.)

So you can measure it and modify the value that is adc_ng_refs[MCUADCindex] which would make adc_ng_convert much more accurate but atm this will be overwritten as soon as the next adc_ng_init is called with some constant value.

The API should expect the adc_ng_refs[] to be const, so that it can be places in ROM (on von-Neummann machines at least). But this does not force an implementation to allocate it in ROM. Instead, the driver could place it in RAM and initialize it with a value read from EEPROM. (I might have an implementation issue so that it does not work, which you might refer to. This is a bug I will fix. I will check this.)

[kfessel]

The API allows to return -ENOTSUP, if this is not supported. (Are you sure about the ATMega32U4? I think I successfully tested this on an Arduino Leonardo.)

according to the manual you can select a 2.56V Vref but messure the the Bandgap of that Reference which is 1.1V -> the alogorithm seems to work but will deliver wrong number since it assumes lowest reference is what is messurable (if i didn't misread) -there is an easy fix for that but i shows that the implementation is quite close to the ATmega feature-set.

The API should expect the adc_ng_refs[] to be const, so that it can be places in ROM (on von-Neummann machines at least). But this does not force an implementation to allocate it in ROM. Instead, the driver could place it in RAM and initialize it with a value read from EEPROM. (I might have an implementation issue so that it does not work, which you might refer to. This is a bug I will fix. I will check this.)

adc_ng_refs[] is not const, it is modified by adc_ng_init when you init your adc your adc_ng will not work if it would be.

if the pin features are moved up ( may be it need s a second driver for the pin configuration or just take gpio_init(Anlog) ) and the special features are removed this would be a great Analog Pin (Input)

[maribu]

alogorithm seems to work but will deliver wrong number since it assumes lowest reference is what is messurable (if i didn't misread)

See https://github.com/maribu/RIOT/blob/964f31bede183a30584586287763850d70a7eac7/drivers/adc_ng/include/adc_ng_internal.h#L250-L263

The index of the reference that can be selected as input has to be explicitly given by the driver. There is no underlying assumption that this has to be the lowest reference voltage (or that reference voltages can indeed be selected as input). This index can also be out of range of the actual reference voltages, if there is a reference voltage that can be used as input but not as reference. (The driver would obviously have to allocate memory for that additional entry. But the ADC NG API will not select this reference voltage as reference voltage, if it is out of range.)

In the implementation of the ATmega ADC I indeed always use the 1.1V as reference voltage. I just double checked in the datasheet of the ATmega32U4, and in Table 24-4 "Input Channel and Gain Selections" on page 314 it says that with ADMUX5..0 set to 0b011110 the 1.1V band gap reference is selected as input. So all correct.

adc_ng_refs[] is not const

You can cast any non-const pointer implicitly into a const pointer of the same type. The const qualifier only tells the compiler that the code should not modify its contents. If the actual array is allocated in read only memory, or in read/write memory, is up to the driver. The API will treat it as read only (as const) to allow for both. The ATmega platform will not profit from this, as even const memory is allocated in RAM on Harvard platforms - but von-Neumann platforms will.

if the pin features are moved up

The entities known to this API are ADC devices and channels. There are no pins in this API.

If an ADC channel is indeed tied to a single pin, an internal reference voltage, an internal NTC, an pair of pins as differential input, or whatever is from the API point of view irrelevant. However, there are mandatory conventions on what number to map to which type of channel: https://github.com/maribu/RIOT/blob/964f31bede183a30584586287763850d70a7eac7/drivers/adc_ng/include/adc_ng_internal.h#L35-L56

[kfessel]

alogorithm seems to work but will deliver wrong number since it assumes lowest reference is what is messurable (if i didn't misread)

See https://github.com/maribu/RIOT/blob/964f31bede183a30584586287763850d70a7eac7/drivers/adc_ng/include/adc_ng_internal.h#L250-L263

The index of the reference that can be selected as input has to be explicitly given by the driver. There is no underlying assumption that this has to be the lowest reference voltage (or that reference voltages can indeed be selected as input). This index can also be out of range of the actual reference voltages, if there is a reference voltage that can be used as input but not as reference. (The driver would obviously have to allocate memory for that additional entry. But the ADC NG API will not select this reference voltage as reference voltage, if it is out of range.)

ah i did not see that you can have a not a reference reference voltage levels (out of range) (the lowest voltage part is about the specific AVR implementation that you provided for adc-ng https://github.com/maribu/RIOT/blob/adc/cpu/atmega_common/periph/adc_ng.c#L208)

In the implementation of the ATmega ADC I indeed always use the 1.1V as reference voltage. I just double checked in the datasheet of the ATmega32U4, and in Table 24-4 "Input Channel and Gain Selections" on page 314 it says that with ADMUX5..0 set to 0b011110 the 1.1V band gap reference is selected as input. So all correct.

And than you select ref index 0 for all ATmega (L208) which is 2560 of ATM32U4 and 1500 for ATM..RF https://github.com/maribu/RIOT/blob/adc/cpu/atmega_common/periph/adc_ng.c#L149

so u need to provide out of range levels for these MCUs.

adc_ng_refs[] is not const

You can cast any non-const pointer implicitly into a const pointer of the same type. The const qualifier only tells the compiler that the code should not modify its contents. If the actual array is allocated in read only memory, or in read/write memory, is up to the driver. The API will treat it as read only (as const) to allow for both. The ATmega platform will not profit from this, as even const memory is allocated in RAM on Harvard platforms - but von-Neumann platforms will.

in https://github.com/maribu/RIOT/blob/adc/drivers/adc_ng/adc_ng.c#L34 it is not const in https://github.com/maribu/RIOT/blob/adc/drivers/adc_ng/adc_ng.c#L74 it is modified (therefore not handled like beeing const)

Since this is done no compilerer will put this array in program-memory even if you use just one ADC and one of its References. Yes, the AVR will not take advantage of refs being const but stm32 (arm) would.

Maybe you talk about driver.refs this is const (even though that provides no benefit for AVR)

even though AVR is Havard if there is an actual constant value it may be written into the program code and not be compitime alocated to RAM.

if the pin features are moved up

The entities known to this API are ADC devices and channels. There are no pins in this API.

If an ADC channel is indeed tied to a single pin, an internal reference voltage, an internal NTC, an pair of pins as differential input, or whatever is from the API point of view irrelevant. However, there are mandatory conventions on what number to map to which type of channel: https://github.com/maribu/RIOT/blob/964f31bede183a30584586287763850d70a7eac7/drivers/adc_ng/include/adc_ng_internal.h#L35-L56

In your example you map channels to pins and change their DDR (and pullup) there for its an abstraction of an analog pin https://github.com/maribu/RIOT/blob/adc/cpu/atmega_common/periph/adc_ng.c#L253 (there might bee reassons to keep th pull up, or the direction (not the default case but its possible)) and you also do this without letting the GPIO system know why not just tell it to change the pin configuration.

[maribu]

in https://github.com/maribu/RIOT/blob/adc/drivers/adc_ng/adc_ng.c#L34 it is not const in https://github.com/maribu/RIOT/blob/adc/drivers/adc_ng/adc_ng.c#L74 it is modified (therefore not handled like beeing const)

These contain the currently selected reference voltage of an ADC. Those change whenever a different reference voltage is selected. They are intentionally not marked as const.

The set of available reference voltages is marked as const from the API layer. The low level driver can allocate them in ROM on von-Neumann architectures. If the low level driver does e.g. read calibrated reference voltages from EEPROM instead, those could be allocated in RAM instead. This freedom of choice is intentionally and explicitly supported.

even though AVR is Havard if there is an actual constant value it may be written into the program code and not be compitime alocated to RAM.

The API wants to allow the low level driver to decide if the driver structs are allocated in RAM or ROM. This requires them to be in the same address space regardless of whether they are actually allocated in RAM or in ROM. For Harvard architectures, this is not possible.

Note that on boards with both peripheral ADCs and external ADCs connected, the drivers might actually have made different design decisions. Thus, both const and non-const reference voltage arrays have to coexist.

In your example you map channels to pins and change their DDR (and pullup) there for its an abstraction of an analog pin https://github.com/maribu/RIOT/blob/adc/cpu/atmega_common/periph/adc_ng.c#L253 (there might bee reassons to keep th pull up, or the direction (not the default case but its possible)) and you also do this without letting the GPIO system know why not just tell it to change the pin configuration.

I map some channels to pins, other channels are mapped to other stuff. From the API point of view, there are no pins. The pin mapping is just an implementation detail. And there is nothing that prevents someone to map e.g. channel 0-7 to pins PA0-PA7 without pull up resistsor, and channel 8-15 to pins PA0-PA7 with pull up resistor.

The advantage of the abstract channel entity is that it can represent any configuration, including differential mode, internally connected inputs, etc. This allows a lot of different features to become available within a single, consistent API.

E.g. let's assume I would use the ADC to measure the temperature and I have some boards with an internal NTC and some with an external NTC instead. I could use the very same code for both boards with this API, only the channel number might differ.

The alternative would be to split out GPIO configuration. This would require a user to perform two steps for initialization, of which the order might be relevant on some platforms, but irrelevant on others. (E.g. an ADC that is not 3.3V tolerant does not want to be connected to a GPIO in output mode that is currently set to high.) Additionally, a user would need to know which GPIO pin can be selected via which value of the ADC muxer. This is significantly more complex to use and introduces sources of bugs. And I'm not sure what the benefit of this is, as it provides no additional features compared to the current API.