Improve RGB driver in pull #6808; solves #6968 #6979

santaimpersonator · 2022-07-12T16:18:17Z

Description of Change

This solution adds a RGB_BUILTIN variable to pull request #6808. With the original solution (#6808), users were limited to controlling a single built-in LED. By creating a new variable RGB_BUILTIN, users are able to control a regular LED and RGB LED with the digitalWrite() function, separately from each other.

Replaces the use of the LED_BUILTIN variable for the RGB LED
Removes the need for BOARD_HAS_NEOPIXEL
Also, renames LED_BRIGHTNESS to match new variable name RGB_BRIGHTNESS

For boards that have a regular LED and RGB LED on their board, users can define the pin configuration (below) and the Blink.ino and BlinkRGB.ino example sketches would operate on the regular and RGB LEDs separately.

static const uint8_t LED_BUILTIN = 13;
static const uint8_t RGB_BUILTIN = SOC_GPIO_PIN_COUNT+2;
#define BUILTIN_LED  LED_BUILTIN // backward compatibility
#define LED_BUILTIN LED_BUILTIN
#define RGB_BUILTIN RGB_BUILTIN
#define RGB_BRIGHTNESS 65

Tests scenarios

I have tested the proposed changes with the regular Blink.ino and the ESP32>GPIO>BlinkRGB.ino example sketches. These tests were performed on an ESP32-WROOM board with a regular LED and RGB LED. The sketches were also tested for the following pin configurations:

Regular and RGB LED:

Blink.ino only controls regular LED
BlinkRGB.ino only controls the RGB LED

static const uint8_t LED_BUILTIN = 13;
static const uint8_t RGB_BUILTIN = SOC_GPIO_PIN_COUNT+2;
#define BUILTIN_LED  LED_BUILTIN // backward compatibility
#define LED_BUILTIN LED_BUILTIN
#define RGB_BUILTIN RGB_BUILTIN
#define RGB_BRIGHTNESS 65

Only RGB LED:

Blink.ino and BlinkRGB.ino both control the RGB LED

static const uint8_t LED_BUILTIN = SOC_GPIO_PIN_COUNT+2;
#define BUILTIN_LED  LED_BUILTIN // backward compatibility
#define LED_BUILTIN LED_BUILTIN
#define RGB_BUILTIN LED_BUILTIN
#define RGB_BRIGHTNESS 65

Only regular LED:

Only Blink.ino works with the regular LED

static const uint8_t LED_BUILTIN = 13;
#define BUILTIN_LED  LED_BUILTIN // backward compatibility
#define LED_BUILTIN LED_BUILTIN

Related links

Closes #6968
*Based on original pull request (#6808) and issue (#6783)

Replaces the use of the `LED_BUILTIN` variable by creating a new variable called `RGB_BUILTIN`. On boards with both a regular LED and RGB LED, this change provides functionality to control either LED. The `LED_BRIGHTNESS` variable is changed to `RGB_BRIGHTNESS`, which aligns more closely with the `RGB_BUILTIN` variable name. `BOARD_HAS_NEOPIXEL` is no longer necessary; it is replaced by `RGB_BUILTIN`.

Update example code for changes with the RGB driver: - Replace `LED_BUILTIN` and `BOARD_HAS_NEOPIXEL` with `RGB_BUILTIN` - Replace `LED_BRIGHTNESS` with `RGB_BRIGHTNESS`

Update board variants for changes with the RGB driver: - Remove `BOARD_HAS_NEOPIXEL` - Define `RGB_BUILTIN` pin - Replace `LED_BRIGHTNESS` with `RGB_BRIGHTNESS` to align with `RGB_BUILTIN` name

CLAassistant · 2022-07-12T16:18:23Z

All committers have signed the CLA.

PilnyTomas

Code looks great, tested on S2 Kaluga and C3.
@P-R-O-C-H-Y tested on S3

SuGlider · 2022-07-14T13:57:24Z

I have mixed feelings about this PR... It looks like it just changes the name of the symbols.
What exactly is the improvement @santaimpersonator?

This solution adds a RGB_BUILTIN variable to pull request #6808.

It actually replaces, it doesn't add anything.

PilnyTomas · 2022-07-14T16:36:53Z

It is related to #6968 where it stated that some boards with both normal LED and RGB LED can use only the RGB. This PR will enable use both LED_BUILTIN and RGB_BUILTIN

santaimpersonator · 2022-07-14T16:42:40Z

I have mixed feelings about this PR... It looks like it just changes the name of the symbols. What exactly is the improvement @santaimpersonator?

This solution adds a RGB_BUILTIN variable to pull request #6808.

It actually replaces, it doesn't add anything.

@SuGlider Yes, from the initial commit, it may seem like I have only replaced variable names. However, my pull request allows boards with a both a regular LED and RGB LED to be controlled with the digitalWrite() function.

The initial RGB driver only provided control on one of those LEDs when used with the Blink.ino and BlinkRGB.ino example codes.

As shown in the test scenarios (above) that I provided, users can now control both LEDs separately (if they wish to define the board that way)

Regular and RGB LED:

Blink.ino only controls regular LED
BlinkRGB.ino only controls the RGB LED

static const uint8_t LED_BUILTIN = 13;
static const uint8_t RGB_BUILTIN = SOC_GPIO_PIN_COUNT+2;
#define BUILTIN_LED  LED_BUILTIN // backward compatibility
#define LED_BUILTIN LED_BUILTIN
#define RGB_BUILTIN RGB_BUILTIN
#define RGB_BRIGHTNESS 65

I caught this issue with our board that we were about to submit a pull request for. Our board has both types of LEDs and the regular LED is already silked with STAT. With the current release, we can only define the board to work with one of the LEDs when using the Blink.ino and BlinkRGB.ino example codes.

If we define the board with the regular LED (STAT), only the Blink.ino example is compatible with the board.

static const uint8_t LED_BUILTIN = 13;
#define BUILTIN_LED  LED_BUILTIN // backward compatibility
#define LED_BUILTIN LED_BUILTIN

If we define the board with BOARD_HAS_NEOPIXEL, both the Blink.ino and BlinkRGB.ino example codes are compatible with the board. However, the Blink.ino example blinks the RGB LED and not the the regular LED that is labeled with STAT (Status LED) on our board.
```
static const uint8_t LED_BUILTIN = SOC_GPIO_PIN_COUNT+2;
#define BUILTIN_LED  LED_BUILTIN // backward compatibility
#define LED_BUILTIN LED_BUILTIN
#define BOARD_HAS_NEOPIXEL
#define LED_BRIGHTNESS 65
```

My changes basically have users define the current BOARD_HAS_NEOPIXEL variable with a value (1). I then renamed that variable to RGB_BUILTIN to align with the LED_BUILTIN variable name (2). Then, I renamed the LED_BRIGHTNESS variable to RGB_BRIGHTNESS match the RGB_BUILTIN name (3). To breakdown these changes, please refer to lines 137-141 in cores/esp32/esp32-hal-gpio.c

Original code:

extern void ARDUINO_ISR_ATTR __digitalWrite(uint8_t pin, uint8_t val)
{
    #ifdef BOARD_HAS_NEOPIXEL
        if(pin == LED_BUILTIN){
            //use RMT to set all channels on/off
            const uint8_t comm_val = val != 0 ? LED_BRIGHTNESS : 0;
            neopixelWrite(LED_BUILTIN, comm_val, comm_val, comm_val);
            return;
        }
    #endif

Have users define the BOARD_HAS_NEOPIXEL variable with a value. Then, use it to replace the value for LED_BUILTIN.

extern void ARDUINO_ISR_ATTR __digitalWrite(uint8_t pin, uint8_t val)
{
    #ifdef BOARD_HAS_NEOPIXEL
        if(pin == BOARD_HAS_NEOPIXEL){
            //use RMT to set all channels on/off
            const uint8_t comm_val = val != 0 ? LED_BRIGHTNESS : 0;
            neopixelWrite(BOARD_HAS_NEOPIXEL, comm_val, comm_val, comm_val);
            return;
        }
    #endif

Renamed the BOARD_HAS_NEOPIXEL variable to RGB_BUILTIN to align with use of LED_BUILTIN in the example codes.

extern void ARDUINO_ISR_ATTR __digitalWrite(uint8_t pin, uint8_t val)
{
    #ifdef RGB_BUILTIN
        if(pin == RGB_BUILTIN){
            //use RMT to set all channels on/off
            const uint8_t comm_val = val != 0 ? LED_BRIGHTNESS : 0;
            neopixelWrite(RGB_BUILTIN, comm_val, comm_val, comm_val);
            return;
        }
    #endif

Renamed the LED_BRIGHTNESS variable to RGB_BRIGHTNESS match the RGB_BUILTIN name.

extern void ARDUINO_ISR_ATTR __digitalWrite(uint8_t pin, uint8_t val)
{
    #ifdef RGB_BUILTIN
        if(pin == RGB_BUILTIN){
            //use RMT to set all channels on/off
            const uint8_t comm_val = val != 0 ? RGB_BRIGHTNESS : 0;
            neopixelWrite(RGB_BUILTIN, comm_val, comm_val, comm_val);
            return;
        }
    #endif

As you can see, the primary change is in (1), where the BOARD_HAS_NEOPIXEL variable is defined with a value. That is then used to replace the value for LED_BUILTIN. The rest of the changes rename variables to align better with their use in the example codes.

santaimpersonator · 2022-07-14T16:50:42Z

It is related to #6968 where it stated that some boards with both normal LED and RGB LED can use only the RGB. This PR will enable use both LED_BUILTIN and RGB_BUILTIN

@PilnyTomas Haha, you beat me to it

SuGlider · 2022-07-14T17:16:26Z

Thanks @santaimpersonator @PilnyTomas for the clarification.
Indeed, it adds value.

it just makes me wonder that maybe we may need to review all the variants and their arduino_pins.h to add the right #define xxx considering each board hardware.... We shall document it ASAP.

santaimpersonator · 2022-07-14T17:47:59Z

@SuGlider Awesome! Thanks for the approval.

NOTE: For reviewing the variant files:

All the SparkFun variants should be fine as they are (with regards to this pull request). The pull request we are about to submit, will be our first ESP32 board with an RGB LED.
In this pull request, I already changed the pins_arduino.h files for the three board variants in #6808

santaimpersonator · 2022-07-15T14:40:51Z

My coworker just pointed this out to me in the Arduino IDE:

Does this Arduino core have a keywords.txt file, where we can include RGB_BUILTIN as a keyword?

PilnyTomas · 2022-07-20T09:38:59Z

Does this Arduino core have a keywords.txt file, where we can include RGB_BUILTIN as a keyword?

Please see PR #7015

* Added support for Cytron Maker Feather AIoT S3. * 1. Select OPI PSRAM by default. 2. Fixed pin name error in variant.cpp. 3. Added definition for RGB_BUILTIN. * Define the RGB_BUILTIN as shown in #6979. * Added pin definition for A12 (Vin Sense).

santaimpersonator added 3 commits July 12, 2022 08:52

Update BlinkRGB example

299a450

Update example code for changes with the RGB driver: - Replace `LED_BUILTIN` and `BOARD_HAS_NEOPIXEL` with `RGB_BUILTIN` - Replace `LED_BRIGHTNESS` with `RGB_BRIGHTNESS`

Update board variants

456a21a

Update board variants for changes with the RGB driver: - Remove `BOARD_HAS_NEOPIXEL` - Define `RGB_BUILTIN` pin - Replace `LED_BRIGHTNESS` with `RGB_BRIGHTNESS` to align with `RGB_BUILTIN` name

SuGlider requested a review from PilnyTomas July 12, 2022 19:03

PilnyTomas approved these changes Jul 14, 2022

View reviewed changes

Merge branch 'master' into master

989b397

SuGlider self-requested a review July 14, 2022 17:18

SuGlider approved these changes Jul 14, 2022

View reviewed changes

Merge branch 'master' into master

cf9e39a

VojtechBartoska merged commit 05ae83a into espressif:master Jul 18, 2022

PilnyTomas mentioned this pull request Jul 20, 2022

Added keywords.txt with RGB_BUILTIN into libraries/ESP32 #7015

Merged

waiweng83 added a commit to CytronTechnologies/arduino-esp32 that referenced this pull request Oct 14, 2022

Define the RGB_BUILTIN as shown in espressif#6979.

577923e

SuGlider mentioned this pull request Mar 2, 2023

Proper LED_BUILTIN for CORE-ESP32 (ESP32C3) board #7914

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve RGB driver in pull #6808; solves #6968 #6979

Improve RGB driver in pull #6808; solves #6968 #6979

santaimpersonator commented Jul 12, 2022 •

edited

Loading

CLAassistant commented Jul 12, 2022 •

edited

Loading

PilnyTomas left a comment

SuGlider commented Jul 14, 2022 •

edited

Loading

PilnyTomas commented Jul 14, 2022

santaimpersonator commented Jul 14, 2022 •

edited

Loading

santaimpersonator commented Jul 14, 2022

SuGlider commented Jul 14, 2022

santaimpersonator commented Jul 14, 2022

santaimpersonator commented Jul 15, 2022

PilnyTomas commented Jul 20, 2022

Improve RGB driver in pull #6808; solves #6968 #6979

Improve RGB driver in pull #6808; solves #6968 #6979

Conversation

santaimpersonator commented Jul 12, 2022 • edited Loading

Description of Change

Tests scenarios

Related links

CLAassistant commented Jul 12, 2022 • edited Loading

PilnyTomas left a comment

Choose a reason for hiding this comment

SuGlider commented Jul 14, 2022 • edited Loading

PilnyTomas commented Jul 14, 2022

santaimpersonator commented Jul 14, 2022 • edited Loading

santaimpersonator commented Jul 14, 2022

SuGlider commented Jul 14, 2022

santaimpersonator commented Jul 14, 2022

santaimpersonator commented Jul 15, 2022

PilnyTomas commented Jul 20, 2022

santaimpersonator commented Jul 12, 2022 •

edited

Loading

CLAassistant commented Jul 12, 2022 •

edited

Loading

SuGlider commented Jul 14, 2022 •

edited

Loading

santaimpersonator commented Jul 14, 2022 •

edited

Loading