Bug: analogRead() for Arduino Due should disable previous ADC channel before enabling new one

[shahokun]

On the Arduino Due (Arduino library 1.6.1), if you read several different analog pins in succession using analogRead(), some of the values may be off. For example, I had temperature sensors A and B, which were reading approximately the same value. When I added an analogRead() of another channel prior to reading tempA, the reading of tempA would drop a significant amount (~50 ADC counts). By alternating between the two sequences every second, I was able to confirm this behavior:

analogRead(tempA);
analogRead(tempB);
// Both readings are approximately the same.

analogRead(chX);
analogRead(tempA);
analogRead(tempB);
// The reading for tempA is significantly lower

I tracked this down to lines 150-156 in wiring_analog.c:

// Enable the corresponding channel
if (ulChannel != latestSelectedChannel) {
    adc_enable_channel( ADC, ulChannel );
    if ( latestSelectedChannel != (uint32_t)-1 )
        adc_disable_channel( ADC, latestSelectedChannel );
    latestSelectedChannel = ulChannel;
}

If analogRead specifies a new channel, the new channel is first enabled, then the previous channel is disabled. Presumably this could have some undesirable effects with the ADC multiplexer, such as extra current draw or some switching noise (this may also be board layout or timing dependent). However, if you switch the order in code, i.e. disable the previous channel, then enable the new channel, the ADC readings are consistent every time.

// Enable the corresponding channel
if (ulChannel != latestSelectedChannel) {
    if ( latestSelectedChannel != (uint32_t)-1 )
        adc_disable_channel( ADC, latestSelectedChannel );
    adc_enable_channel( ADC, ulChannel );

    latestSelectedChannel = ulChannel;
}

I think it would be logical to disable the previous ADC channel prior to enabling the next one, and could seemingly prevent multiple channel read errors such as this one.

[maxlazarus]

I just had an issue with this as well using an Atmega2560 Arduino, would appreciate this fix.

[facchinm]

Could you check if the same misbehaviour happens after applying #2823?
It's a Due-only patch that was never merged for its slight performance drop on single channel reads (see https://groups.google.com/a/arduino.cc/forum/#!topic/developers/IdhTIT0V5GY for extended considerations)
If it solves the issue consider leaving a comment on the PR to speed-up its inclusion 😉

[shahokun]

The same issue applies after applying #2823. I believe that this particular issue has to do with the order of actions: instead of enabling the ADC channel -> disable the previous ADC channel, disable the previous ADC channel -> enable the desired ADC channel. This could be done on top of patch #2823 or independent of it.

I confirmed that applying #2823, followed by switching the order of operations, results in the seemingly correct readings:

if (adc_get_channel_status(ADC, ulChannel) != 1) {
    if ( latestSelectedChannel != (uint32_t)-1 && ulChannel != latestSelectedChannel)
        adc_disable_channel( ADC, latestSelectedChannel );
    adc_enable_channel( ADC, ulChannel );

    latestSelectedChannel = ulChannel;
}

[cmaglie]

Hi @shahokun,

If you disable all channels at the same time the ADC is turned off and the SAM3X will add a STARTUP time on the next conversion, that's the reason why we keep at least one channel enabled all the time: doing this way the ADC is always on and performs at the maximum sample-rate available (700Khz with CPU clock @96mhz @84Mhz on the Due board), see #1418 for a more detailed discussion.

About the wrong readings, it may be due to high impedance of the inputs, since the ADC is multiplexed between channels, it may be too quick for the sample/hold to track the source if the source is too weak. If you don't need high speed reading a workaround may be to add a small delay to give enough time to the source to stabilize, something like:

analogRead(0); // dummy read: enable channel 0
delay(10);
value = analogRead(0); // the real conversion

The delay may vary depending on the input impedance.

[cmaglie]

Closing this one, feel free to reopen if there are more comments.