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
pgpsxrMuHhtVc.pgp
Description: OpenPGP digital signature
_______________________________________________ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
