From f70f45ee677a2a39a759052a356e0c5d82e25424 Mon Sep 17 00:00:00 2001 From: James Laird-Wah Date: Fri, 28 Sep 2018 00:40:18 +1000 Subject: [PATCH] RGB Matrix refactoring to open up for new drivers (#3913) * rgb_matrix: use a driver ops struct This is intended to avoid #ifdef proliferation on adding more drivers, eg. model01, which use different architectures. * rgb_matrix: document driver struct members * rgb_matrix: remove unused LED testing code * rgb_matrix: don't build into IS31x drivers unless being used * rgb_matrix: refactor make config options This ensures that the necessary files are included for any custom RGB_MATRIX_ENABLE value, without having to add entries here for specific boards. This particularly affects model01 because its controller is integrated and won't be used anywhere else, so it's preferable not to put it in common_features.mk. This now validates the value of RGB_MATRIX_ENABLE. It was necessary to fix an error in ergodox_ez rules.mk using the wrong comment separator, yielding an invalid value. * IS31x drivers: don't write the control registers all the time This is only needed when they are changed. This is done in init() and board- or keymap-specific code is free to make further changes. * rgb_matrix: move structs from chip drivers to rgb_matrix_drivers.c This approach is specific to the rgb_matrix functionality, so keep it neatly separated from the raw chip drivers. --- common_features.mk | 24 ++++---- drivers/issi/is31fl3731.c | 1 - drivers/issi/is31fl3733.c | 3 +- keyboards/ergodox_ez/rules.mk | 2 +- quantum/rgb_matrix.c | 113 ++-------------------------------- quantum/rgb_matrix.h | 17 ++++- quantum/rgb_matrix_drivers.c | 82 ++++++++++++++++++++++++ 7 files changed, 113 insertions(+), 129 deletions(-) create mode 100644 quantum/rgb_matrix_drivers.c diff --git a/common_features.mk b/common_features.mk index 7af7789808..6c835abde6 100644 --- a/common_features.mk +++ b/common_features.mk @@ -114,37 +114,35 @@ ifeq ($(strip $(RGBLIGHT_ENABLE)), yes) endif endif -ifeq ($(strip $(RGB_MATRIX_ENABLE)), yes) +RGB_MATRIX_ENABLE ?= no +VALID_MATRIX_TYPES := yes IS31FL3731L IS31FL3733L custom +ifneq ($(strip $(RGB_MATRIX_ENABLE)), no) +ifeq ($(filter $(RGB_MATRIX_ENABLE),$(VALID_MATRIX_TYPES)),) + $(error RGB_MATRIX_ENABLE="$(RGB_MATRIX_ENABLE)" is not a valid matrix type) +endif OPT_DEFS += -DRGB_MATRIX_ENABLE - OPT_DEFS += -DIS31FL3731 - COMMON_VPATH += $(DRIVER_PATH)/issi - SRC += is31fl3731.c - SRC += i2c_master.c SRC += $(QUANTUM_DIR)/color.c SRC += $(QUANTUM_DIR)/rgb_matrix.c + SRC += $(QUANTUM_DIR)/rgb_matrix_drivers.c CIE1931_CURVE = yes endif +ifeq ($(strip $(RGB_MATRIX_ENABLE)), yes) + RGB_MATRIX_ENABLE = IS31FL3731 +endif + ifeq ($(strip $(RGB_MATRIX_ENABLE)), IS31FL3731) - OPT_DEFS += -DRGB_MATRIX_ENABLE OPT_DEFS += -DIS31FL3731 COMMON_VPATH += $(DRIVER_PATH)/issi SRC += is31fl3731.c SRC += i2c_master.c - SRC += $(QUANTUM_DIR)/color.c - SRC += $(QUANTUM_DIR)/rgb_matrix.c - CIE1931_CURVE = yes endif ifeq ($(strip $(RGB_MATRIX_ENABLE)), IS31FL3733) - OPT_DEFS += -DRGB_MATRIX_ENABLE OPT_DEFS += -DIS31FL3733 COMMON_VPATH += $(DRIVER_PATH)/issi SRC += is31fl3733.c SRC += i2c_master.c - SRC += $(QUANTUM_DIR)/color.c - SRC += $(QUANTUM_DIR)/rgb_matrix.c - CIE1931_CURVE = yes endif ifeq ($(strip $(TAP_DANCE_ENABLE)), yes) diff --git a/drivers/issi/is31fl3731.c b/drivers/issi/is31fl3731.c index 4d0d6b8a5e..c9155f5a37 100644 --- a/drivers/issi/is31fl3731.c +++ b/drivers/issi/is31fl3731.c @@ -268,4 +268,3 @@ void IS31FL3731_update_led_control_registers( uint8_t addr1, uint8_t addr2 ) } } } - diff --git a/drivers/issi/is31fl3733.c b/drivers/issi/is31fl3733.c index 4098b54689..c198ec5174 100644 --- a/drivers/issi/is31fl3733.c +++ b/drivers/issi/is31fl3733.c @@ -24,10 +24,10 @@ #include "wait.h" #endif -#include "is31fl3733.h" #include #include "i2c_master.h" #include "progmem.h" +#include "rgb_matrix.h" // This is a 7-bit address, that gets left-shifted and bit 0 // set to 0 for write, 1 for read (as per I2C protocol) @@ -250,4 +250,3 @@ void IS31FL3733_update_led_control_registers( uint8_t addr1, uint8_t addr2 ) } } } - diff --git a/keyboards/ergodox_ez/rules.mk b/keyboards/ergodox_ez/rules.mk index dfbdba10d6..ef2aefbb9a 100644 --- a/keyboards/ergodox_ez/rules.mk +++ b/keyboards/ergodox_ez/rules.mk @@ -83,6 +83,6 @@ SWAP_HANDS_ENABLE= yes # Allow swapping hands of keyboard SLEEP_LED_ENABLE = no API_SYSEX_ENABLE = no RGBLIGHT_ENABLE = yes -RGB_MATRIX_ENABLE = no // enable later +RGB_MATRIX_ENABLE = no # enable later LAYOUTS = ergodox diff --git a/quantum/rgb_matrix.c b/quantum/rgb_matrix.c index 5fb59af8c0..807e4d62d1 100644 --- a/quantum/rgb_matrix.c +++ b/quantum/rgb_matrix.c @@ -18,7 +18,6 @@ #include "rgb_matrix.h" -#include "i2c_master.h" #include "progmem.h" #include "config.h" #include "eeprom.h" @@ -111,29 +110,15 @@ void map_row_column_to_led( uint8_t row, uint8_t column, uint8_t *led_i, uint8_t } void rgb_matrix_update_pwm_buffers(void) { -#ifdef IS31FL3731 - IS31FL3731_update_pwm_buffers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); - IS31FL3731_update_led_control_registers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); -#elif defined(IS31FL3733) - IS31FL3733_update_pwm_buffers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); - IS31FL3733_update_led_control_registers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); -#endif + rgb_matrix_driver.flush(); } void rgb_matrix_set_color( int index, uint8_t red, uint8_t green, uint8_t blue ) { -#ifdef IS31FL3731 - IS31FL3731_set_color( index, red, green, blue ); -#elif defined(IS31FL3733) - IS31FL3733_set_color( index, red, green, blue ); -#endif + rgb_matrix_driver.set_color(index, red, green, blue); } void rgb_matrix_set_color_all( uint8_t red, uint8_t green, uint8_t blue ) { -#ifdef IS31FL3731 - IS31FL3731_set_color_all( red, green, blue ); -#elif defined(IS31FL3733) - IS31FL3733_set_color_all( red, green, blue ); -#endif + rgb_matrix_driver.set_color_all(red, green, blue); } bool process_rgb_matrix(uint16_t keycode, keyrecord_t *record) { @@ -196,47 +181,6 @@ void rgb_matrix_test(void) { } } -// This tests the LEDs -// Note that it will change the LED control registers -// in the LED drivers, and leave them in an invalid -// state for other backlight effects. -// ONLY USE THIS FOR TESTING LEDS! -void rgb_matrix_single_LED_test(void) { - static uint8_t color = 0; // 0,1,2 for R,G,B - static uint8_t row = 0; - static uint8_t column = 0; - - static uint8_t tick = 0; - tick++; - - if ( tick > 2 ) - { - tick = 0; - column++; - } - if ( column > MATRIX_COLS ) - { - column = 0; - row++; - } - if ( row > MATRIX_ROWS ) - { - row = 0; - color++; - } - if ( color > 2 ) - { - color = 0; - } - - uint8_t led[8], led_count; - map_row_column_to_led(row,column,led,&led_count); - for(uint8_t i = 0; i < led_count; i++) { - rgb_matrix_set_color_all( 40, 40, 40 ); - rgb_matrix_test_led( led[i], color==0, color==1, color==2 ); - } -} - // All LEDs off void rgb_matrix_all_off(void) { rgb_matrix_set_color_all( 0, 0, 0 ); @@ -817,7 +761,7 @@ void rgb_matrix_indicators_user(void) {} // } void rgb_matrix_init(void) { - rgb_matrix_setup_drivers(); + rgb_matrix_driver.init(); // TODO: put the 1 second startup delay here? @@ -841,33 +785,6 @@ void rgb_matrix_init(void) { eeconfig_debug_rgb_matrix(); // display current eeprom values } -void rgb_matrix_setup_drivers(void) { - // Initialize TWI - i2c_init(); -#ifdef IS31FL3731 - IS31FL3731_init( DRIVER_ADDR_1 ); - IS31FL3731_init( DRIVER_ADDR_2 ); -#elif defined (IS31FL3733) - IS31FL3733_init( DRIVER_ADDR_1 ); -#endif - - for ( int index = 0; index < DRIVER_LED_TOTAL; index++ ) { - bool enabled = true; - // This only caches it for later -#ifdef IS31FL3731 - IS31FL3731_set_led_control_register( index, enabled, enabled, enabled ); -#elif defined (IS31FL3733) - IS31FL3733_set_led_control_register( index, enabled, enabled, enabled ); -#endif - } - // This actually updates the LED drivers -#ifdef IS31FL3731 - IS31FL3731_update_led_control_registers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); -#elif defined (IS31FL3733) - IS31FL3733_update_led_control_registers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); -#endif -} - // Deals with the messy details of incrementing an integer uint8_t increment( uint8_t value, uint8_t step, uint8_t min, uint8_t max ) { int16_t new_value = value; @@ -910,28 +827,6 @@ uint8_t decrement( uint8_t value, uint8_t step, uint8_t min, uint8_t max ) { // } // } -void rgb_matrix_test_led( uint8_t index, bool red, bool green, bool blue ) { - for ( int i=0; i. + */ + +#include "rgb_matrix.h" + +/* Each driver needs to define the struct + * const rgb_matrix_driver_t rgb_matrix_driver; + * All members must be provided. + * Keyboard custom drivers can define this in their own files, it should only + * be here if shared between boards. + */ + +#if defined(IS31FL3731) || defined(IS31FL3733) + +#include "i2c_master.h" + +static void init( void ) +{ + i2c_init(); +#ifdef IS31FL3731 + IS31FL3731_init( DRIVER_ADDR_1 ); + IS31FL3731_init( DRIVER_ADDR_2 ); +#else + IS31FL3733_init( DRIVER_ADDR_1 ); +#endif + for ( int index = 0; index < DRIVER_LED_TOTAL; index++ ) { + bool enabled = true; + // This only caches it for later +#ifdef IS31FL3731 + IS31FL3731_set_led_control_register( index, enabled, enabled, enabled ); +#else + IS31FL3733_set_led_control_register( index, enabled, enabled, enabled ); +#endif + } + // This actually updates the LED drivers +#ifdef IS31FL3731 + IS31FL3731_update_led_control_registers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); +#else + IS31FL3733_update_led_control_registers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); +#endif +} + +#ifdef IS31FL3731 +static void flush( void ) +{ + IS31FL3731_update_pwm_buffers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); +} + +const rgb_matrix_driver_t rgb_matrix_driver = { + .init = init, + .flush = flush, + .set_color = IS31FL3731_set_color, + .set_color_all = IS31FL3731_set_color_all, +}; +#else +static void flush( void ) +{ + IS31FL3733_update_pwm_buffers( DRIVER_ADDR_1, DRIVER_ADDR_2 ); +} + +const rgb_matrix_driver_t rgb_matrix_driver = { + .init = init, + .flush = flush, + .set_color = IS31FL3733_set_color, + .set_color_all = IS31FL3733_set_color_all, +}; +#endif + +#endif