GrumpyOldPizza / ArduinoCore-stm32l0

Arduino Core for STM32L0
125 stars 67 forks source link

analogInputToDigitalPin and passing ADC channel numbers does not work #172

Closed matthijskooijman closed 3 years ago

matthijskooijman commented 3 years ago

This macro is intended to translate ADC channel numbers to "digital" pin numbers and is defined as: https://github.com/GrumpyOldPizza/ArduinoCore-stm32l0/blob/b1cf1cd6eba2f06bbe4edbe76aa43ae5971e8d8c/cores/arduino/WVariant.h#L96

And the digitalWrite function supposedly supports both analog channel numbers, or "digital" pin numbers:

https://github.com/GrumpyOldPizza/ArduinoCore-stm32l0/blob/b1cf1cd6eba2f06bbe4edbe76aa43ae5971e8d8c/cores/arduino/wiring_analog.c#L109-L112

However, this only works if:

  1. The digital pin number of A0 is more than NUM_ANALOG_INPUTS, and
  2. All analog pins have consecutive digital pin numbers

Looking at a few variants, it seems that the second requirement is not satisfied (checked NUCLEO-L073RZ and Gnat-L082CZ), making this work erratically.

Since reordering the analog input pins is probably not feasible (and probably not wanted either), another solution would be to change the implementation to use a lookup table that maps channel numbers to Ax constants. This can be defined by variants or just globally in the core, using the PIN_Ax constants already defined. E.g. something like:

const analogInputToDigitalPinTable[NUM_ANALOG_INPUTS] = {
#if NUM_ANALOG_INPUTS > 0
  A0,
#endif
#if NUM_ANALOG_INPUTS > 1
  A1,
#endif
#if NUM_ANALOG_INPUTS > 2
  A2,
#endif
... etc. up to A15
};

Also, note that the check in analogRead() (as shown above) currently does pretty much the same thing as analogInputToDigitalPin(), so the former could be rewritten to use the latter.

GrumpyOldPizza commented 3 years ago

It is all kind of a historic mess. The original AVR core allows "analogRead(0)" ... which should map to "analogRead(A0)".

IMHO it should perhaps coded best as ArduinCore-arc32 does it (not as ArduinoCore-samd, where ArduinoCore-stm32l0 is derived from):

/* allow for channel or pin numbers */
if (pin < 6) pin += A0;

One could argue now whether it's good to use "6" or the possible set of linear A0-A5 (or A0-A7) ...

All the cores I am doing are exposing only this linear range, so the macro in use (nalogInputToDigitalPin(P)) would be sufficient as well. In reality is just requires a variant.cpp where you keep any analog inputs from D0-D5 (or D0-D7).

GNAT exposes only A0-A4. NUCLEO-L073 exposes only A0-A5. So I do not quite understand what is not supposed to work as is, and why (other than it might be not that nice or perfect).

GrumpyOldPizza commented 3 years ago

One more detail perhaps, mainly as all of this is confusing.

There is one global table, g_APinDescription[], which maps all pins (digital index) to real physical GPIOs. And of course the internal STM32L0 specific analog read channels are remapped that way. So "A0" does not reallty map to the first input on the ADC peripheral.

Hence the question you raise seems to really be cosmetical.

matthijskooijman commented 3 years ago

Ah, it seems that indeed, the problem I mentioned does not exist. I wrote a condition "All analog pins have consecutive digital pin numbers", which I then interpreted as requiring that the ADC channel numbers of the analog pins should be consecutive, but that's not the case (since passing "channel" numbers to analogWrite() will always translate to the Ax pin and then look up the corresponding channel, it will never just use the "channel" number passed to analogWrite() directly as the ADC channel number.

So indeed, it should just work, sorry for the noise (but thanks for you quick response, as always :-)