Re: [riot-devel] ADC API resolution
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
On 1/8/20 3:51 PM, Marian Buschsieweke wrote:
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
On Wed, 8 Jan 2020 14:59:18 +0100
Koen Zandberg wrote:
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 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
Re: [riot-devel] ADC API resolution
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
On Wed, 8 Jan 2020 14:59:18 +0100
Koen Zandberg wrote:
> 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 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
Re: [riot-devel] ADC API resolution
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 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
Re: [riot-devel] ADC API resolution
Hi, thanks for your reply. On Tue, 7 Jan 2020 09:00:54 +0100 Hauke Petersen 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
[riot-devel] ADC API resolution
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 pgpl6bbrRXU73.pgp Description: OpenPGP digital signature ___ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel
