Support for Top and Bottom Color LED deck variants #1553
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the Color LED deck driver to support multiple deck instances (bottom and top) simultaneously. The driver transitions from static global variables to a context-based architecture that maintains separate state for each deck instance.
Key changes:
- Introduces a
colorLedContext_tstructure to encapsulate per-instance state - Creates separate deck drivers for bottom (PID 0x13) and top (PID 0x14) deck positions
- Updates parameter and log groups to use instance-specific names (
colorLedBotandcolorLedTop)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/deck/drivers/src/color_led_deck.c | Refactors driver from global state to context-based architecture supporting two deck instances with separate parameters, logs, and I2C addressing |
| examples/app_color_led_cycle/src/led_cycle.c | Updates example to detect which deck instance is attached and use the appropriate parameter/log groups |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
For this to work with both simultaneously we need unique I2C addresses for top and bottom. Converting this to draft until then. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| static wrgb_t output; | ||
| if (brightnessCorr) { | ||
| wrgb_t output; |
There was a problem hiding this comment.
The output variable is now declared inside the conditional block but is used after the block ends. While this works in C99, the variable should be declared before the conditional to improve code clarity, or the usage should be moved inside appropriate scopes. Consider moving the declaration to line 373 before the conditional.
|
I2C address changing not working consistently |
…ion for bottom and top variants
- Configure GPIO 11 for I2C address selection on top deck only - Make debug messages more specific and unique for easier troubleshooting - Clarify GPIO 0 and GPIO 11 purposes in error messages GPIO 11 controls the I2C address to differentiate between top and bottom decks when both are installed, so it only needs to be configured for the top deck variant.
Red gets 1.54× more current (lower V_F despite higher R_sense). Sets physics-based baseline; perceptual result may need tuning.
- Add CMD_GET_LED_POSITION and CMD_GET_LED_CURRENT commands - Bump protocol version requirement to v2 - Add ledPosition and ledCurrent[4] fields to context - Refactor task loop to use dedicated polling functions - Poll LED current every 1000ms, thermal status every 100ms - Add comprehensive documentation for all parameters and logs - Increase RXBUFFERSIZE to 9 bytes for larger responses
gemenerik
force-pushed
the
rik/color_led_deck
branch
from
11b8b52 to
6ece68b
Compare
|
Rebasing on / merging with master was tough, dropping this PR in favor of #1561 |
EDIT: Replaced by #1561
This PR adds proper distinction between the top and bottom variants of the Color LED deck. Previously, the driver treated them as a single generic deck type without differentiating between mounting positions. The driver now supports both variants simultaneously, with each maintaining separate state, parameters, and logging. The parameter and log group names have changed from
colorledtocolorLedBotandcolorLedTop. ThebrightnessCorrparameter was shortened tobrightCorrto stay within firmware name length limits. Detection parameters are nowdeck.bcColorLedBotanddeck.bcColorLedTop. The example app has been updated to detect which variant is present.GPIO initialization now includes proper error handling with improved debug messages. GPIO 11 is now configured for the top deck to set the I2C address selection.
The driver now supports LED position detection and current monitoring. New log variables
ledPos,ledCurR,ledCurG,ledCurB, andledCurWreport the hardware-detected position and real-time LED current in milliamps. Note: LED current readings are PWM-based and only provide stable values at full intensity (when PWM duty cycle is 100%). This feature (and the LED position detection feature) is primarily intended for production testing. For full duty cycle, brightness correction may need to be disabled depending on the color channel being tested. The task loop has been refactored to use dedicated polling functions with separate intervals (thermal: 100ms, current: 1000ms). LED luminance correction now accounts for the red LED's higher actual current due to lower forward voltage. Doxygen documentation has been added for all parameters and log variables.Protocol version requirement bumped to v2 with new commands CMD_GET_LED_POSITION and CMD_GET_LED_CURRENT. RXBUFFERSIZE increased to 9 bytes.
Requires bitcraze/color-led-deck-firmware#3