ADC API resolution

Development archive
1

Hi,

I just noticed that most of the ADC implementations providing their own adc_res_t do not cover all values. The API documentation states that adc_sample() should return -1 on unsupported resolutions. This indicates that all possible resolutions have to be defined in any case, so that a user could check at run time which resolutions are provided.

However: Wouldn't it make more sense to only provide the enum values actually supported? This would have two advantages:

1. Currently all places where adc_res_t is provided need to be updated when new    resolutions are added, resulting in some maintenance effort 2. Only having the resolutions defined that are actually supported would result    in compile time errors, which are much easier to spot and debug than run time    errors

Additionally, use cases where users needed to determine available resolutions could be address by e.g. defining HAVE_ADC_RES_10BIT when ADC_RES_10BIT is supported. And ADC_RES_MAX could be provided for the highest resolution enum and ADC_RES_MAX_BITS for the number of bits this has. This would allow to determine the resolution at compile time, resulting in less overhead in terms of both runtime and memory.

But: As currently the approach to detect available resolutions would result in compile time errors (when testing for resolutions not covered in the enum), maybe nobody actually needs this?

Kind regards, Marian

2

Hi,

keep in mind that just because an enum value is not defined, it does not prevent code like

adc_res_t res = 77;
adc_init(.., res);

Also, calling `adc_init(..., 1234)` is completely fine for the compiler...

Hence the two fold approach in the current implementations: each CPU should define only the ADC_RES_X values it actually supports and on top adc_sample() *should* return -1 on unsupported values to catch mishaps as stated above.

But I do like the idea of adding defines like `HAVE_ADC_RES_X` and `_MAX` (and `_MIN`).

Cheers, Hauke

3

Hi,

thanks for your reply.

Hi,

keep in mind that just because an enum value is not defined, it does not prevent code like

adc_res_t res = 77;
adc_init(.., res);

Also, calling `adc_init(..., 1234)` is completely fine for the compiler...

To me, this is a text book example of garbage in, garbage out. I personally don't expect someone to use random numbers out of the head of their mind instead of the enum constants and expecting things to just somehow magically work.

How about we add a precondition statement to the API documentation that only values provided via enum constants are valid input for the resolution?

But if we cannot assume a minimum level of common sense being applied to the users source code, why not at least use assert()? This way at least production code doesn't have to pay the overhead of checking for completely insane bugs.

(Don't get me wrong: I personally very much in favor of doing proper error handling even at the expense of some overhead. But adding overhead to check for completely crazy stuff seams not to be a good trade off to me.)

Hence the two fold approach in the current implementations: each CPU should define only the ADC_RES_X values it actually supports and on top adc_sample() *should* return -1 on unsupported values to catch mishaps as stated above.

Sadly, neither is it (consistently) implemented like this nor documented this way. But if there is an agreement that only the enum constants actually supported should be defined, I can open a PR to document this. But let's keep the discussion on how to handle it if users call adc_sample() with a resolution not provided by the enum going for while.

While I'm at it: How should adc_sample() behave if the line parameter is out of range? This is something that can easily happen, e.g. when compiling code written for one board for a different board. Again: I would say a precondition added to the doc and an assert() would be this best solution here.

I'd also like to add that the API is not safe to be called from IRQ context, as (at least some) implementations use a mutex to serialize access to the ADC.

(Btw: If we would replace the `ADC_LINE()` macro by a `static inline adc_t adc_line(unsinged num)` function, we could use `_Static_assert()` to generate compile time errors there as well. For backward compatibility an macro ADC_LINE() calling that function could be provided.)

But I do like the idea of adding defines like `HAVE_ADC_RES_X` and `_MAX` (and `_MIN`).

I'll open a PR for that. (I will also add the minimum resolution in that.)

Kind regards, Marian

4

Hi Marian,

I agree that users are unlikely to produce that garbage when passing value to the adc_sample function directly. More likely those kind of values are produced in cases where values are deducted programmatically via (potentially broken) code, or from broken indirect configuration (nested defines…). The main point is simply that the compiler does not catch this and we need to handle it somehow -> which we agree upon as it seems :slight_smile:

�So +1 in documenting the preconditions correctly, checking the line and res values using assertions and not returning any specific code. How about we change the API doc to something @return <0 for any internal error or similar, to still allow certain peripherals to signal an error and mark the result invalid.

While touching this API, would it make sense to also change the return type from int to int32_t? This would allow for 16-bit ADCs on 16- and 8-bit platforms… Just thinking loud here :slight_smile:

Cheers (and thanks for tackling this!), Hauke

5

Hi all,

A month or so back I added ADC peripherals bindings for the MicroPython fork. As this had to be generic across boards I also stumbled over a missing run-time indicator for which resolutions are supported. At that moment I worked around it by iterating over all possible ADC resolutions until a supported resolution was tried. Of course this is a bit suboptimal.

I’m very much in favor of some way to determine the available resolutions and min/max resolutions of an ADC. I was thinking of a function such as adc_res_t adc_res_max(adc_line_t line) to be able to support different ADC peripherals on a single image.

Just my 2 cents on this issue. I’m already glad that this is tackled.

Cheers, Koen

6

Hi,

I was thinking of a function such as `adc_res_t adc_res_max(adc_line_t line)` to be able to support different ADC peripherals on a single image.

good point! This could be provided with no run-time overhead in drivers/include/periph/adc.h e.g. via:

    #if !defined(HAVE_ADC_RES_MAX)     static inline adc_res_t adc_res-max(adc_line_t line) {         (void)line;         return ADC_RES_NUMOF - 1;     }     #endif /* HAVE_ADC_RES_MAX */

And overwritten as needed. This would also make retro-fitting external ADC drivers to that API easier.

Kind regards, Marian

7

Hej,

@Koen I like that idea! However, we must be careful to differentiate between the actual bit-width (6,8,10,…) of the ADC and the value assigned to the enum members of adc_res_t. While a user might be interested in the former for result shaping etc, the latter can have any arbitrary values, mostly concerning bit positions in some register. So we might need some lookup table or similar to translate from the enum members to the actual bit-width values…

Cheers, Hauke