Added TXLed to Leonardo pin definition file

[NicoHood]

Should improve PR #3129

• Adds TX Led as pin 30
• Adds missing digitalPinHasPWM()
• Adds LED_BUILTIN_RX LED_BUILTIN_TX constants

[cmaglie]

AFAIK I still see two problems:

• The variant files for AVR must have all the analog pins allocated at the end of the array, so the new LED_BUILTIN_TX should be the number 18 and all the old pins from 18 to 30 should be shifted up by one (so, A0 becomes 19, A1 becomes 20, etc.).
• NUM_DIGITAL_PINS becomes 31 (from 30).

/cc @matthijskooijman @damellis

[matthijskooijman]

I wonder if shifting pin numbers will cause problems. People should not be using pin numbers directly, but I suspect they do (from when the Ax constants were not available yet, perhaps)?

[NicoHood]

Why does it have to be at the array end?

What about adding digitalPinHasPWM(p) at least?

[cmaglie]

@matthijskooijman
Are you aware if someone is using high pin numbers like digitalWrite(22, HIGH); instead of digitalWrite(A4, HIGH)? Maybe some library?

BTW that seems quite odd, because such numbers are not printed anywhere on the silk of the board, the only thing a user knows is that the analog pin 0 is marked as A0, so the typical usage may be:

analogRead(0); // OK
analogRead(A0); // OK
//digitalWrite(0, HIGH); // This is not possible because 0 is D0 and not A0
digitalWrite(A0, HIGH); // OK

If I really want to use 22 the only way to obtain it in a general way (unless you explicitly look at the variant file, but this is what we are trying to avoid) is by doing the following operation: NUM_DIGITAL_PINS - NUM_ANALOG_PINS + 4 but this usage force us to put the analog pins at the end.

[cmaglie]

@NicoHood
The digitalPinHasPWM change looks good.

[matthijskooijman]

Are you aware if someone is using high pin numbers like digitalWrite(22, HIGH); instead of digitalWrite(A4, HIGH)? Maybe some library?

Just this morning someone asked on IRC what "pin 60 on the mega" is. Apparently the Marlin 3D printer firmware uses this:

https://github.com/MarlinFirmware/Marlin/blob/fcc15f4897ddabef35d1d51cb08513a3b8fbcc83/Marlin/pins.h#L560

I'm not sure if this should be supported (the library can be fixed in a backward compatible way easily by just using Ax). Any idea how long the Ax macros have been available?

[NicoHood]

Same for the pro micro docs:

And the mega is not important here. The usage is stupid though, but not related to this 32u4 issue.

I just dont understand why analog pins have to be at the end of the array.

Anyways: Its stupid to not use the Ax definitions and that should not be a problem of Arduino. 1.6.x introduced a lot of changes anyways, especially the new USB-Core so you have to update most sketches anyways (Add include HID and Keyboard for example). And this so fast to fix, shouldnt be a real problem, even for library maintainers.

[cmaglie]

@matthijskooijman
I see, are they using the mega variant or are they providing their own variant? BTW it would have been much better to use An constants.

@NicoHood
the reason is that there are some core's functions that currently relies on this fact, take the analogRead(..) for example, you can do both:

analogRead(A0); or analogRead(0);

this is possible because internally there is a check like this: https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/wiring_analog.c#L44

as you can see analogRead assumes that the analog pins, in the 32u4 case, are starting from 18 (but it should really be NUM_DIGITAL_PINS - NUM_ANALOG_PINS again assuming that all the analogs pins are at the end, but if we increase NUM_DIGITAL_PINS this is no more true).

[NicoHood]

Well that is easy to fix though. add -19 instead. what about that?

[NicoHood]

So what about this now? Can we add a pin definition for the 32u4 TX Led now? Then we could also use USART SPI for the FastLED library. This is a really nice feature I'd like to see, but it requires an additional pin for the pin. Its broken out on some board, so you could really use this pin.

[NicoHood]

I looked at the source again. This check is redundant it seems:
https://github.com/NicoHood/Arduino/blob/led_fix/hardware/arduino/avr/cores/arduino/wiring_analog.c#L49-L50

You cannot input normal pin numbers anyways. So putting the TX Led at the end should not cause any problem. I havent tested this on hardware yet, but the subtraction doesnt seem to cause any problem at all.

Putting it at the end keeps the original digital pin numbers though (instead of Ax if someone used this instead). Its just an addition. But I could also fix this by adding it in the middle. I will do some tests...

Also the analogRead is not save, if someone calls it with a wrong pin number. But this applies to all Arduino boards.

BTW: FastLED now also supports the TX Led as XCK pin. However its mapped to pin 24, but if this PR is merged, I will change that and add the missing Ax pins as well: FastLED/FastLED#214

[NicoHood]

Do we also still need this workaround? Since 1.5.6 we have a newer toolchain, so the file should be patched by now I guess?
https://github.com/arduino/Arduino/pull/3680/files#diff-13f3e6ae9fe462e6157b4c993787aafaR28

Or why dont you open a PR if those values are still wrong?
https://github.com/rgwan/avr-libc/blob/master/avr-libc/include/avr/iom32u4.h#L882

[NicoHood]

I tested this now. All analog reads work as expected, since we never count from the end. We leave out the offset (18) and read from those analog ports. Since 30 is no analog port it will just not work like any other non analog pin.

I see no reason why this PR isnt ready for merging.

// the setup function runs once when you press reset or power the board
void setup() {
  // initialize digital pin 13 as an output.
  pinMode(30, OUTPUT);
}

// the loop function runs over and over again forever
void loop() {
  digitalWrite(30, HIGH);   // turn the LED on (HIGH is the voltage level)
  delay(1000);              // wait for a second
  digitalWrite(30, LOW);    // turn the LED off by making the voltage LOW
  delay(1000);              // wait for a second

  for (int i = A0; i <= A11; i++) {
    Serial.print(i); // Converted to int 18-29
    Serial.print("\t");
    Serial.println(analogRead(i));
  }
}

[matthijskooijman]

You cannot input normal pin numbers anyways. So putting the TX Led at the end should not cause any problem. I havent tested this on hardware yet, but the subtraction doesnt seem to cause any problem at all.

What do you mean here? You can use analogRead(A0), which will, due to that check you linked, be the same as analogRead(0)?

[NicoHood]

analogRead(0) will read A0 from what I understood. However it is not pin number 0. So pin numbers (except the A prefix) are not supported anyways. So we are not adding or removing features here and anything should be fine

AnalogRead(30) will just not work, as analogRead(12) for example.

[NicoHood]

Awesome :)