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 On 08/01/2020 14.30, Hauke Petersen wrote: > 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 :-) > > 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 :-) > > Cheers (and thanks for tackling this!), > Hauke > > On 1/7/20 10:09 AM, Marian Buschsieweke wrote: >> 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 > > > _______________________________________________ > devel mailing list > devel@riot-os.org > https://lists.riot-os.org/mailman/listinfo/devel
signature.asc
Description: OpenPGP digital signature
_______________________________________________ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
