periph: API change of macros ADC_LINE and DAC_LINE

[gschorcht]

Contribution description

This PR changes the definition of the ADC_LINE and DAC_LINE macros to make them more consistent with

• the description what they are,
• the definition of the GPIO_PIN macro, and
• the definition of devices and channels.

The need to change them came up during the development of the extension APIs for ADC and DAC extension API as required by issue #9582 and already realized for GPIO in PR #9860.

According to the documentation of ADC and DAC

An ADC line in this context is a tuple consisting out of a hardware ADC device (an ADC functional unit on the MCU) and an ADC channel connected to pin.
Similar to the ADC driver interface (ADC), the DAC interface uses the concept of lines, corresponds to a tuple of a DAC device and a DAC output channel.

a line refers to a channel y of device x. Thus, the *_LINE macros should be defined with two parameters x and y, the device and the channel. According to the default definition of GPIO_PIN, which uses the same concept, the definitions should look like:

#define ADC_LINE(x,y)        ((adc_t)((x & 0) | y))
#define DAC_LINE(x,y)        ((dac_t)((x & 0) | y))

The channels of the MCU, if there are, would then be device 0.

Changing the macros in this way would it make possible to define the ADC_EXT_LINE and DAC_EXT_LINE macros in the extension APIs in a consistent way with GPIO extension API where x refers the extension device and y the channel.

#define ADC_EXT_LINE(x,y)      ....
#define DAC_EXT_LINE(x,y)      ....

Testing procedure

The following test applications have to pass on any board that support ADC and DAC peripherals:

make -C tests/periph_adc BOARD=...
make -C tests/periph_dac BOARD=...
make -C tests/driver_mq3 BOARD=...
make -C tests/driver_io1_xplained BOARD=...

Issues/PRs references

#9582 Issue with proposal for peripheral extension API
#9860 and #9958 Implementation of GPIO extension API

[ZetaR60]

NAK as currently written.

I think this does not fit the intent of ADC_LINE(x) and DAC_LINE(x). The documentation is not clear, but I think that based on the usage in e.g. tests/periph_adc and tests/periph_dac that these macros are intended to map an ordinal to the MCU specific encoding of adc_t line or dac_t line (which is internally encoded as a tuple of device and pin). If there was an equivalent macro for periph/gpio it would the inverse function of GPIO_PIN(x,y).

The documentation for ADC_LINE(x) and DAC_LINE(x) definitely needs some improvement to clarify what it actually does. Also, I think that an equivalent of GPIO_PIN(x,y) for periph/adc and periph/dac could be added, just not in this way.

[gschorcht]

@ZetaR60

Also, I think that an equivalent of GPIO_PIN(x,y) for periph/adc and periph/dac could be added, just not in this way.

Any suggestion?

For me, the intent of the ADC_LINE and DAC_LINE macros become not really clear from the documentation. No CPU implementation overrides the default macro. All of them just use the x in ADC_LINE(x) as it is. In fact, x simply identifies the x-th's channel on first device, the MCU, how ever they are mapped to the ports and the pins internally by the implementation of periph/adc and periph/dac.

So why not to say, device 0 is the MCU and all other devices are extensions.

Maybe, @haukepetersen could clearify that.

[ZetaR60]

I think that another pair of macros could be added that are the inverse functions of ADC_LINE(x) and DAC_LINE(x), and they would take two arguments (nth internal device and mth line). Maybe something like ADC_LINE_INV(x,y), though there is probably a better name. The documentation should also be clarified. Then when adding stuff for the extension interface, we can add a pair of macros that are something like ADC_EXT_LINE_INV(x,y) that maps external devices and their lines to the adc_t and dac_t representation.

We should see what @haukepetersen has to say first though.

[haukepetersen]

Sorry for only seeing this now (don't have much time to spare for periph stuff lately)...

I don't like this change, as it defeats the 'line' concept: the idea is, to have from a users perspective only a set of logical lines. It is up to the driver implementation and board configuration, to map these lines onto ADC dev / channel tuples.

E.g. look at #7277 (a little outdated, but can't find any other example right now): here some different channels on different ADC devices are mapped onto logical lines like this:

static const adc_conf_t adc_config[] = {
     { .pin = GPIO_PIN(PORT_A,  3), .dev = ADC_1, .chan =  3 },
     { .pin = GPIO_PIN(PORT_C,  0), .dev = ADC_1, .chan = 10 },
     { .pin = GPIO_PIN(PORT_C,  3), .dev = ADC_1, .chan = 13 },
     { .pin = GPIO_PIN(PORT_F,  3), .dev = ADC_3, .chan =  9 },
     { .pin = GPIO_PIN(PORT_F,  5), .dev = ADC_3, .chan = 15 },
     { .pin = GPIO_PIN(PORT_F, 10), .dev = ADC_3, .chan =  8 }
 };

Remember, that this config is CPU specific to STM32 devices and it is up to the CPU to define a config format that makes sense...

In general I am pretty convinced that the way forward with the periph_adc driver is to leave it 'as is', but introduce another, more advanced and complex interface for support of many more features. Same is true e.g. for PWM (as in dma driver PWM), and possibly other periph drivers as well.

[gschorcht]

I don't like this change, as it defeats the 'line' concept: the idea is, to have from a users perspective only a set of logical lines. It is up to the driver implementation and board configuration, to map these lines onto ADC dev / channel tuples.

That's okay for me. The concept behind the was not really clear to me from the documentation. Therefore I were asking for clarification.

My intention to introduce ADC_LINE(x,y) was simply to have a consistent way of how channels are addressed when we have not only onboard ADC devices but also external ADC devices. The question arose in conjunction with the PR #10527 and PR #10512 when I was looking for a way to determine the total number of existing channels and to iterate over all these channels.

for (int i = 0; i < ADC_DEV_NUMOF; i++) {
   for (int j = 0; j < adc_channels(ADC_DEV(x)); j++) {
        adc_sample(ADC_LINE(x, y), res);
    }
}

ADC_LINE(x,y) would have helped a lot to integrate external ADC devices virtually to the board from the users perspective.

Without having ADC_LINE(x,y) the iteration over all ADC channels with extension API would look like the following:

/* first the onboard channels */
for (int i = 0; i < ADC_NUMOF; i++) {
    adc_sample(ADC_LINE(x), res);
}

/* and then the channels provided by external devices */
for (int i = 0; i < ADC_EXT_NUMOF; i++) {
   for (int j = 0; j < adc_channels(ADC_EXT_LINE(i,0)); j++) {
        adc_sample(ADC_EXT_LINE(i, j), res);
    }
}

The definition of a ADC_NUMOF macro over all channels (onboard and external) is nearly impossible.

[gschorcht]

There was a suggestion from @ZetaR60 for dealing with ADC_LINE and DAC_LINE macros for extension APIs in #9582 (comment) but I'm not really lucky with the FOO_EXT_LINE definition since it becomes quite complex and erro-prone for multiple ADC or DAC extension devices.

[gschorcht]

@haukepetersen

In general I am pretty convinced that the way forward with the periph_adc driver is to leave it 'as is'

One further question about the existing interface I have is: How should the ADC_RES_16BIT resolution be handled on a platform where int is only 16 bit and -1 indicates an error? Theoretically, on such a platform only 15 bit resolution is possible, right?

[ZetaR60]

@haukepetersen Just to be clear about what you said: Does ADC_LINE(x) and DAC_LINE(x) map an ordinal to a device+channel tuple?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[gschorcht]

This PR is closed in favor of PR #13247 which should allow to integrate external ADC devices. For DAC devices another solution is required.