Implement simple RGB driver via digitalWrite; solving #6783 #6808

PilnyTomas · 2022-05-26T11:37:58Z

Description of Change

This solution enables users to use Blink example with RGB LED on board.
When using function digitalWrite with constant LED_BUILTIN as pin parameter an RGB LED driver in the background will set white / off.
Added new example BlinkRGB extending the original Blink example with a demonstration of separate RGB channel setup.

Tests scenarios

Tested on
S3 DevkitC
C3 DevkitC
S2 Kaluga

Related links

Closes #6783

igrr · 2022-05-26T11:42:33Z

cores/esp32/esp32-hal-gpio.c

@@ -91,6 +91,13 @@ static InterruptHandle_t __pinInterruptHandlers[SOC_GPIO_PIN_COUNT] = {0,};

 extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)
 {
+#ifdef BOARD_HAS_NEOPIXEL


Could we consider moving the board-specific code to the variants folder?

I'm worried that the more similar features we add for specific boards, the more complex the core code will become. It would be better to handle this in the board variant folder. It would also demonstrate authors of 3rd party board packages how to add such custom functionality.

To avoid duplication of code, you can still define RGBLedWrite somewhere in the core, but preferably in a separate source file (esp32-hal-rgb-led.c?)

I'm a little bit confused about what should go where.
pinMode -> variants/ ... new file?
RGBLedWrite -> esp32-hal-rgb-led.c ?

Should be solved in 583e030

…n variatn.cpp

me-no-dev · 2022-05-27T08:46:47Z

cores/esp32/esp32-hal-rgb-led.h

+*/
+
+//#ifdef BOARD_HAS_NEOPIXEL
+  void RGBLedWrite(uint8_t pin, uint8_t red_val, uint8_t green_val, uint8_t blue_val);


please cleanup the commented includes and ifdefs. Arduino starts function names with lower case. I would suggest that you rename the function to neopixelWrite. rgbLed is very general and should be attributed to actual RGB Led with separate leads for each color (and controlled through PWM).

me-no-dev · 2022-05-27T09:05:04Z

cores/esp32/esp32-hal-rgb-led.c

@@ -0,0 +1,51 @@
+#include "esp32-hal-rgb-led.h"
+
+#ifdef BOARD_HAS_NEOPIXEL


seems you have the guard here but it is commented in the header?

me-no-dev · 2022-05-27T09:06:13Z

cores/esp32/esp32-hal-rgb-led.c

+  static bool initialized = false;
+
+  uint8_t _pin;
+  if(pin == LED_BUILTIN){


this should be evaluated only if the board has neopixel, else LED_BUILTIN would be lower than SOC_GPIO_PIN_COUNT

The entire function was safeguarded #ifdef BOARD_HAS_NEOPIXEL
But I will remove safeguard for the entire function a wrap only this.

me-no-dev · 2022-05-27T09:07:27Z

cores/esp32/esp32-hal-rgb-led.c

+#ifdef BOARD_HAS_NEOPIXEL
+
+void RGBLedWrite(uint8_t pin, uint8_t red_val, uint8_t green_val, uint8_t blue_val){
+  log_d("RGB: %d %d %d", red_val, green_val, blue_val);


such logs should be with level "Verbose" instead. Blinking led will print a lot of things :)

this was also a forgotten debug output. I will remove it in the next commit.

me-no-dev · 2022-05-27T09:10:18Z

cores/esp32/esp32-hal-rgb-led.h

+#include "soc/soc_caps.h"
+#include "pins_arduino.h"
+*/
+


You should have safeguard against LED_BRIGHTNESS not being defined:

#ifndef LED_BRIGHTNESS #define LED_BRIGHTNESS 64 #endif

me-no-dev · 2022-05-27T09:12:45Z

variants/esp32s2/variant.cpp

+
+extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)
+{
+  log_d("foo");


me-no-dev · 2022-05-27T09:13:07Z

variants/esp32c3/variant.cpp

+
+extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)
+{
+  log_d("foo");


me-no-dev · 2022-05-27T09:13:17Z

variants/esp32s3/variant.cpp

+
+extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)
+{
+  log_d("foo");


me-no-dev · 2022-05-27T09:16:06Z

variants/esp32c3/variant.cpp

@@ -0,0 +1,13 @@
+#include "esp32-hal-rgb-led.h"
+
+extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)


why is this necessary? pinMode in the core should just return if pin is not valid.

I was trying to implement this #6808 (review)

I understand, I just question the necessity to have such code in the variant just to get something as common as neopixel on the board working.
@igrr any comments?

Something like this in the core's __pinMode would skip execution:

if(pin > SOC_GPIO_PIN_COUNT && pin == LED_BUILTIN){ return; // or pin -= SOC_GPIO_PIN_COUNT; to allow setting the mode. }

@PilnyTomas this is still outstanding. Requiring variant.cpp to blink a led is not desired situation.

@igrr Could you please give us more info about why should we have board-specific code in the variants folder instead of having in the common file?
Should I keep it like this, or revert back to the common file?

I was thinking that this is behavior we want only on specific boards which have a WS2812B, and where we'd like to provide such emulation.

Makers of other boards should be able implement some different logic, or opt out of such emulation. In the latter case, the code for emulating an LED ideally shouldn't be compiled or linked.

if you do not have that LED defined, code will not be compiled anyway. requiring all boards with RGB leds to include a variant cpp for two lines is a bit too much IMO. There are plenty boards with RGB leds

Although, given that you are introducing a BOARD_HAS_NEOPIXEL macro, maybe we can indeed move this functionality into the core.

* Improve RGB LED Driver 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 BlinkRGB example 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 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 Co-authored-by: Rodrigo Garcia <rodrigo.garcia@espressif.com> Co-authored-by: Vojtěch Bartoška <76958047+VojtechBartoska@users.noreply.github.com>

PilnyTomas added 3 commits May 25, 2022 10:41

Initial implementation of RGB driver via digitalWrite

6967d8f

Moved constants to pins_arduino.h

cd31356

Changed pin definition + added example

51ce37d

PilnyTomas changed the title ~~Implement simple RGB driver via digitalWriteů solving #6783~~ Implement simple RGB driver via digitalWrite; solving #6783 May 26, 2022

igrr reviewed May 26, 2022

View reviewed changes

PilnyTomas added 4 commits May 26, 2022 14:20

Wrapped BlinkRGB in #ifdef BOARD_HAS_NEOPIXEL

1e363b7

Removed forgotten log from example

50b6356

Moved RGBLedWrite to new file esp32-hal-rgb-led and created pinMode i…

583e030

…n variatn.cpp

Updated example - lowered single channel brightness to LED_BRIGHTNESS

8786e83

me-no-dev reviewed May 27, 2022

View reviewed changes

Changed function name from RGBLedWrite to neopixelWrite + code polishing

10205b0

VojtechBartoska assigned PilnyTomas May 30, 2022

VojtechBartoska requested a review from me-no-dev June 15, 2022 12:27

VojtechBartoska added this to the 2.0.4 milestone Jun 15, 2022

Moved pinSetup portion related to RGB back to common file

c0aa3b8

me-no-dev merged commit 097ff8b into espressif:master Jun 24, 2022

me-no-dev mentioned this pull request Jun 24, 2022

add two more feather S3s #6881

Merged

This was referenced Jul 11, 2022

Improve RGB Driver #6968

Closed

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

Merged

robertlipe referenced this pull request in PlummersSoftwareLLC/NightDriverStrip May 19, 2024

Don't use onboard pixel

e7ffc49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement simple RGB driver via digitalWrite; solving #6783 #6808

Implement simple RGB driver via digitalWrite; solving #6783 #6808

PilnyTomas commented May 26, 2022 •

edited by VojtechBartoska

Loading

igrr May 26, 2022

PilnyTomas May 26, 2022

PilnyTomas May 26, 2022

me-no-dev May 27, 2022

me-no-dev May 27, 2022

me-no-dev May 27, 2022

PilnyTomas May 27, 2022

me-no-dev May 27, 2022

PilnyTomas May 27, 2022

me-no-dev May 27, 2022

me-no-dev May 27, 2022

me-no-dev May 27, 2022

me-no-dev May 27, 2022

me-no-dev May 27, 2022

PilnyTomas May 27, 2022

me-no-dev May 27, 2022

me-no-dev Jun 16, 2022

PilnyTomas Jun 17, 2022

igrr Jun 17, 2022

me-no-dev Jun 17, 2022

igrr Jun 17, 2022

		@@ -0,0 +1,51 @@
		#include "esp32-hal-rgb-led.h"

		#ifdef BOARD_HAS_NEOPIXEL

		@@ -0,0 +1,13 @@
		#include "esp32-hal-rgb-led.h"

		extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)

Implement simple RGB driver via digitalWrite; solving #6783 #6808

Implement simple RGB driver via digitalWrite; solving #6783 #6808

Conversation

PilnyTomas commented May 26, 2022 • edited by VojtechBartoska Loading

Description of Change

Tests scenarios

Related links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PilnyTomas commented May 26, 2022 •

edited by VojtechBartoska

Loading