Variants: Changed 'built-in' LED from RED to GREEN on R1, C33 and H7.

[bogdanarduino]

In ArduinoCore-zephyr\cores\arduino\Arduino.h, the built-in LED index was changed from 0 (RED) to 1 (GREEN).

[pillo79]

This PR is changing a global default, which must be available even on boards that only have 1 LED defined.
Please fix this by setting LED_BUILTIN to the appropriate number or macro in each variant (variants/xxx/pins_arduino.h).

[sebromero]

@pillo79 @bogdanarduino Maybe instead of changing it in the variant file, it might be cleaner to change it in the device tree overlay? I suppose builtin-led-gpios can have duplicate entries. So, the first index could always refer to the BUILTIN_LED, while the other entries are for RGB etc. e.g. for Giga it could be:

builtin-led-gpios = <&gpioj 13 GPIO_ACTIVE_LOW> // Green
                    <&gpioi 12 GPIO_ACTIVE_LOW>, // Red
                    <&gpioj 13 GPIO_ACTIVE_LOW>, // Green
                    <&gpioe 3 GPIO_ACTIVE_LOW>; // Blue

Wdyt?

[facchinm]

Compeltely agree with @sebromero , the right way to tackle this is via device tree

[bogdanarduino]

Thank you all for your feedback! Please recheck the updated fix when you have time.

[pennam]

Hi @bogdanarduino could you please reword the commits and the PR title to make CI happy ? The pattern is

<scope>: <description>

[pennam]

not a dts expert here, but I'm wondering if there is any specific reason to add a new

/ {
	zephyr,user {

instead of changing what we already have some lines above

		builtin-led-gpios = <&ioport1 7 GPIO_ACTIVE_LOW>,
                            <&ioport4 0 GPIO_ACTIVE_LOW>,
                            <&ioport8 0 GPIO_ACTIVE_LOW>;

[sebromero]

@pillo79 Do we have your blessing?

[pillo79]

@sebromero, I discussed this with @facchinm right now - the way the code currently uses builtin-led-gpios is not very effective, because only the first entry is ever used.

We settled on:

• @bogdanarduino, please duplicate the GREEN entry in the existing boards, so that the sequence in the list is effectively G-R-G-B . This will "fix" the current LED_BUILTIN macro to use the green LED.
• as a next step, we will add code that uses indexes 1-3 of that array (when the variant defines them) to set LEDR, LEDG and LEDB in that sequence. That way the current list will not change. This also supports targets that have separate builtin and RGB LEDs.

[bogdanarduino]

@sebromero, I discussed this with @facchinm right now - the way the code currently uses builtin-led-gpios is not very effective, because only the first entry is ever used.

We settled on:

* @bogdanarduino, please _duplicate the GREEN entry_ in the existing boards, so that the sequence in the list is effectively G-R-G-B . This will "fix" the current LED_BUILTIN macro to use the green LED.

* as a next step, we will add code that uses indexes 1-3 of that array (when the variant defines them) to set `LEDR`, `LEDG` and `LEDB` in that sequence. That way the current list will not change. This also supports targets that have separate builtin and RGB LEDs.

Ok, clear, I will do the change very soon. Thank you for reviewing!

[pillo79]

Thanks! 👍