Analog pin handling generalizations

[matthijskooijman]

Here's a change to the handling of analog pins in analogRead. It removes some hardcoded, MCU-based processing, that can just be generalized using the value of A0 instead. This makes it easier for third party to get working analog pins, without changing anything for the existing boards and variants.

I originally wanted to combine this fix with a fix for #1883, but it seems that issue might be more complicated than I initially thought, so let's do this one first.

[matthijskooijman]

I just added two partly-related fixes for the robot variant files.

[pbrook]

This breaks boards that do not have an ADC, e.g. the atmega32u2.
On these boards NUM_ANALOG_INPUTS is 0 and A0 does not exist (the ADMUX/ADC* registers are also absent).

[matthijskooijman]

Good point, thanks. I'll fix this up when I find some time :-)

[matthijskooijman]

I've rebased and fixed the issue pointed out by @pbrook, this should now work when A0 is absent again.

[damellis]

This looks like it will break variants that define analogPinToChannel(), since the current code doesn't do any additional subtraction in such cases (with the exception of the ATmega32U4). It also seems to assume that the analog pin numbers are consecutive, which isn't always the case (and is one of the reasons why you might want to use analogPinToChannel() instead).

[matthijskooijman]

@damellis, I had interpreted "analogPinToChannel" as a mapping from the analog pin number (e.g., A0, A1, A2) to the corresponding channel on the ADC - not a mapping from the digital pin number to the analog pin number.

If the intent is to let analogPinToChannel do the entire mapping, then it seems weird that the old code (for the 32u4) subtracted 18 already and then passed to analogPinToChannel, though that might have been some historical issue.

It's true that this assumes analog pin numbers are consecutive, but I believe this assumption might already have been present, also in other code. Supporting non-consecutive pin numbers does seem like a good idea to me, though I'm not so sure how feasible this is. Perhaps a second mapping define could be allowed for this mapping?

As for non-consecutive pin numbers, an example can be found in the Leonardo, see #1883. There, a number of the digital pins can also be used as analog pins. This means that the A6 "channel" maps to D4 for example. Yet, instead of defining them like that, the A6 "pin" is defined as a duplicate of D4, preserving consecutiveness with the other analog pins. This might be because of the required consecutiveness, but also to prevent unambiguity: If I call analogRead(4) - did I mean to read from the fourth channel / analog pin (A4) or from the channel belonging to D4 (== A6). I'm not sure if there's anything we can do to solve this ambiguity (nor if that should affect this PR in any way).

Any specific ideas on what would need to happen to support non-consecutive pin numbers?

[damellis]

I think the idea was to allow variants to take full control of the pin to channel mapping by defining analogPinToChannel(). The 32U4 subtraction is an exception that was inserted to maintain backwards compatibility. In general, though, moving as much of the mapping as possible out of the analogRead() function and into the variant (through analogPinToChannel()) seems to me like the right way to go. In the case that there's no analogPinToChannel() macro defined, then, yes, it might be good to generalize the default mapping by subtracting A0 instead of hard-coding specific values for only specific microcontrollers. But I think we should defer to analogPinToChannel() if it exists.

[pbrook]

There are three fundamental assumptions that user code (sketches) can make:
(a) analogWrite(4) selects the analog channel number 4.
(b) analogWrite(A6) selects the analog channel number 6
(c) A6 == 4 i.e. consistent numbering for the digital and analog aliases.

It is impossible to make all of the above work on a Leonardo. We get to pick which two (at most).

The current implementation breaks (c). From the discussion above it is unclear whether this is a deliberate decision, or a bug that should be fixed. If it is a bug then we need to decide which of (a) and (b) we want to break.

Personally I think breaking (b) would be a really bad idea. Ideally I'd also make (c) true as it avoids a lot of [bad] surprising behavior in corner cases. I do not know how common (a) is in practice.

[matthijskooijman]

Personally I think breaking (b) would be a really bad idea. Ideally I'd also make (c) true as it avoids a lot of [bad] surprising behavior in corner cases. I do not know how common (a) is in practice.

analogWrite documentation says:

int analogPin = 3;   // potentiometer connected to analog pin 3
val = analogRead(analogPin);   // read the input pin

In other words, (a) is the recommended practice. I think that (b) is actually the lesser-known alternative notation (but since it is explicit, I really like it best). Seems there is no alternative but to keep (c) broken.

As for using analogPinToChannel, IIUC @damellis is saying that it should take care of the entire mapping - meaning both the digital pin number to analog pin number (e.g. 0 for the pin labeled A0, 1 for the pin labeled A1, etc.) as well as the analog pin number to ADC channel. So far, I had assumed it was just for the latter (since for the 32u4 the pin -= 18 takes care of the first part).

Letting analogPinToChannel take care of everything makes sense, this just moves more stuff into the macro. I guess I'll adapt the PR to do this. I'm inclined to remove the special handling of 32u4, though. Any idea if there are variants out there that rely on this behaviour?

[matthijskooijman]

Any idea if there are variants out there that rely on this behaviour?

Even if there are, it should be easy to adapt them to handle both the analog pin numbers and digital pin numbers (required after we remove the exception). But since these adapted variants still work for the older Arduino versions with the workaround, we aren't introducing any new, hard-to-fix problems.

[matthijskooijman]

And I'm doubting again about the best way forward. While looking around I found Issue 727 which originally pointed out similar issues. In that context, this mail proposed a nice solution where both digitalPinToAnalogPin and an analogPinToChannel could be define, which together handle both sides of this translation (the first maps pin numbers into the [0,NUM_ANALOG_INPUTS> range, the latter maps that to the corresponding ADC channel). This sounds like a great solution, but it was never implemented. Also since #1368 was merged, analogPinToChannel has been made available to non-32u4 boards to apply both conversions in one macro, so changing its meaning to only do the latter part of the conversion could break existing variants.

To find out if that would be a problem, I tried to collect a sample of variant files from the wild. I probably didn't get all, but this from the first couple of pages in Google and github search (in no particular order):

tiny8: #define analogPinToChannel(p) ( (p) < 6 ? (p) : (p) - 6 )

Calunium: #define analogPinToChannel(p) ( (p) < NUM_ANALOG_INPUTS ? NUM_ANALOG_INPUTS - 1 - (p) : -1 )

Bobuino: #define analogPinToChannel(p) ( (p) < NUM_ANALOG_INPUTS ? NUM_ANALOG_INPUTS - (p) : -1 )

Luino: #define analogPinToChannel(p) ((p)>=17?(p)-17:(p))

Legoino, Flora, Sparkfun Pro Micro, Sparkfun minibench: #define analogPinToChannel(P) ( pgm_read_byte( analog_pin_to_channel_PGM + (P) ) )

Sanguino: #define analogPinToChannel(p) ((p < 8) ? (p) : 31 - (p))

This suggests that pretty much all of the variants handle both aspects of the conversion, except for the 32u4 based boards, which isn't a big surprise.

At first glance, it seems we could just implement the split into digitalPinToAnalogPin and analogPinToChannel (and add a default for both). For boards like Luino and the tiny8, the default digitalPinToAnalogPin would be sufficient. And even though their analogPinToChannel can handle pin numbers, they never get passed to it anymore, which is ok.

However, for the other boards, the analog pins are numbered in reverse, so the default digitalPinToAnalogPin would not work (it would break in a way that analogPinToChannel couldn't fix anymore). This means that these boards could not be supported anymore.

What could work is to introduce two new macros instead of reusing analogPinToChannel and still support that one as backward compatibility. E.g. support a digitalPinToAnalogChannel and analogChannelToAdcChannel macro (where "analog channel" is used to the channels numbered in analog pin order, and "ADC channel is used for the actual channels on the chip). Other names are welcome, I just wrote something down.

So, I can see three options:

• Let analogPinToChannel handle the complete digital-pin-or-analog-channel to adc-channel conversion. This breaks existing 32u4 boards which assume that the digital-pin to analog-channel part is already done.
• Introduce digitalPinToAnalogPin and reduce analogPinToChannel to only handle analog-channel to adc-channel conversion (like now happens for 32u4). This breaks all non-32u4 with non-standard digital-pin to analog-channel mappings.
• Introduce two new macros and keep supporting analogPinToChannel with the current meaning (e.g. different for 32u4 and other boards). This should not break any boards and provide maximum flexibility.

It seems that the last option is best, and the amount of legacy code we keep carrying around is limited (and can be removed at some point in the far future).

Any suggestions for good names for these macros?

[PaulStoffregen]

Please try not to hard-code an assumption that all chips have an analog mux that feeds the analog capable pins to a single on-chip ADC.

Already Teensy 3.1 has two actual ADCs, where some pins mux to ADC0, others to ADC1, some to both. Some also can mux to a programmable gain amplifier (a feature even some AVR chips have).

Many more microcontrollers have been announced with more than one ADC. In the not-too-distant future, probably only the smallest & cheapest microcontrollers will have only a single ADC and a simple mux.

Edit: Atmel's SAM4E chips now have two ADCs. Some of the Freescale Kinetis K60 parts have 4 ADCs on-chip and a pretty incredible number of pins that can mux to those 4 ADCs. As more microcontrollers move to 65 & 45 nm silicon over the next few years, we'll probably see even more capable chips. It's very, very easy for a semiconductor manufacturer to include more instances of modules like the ADC as the transistor density makes those features cheap.

[matthijskooijman]

@PaulStoffregen thanks for your comment, though I'm not really sure how it fits in? Even with multiple ADCs, a read should be mapped to one of them. Each ADC's channels could be mapped into a linear range as normal?

In any case, the interface we are talking about here is one offered by the Arduino AVR core to its variants. I guess it should be safe to make these assumptions for the AVR target? For the SAM core, the analogRead works differently already, by using a big array of pin info, so these macros are not needed there.

[CLAassistant]

All committers have signed the CLA.