Hi,
thanks for your reply.
On Tue, 7 Jan 2020 09:00:54 +0100
Hauke Petersen <hauke.peter...@fu-berlin.de> wrote:
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
Cheers,
Hauke
On 12/26/19 10:01 AM, Marian Buschsieweke wrote:
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
_______________________________________________
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel
