Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jun 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6967d8f
Initial implementation of RGB driver via digitalWrite
PilnyTomas May 25, 2022
cd31356
Moved constants to pins_arduino.h
PilnyTomas May 25, 2022
51ce37d
Changed pin definition + added example
PilnyTomas May 26, 2022
1e363b7
Wrapped BlinkRGB in #ifdef BOARD_HAS_NEOPIXEL
PilnyTomas May 26, 2022
50b6356
Removed forgotten log from example
PilnyTomas May 26, 2022
583e030
Moved RGBLedWrite to new file esp32-hal-rgb-led and created pinMode i…
PilnyTomas May 26, 2022
8786e83
Updated example - lowered single channel brightness to LED_BRIGHTNESS
PilnyTomas May 27, 2022
10205b0
Changed function name from RGBLedWrite to neopixelWrite + code polishing
PilnyTomas May 27, 2022
c0aa3b8
Moved pinSetup portion related to RGB back to common file
PilnyTomas Jun 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Moved RGBLedWrite to new file esp32-hal-rgb-led and created pinMode i… 
…n variatn.cpp
  • Loading branch information
@PilnyTomas
PilnyTomas committed May 26, 2022
commit 583e0301427b6e98a87c3fa94fa100d2fc19cb7f
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ set(CORE_SRCS
cores/esp32/esp32-hal-matrix.c
cores/esp32/esp32-hal-misc.c
cores/esp32/esp32-hal-psram.c
cores/esp32/esp32-hal-rgb-led.c
cores/esp32/esp32-hal-sigmadelta.c
cores/esp32/esp32-hal-spi.c
cores/esp32/esp32-hal-time.c
Expand Down
54 changes: 0 additions & 54 deletions cores/esp32/esp32-hal-gpio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,6 @@ 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
if (pin == LED_BUILTIN){
__pinMode(LED_BUILTIN-SOC_GPIO_PIN_COUNT, mode);
return;
}
#endif

if (!GPIO_IS_VALID_GPIO(pin)) {
log_e("Invalid pin selected");
return;
Expand Down Expand Up @@ -132,53 +125,6 @@ extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)
}
}

#ifdef BOARD_HAS_NEOPIXEL
void RGBLedWrite(uint8_t pin, uint8_t red_val, uint8_t green_val, uint8_t blue_val){
rmt_data_t led_data[24];
static rmt_obj_t* rmt_send = NULL;
static bool initialized = false;

uint8_t _pin;
if(pin == LED_BUILTIN){
_pin = LED_BUILTIN-SOC_GPIO_PIN_COUNT;
}else{
_pin = pin;
}

if(!initialized){
if((rmt_send = rmtInit(_pin, RMT_TX_MODE, RMT_MEM_64)) == NULL){
log_e("RGB LED driver initialization failed!");
rmt_send = NULL;
return;
}
rmtSetTick(rmt_send, 100);
initialized = true;
}

int color[] = {green_val, red_val, blue_val}; // Color coding is in order GREEN, RED, BLUE
int i = 0;
for(int col=0; col<3; col++ ){
for(int bit=0; bit<8; bit++){
if((color[col] & (1<<(7-bit)))){
// HIGH bit
led_data[i].level0 = 1; // T1H
led_data[i].duration0 = 8; // 0.8us
led_data[i].level1 = 0; // T1L
led_data[i].duration1 = 4; // 0.4us
}else{
// LOW bit
led_data[i].level0 = 1; // T0H
led_data[i].duration0 = 4; // 0.4us
led_data[i].level1 = 0; // T0L
led_data[i].duration1 = 8; // 0.8us
}
i++;
}
}
rmtWrite(rmt_send, led_data, 24);
}
#endif

extern void ARDUINO_ISR_ATTR __digitalWrite(uint8_t pin, uint8_t val)
{
#ifdef BOARD_HAS_NEOPIXEL
Expand Down
3 changes: 0 additions & 3 deletions cores/esp32/esp32-hal-gpio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,6 @@ extern "C" {
#define digitalPinToDacChannel(pin) (((pin) == DAC_CHANNEL_1_GPIO_NUM)?0:((pin) == DAC_CHANNEL_2_GPIO_NUM)?1:-1)

void pinMode(uint8_t pin, uint8_t mode);
#ifdef BOARD_HAS_NEOPIXEL
void RGBLedWrite(uint8_t pin, uint8_t red_val, uint8_t green_val, uint8_t blue_val);
#endif
void digitalWrite(uint8_t pin, uint8_t val);
int digitalRead(uint8_t pin);

Expand Down
51 changes: 51 additions & 0 deletions cores/esp32/esp32-hal-rgb-led.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#include "esp32-hal-rgb-led.h"

#ifdef BOARD_HAS_NEOPIXEL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rmt_data_t led_data[24];
static rmt_obj_t* rmt_send = NULL;
static bool initialized = false;

uint8_t _pin;
if(pin == LED_BUILTIN){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_pin = LED_BUILTIN-SOC_GPIO_PIN_COUNT;
}else{
_pin = pin;
}

if(!initialized){
if((rmt_send = rmtInit(_pin, RMT_TX_MODE, RMT_MEM_64)) == NULL){
log_e("RGB LED driver initialization failed!");
rmt_send = NULL;
return;
}
rmtSetTick(rmt_send, 100);
initialized = true;
}

int color[] = {green_val, red_val, blue_val}; // Color coding is in order GREEN, RED, BLUE
int i = 0;
for(int col=0; col<3; col++ ){
for(int bit=0; bit<8; bit++){
if((color[col] & (1<<(7-bit)))){
// HIGH bit
led_data[i].level0 = 1; // T1H
led_data[i].duration0 = 8; // 0.8us
led_data[i].level1 = 0; // T1L
led_data[i].duration1 = 4; // 0.4us
}else{
// LOW bit
led_data[i].level0 = 1; // T0H
led_data[i].duration0 = 4; // 0.4us
led_data[i].level1 = 0; // T0L
led_data[i].duration1 = 8; // 0.8us
}
i++;
}
}
rmtWrite(rmt_send, led_data, 24);
}

#endif
22 changes: 22 additions & 0 deletions cores/esp32/esp32-hal-rgb-led.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#ifndef MAIN_ESP32_HAL_RGB_LED_H_
#define MAIN_ESP32_HAL_RGB_LED_H_

#ifdef __cplusplus
extern "C" {
#endif

#include "esp32-hal.h"
/*
#include "soc/soc_caps.h"
#include "pins_arduino.h"
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have safeguard against LED_BRIGHTNESS not being defined:

#ifndef LED_BRIGHTNESS
#define LED_BRIGHTNESS 64
#endif

//#ifdef BOARD_HAS_NEOPIXEL
void RGBLedWrite(uint8_t pin, uint8_t red_val, uint8_t green_val, uint8_t blue_val);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

//#endif

#ifdef __cplusplus
}
#endif

#endif /* MAIN_ESP32_HAL_RGB_LED_H_ */
1 change: 1 addition & 0 deletions cores/esp32/esp32-hal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ void yield(void);
#include "esp32-hal-timer.h"
#include "esp32-hal-bt.h"
#include "esp32-hal-psram.h"
#include "esp32-hal-rgb-led.h"
#include "esp32-hal-cpu.h"

void analogWrite(uint8_t pin, int value);
Expand Down
13 changes: 13 additions & 0 deletions variants/esp32c3/variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#include "esp32-hal-rgb-led.h"

extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to implement this #6808 (review)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
log_d("foo");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgotten?

#ifdef BOARD_HAS_NEOPIXEL
if (pin == LED_BUILTIN){
__pinMode(LED_BUILTIN-SOC_GPIO_PIN_COUNT, mode);
return;
}
#endif
__pinMode(pin, mode);
}
13 changes: 13 additions & 0 deletions variants/esp32s2/variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#include "esp32-hal-rgb-led.h"

extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)
{
log_d("foo");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgotten?

#ifdef BOARD_HAS_NEOPIXEL
if (pin == LED_BUILTIN){
__pinMode(LED_BUILTIN-SOC_GPIO_PIN_COUNT, mode);
return;
}
#endif
__pinMode(pin, mode);
}
13 changes: 13 additions & 0 deletions variants/esp32s3/variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#include "esp32-hal-rgb-led.h"

extern void ARDUINO_ISR_ATTR __pinMode(uint8_t pin, uint8_t mode)
{
log_d("foo");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgotten?

#ifdef BOARD_HAS_NEOPIXEL
if (pin == LED_BUILTIN){
__pinMode(LED_BUILTIN-SOC_GPIO_PIN_COUNT, mode);
return;
}
#endif
__pinMode(pin, mode);
}