LED_BUILTIN in variant header file needs to be a define

[bperrybap]

There was lots of discussion about this several years ago on the Arduino developers lists and the resolution was to always ensure that there is a LED_BUILTIN define so that sketches and libraries could run time detect the presence of a built in LED.
So for years there has been a LED_BUILTIN define on all the Arduino.cc supplied AVR cores and some of the newer non AVR cores.
All the variant files in the esp8266 core declare LED_BUILTIN as a static const uint8_t
The problem with not having a define is that it breaks any code that depends on LED_BUILTIN as being a define to enable/disable use of a built in LED.
There are two solutions

• change the const declaration to be a define (which is what the Arduino.cc devs did in the AVR core)
• create a define that matches the variable name.

Either will work.
My suggestion would be to use the latter and create a define that matches the variable name.
i.e.

#define LED_BUILTIN LED_BUILTIN
static const uint8_t LED_BUILTIN = 2;//new ESP-12E GPIO2

That way you get the best of both worlds.
A define that code can use to compile time verify the existence of a built in led and a variable by the same name that allows for a typed variable rather than using a simple define.

This ensures full backward compatibility with other cores and older IDEs.
Doing something like adding a new PIN_LED_BUILTIN define is not a workable solution as it is not backward compatible and would require updating code ot use messy Arduino defines to try to compile detect which core and which core version is being used to know what defines or variable names to use.

[d-a-v]

@bperrybap In the latest git version of core, we have a LED menu for the generic board which uses such a #define. Would give a try ?

[bperrybap]

I'm not seeing it in either the latest 2.4 core available through the IDE boards manager or on the master git branch.
Where is it? branch? Can you provide a link to the commit that adds this?
All I see in in the master branch is support for the ability to set a USERLED define in the boards.txt file and the platform.txt file.
The name of the define must be LED_BUILTIN and it needs to work for all variants that use a built in LED pin. i.e. if LED_BUILTIN is defined a built in led is on that pin.
You can have both a define and a const int with the name of LED_BUILTIN but you must have the define so that code can detect its existence at compile time.
From what I can see, it appears that you still need to add the LED_BUILTIN define I mentioned above to all the variant files.
Here is a sample sketch that can be used to test it.
You will see that without the define it will fail to detect a valid LED_BUIILTIN pin:

#ifndef LED_BUILTIN
#warning no LED_BUILTIN define
#endif
void setup(void)
{
	Serial.begin(115200);
#ifndef LED_BUILTIN
	Serial.println("LED_BUILTIN is not defined");
#else
	Serial.print("LED_BUILTIN is using pin: ");
	Serial.println(LED_BUILTIN);
#endif
}
void loop(void){}

With what I have seen in the master branch, you could modify each variant file like this:
From this:

static const uint8_t LED_BUILTIN = USERLED;

to this:

#define LED_BUILTIN LED_BUILTIN
static const uint8_t LED_BUILTIN = USERLED;

[d-a-v]

I myself can see it in the menu. If you updated from git (master branch) you need to restart the IDE so it reloads board.txt defining the menu. The menu only shows with generic boards esp8266 or esp8285 selected.
The missing define you mention is in boards.txt (-DUSERLED=xxx).

The generic variant uses the define if provided, otherwise the former value is used as default.

I'm not saying it works otherwise I wouldn't ask to try. Still I'm confident.

[d-a-v]

LED_BUILTIN not beeing a define, your sketch should not test for it.
For my test I moved the Serial.println(LED_BUILTIN); outside from the #if.

[bperrybap]

The define I mentioned was named LED_BUILTIN and it isn't being used.
It isn't in the files you provided links to.
Like I said, you must provide a define (not a variable) named LED_BUILTIN
It must be a define and must be spellled exactly like that.
This is the way the Arduino IDE has defined it to work and the way it works on ALL other cores.
If you want to be compatible with the Arduino IDE and all the other cores you must provide a define.
You can provide both a define and a variable but you must provide a define named LED_BUILTIN.
Run the simple test sketch I provided above to see if it is working.
But based on what I'm seeing, it isn't going to work, since there is no define named LED_BUILTIN.

[bperrybap]

Code does need be able check for its existence at compile time.
This is because not all Arduino boards/devices have a built in LED.
Devices/boards that do not have a built in led will not define/declare the symbol LED_BUILTIN in their variant file.

Just having a const int named LED_BUILTIN isn't good enough.
By having it as a define, code can use conditional compilation to self adjust to not using it by detecting it at compile time and removing any code that depends on it.
And that is why the Arduino.cc development team decided that LED_BUILTIN must be a define.

IMO using LED_BUILTIN would have made more sense to use rather than USERLED to define the default built in LED since the define LED_BUILTIN is needed anyway.

[bperrybap]

For my test I moved the Serial.println(LED_BUILTIN); outside from the #if.

You can't do that.
In order to test if the esp8266 core is compatible with the way LED_BUILTIN is supposed to work, you must run the sketch the way I wrote it.

[d-a-v]

I think you've made the point here.
To remain compatible with arduino.cc a define is needed.
Spelled like LED_BUILTIN.
For all boards, so sketches can check it on all arduino-compatible boards.
So we'll work on this.
Thank you for your patience so far.

[d-a-v]

#4276

[d-a-v]

PR is merged