Fix up a few more usermods #4601
Conversation
I've just assigned an arbitrary ID number. The ID system should get removed entirely; we don't want to have to change a base system header to add each module.
Rename a macro that unfortunately collides with the ESP32 platform. This allows re-enabling this usermod.
The String constructors are ambiguous with uint8_ts.
WalkthroughThis pull request updates several usermod files across different modules. In the BME68X module, debug message macros were renamed, and the file header comment was corrected. In the MAX17048 module, data types for voltage and percent decimals were changed, and the README was revised to replace library dependency instructions with a custom configuration directive. The HttpPullLightControl module now uses a pointer for JSON deserialization, and the Word Clock Matrix module features a variable rename, a simplified preset method, and an updated ID return value. Significant modifications were made to the BH1750 module, including the introduction of a new class and restructuring of methods for MQTT integration. The SN_Photoresistor module also saw the addition of a new class and changes in how sensor readings are managed. Several other modules introduced new files, modified existing configurations, and updated their JSON structures. Changes
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (2)
usermods/MAX17048_v2/readme.md (1)
17-17: Inconsistency in example configuration.The example shows adding
AHT10to custom_usermods but the usermod being documented isMAX17048_v2. This could be confusing for users.- custom_usermods = ${env:d1_mini.custom_usermods} AHT10 + custom_usermods = ${env:d1_mini.custom_usermods} MAX17048_v2usermods/BME68X_v2/BME68X_v2.cpp (1)
1094-1095: Suggestion: ImproveDEBUG_PRINTFFormatting
TheDEBUG_PRINTFstatement insaveState()concatenates string literals with theOK_MSGmacro directly, which might lead to missing spacing in the output. Consider revising it for clarity. For example:- DEBUG_PRINTF("(saved %d times)" OK_MSG "\n", stateUpdateCounter); + DEBUG_PRINTF("(saved %d times %s)\n", stateUpdateCounter, OK_MSG);This adjustment ensures a clear separation between the saved count message and the success indicator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
usermods/BME68X_v2/BME68X_v2.cpp(12 hunks)usermods/MAX17048_v2/MAX17048_v2.cpp(1 hunks)usermods/MAX17048_v2/readme.md(1 hunks)usermods/usermod_v2_HttpPullLightControl/usermod_v2_HttpPullLightControl.cpp(1 hunks)usermods/word-clock-matrix/word-clock-matrix.cpp(3 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/MAX17048_v2/readme.md
[uncategorized] ~13-~13: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...enable, compile with USERMOD_MAX17048 define in the build_flags (e.g. in `platformio...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: wled_build / Build Enviornments (usermods)
🔇 Additional comments (16)
usermods/MAX17048_v2/readme.md (1)
12-12: Simplified configuration approach.You've changed the configuration to use
custom_usermodsinstead of requiring external library dependencies, which streamlines the setup process for users.usermods/usermod_v2_HttpPullLightControl/usermod_v2_HttpPullLightControl.cpp (1)
300-303:✅ Verification successful
Switched to pointer for JSON document.
The code now uses a dereferenced pointer
*pDocfor JSON operations instead of a local variable. This change suggests the JSON document is now being managed elsewhere, likely improving memory efficiency.
🏁 Script executed:
#!/bin/bash # Check if pDoc is properly declared elsewhere in the file rg -A 2 -B 2 "JsonDocument\s+\*pDoc" --type cppLength of output: 541
Approved: Global JSON document pointer verified
The usage of
*pDocis correct as the pointer is declared globally inwled00/wled.hand initialized appropriately based on the platform. The changes improve memory management, and no further modifications are needed.usermods/word-clock-matrix/word-clock-matrix.cpp (3)
39-40: Improved variable naming for clarity.Renaming from
segtotext_segbetter clarifies the variable's purpose as representing text segments.
70-70:✅ Verification successful
Method signature changed to simplify calls.
The
savePresetmethod now takes only one parameter instead of two, simplifying the API.
🏁 Script executed:
#!/bin/bash # Check the declaration of savePreset method and other calls to it rg "savePreset\(" --type cpp usermods/word-clock-matrixLength of output: 126
Simplified savePreset API Usage Verified
ThesavePresetcall inusermods/word-clock-matrix/word-clock-matrix.cppnow correctly uses a single parameter (savePreset(13)), which matches the updated one-parameter method signature. No additional adjustments are needed.
332-332:❓ Verification inconclusive
Changed usermod ID from constant to hardcoded value.
The ID has been changed from using a constant
USERMOD_ID_WORD_CLOCK_MATRIXto a hardcoded value500. This could potentially cause conflicts if another usermod uses the same ID.
🏁 Script executed:
#!/bin/bash # Check if other usermods use ID 500 or if there's a constant definition rg "return\s+500" --type cpp usermods rg "USERMOD_ID_WORD_CLOCK_MATRIX" --type cpp # Check if there's a usermod ID registry or documentation rg "usermod\s+id" --type cppLength of output: 182
Action Required: Verify Unique Usermod ID Implementation
The commit changes the usermod ID in usermods/word-clock-matrix/word-clock-matrix.cpp (line 332) to a hardcoded value (
500) instead of using the previously defined constant (USERMOD_ID_WORD_CLOCK_MATRIX). While our grep searches did not uncover any duplicate usage or references to the constant elsewhere, we couldn’t conclusively verify that no other usermod inadvertently uses 500. Please double-check the repository (or related documentation) to ensure that this hardcoded value does not conflict with IDs assigned to other usermods. If available, consider defining and using a centralized constant to mitigate future risks.usermods/MAX17048_v2/MAX17048_v2.cpp (1)
38-39: Changed variable types from uint8_t to unsigned.The data types for
VoltageDecimalsandPercentDecimalshave been changed fromuint8_ttounsigned. This modification could potentially allow for larger values, although the practical impact is likely minimal as these represent decimal places.Both types are unsigned integers, but
unsignedis platform-dependent whileuint8_tis explicitly 8 bits. Since these variables are used for decimal places (typically small values), the larger range ofunsignedisn't necessary but doesn't harm the functionality.usermods/BME68X_v2/BME68X_v2.cpp (10)
2-2: Correct File Header Updated
The file header now correctly references the implementation file (usermod_BMW68X.cpp) rather than the header file. This update improves documentation accuracy.
36-39: Renamed Debug Message Macros for Consistency
The debug message macros have been updated to useOK_MSG,FAIL_MSG,WARN_MSG, andDONE_MSGconsistently. This change helps standardize logging output throughout the module.
331-331: UsingFAIL_MSGfor I2C Error Reporting
At line 331 the error message now appendsFAIL_MSGwhen I2C is not activated. This ensures that any failure is logged with the standardized error message format.
355-355: Uniform Success Messaging in Setup
The successful initialization message now usesDONE_MSG(prefixed byINFO_COLUMN) at line 355 to provide consistent feedback.
1028-1034: Standardized Messaging for BSEC Status Errors/Warnings
WithincheckIaqSensorStatus(), the branches handlingiaqSensor.bsecStatusnow append either an error code (usingFAIL_MSG) or a warning code (usingWARN_MSG). This ensures that error conditions are clearly and uniformly logged.
1040-1054: Uniform Debug Logging for BME68X Sensor Status
In the section checkingiaqSensor.bme68xStatus, the revised code usesFAIL_MSGfor error conditions,WARN_MSGfor warnings, andOK_MSGwhen everything is operating normally. This uniform approach enhances consistency and eases troubleshooting.
1064-1072: Improved Calibration File Logging inloadState()
TheloadState()function now logs the status of opening and reading the calibration file usingFAIL_MSGandOK_MSG. This provides consistent and clear feedback during file I/O operations.
753-753: Consistent Confirmation inaddToConfig()
At the end ofaddToConfig(), the use ofDEBUG_PRINTLN(F(OK_MSG));confirms that the configuration content was created successfully, maintaining a uniform logging style.
834-834: Clear Completion Log inreadFromConfig()
PrintingDEBUG_PRINTLN(F(OK_MSG));at line 834 clearly signals the successful reading of configuration values and aligns with the overall debug macro usage.
848-852: Consistent Feedback for Calibration File Deletion
Within the calibration file deletion branch inreadFromConfig(), the use ofDEBUG_PRINTLN(F(OK_MSG));on success andDEBUG_PRINTLN(F(FAIL_MSG));on failure ensures that file deletion outcomes are logged uniformly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
usermods/MAX17048_v2/readme.md (2)
12-12: Clarify the Compilation Directive UpdateThe newly added instruction clearly directs users to add
"MAX17048_v2"to theplatformio.inienvironment'scustom_usermodsfor the build. Ensure that it’s evident that this entry should be appended rather than replacing any existing custom usermods (as demonstrated later in the example).
17-17: Validate the Custom_usermods FormattingThe updated line
custom_usermods = ${env:d1_mini.custom_usermods} MAX17048_v2correctly extends the existing
custom_usermodssetting withMAX17048_v2. Please double-check that using a space as the delimiter meets the expected format for your project's PlatformIO configuration. A comma or another form of delimiter might be required in some environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/MAX17048_v2/readme.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/MAX17048_v2/readme.md
[uncategorized] ~13-~13: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...enable, compile with USERMOD_MAX17048 define in the build_flags (e.g. in `platformio...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
Get this usermod to build, and restore the interconnect with its partner usermod_v2_rotary_encoder_ui_ALT.
|
IMO there are far too many unmaintained (and probably non-functioning) usermods that would be best removed. |
Fix up the cross module binding for usermods/seven_segment_display_reloaded. This requires splitting off headers for BH1750_v2 and SN_Photoresistor.
When debugging, print each usermod's ID as it's loaded.
Break the actual JSON assembly apart from the file writing code. This permits calling it in other contexts, allowing us to pull the live config data even if the filesystem is out of date.
Add a new JSON out target to pull the live config data and use that for the usermod settings page. Should fix issues with outdated contents.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
usermods/seven_segment_display_reloaded/seven_segment_display_reloaded.cpp (1)
412-438: Well-implemented automatic brightness control.This code effectively integrates with either light sensor type when available, using their readings to automatically adjust display brightness. The conditional compilation ensures clean integration without redundant code.
The debug message in the BH1750 section (line 431) provides helpful information for troubleshooting that isn't present in the SN_Photoresistor section. For consistency, consider adding similar debug output to the SN_Photoresistor section as well.
if (ptr != nullptr) { uint16_t lux = ptr->getLastLDRValue(); uint16_t brightness = map(lux, 0, 1000, umSSDRBrightnessMin, umSSDRBrightnessMax); if (bri != brightness) { + DEBUG_PRINTF("Adjusting brightness based on LDR value: %d, new brightness: %d\n", lux, brightness); bri = brightness; stateUpdated(1); } }usermods/usermod_v2_rotary_encoder_ui_ALT/platformio_override.sample.ini (1)
4-6: Validate environment extension.
Extendingenv:esp32dev_V4and appending therotary_encoder_ui_ALTusermod ensures the environment inherits all required flags. Double-check thatenv:esp32dev_V4includes the correct compiler/linker flags for the rotary encoder functionality.Consider documenting the added usermod in a comment or README so new maintainers can quickly identify the purpose and dependencies of this environment.
Also applies to: 8-8
usermods/BH1750_v2/BH1750_v2.h (1)
10-28: Measurement interval configuration.
Defining flexible min/max intervals (500ms to 10s) along with an initial delay (10s) is a good balance of configurability. Ensure that read frequencies are not too frequent for the BH1750 sensor or your application’s performance constraints. Also verify that the offset logic meets your reporting threshold requirements.Consider adding a brief doc comment indicating typical recommended intervals and guidelines for offset adjustments to help future maintainers.
usermods/SN_Photoresistor/SN_Photoresistor.h (1)
45-47: Typo in comment referencing temperature rather than luminance
This comment incorrectly referencesgetTemperatureinstead of luminance or photoresistor reading, suggesting leftover text from another sensor module.- // flag to indicate we have finished the first getTemperature call + // flag to indicate we have finished the first luminance readingusermods/SN_Photoresistor/SN_Photoresistor.cpp (3)
9-12: Validate sensor bounds more thoroughly
WhilecheckBoundSensorchecks if a reading differs by at leastmaxDiffor if the old value was NaN, it doesn’t handle all edge cases. If your sensor occasionally returns spurious large values, consider additional validation or error tolerance to avoid unstable results.
49-51: Check offset logic usage
You are applyingcheckBoundSensor(currentLDRValue, lastLDRValue, offset). Ensureoffsetis scaled consistently with your sensor values (lux?). If your typical readings could be in the hundreds or thousands, an offset of 5 might be too small or too large.
122-122: Potential confusion indisabledlogic
Using!(top[FPSTR(_enabled)] | !disabled);is readable in Arduino JSON syntax but can be confusing. Consider assigning the JSON boolean to a local variable first or using clearer structures so future maintainers can quickly understand it.- disabled = !(top[FPSTR(_enabled)] | !disabled); + bool enabledOption = top[FPSTR(_enabled)] | !disabled; + disabled = !enabledOption;usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h (5)
7-10: Verify ESP32 threading implications.
DefiningFLD_ESP32_USE_THREADSmay introduce concurrency issues if other parts of the system also rely on heavy tasks or tight deadlines. Ensure that essential tasks (e.g., LED updates) are not delayed.
31-37: Clarify default display configuration.
The conditional logic sets different defaults forFLD_TYPE. It might be more transparent to explicitly define a user-facing comment that clarifies each display fallback for both I2C and SPI modes.
51-62: Use consistent naming for display enums.
TheDisplayTypeenum lists device models (e.g.,SSD1306_64) alongside brand-based descriptions (e.g.,SH1106). To avoid confusion, consider a consistent naming scheme or applyingDISPLAYTYPE_prefixes for clarity.
128-140: Promote user-friendly messages.
Storing string constants in_name,_enabled,_contrast, etc., is good for memory usage. For user-friendliness, consider clarifying or localizing these messages (e.g., “Contrast Level” instead of just “contrast”).
141-178: Comment on specialized display knowledge.
Lines 141-149 reference certain display constructors and mention checking the gallery or wiki. This is helpful but might be better placed in an external doc or README, linked from within the code, to prevent code clutter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
usermods/BH1750_v2/BH1750_v2.cpp(1 hunks)usermods/BH1750_v2/BH1750_v2.h(1 hunks)usermods/SN_Photoresistor/SN_Photoresistor.cpp(1 hunks)usermods/SN_Photoresistor/SN_Photoresistor.h(1 hunks)usermods/SN_Photoresistor/usermods_list.cpp(0 hunks)usermods/seven_segment_display_reloaded/library.json(1 hunks)usermods/seven_segment_display_reloaded/setup_deps.py(1 hunks)usermods/seven_segment_display_reloaded/seven_segment_display_reloaded.cpp(1 hunks)usermods/usermod_v2_brightness_follow_sun/library.json(1 hunks)usermods/usermod_v2_brightness_follow_sun/usermod_v2_brightness_follow_sun.cpp(1 hunks)usermods/usermod_v2_four_line_display_ALT/4LD_wled_fonts.h(1 hunks)usermods/usermod_v2_four_line_display_ALT/library.json(1 hunks)usermods/usermod_v2_four_line_display_ALT/library.json.disabled(0 hunks)usermods/usermod_v2_four_line_display_ALT/platformio_override.sample.ini(1 hunks)usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h(1 hunks)usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.cpp(2 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/library.json(1 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/platformio_override.sample.ini(1 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/readme.md(0 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py(1 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp(1 hunks)wled00/cfg.cpp(3 hunks)wled00/data/settings_um.htm(1 hunks)wled00/dmx_input.cpp(2 hunks)wled00/fcn_declare.h(1 hunks)wled00/improv.cpp(1 hunks)wled00/json.cpp(2 hunks)wled00/presets.cpp(1 hunks)wled00/set.cpp(1 hunks)wled00/um_manager.cpp(1 hunks)wled00/wled.cpp(3 hunks)wled00/wled.h(1 hunks)wled00/wled_server.cpp(1 hunks)
💤 Files with no reviewable changes (3)
- usermods/usermod_v2_four_line_display_ALT/library.json.disabled
- usermods/SN_Photoresistor/usermods_list.cpp
- usermods/usermod_v2_rotary_encoder_ui_ALT/readme.md
✅ Files skipped from review due to trivial changes (4)
- usermods/usermod_v2_four_line_display_ALT/4LD_wled_fonts.h
- usermods/usermod_v2_four_line_display_ALT/library.json
- usermods/usermod_v2_brightness_follow_sun/library.json
- wled00/set.cpp
🧰 Additional context used
🧬 Code Definitions (6)
wled00/improv.cpp (2)
wled00/fcn_declare.h (1) (1)
serializeConfigToFS(31-31)wled00/cfg.cpp (2) (2)
serializeConfigToFS(691-708)serializeConfigToFS(691-691)
wled00/um_manager.cpp (2)
wled00/wled.cpp (2) (2)
setup(312-528)setup(312-312)wled00/fcn_declare.h (1) (1)
setup(454-454)
wled00/wled.cpp (3)
wled00/fcn_declare.h (2) (2)
serializeConfigToFS(31-31)getModCount(472-472)wled00/cfg.cpp (2) (2)
serializeConfigToFS(691-708)serializeConfigToFS(691-691)wled00/um_manager.cpp (2) (2)
getModCount(85-85)getModCount(85-85)
wled00/cfg.cpp (1)
wled00/fcn_declare.h (3) (3)
serializeConfigToFS(31-31)serializeConfig(30-30)releaseJSONBufferLock(504-504)
usermods/BH1750_v2/BH1750_v2.cpp (3)
usermods/BH1750_v2/BH1750_v2.h (4) (4)
name(70-70)root(79-79)root(82-82)root(85-85)usermods/sensors_to_mqtt/sensors_to_mqtt.cpp (2) (2)
name(69-97)name(69-69)usermods/SN_Photoresistor/SN_Photoresistor.cpp (8) (8)
checkBoundSensor(9-12)checkBoundSensor(9-9)addToJsonInfo(70-89)addToJsonInfo(70-70)addToConfig(95-107)addToConfig(95-95)readFromConfig(112-133)readFromConfig(112-112)
usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h (1)
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (3) (3)
enabled(299-299)root(350-350)root(360-360)
🪛 Cppcheck (2.10-2)
wled00/um_manager.cpp
[error] 23-23: Comparing pointers that point to different objects
(comparePointers)
🪛 Ruff (0.8.2)
usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py
1-1: Undefined name Import
(F821)
4-4: Undefined name env
(F821)
8-8: Undefined name env
(F821)
usermods/seven_segment_display_reloaded/setup_deps.py
1-1: Undefined name Import
(F821)
4-4: Undefined name env
(F821)
7-7: Undefined name env
(F821)
9-9: Undefined name env
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: wled_build / Build Enviornments (usermods)
🔇 Additional comments (47)
usermods/seven_segment_display_reloaded/library.json (1)
3-5: Build configuration setup looks good.The addition of the build configuration with the extraScript property correctly references the setup_deps.py file which will be executed during the build process.
wled00/data/settings_um.htm (1)
16-16: API endpoint update is appropriate.Changing the fetch URL from '/cfg.json' to '/json/cfg' aligns with modern API practices where resources are accessed through structured endpoint paths rather than directly accessing files.
usermods/usermod_v2_brightness_follow_sun/usermod_v2_brightness_follow_sun.cpp (1)
130-131: Properly registered usermod implementation.The addition of the static instantiation and REGISTER_USERMOD macro is essential for the usermod to be recognized by the system. This change appropriately completes the implementation, allowing the brightness follow sun functionality to be available to users.
usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py (1)
1-8: Dependency setup script structure is correct.The script correctly imports the environment configuration and checks for the presence of required user modules before adding appropriate preprocessor definitions. This approach ensures that the usermod will work properly when its dependencies are present.
The static analysis warnings about undefined names
Importandenvcan be ignored as these are provided by the PlatformIO build environment in which this script runs.🧰 Tools
🪛 Ruff (0.8.2)
1-1: Undefined name
Import(F821)
4-4: Undefined name
env(F821)
8-8: Undefined name
env(F821)
wled00/um_manager.cpp (1)
22-27: Enhanced setup with useful debug output.The previous one-line implementation has been expanded to include debug printing that identifies which usermod is being set up. This is a helpful change for troubleshooting and tracking initialization.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 23-23: Comparing pointers that point to different objects
(comparePointers)
usermods/usermod_v2_rotary_encoder_ui_ALT/library.json (1)
2-5:✅ Verification successful
Updated module name and added build configuration.
The name has been simplified by removing the "v2" prefix, and a build configuration has been added with an extra script reference.
🏁 Script executed:
#!/bin/bash # Verify that the setup_deps.py file exists in the directory if [ -f "usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py" ]; then echo "setup_deps.py exists" cat "usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py" | head -n 20 else echo "Error: setup_deps.py does not exist" fiLength of output: 506
Module name update and build configuration verified
- The module name in
usermods/usermod_v2_rotary_encoder_ui_ALT/library.jsonhas been correctly updated to"rotary_encoder_ui_ALT"by removing the "v2" prefix.- The build configuration now includes
"extraScript": "setup_deps.py", and verification confirms thatsetup_deps.pyexists with the expected content.wled00/dmx_input.cpp (2)
25-25: Variable rename for consistency: doSerializeConfig → configNeedsWrite.The variable has been renamed for consistency with other parts of the codebase while maintaining the same functionality.
43-43: Variable rename for consistency: doSerializeConfig → configNeedsWrite.The variable has been renamed for consistency with other parts of the codebase while maintaining the same functionality.
wled00/fcn_declare.h (1)
30-31: Improved configuration serialization API.The serialization process has been refactored to better separate concerns:
serializeConfig()now takes a JsonObject parameter, allowing serialization without direct filesystem access- A new
serializeConfigToFS()function has been added to handle the filesystem writing aspectThis change improves modularity and allows for more flexible handling of configuration data.
wled00/improv.cpp (1)
275-275: Function call replaced for better clarityThe change from
serializeConfig()toserializeConfigToFS()aligns with the broader refactoring in this PR to separate configuration serialization logic from file system operations. This function properly writes the configuration to the filesystem.wled00/wled_server.cpp (1)
331-332: Variable renamed for better semantic clarityChanging
doSerializeConfigtoconfigNeedsWriteimproves readability by more clearly indicating that the configuration needs to be saved to persistent storage. This aligns with similar changes across the codebase.wled00/presets.cpp (1)
245-246: Variable renamed for consistencyChanging
doSerializeConfigtoconfigNeedsWritemaintains consistency with the same change made in other files. This provides a clearer indication that the configuration should be written to persistent storage.usermods/seven_segment_display_reloaded/setup_deps.py (1)
1-9: New build dependency script implemented correctlyThis script conditionally adds preprocessor definitions based on which usermods are enabled, allowing the Seven Segment Display to integrate with light sensors from other modules if present. The implementation is clean and follows standard PlatformIO extension patterns.
Don't worry about the static analysis warnings - these names (
Import,env) are defined within the PlatformIO build environment where this script executes.🧰 Tools
🪛 Ruff (0.8.2)
1-1: Undefined name
Import(F821)
4-4: Undefined name
env(F821)
7-7: Undefined name
env(F821)
9-9: Undefined name
env(F821)
wled00/wled.h (1)
880-881: Improved variable naming for better clarity.The renaming of the variable from
doSerializeConfigtoconfigNeedsWriteis a good change. The new name more clearly describes the variable's purpose as a flag indicating when configuration needs to be written to persistent storage.usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
30-32: Good addition of conditional header inclusion.This change improves modularity by conditionally including the Four Line Display header only when that usermod is enabled. This follows proper conditional compilation patterns already used elsewhere in the file and allows for better dependency management.
usermods/seven_segment_display_reloaded/seven_segment_display_reloaded.cpp (2)
2-7: Well-structured conditional inclusion for light sensor modules.The conditional inclusion of these two light sensor headers enhances modularity, allowing the Seven Segment Display usermod to optionally work with either sensor type without requiring them.
396-401: Good usermod lookup implementation.Proper initialization of the sensor pointers using the UsermodManager lookup system ensures the modules can be accessed only when enabled, avoiding null pointer issues.
wled00/json.cpp (3)
1030-1034: Good modernization using enum class for JSON targetsSwitching from defined constants to a properly scoped enum class is a great improvement for type safety and code clarity. This change makes the codebase more maintainable by preventing accidental misuse of these values.
1036-1044: Consistent replacement of constant values with enumThe URL parsing logic has been consistently updated to use the new enum values, maintaining the same functionality while improving code readability and maintainability.
1066-1087: Improved API design with explicit JsonObject parameterThe call to
serializeConfignow takes a JsonObject parameter, which makes the API contract more explicit and matches the function signature change incfg.cpp. This is a good architectural improvement.wled00/wled.cpp (4)
196-196: Meaningful variable rename for better clarityChanging from
doSerializeConfigtoconfigNeedsWritemakes the variable's purpose clearer and more descriptive. This improves code readability.
203-203: Updated function call to match new implementationThe call to
serializeConfig()has been replaced withserializeConfigToFS()to match the new implementation in cfg.cpp, maintaining the correct functionality while improving the naming to better describe what the function does.
226-226: Consistent variable rename in conditional logicThe condition in the reboot check has been updated to use the renamed
configNeedsWritevariable, maintaining consistency across the codebase.
442-442: Enhanced debug logging with user module countAdding the count of loaded user modules to the debug output improves diagnostics capabilities, making it easier to verify the correct loading of usermods during setup.
wled00/cfg.cpp (3)
691-708: Good separation of concerns for configuration serializationThe refactoring of
serializeConfiginto two distinct functions (serializeConfigToFSandserializeConfig) is an excellent improvement. This separates file operations from data structure handling, following the single responsibility principle and making the code more maintainable.
710-710: Well-structured implementation of JsonObject serializationThe new implementation of
serializeConfignow takes a JsonObject parameter, allowing for greater flexibility in how the serialized configuration is used. This design enables reuse of the serialization logic for different purposes.
674-674: Consistent usage of new function name in all call sitesThe calls to the old
serializeConfig()function have been updated to use the newserializeConfigToFS()function, ensuring consistency throughout the codebase.Also applies to: 688-688
usermods/usermod_v2_four_line_display_ALT/platformio_override.sample.ini (3)
2-2: Updated default environment settingThe default environment has been updated from
esp32devtoesp32dev_fld, which better reflects the specific configuration for the four-line display usermod.
4-6: Improved modularity with environment inheritanceThe new environment
esp32dev_fldextends fromesp32dev_V4instead of duplicating settings, promoting code reuse and easier maintenance. This approach makes updates to the base environment automatically apply to this specialized environment.
7-12: Streamlined build configurationThe build flags now reference
${env:esp32dev_V4.build_flags}instead of repeating common flags, and the specific display configuration is cleanly separated. The removal of redundant library dependencies also simplifies the configuration.usermods/usermod_v2_rotary_encoder_ui_ALT/platformio_override.sample.ini (2)
2-2:✅ Verification successful
Confirm environment availability.
Changing the default environment toesp32dev_rerelies on it being defined or extended correctly. Verify that this environment is recognized by PlatformIO and that any referenced configuration is properly set up.
🏁 Script executed:
#!/bin/bash # Description: Check for references to [env:esp32dev_re] in the repository. rg "\[env:esp32dev_re\]" -A 10Length of output: 973
Action Required: Verify Extended Environment Configuration
The default environment is correctly set toesp32dev_reand is defined in the configuration file, as confirmed by the grep output. Note that this environment extendsenv:esp32dev_V4, so please ensure that the settings inenv:esp32dev_V4are properly configured and meet your intended requirements.
- File:
usermods/usermod_v2_rotary_encoder_ui_ALT/platformio_override.sample.ini- Key Points:
- The
[env:esp32dev_re]section exists as expected.- It extends from
env:esp32dev_V4, so double-check that all inherited configurations (likebuild_flagsandcustom_usermods) are accurate.
12-12:❓ Verification inconclusive
Check usage of pin 0.
Defining-D ENCODER_SW_PIN=0can cause conflicts on some boards (e.g., boot modes on ESP32 or use as a strapping pin). Ensure that this does not interfere with normal operation.
🏁 Script executed:
#!/bin/bash # Description: Check for pin usage conflicts with pin 0 in the codebase. rg "PIN[^A-Za-z0-9_]?0"Length of output: 1244
Attention: Verify Pin 0 Configurations Across Modules
Defining
-D ENCODER_SW_PIN=0inusermods/usermod_v2_rotary_encoder_ui_ALT/platformio_override.sample.inishould be reviewed in the context of other pin assignments in the codebase. Thergoutput shows multiple instances where pin 0 is used (e.g.,BTNPIN=0in several configuration files andDEEPSLEEP_WAKEUPPINinusermods/deep_sleep/deep_sleep.cpp). Since pin 0 on platforms like the ESP32 can serve as a boot or strapping pin, please verify that its usage here does not introduce boot conflicts or interfere with critical operations. If potential conflicts are identified, consider reassigningENCODER_SW_PINto a different pin that is safe for use on your target boards.usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.cpp (2)
1-2: Header inclusion refactor.
Replacing old includes withusermod_v2_four_line_display.hand4LD_wled_fonts.hcentralizes display definitions and font assets, enhancing maintainability. This change looks good.
1072-1072:✅ Verification successful
Usermod registration check.
The registration macroREGISTER_USERMOD(usermod_v2_four_line_display_alt);is correct for exposing the usermod to WLED. Confirm that no duplicates exist for the same usermod name.
🏁 Script executed:
#!/bin/bash # Description: Verify that there is only one registration for FourLineDisplayUsermod. rg "REGISTER_USERMOD\("Length of output: 4794
Usermod registration confirmed.
The registration macro inusermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.cppusingREGISTER_USERMOD(usermod_v2_four_line_display_alt);is correct. The search output confirms that only one registration exists for this usermod, and there are no duplicates.usermods/BH1750_v2/BH1750_v2.h (2)
6-8: Robust MQTT dependency check.
The preprocessor directive ensures compilation halts if MQTT is disabled, preventing malfunction at runtime. This is a clean, proactive approach.
30-92: New BH1750 usermod class structure.
Class encapsulation and straightforward member variables promote clarity. MQTT initialization and Home Assistant discovery are clearly separated, boosting maintainability.usermods/SN_Photoresistor/SN_Photoresistor.h (1)
34-91: Overall structure is clear and extensible
The class follows a well-organized pattern for usermods and neatly separates configuration from runtime logic. It includes clear, dedicated methods for setup, loop, configuration, and JSON serialization. No issues beyond minor points mentioned above.usermods/SN_Photoresistor/SN_Photoresistor.cpp (1)
14-24: Clarify the luminance calculation
The formulaamps * 1000000 * 2.0may need comments or references explaining why multiplying by 2.0 is appropriate. Ensure this logic reflects the real-world behavior of your photoresistor circuit.Do you want to confirm this calibration? You could, for instance, measure known light levels and compare them to the sensor output.
usermods/BH1750_v2/BH1750_v2.cpp (4)
11-14: Potential risk of misinterpreting zero measurements
The condition(newValue == 0.0 && prevValue > 0.0)flags any transition to zero as a significant change. Verify that legitimate zero-lux readings (e.g., in total darkness) won’t falsely trigger unwanted behavior.
20-22: Good use of_createMqttSensorfor dynamic discovery
The method effectively creates and publishes an MQTT sensor definition for Home Assistant discovery, making the device easily configurable. No issues found here.
82-94: Efficient handling of MQTT initialization in the loop
Conditionally calling_mqttInitialize()on the first publish is a good approach to reduce overhead and ensure that topics are set up only when needed. This looks correct.
113-114: Distinct states for “disabled” vs. “not found”
Providing separate statuses for a disabled sensor and a sensor that isn’t physically detected (line 115: “Not Found”) improves troubleshooting by differentiating between user configuration vs. hardware presence.usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h (5)
179-205: Ensureloop()efficiency with frequent screen updates.
Repeatedly callingredraw()orupdateBrightness()can cause performance bottlenecks if performed too often. Validate thatloop()doesn’t hamper other processes, especially on resource-constrained devices.
279-305: Override configuration and confirm usage.
TheaddToConfigandreadFromConfigmethods are vital to store and retrieve usermod settings. If you rely on these stored values for display pins or performance parameters, confirm they’re used consistently insetup()orloop()to avoid mismatches.
306-314: ID consistency is well-implemented.
ReturningUSERMOD_ID_FOUR_LINE_DISPingetId()helps modules identify the usermod. Good practice is to ensure that this ID remains unique across the entire codebase and that references to it use the same constant.
1-4:❓ Verification inconclusive
Confirm hardware I2C availability.
By undefiningU8X8_NO_HW_I2C, you are forcing the use of hardware I2C. Please ensure the platform and pins you're using actually support hardware I2C.Run this script to locate any other references to
U8X8_NO_HW_I2Cor I2C usage:
🏁 Script executed:
#!/bin/bash rg -A 3 "U8X8_NO_HW_I2C"Length of output: 489
Confirm Hardware I2C Support
Inusermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h, the#undef U8X8_NO_HW_I2Cdirective forces the library to use hardware I2C. The search confirms that this is the only occurrence in the repository, so please double-check that the target platform and designated pins genuinely support hardware I2C—especially if your setup might be limited to software I2C.
64-100:❓ Verification inconclusive
Assess singleton pattern and concurrency.
Using a staticFourLineDisplayUsermod* instancecombined with threading can lead to synchronization challenges. Confirm that no concurrency or initialization race conditions arise if multiple usermods or tasks callgetInstance()concurrently.
🏁 Script executed:
#!/bin/bash # Searching the codebase for concurrent references to getInstance() ast-grep --pattern $'getInstance($_)'Length of output: 38
Attention: Verify Thread Safety on Singleton Initialization
The current implementation of
FourLineDisplayUsermodrelies on a singleton pattern without explicit synchronization. Although our search for concurrent calls togetInstance()didn’t return any matches, this doesn’t guarantee that concurrent instantiation couldn’t occur—especially when threading is enabled (underARDUINO_ARCH_ESP32withFLD_ESP32_USE_THREADS). Please manually verify that only one instance is ever created in practice. In particular, confirm that:
- The construction of the singleton is exclusively performed in a single-threaded context.
- No concurrent tasks or usermod invocations call
getInstance()before initialization is fully complete.- If there is any chance of concurrent instantiation, consider adding a mutex or similar thread-safety mechanism around the initialization logic.
| "name:": "seven_segment_display_reloaded", | ||
| "build": { | ||
| "extraScript": "setup_deps.py" | ||
| } |
There was a problem hiding this comment.
Fix the JSON key typo in the name property.
The "name:" key contains an extra colon character that should be removed as it's not valid JSON syntax. The correct format should just be "name".
- "name:": "seven_segment_display_reloaded",
+ "name": "seven_segment_display_reloaded",📝 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.
| "name:": "seven_segment_display_reloaded", | |
| "build": { | |
| "extraScript": "setup_deps.py" | |
| } | |
| "name": "seven_segment_display_reloaded", | |
| "build": { | |
| "extraScript": "setup_deps.py" | |
| } |
| // allows this library to report to the user how long until the first | ||
| // measurement | ||
| bool getLuminanceComplete = false; | ||
| uint16_t lastLDRValue = -1000; |
There was a problem hiding this comment.
Suspicious negative initialization for uint16_t lastLDRValue
uint16_t cannot represent negative values. Initializing it to -1000 may cause unexpected wrapping. Consider using a signed type if you need to store negative sentinel values or initialize it to 0.
- uint16_t lastLDRValue = -1000;
+ int16_t lastLDRValue = -1000; // or another sentinel approach📝 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.
| uint16_t lastLDRValue = -1000; | |
| int16_t lastLDRValue = -1000; // or another sentinel approach |
| // Next variables hold the previous known values to determine if redraw is | ||
| // required. | ||
| String knownSsid = apSSID; | ||
| IPAddress knownIp = IPAddress(4, 3, 2, 1); | ||
| uint8_t knownBrightness = 0; | ||
| uint8_t knownEffectSpeed = 0; | ||
| uint8_t knownEffectIntensity = 0; | ||
| uint8_t knownMode = 0; | ||
| uint8_t knownPalette = 0; | ||
| uint8_t knownMinute = 99; | ||
| uint8_t knownHour = 99; | ||
| byte brightness100; | ||
| byte fxspeed100; | ||
| byte fxintensity100; | ||
| bool knownnightlight = nightlightActive; | ||
| bool wificonnected = interfacesInited; | ||
| bool powerON = true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memory semantics for volatile fields.
volatile bool fields like drawing and lockRedraw safeguard from some compiler optimizations, but they do not inherently protect against race conditions. If multiple threads read or write these fields, consider mutex locks or atomic operations.
Fix up some more usermods that I missed in #4480.
Summary by CodeRabbit