analogio: correct ADC scaling to 16-bit range

[PaintYourDragon]

Affects all versions of CircuitPython, all architectures.

The approach currently used for scaling ADC inputs to 16-bit range never actually reaches 65535. For example, the oft-seen Python snippet:
coefficient = adc.value / 65535
will never return 1.0 under any circumstance.

There are two ways this might be corrected: fix the conversion (which might break or alter existing code behavior), or provide a constant that can be used for division to correctly attain 0.0 to 1.0 range.

Regarding first approach: although a shift-right is correct when decimating an integer to fewer bits, the inverse operation — shifting left (as currently in common_hal_analogio_analogin_get_value()) — isn’t mathematically correct if the goal is to cover the full 16-bit range (0-65535) from a sub-16-bit input (as all currently-supported ADCs currently are, I think…max is 14 bits in nRF52?). Correctly scaled, the least bits (all 0 resulting from the left-shift operation) should have the equivalent number of most-significant bits copied down into that space to provide a full linear “stretch.”

(This can be more easily visualized by picturing the same operation with fewer bits…as in, suppose the ADC was 4 bits and the output range was 8 bits. A raw ADC reading of 0xF, if shifted only, would yield 0xF0…but…conceptually…think about it…you really want 0xFF. This is an old bugbear from image processing days.)

There are a few solutions that involve multiplication and division, multiplication and a shift, or two shifts and an OR. As long as the ADC input is 8 bits or more, the easiest is just to code the specific two-shifts-and-OR as required for each device’s particular ADC resolution. e.g. for SAMD’s 12-bit ADC, it would be:
return (value << 4) | (value >> 8);
and for nRF’s 14-bit ADC, it’s:
return (value << 2) | (value >> 12);
(Things get a little more involved if the ADC resolution is less than 8 bits — I won’t cover it here, as it’s unlikely to happen in any supported devices.)

The resulting 0-65535 output would match the documentation and appeals to my sense or order and mathematical precision…BUT, as previously mentioned, could break or change behavior of existing code that might be calibrated for specific analogio values or ranges. e.g. on SAMD, adc.value currently ranges from 0 to 65520, not 65535. (Counter argument, it might actually “fix” some code that’s expecting 0 to 65535 but isn’t getting it.)

An alternate approach would be for analogio to provide something like a “top” or “limit” value which could be used as a divisor for a 0.0-1.0 range, for example:
coefficient = adc.value / adc.top
top/limit would be the constant
((1 << ADCBITS) - 1) << (16 - ADCBITS)
to match the maximum value currently returned by that code with only the shift-left operation. Existing code would continue to function exactly as before, for better or for worse.

[kevinjwalters]

In the comments in micropython#5033 you can see how MicroPython does this for read_u16() but I believe that was a fresh implementation, i.e. no impact to existing code/users.

[dhalbert]

The essence of what MicroPython is doing is this:

    // Scale raw reading to 16 bit value using a Taylor expansion (for 8 <= bits <= 16)
    uint32_t u16 = raw << (16 - adc_bit_width) | raw >> (2 * adc_bit_width - 16);

[dhalbert]

We can fix this in 7.0.0 or on another major version. Breaking changes are allowed on those boundaries.

[kevinjwalters]

There are other library implementations still using zero padding. I put issues in for two I know about, may well be more.

• adafruit_mcp3xxx.Analogin - correct ADC scaling to 16-bit range to match analogio.AnalogIn Adafruit_CircuitPython_MCP3xxx#43
• adafruit_ads1x15.Analogin - correct ADC scaling to 16-bit range to match analogio.AnalogIn #43  Adafruit_CircuitPython_ADS1x15#92

[kevinjwalters]

I think the nrf change is buggy in 8.1.0. It's a 14 bit value but it always has bits 2 and 3 (originally I described this incorrectly as 0 and 1) as 00 because the ADC is 12bit in this mode of operation. I noticed this from some real tests (everyone needs a hobby):

Adafruit CircuitPython 8.1.0 on 2023-05-22; Adafruit CLUE nRF52840 Express with nRF52840
>>> import board, analogio
>>> adc = analogio.AnalogIn(board.P4)
>>> value = adc.value  # Measure AA battery
>>> conversion = adc.reference_voltage / (2**16 - 1)
>>> value = adc.value   # Measure 3.3V
>>> print("{:d} {:.4f}V {:016b}".format(value, value * conversion, value))
65523 3.2994V 1111111111110011
>>> value = adc.value   # Measure 3.3V again
>>> print("{:d} {:.4f}V {:016b}".format(value, value * conv, value))
65507 3.2986V 1111111111100011

I think that's giving 65523 as maximum which has 12 bits from ADC (bold) and only 2 bits added in (bold) 1111111111110011

circuitpython/ports/nrf/common-hal/analogio/AnalogIn.c

Lines 128 to 129 in 5ef1f2d

	// Stretch 14-bit ADC reading to 16-bit range
	return (value << 2) | (value >> 12);

Should that be return (value << 2) | (value >> 10); ?