Usermod SbusControl #4464
Usermod SbusControl #4464
Conversation
Add a SBusControl UserMod to control brightness and presets via a RC Remote with SBus receiver
Add a SBusControl UserMod to control brightness and presets via a RC Remote with SBus receiver
|
Thank you for your submission. Can you please add a little more detail to your readme file so that users understand exactly what this usermod is for and how to use it please |
wled00/usermods_list.cpp
Outdated
| @@ -486,4 +490,8 @@ void registerUsermods() | |||
| #ifdef USERMOD_RF433 | |||
| UsermodManager::add(new RF433Usermod()); | |||
| #endif | |||
|
|
|||
| #ifdef USERMOD_SBUS_CONTROL | |||
| usermods.add(new SbusControl()); | |||
wled00/usermods_list.cpp
Outdated
| @@ -250,6 +250,10 @@ | |||
| #include "../usermods/usermod_v2_RF433/usermod_v2_RF433.h" | |||
| #endif | |||
|
|
|||
| #ifdef USERMOD_SBUS_CONTROL | |||
| #include "../usermods/sbus_control/usermod_sbus_control.h" | |||
|
|
||
| uint8_t getSwitchPosition(int16_t ch_value) | ||
| { | ||
| Serial.println(ch_value); |
| uint8_t last_mode_ = 1; | ||
| uint8_t last_bri_ = 1; | ||
|
|
||
| bfs::SbusRx sbus_rx_= {&Serial1, static_cast<int8_t>(sbus_pin_), 0, true}; |
There was a problem hiding this comment.
use pin manager to reserver the pin (or it can be assigned to other functions as well, leading to conflicts)
|
|
||
| void setup() override | ||
| { | ||
| sbus_rx_ = bfs::SbusRx(&Serial1, static_cast<int8_t>(sbus_pin_), 0, true); |
There was a problem hiding this comment.
this will also assign GPIO0 to serial, intentional or an oversight?
also: Serial1 is used by DMX as well. @netmindz can Serial1 be used here without restrictions? If not: there is another UM doing it this way that may need updating.
WalkthroughThe pull request introduces a new SBUS control usermod within WLED. The update adds documentation and a C++ class to handle SBUS input for controlling LED brightness and modes. It also integrates the usermod into the system by defining a new usermod ID in global configuration files, updating an enum for pin ownership, and registering the usermod for use. Changes
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
usermods/sbus_control/usermod_sbus_control.h (1)
13-21: 🛠️ Refactor suggestion❓ Verification inconclusive
Use pin manager to allocate pins and add debug output.
The pin allocation looks good, but there are a few improvements needed:
- Add more debug output to help with troubleshooting.
- Consider potential conflicts with DMX that also uses Serial1.
Apply this diff:
PinManagerPinType pins[2] = { { sbus_pin_rx_, false }, { sbus_pin_tx_, true } }; if (!PinManager::allocateMultiplePins(pins, 2, PinOwner::UM_SBUS_CONTROL)) { - DEBUG_PRINTF("SBUS_CONTROL pin allocation failed! Pin rx %d, Pin tx %d\n", sbus_pin_rx_, sbus_pin_tx_); + DEBUG_PRINTF("[SBUS] Pin allocation failed! Pin rx %d, Pin tx %d\n", sbus_pin_rx_, sbus_pin_tx_); return; } +DEBUG_PRINTF("[SBUS] Pins allocated successfully. Pin rx %d, Pin tx %d\n", sbus_pin_rx_, sbus_pin_tx_);Run this script to check for potential conflicts with DMX:
🏁 Script executed:
#!/bin/bash # Description: Check if DMX usermod is using Serial1 # Search for DMX usermod using Serial1 rg -A 5 'Serial1.*DMX'Length of output: 1106
Enhance Debug Logging & Verify DMX Serial1 Conflict
The pin allocation code now correctly uses the pin manager and improved debug messages are added. However, please note that DMX code (in
wled00/src/dependencies/dmx/ESPDMX.cpp) also initializes and usesSerial1, so ensure that running both SBUS and DMX concurrently won’t cause conflicts.
- The updated diff snippet adds both failure and success debug logs.
- Action: Double-check the initialization and usage of Serial1 in both SBUS and DMX modules to avoid resource conflicts.
PinManagerPinType pins[2] = { { sbus_pin_rx_, false }, { sbus_pin_tx_, true } }; if (!PinManager::allocateMultiplePins(pins, 2, PinOwner::UM_SBUS_CONTROL)) { - DEBUG_PRINTF("SBUS_CONTROL pin allocation failed! Pin rx %d, Pin tx %d\n", sbus_pin_rx_, sbus_pin_tx_); + DEBUG_PRINTF("[SBUS] Pin allocation failed! Pin rx %d, Pin tx %d\n", sbus_pin_rx_, sbus_pin_tx_); return; } +DEBUG_PRINTF("[SBUS] Pins allocated successfully. Pin rx %d, Pin tx %d\n", sbus_pin_rx_, sbus_pin_tx_);
🧹 Nitpick comments (6)
usermods/sbus_control/usermod_sbus_control.h (2)
56-62: Add error handling for invalid switch positions.The switch position calculation needs validation and error handling.
Apply this diff:
uint8_t swicth_1 = getSwitchPosition(data.ch[ch_mode_select_1_ - 1]); uint8_t swicth_2 = getSwitchPosition(data.ch[ch_mode_select_2_ - 1]); +if (swicth_1 > 2 || swicth_2 > 2) { + DEBUG_PRINTF("[SBUS] Invalid switch position: sw1=%d, sw2=%d\n", swicth_1, swicth_2); + return; +} uint8_t mode = swicth_1 + (swicth_2 * 3) + 1;Also, fix the typo in variable names:
-uint8_t swicth_1 = getSwitchPosition(data.ch[ch_mode_select_1_ - 1]); -uint8_t swicth_2 = getSwitchPosition(data.ch[ch_mode_select_2_ - 1]); +uint8_t switch_1 = getSwitchPosition(data.ch[ch_mode_select_1_ - 1]); +uint8_t switch_2 = getSwitchPosition(data.ch[ch_mode_select_2_ - 1]);
126-132: Add documentation for switch position calculation.The switch position calculation logic needs documentation to explain the value ranges.
Apply this diff:
uint8_t getSwitchPosition(int16_t ch_value) { - // Decoding a switch position from ch_values + // Decoding a 3-position switch from SBUS channel value (172-1811): + // - Low position (0): 172-666 + // - Middle position (1): 667-1333 + // - High position (2): 1334-1811 if (ch_value < 666) return 0; if (ch_value < 1333) return 1; return 2; }usermods/sbus_control/readme.md (4)
1-3: Improve clarity and consistency in the introduction.
The introductory sentences would benefit from consistent capitalization (e.g., "SBUS" rather than "sbus") and a correction of potential typos (e.g., "RC plains" should likely be "RC planes"). Also, consider rephrasing “LEDController” to “LED controller” for enhanced readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: Possible missing article found.
Context: ...32 to the receiver, WLED can be used as LEDController in RC plains, cars or ships. You can ...(AI_HYDRA_LEO_MISSING_A)
5-6: Clarify control instructions and fix grammatical errors.
The sentence "You can change the brightness and between 9 presets." seems to be missing a verb—consider updating it to "You can adjust the brightness and switch between 9 presets." Additionally, standardize terms such as "Leds" to "LEDs" and clarify "APP or WebUi" to "the app or Web UI".
7-11: Enhance build instructions for clarity and proper formatting.
Consider rephrasing the instruction to:
"To build this user mod, add the following lines to your target in the platformio.ini file:"
This change addresses the missing comma and subject-verb agreement (use "need" instead of "needs"). Also, reformat the URL on line 10 using markdown—for example,[sbus library](https://github.com/bolderflight/sbus.git)—to comply with markdown best practices and improve readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...e done via APP or WebUi. To build this usermod the following lines needs to be added t...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
10-10: Bare URL used
null(MD034, no-bare-urls)
12-15: Improve hardware setup instructions for clarity.
Reword the hardware instructions to avoid ambiguity. For example, consider:
"Connect the receiver’s ground to the ESP32 ground. Connect the receiver’s SBUS output (sbus_out) to the ESP32 pin configured for SBUS input (sbus_in) in your configuration."
This revision resolves repetitive wording and ensures clarity for users following the setup.🧰 Tools
🪛 LanguageTool
[duplication] ~15-~15: Possible typo: you repeated a word.
Context: ... pin with ESP32 pin configured for sbus_in in config.(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
usermods/sbus_control/readme.md(1 hunks)usermods/sbus_control/usermod_sbus_control.h(1 hunks)wled00/const.h(1 hunks)wled00/pin_manager.h(1 hunks)wled00/usermods_list.cpp(2 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/sbus_control/readme.md
[uncategorized] ~3-~3: Possible missing article found.
Context: ...32 to the receiver, WLED can be used as LEDController in RC plains, cars or ships. You can ...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...e done via APP or WebUi. To build this usermod the following lines needs to be added t...
(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~15-~15: Possible typo: you repeated a word.
Context: ... pin with ESP32 pin configured for sbus_in in config.
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
usermods/sbus_control/readme.md
10-10: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (3)
wled00/pin_manager.h (1)
76-77: LGTM!The new enumerator follows the existing pattern and is correctly associated with the usermod ID.
wled00/usermods_list.cpp (1)
257-259: LGTM!The include directive and usermod registration follow the existing pattern.
Also applies to: 502-504
wled00/const.h (1)
209-209: LGTM!The new usermod ID follows the existing pattern.
| static constexpr auto* kConfigNodeMode2Ch PROGMEM = "ModeSelect2Channel"; | ||
| static constexpr auto* kConfigNodeSbusPinRx PROGMEM = "SbusPinRx"; | ||
| static constexpr auto* kConfigNodeSbusPinTx PROGMEM = "SbusPinTx"; | ||
| static constexpr auto* kConfigNodeUpdatePeriod PROGMEM = "UpdatePrtiodMs"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo in configuration key.
There's a typo in the configuration key for update period.
Apply this diff:
- static constexpr auto* kConfigNodeUpdatePeriod PROGMEM = "UpdatePrtiodMs";
+ static constexpr auto* kConfigNodeUpdatePeriod PROGMEM = "UpdatePeriodMs";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static constexpr auto* kConfigNodeUpdatePeriod PROGMEM = "UpdatePrtiodMs"; | |
| static constexpr auto* kConfigNodeUpdatePeriod PROGMEM = "UpdatePeriodMs"; |
| int16_t brightness = data.ch[ch_brightness_ - 1]; | ||
| brightness = std::min(brightness, static_cast<int16_t>(2000)); | ||
| brightness -= 250; | ||
| brightness = std::max(brightness, static_cast<int16_t>(0)); | ||
| brightness /= 7; | ||
| if (brightness != last_bri_) | ||
| { | ||
| bri = brightness; | ||
| applyFinalBri(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking for brightness calculation.
The brightness calculation needs bounds checking to prevent potential issues with invalid values.
Apply this diff:
int16_t brightness = data.ch[ch_brightness_ - 1];
+if (brightness < 0 || brightness > 2047) {
+ DEBUG_PRINTF("[SBUS] Invalid brightness value: %d\n", brightness);
+ return;
+}
brightness = std::min(brightness, static_cast<int16_t>(2000));
brightness -= 250;
brightness = std::max(brightness, static_cast<int16_t>(0));
brightness /= 7;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int16_t brightness = data.ch[ch_brightness_ - 1]; | |
| brightness = std::min(brightness, static_cast<int16_t>(2000)); | |
| brightness -= 250; | |
| brightness = std::max(brightness, static_cast<int16_t>(0)); | |
| brightness /= 7; | |
| if (brightness != last_bri_) | |
| { | |
| bri = brightness; | |
| applyFinalBri(); | |
| } | |
| int16_t brightness = data.ch[ch_brightness_ - 1]; | |
| if (brightness < 0 || brightness > 2047) { | |
| DEBUG_PRINTF("[SBUS] Invalid brightness value: %d\n", brightness); | |
| return; | |
| } | |
| brightness = std::min(brightness, static_cast<int16_t>(2000)); | |
| brightness -= 250; | |
| brightness = std::max(brightness, static_cast<int16_t>(0)); | |
| brightness /= 7; | |
| if (brightness != last_bri_) | |
| { | |
| bri = brightness; | |
| applyFinalBri(); | |
| } |
| @@ -251,7 +251,11 @@ | |||
| #endif | |||
|
|
|||
| #ifdef USERMOD_BRIGHTNESS_FOLLOW_SUN | |||
| #include "../usermods/usermod_v2_brightness_follow_sun/usermod_v2_brightness_follow_sun.h" | |||
| #include "../usermods/usermod_v2_brightness_follow_sun/usermod_v2_brightness_follow_sun.h" | |||
There was a problem hiding this comment.
please revert unnecessary change
Adding a usermod to control WLED via a RCRemote with a sbus receiver connected to an ESP32.
Brightness can be changed.
It can be switched between 9 Presets.
Summary by CodeRabbit
These enhancements expand LED control capabilities with streamlined integration and clear, user-friendly guidance for setup and operation.