More usermod fixes #4623
More usermod fixes #4623
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.
Get this usermod to build, and restore the interconnect with its partner usermod_v2_rotary_encoder_ui_ALT.
Fix up the cross module binding for usermods/seven_segment_display_reloaded. This requires splitting off headers for BH1750_v2 and SN_Photoresistor.
WalkthroughMultiple usermods have been updated across the codebase. Changes include restructuring sensor integrations, revising MQTT and JSON handling, renaming debug macros, and updating variable types. New header files, classes, and setup scripts have been added for modules such as BH1750, SN_Photoresistor, four-line display, and rotary encoder. Additionally, configuration files and PlatformIO environments have been refined, obsolete registrations and dependency directives removed, and conditional compilation mechanisms improved. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 6
🧹 Nitpick comments (10)
usermods/seven_segment_display_reloaded/setup_deps.py (1)
1-9: Good dependency management scriptThis script properly adds the necessary preprocessor definitions based on which usermods are enabled, allowing for conditional compilation and integration between modules.
Consider adding a comment explaining that
Importandenvare PlatformIO build script conventions. This would make the code more readable for developers unfamiliar with PlatformIO:+# This is a PlatformIO build script. 'Import' and 'env' are provided by PlatformIO. Import('env') usermods = env.GetProjectOption("custom_usermods","").split()🧰 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)
usermods/word-clock-matrix/word-clock-matrix.cpp (1)
39-40: Variable name improved for clarity.The variable name
seghas been changed totext_segwhich better describes its purpose in processing text segments. This improves code readability without changing functionality.usermods/BH1750_v2/BH1750_v2.h (1)
25-29: Consider making offset value a float for finer control.Currently,
USERMOD_BH1750_OFFSET_VALUEis an integer set to 1. If your BH1750 sensor needs more granular offset control (e.g., fractional lux differences), consider using a float for the offset to avoid ignoring small measurement variations.-#ifndef USERMOD_BH1750_OFFSET_VALUE -#define USERMOD_BH1750_OFFSET_VALUE 1 -#endif +#ifndef USERMOD_BH1750_OFFSET_VALUE +#define USERMOD_BH1750_OFFSET_VALUE 1.0f +#endifusermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.cpp (1)
937-1069: Complex re-initialization logic after config reload.Re-initializing SPI or I2C pins, applying
u8x8_Setup, and callingstartDisplay()is comprehensive. Consider extracting this into smaller helper methods for readability, but overall the logic is correct. The usage of overlays on reconfig is a nice user experience touch.usermods/SN_Photoresistor/SN_Photoresistor.h (1)
4-33: Consider replacing#definewithconstexpror static constants.
Using#definemay cause namespace pollution and offers no type checking. Transitioning toconstexpror static constants within a namespace or class scope can enhance maintainability and type safety.usermods/SN_Photoresistor/SN_Photoresistor.cpp (2)
9-12: Check floating-point comparisons incheckBoundSensor.
The function uses direct<=and>=for floating-point comparisons. While this is acceptable for a threshold-based bound, keep in mind potential floating-point precision issues. Consider ensuringmaxDiffis suitably sized relative to expected measurement noise.
109-133: Ensure guard for invalid JSON types or missing fields.
readFromConfiggracefully falls back to defaults if any fields are missing, which is helpful. However, you might want additional checks for out-of-range values to avoid nonsensical configurations (e.g., negative intervals).usermods/BH1750_v2/BH1750_v2.cpp (2)
11-14: Avoid equality checks on floating-point (newValue == 0.0).
While it’s valid to flag a “sensor stuck at zero” condition, consider an epsilon threshold to account for floating-point precision errors.
23-52: Use consistent naming for device classes and measurement units.
The_createMqttSensormethod setsdevice_class = "Illuminance"andunit_of_measurement = " lx". Ensure you maintain these strings consistently if other sensor usermods also produce brightness data to avoid confusion across devices in Home Assistant.usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h (1)
295-305: Persist all relevant user settings in config.
WhileaddToConfig()andreadFromConfig()are declared, it’s unclear which fields are stored or loaded. If you intend to preserve user settings for display flipping, clock mode, contrast, etc., ensure these methods fully implement saving and loading each relevant variable.Do you want help generating code snippets that serialize or deserialize these fields?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
usermods/BH1750_v2/BH1750_v2.cpp(1 hunks)usermods/BH1750_v2/BH1750_v2.h(1 hunks)usermods/BME68X_v2/BME68X_v2.cpp(12 hunks)usermods/MAX17048_v2/MAX17048_v2.cpp(1 hunks)usermods/MAX17048_v2/readme.md(1 hunks)usermods/PWM_fan/setup_deps.py(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_HttpPullLightControl/usermod_v2_HttpPullLightControl.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)usermods/word-clock-matrix/word-clock-matrix.cpp(3 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
🧰 Additional context used
🧬 Code Definitions (4)
usermods/SN_Photoresistor/SN_Photoresistor.cpp (2)
usermods/SN_Photoresistor/SN_Photoresistor.h (4)
lastLDRValue(69-72)root(74-74)root(84-84)root(89-89)usermods/BH1750_v2/BH1750_v2.h (1)
root(79-79)
usermods/BH1750_v2/BH1750_v2.h (2)
usermods/seven_segment_display_reloaded/seven_segment_display_reloaded.cpp (12)
root(357-378)root(357-357)root(452-468)root(452-452)root(474-480)root(474-474)root(486-497)root(486-486)root(540-542)root(540-540)root(544-574)root(544-544)usermods/SN_Photoresistor/SN_Photoresistor.h (3)
root(74-74)root(84-84)root(89-89)
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)
enabled(299-299)root(350-350)root(360-360)
usermods/BH1750_v2/BH1750_v2.cpp (4)
usermods/BH1750_v2/BH1750_v2.h (1)
name(70-70)usermods/sensors_to_mqtt/sensors_to_mqtt.cpp (2)
name(69-97)name(69-69)usermods/BME280_v2/BME280_v2.cpp (4)
name(131-159)name(131-131)topic(161-168)topic(161-161)usermods/AHT10_v2/AHT10_v2.cpp (2)
name(85-113)name(85-85)
🪛 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)
🪛 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 (7)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (44)
usermods/MAX17048_v2/readme.md (2)
12-12: Streamlined installation documentation.The shift from explicit library dependencies to the custom_usermods mechanism simplifies the installation process and aligns with how other usermods are configured.
17-17: Updated example configuration to match installation instructions.The example now properly demonstrates the use of custom_usermods directive, making it consistent with the instructions provided earlier in the document.
usermods/usermod_v2_four_line_display_ALT/4LD_wled_fonts.h (1)
2-2: Minor formatting improvement.Removed unnecessary blank lines before the #pragma once directive, making the code cleaner.
usermods/seven_segment_display_reloaded/seven_segment_display_reloaded.cpp (4)
2-7: Enhanced modularity with conditional includes.Added conditional inclusion of light sensor headers based on compile-time flags, improving modularity and reducing dependencies when these sensors are not needed.
99-108: Improved type safety with conditional pointer declarations.Declared sensor-specific pointers for each supported light sensor type, with fallback to void pointers when the corresponding usermod is not enabled.
396-401: Added sensor lookup during setup.Properly initializes sensor pointers by looking up corresponding usermods during setup, enabling cross-usermod interaction.
412-438: Implemented cross-usermod integration for automatic brightness.Added support for two different light sensor usermods (SN_Photoresistor and BH1750) to enable automatic brightness adjustment. The BH1750 implementation includes additional debug logging.
Worth noting that both implementations share similar logic:
- Check if brightness is enabled and sufficient time has passed
- Retrieve light value from the respective sensor
- Map value to brightness range
- Update brightness if changed
This is a good example of code that leverages conditional compilation to support multiple sensor types while maintaining a clean implementation.
usermods/PWM_fan/setup_deps.py (1)
10-10: Improved error condition specificity.The conditional has been modified from a general
elseto a specificelif, which explicitly checks for the presence of the PWM_fan usermod. This makes the error condition more targeted and clearer to understand.usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py (1)
1-8: Well-structured dependency setup script.This new file appropriately configures the dependency between the rotary encoder usermod and the four-line display. It checks for either naming convention of the display module and sets up the necessary preprocessor definition.
Don't worry about the static analysis warnings about undefined names (
Importandenv). These are false positives as these variables are provided by the PlatformIO build system.🧰 Tools
🪛 Ruff (0.8.2)
1-1: Undefined name
Import(F821)
4-4: Undefined name
env(F821)
8-8: Undefined name
env(F821)
usermods/seven_segment_display_reloaded/library.json (1)
2-5: Fixed JSON syntax and added build configuration.Two important improvements:
- Fixed the syntax error in the "name" field (removed the colon after the key)
- Added proper build configuration to include the setup_deps.py script during the build process
These changes ensure proper JSON formatting and provide the necessary build integration.
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (1)
30-32: Added proper conditional compilation for four-line display integration.The addition of conditional include for the four-line display header enhances modularity by making this dependency optional. This works in conjunction with the newly added setup_deps.py script, which defines the USERMOD_FOUR_LINE_DISPLAY macro when the display module is enabled.
usermods/usermod_v2_brightness_follow_sun/usermod_v2_brightness_follow_sun.cpp (1)
130-131: Proper usermod registration addedThe addition of a static instance and registration of the
UsermodBrightnessFollowSunclass is essential for making this usermod available to the WLED system. This follows the standard WLED pattern for usermod registration.usermods/usermod_v2_rotary_encoder_ui_ALT/library.json (1)
3-5: Good addition of build configurationAdding the
extraScriptto handle dependencies is a good practice. This allows for conditional compilation based on which usermods are enabled.usermods/usermod_v2_HttpPullLightControl/usermod_v2_HttpPullLightControl.cpp (1)
300-300:❓ Verification inconclusive
JSON handling updated to use shared document pointer
The code has been updated to use a pointer to a document (
pDoc) instead of a local document instance (doc). This change aligns with the buffer locking mechanism used earlier in the method.Since we don't see the variable declaration for
pDocin this file, please verify that it's properly defined as a member variable in the header file:Also applies to: 303-303
🏁 Script executed:
#!/bin/bash # Check if pDoc is properly defined in the header file cat usermods/usermod_v2_HttpPullLightControl/usermod_v2_HttpPullLightControl.h | grep -A 5 "pDoc"Length of output: 97
Action Required: Manually Verify
pDocDeclarationThe updated code now uses a shared document pointer (
pDoc), aligning with the buffer locking mechanism. However, the initial script did not return any output when searching forpDocinusermod_v2_HttpPullLightControl.h. Please manually verify thatpDocis properly declared as a member variable in the header file. This check also applies to the code at line 303.
- Ensure that
pDocis declared inusermod_v2_HttpPullLightControl.h.- Confirm that its declaration aligns with the updated JSON deserialization approach.
usermods/BME68X_v2/BME68X_v2.cpp (3)
2-2: File header updated to match file content.The file header has been correctly updated from "usermod_BMW68X.h" to "usermod_BMW68X.cpp" to match the actual file content, which improves documentation accuracy.
36-39: Debug message macro constants renamed for clarity.Debug message macros have been renamed by adding the
_MSGsuffix to clearly indicate their purpose as message-related constants. This improves code readability and makes the codebase more maintainable.
331-331: Debug message references updated consistently.All references to the renamed debug message macros have been consistently updated throughout the file. This ensures the code functions correctly with the new macro names.
Also applies to: 355-355, 567-567, 753-753, 834-834, 848-848, 851-851, 1029-1029, 1033-1033, 1044-1044, 1048-1048, 1053-1053, 1066-1066, 1071-1071, 1087-1087, 1094-1094
usermods/MAX17048_v2/MAX17048_v2.cpp (1)
38-39: Variable types changed from uint8_t to unsigned.The variable types for
VoltageDecimalsandPercentDecimalshave been changed fromuint8_ttounsigned. This change is unlikely to affect functionality as the existing values (3 and 1) are well within both type ranges, but it might increase memory usage slightly.The change may have been made for consistency with other parts of the code or to anticipate future needs for larger values.
usermods/word-clock-matrix/word-clock-matrix.cpp (1)
70-70:✅ Verification successful
Method parameter removed from savePreset call.
The boolean parameter (
false) has been removed from thesavePreset(13)call. This suggests the method signature has been simplified elsewhere in the codebase, likely makingfalsethe default behavior.To verify this change is correct, please check the implementation of the
savePresetmethod to ensure it can now be called without the second parameter:
🏁 Script executed:
#!/bin/bash # Search for the savePreset method implementation rg "void +savePreset" -A 3Length of output: 602
Action: Confirmed removal of unnecessary parameter in
savePresetcallAfter reviewing the function signature in the repository (see
wled00/fcn_declare.handwled00/presets.cpp), it’s clear thatsavePresetnow accepts a primary parameter (byte index) with the remaining parameters defaulted (const char* pname = nullptrandJsonObject saveobj = JsonObject()). The earlier boolean parameter has been intentionally removed, and callingsavePreset(13)appropriately leverages the default values.usermods/usermod_v2_rotary_encoder_ui_ALT/platformio_override.sample.ini (3)
2-2: Default environment changed to esp32dev_re.The default environment has been updated to use a custom environment specifically for the rotary encoder module.
4-7: New custom environment created for the rotary encoder.A new environment
esp32dev_rehas been created that extends theesp32dev_V4environment and adds therotary_encoder_ui_ALTusermod to the build. This provides a specialized build configuration for the rotary encoder functionality.
8-12:✅ Verification successful
Build flags updated to use parent environment flags.
The build flags now reference the parent environment's flags (
${env:esp32dev_V4.build_flags}) instead of the common ESP32 flags. The-D USERMOD_ROTARY_ENCODER_UIflag has been removed, which may have been moved to a different configuration location or is now included in the parent's flags.Note that pins and GPIO configuration flags are still properly defined.
To verify that the
USERMOD_ROTARY_ENCODER_UIflag is defined elsewhere:
🏁 Script executed:
#!/bin/bash # Search for USERMOD_ROTARY_ENCODER_UI definition in other configuration files rg "USERMOD_ROTARY_ENCODER_UI" --glob "*.ini"Length of output: 109
Attention: Build Flags Update Verified – Parent Environment Flags in Use
The configuration now correctly references the parent environment flags via
${env:esp32dev_V4.build_flags}. The-D USERMOD_ROTARY_ENCODER_UIflag is no longer active and appears only as a commented-out line (as seen in the search output), which confirms the intended removal. The remaining pin and GPIO configuration flags are correctly defined.
- The commented-out entry (
; -D USERMOD_ROTARY_ENCODER_UI) inplatformio_override.sample.iniconfirms that the flag isn’t active and likely maintained only for historical reference.- All active definitions, including the build flags and pin settings, match the intended updates.
usermods/usermod_v2_four_line_display_ALT/platformio_override.sample.ini (1)
2-12: Confirm environment definitions and consistency with parent environment.You changed
default_envstoesp32dev_fldand set[env:esp32dev_fld]to extendenv:esp32dev_V4, referencing its build flags and appendingfour_line_display_ALTtocustom_usermods. Ensure:
env:esp32dev_V4is indeed defined in a relevant.inior shared config.- Removal of any previously listed library dependencies does not break the build if the usermod or environment still relies on them.
Would you like to run a quick check that verifies the existence of
env:esp32dev_V4and inspects library usage?usermods/BH1750_v2/BH1750_v2.h (2)
2-8: MQTT dependency enforced.This usermod explicitly errors out if MQTT is disabled, which is appropriate if MQTT-based functionality is essential. Just ensure users who disable MQTT are aware that they cannot build this usermod.
30-93: Class structure is clear.The class has well-defined private members and public methods. The usage of
lastLuxas a float is appropriate for illuminance. Just verify that your loop logic in the corresponding .cpp carefully checks the difference against the integer offset in a float comparison context.usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.cpp (4)
46-54: Direct hardware commands for contrast voltage level.This method directly sends commands to the display for contrast tuning. It looks correct, but keep an eye on concurrency if this function is ever called from multiple contexts. Currently, the guard
if (type == NONE || !enabled) return;is good for ensuring it only runs when appropriate.
215-261: Verify handling of user-specified pins.Your
setup()method deallocates pins if they are invalid (negative), then setstype = NONE. This is a good fallback. Just confirm you log or notify the user when the display type is set to NONE, so they can diagnose hardware pin misconfiguration.
400-466: Concurrency approach is solid.Locking via
lockRedrawand thewhile (drawing && millis()-now < 125) delay(1);approach helps avoid race conditions when updating brightness, speed, intensity, and status icons. This looks well-thought-out and should maintain display stability.
916-935: Persistent configuration writes look fine.Storing and retrieving structured data into the
cfg.json(top-level “um” object) is straightforward. The property naming is consistent (enabled,type,pin, etc.). The debug statement also helps confirm that config was saved.usermods/SN_Photoresistor/SN_Photoresistor.h (2)
44-44: Verify potential overflow calculations forlastMeasurement.
The expressionUINT32_MAX - (USERMOD_SN_PHOTORESISTOR_MEASUREMENT_INTERVAL - USERMOD_SN_PHOTORESISTOR_FIRST_MEASUREMENT_AT)is clever but can be error-prone. Consider verifying boundary conditions (e.g., ifUSERMOD_SN_PHOTORESISTOR_MEASUREMENT_INTERVAL < USERMOD_SN_PHOTORESISTOR_FIRST_MEASUREMENT_AT) to ensure no unintended overflow or negative values in the sub-expression.
34-91: General class design looks good.
The overall structure of theUsermod_SN_Photoresistorclass is clear, with well-defined private members and public methods for sensor reading, JSON integration, and configuration. The approach to storing repeated string literals inPROGMEMhelps reduce flash usage. No major issues found.usermods/SN_Photoresistor/SN_Photoresistor.cpp (6)
14-25: Validate potential range forgetLuminance.
The formula for lux uses an empirical factor (* 2.0) on microamps. Confirm that high-intensity light won't exceed65535. Also ensure that zero or near-zero reads don’t cause unexpected behavior in the calling code.
33-37: Honor thedisabledflag.
The checkif (disabled || strip.isUpdating()) return;properly skips sensor routines while disabled or strip is refreshing. This helps avoid potential concurrency issues and ensures performance is not impacted unnecessarily.
40-46: Incremental reading logic.
The logic to skip measurements until enough time has passed is clear and avoids rapid sensor updates. Validate additional edge cases, such as whennow < lastMeasurementdue to timer wraparound (though unlikely in typical operation).
48-49: Smart usage ofcheckBoundSensor.
This helps prevent excessive MQTT messages when the change is within the user-defined offset. A well-structured approach to reduce data noise.
69-89: Clean JSON info addition for debugging.
Reporting the remaining time until the first reading (if not complete) is a nice user-friendly feature. The “Luminance” field is appropriately nested under"u".
92-107: Storing configuration with intuitive keys.
Using descriptive property names like"supplied-voltage","read-interval-s", etc. helps keep the JSON structure discoverable. No issues found inaddToConfig.usermods/BH1750_v2/BH1750_v2.cpp (4)
16-22: MQTT initialization approach is sound.
Using_mqttInitialize()once upon first data publish avoids unnecessary overhead. The check forHomeAssistantDiscoveryis properly placed.
61-102: loop logic effectively manages update timing.
The check forstrip.isUpdating()and the schedule (minReadingInterval,maxReadingInterval) ensures it doesn’t flood MQTT. This is a best practice for sensor updates.
105-128: Proper JSON output debug.
Using a separate nested array for “Luminance” is consistent with other usermods. The code gracefully handles cases when the device is disabled or the sensor is not found.
130-173: Overall configuration handling.
Reading and writing config with fallback defaults is prudent. The usage ofgetJsonValueis consistent and robust. No major issues.usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h (3)
7-9: Confirm concurrency handling for ESP32 threads.
DefiningFLD_ESP32_USE_THREADSmay lead to concurrency issues if the display update and other tasks share resources unsafely. Please verify that you have proper locking mechanisms in place (e.g., semaphores, mutexes) to avoid data races or inconsistent states when drawing to the display concurrently.Would you like me to provide a script to scan for concurrency-related methods or references to confirm safe usage?
79-80: Verify creation and error handling ofU8X8pointer.
Although you’ve declared a pointer toU8X8, the provided snippet does not show error handling if the display allocation or initialization fails. Ensure thatu8x8is instantiated duringsetup()orstartDisplay()and confirm that you gracefully handle failures (e.g., fallback or disabled state).
128-140: Confirm static string definitions in your .cpp implementation.
Thesestatic const chardeclarations (e.g.,_enabled,_contrast, etc.) should have matching definitions in the corresponding .cpp file. If they are never defined, linking errors may occur.
| volatile bool drawing = false; | ||
| volatile bool lockRedraw = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider more robust synchronization than volatile booleans.
Using volatile bool drawing and volatile bool lockRedraw alone may not be sufficient for multi-threaded scenarios if actions on these flags happen concurrently. In certain situations, atomic operations or mutex-based synchronization might be more reliable for preventing race conditions.
There was a problem hiding this comment.
This is outside the scope of this PR; the goal here is to get modules to compile, I don't intend to second-guess or refactor the original author's logic.
There was a problem hiding this comment.
@willmmiles Understood! You're right to keep the PR focused on getting modules to compile without changing the original author's implementation logic. I'll respect that scope limitation.
| // gets called once at boot. Do all initialization that doesn't depend on | ||
| // network here | ||
| void setup() override; | ||
|
|
||
| // gets called every time WiFi is (re-)connected. Initialize own network | ||
| // interfaces here | ||
| void connected() override; | ||
|
|
||
| /** | ||
| * Da loop. | ||
| */ | ||
| void loop() override; | ||
|
|
||
| //function to update lastredraw | ||
| inline void updateRedrawTime() { lastRedraw = millis(); } | ||
|
|
||
| /** | ||
| * Redraw the screen (but only if things have changed | ||
| * or if forceRedraw). | ||
| */ | ||
| void redraw(bool forceRedraw); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure robust startup and redraw logic.
The setup(), connected(), and redraw() methods must integrate seamlessly. If u8x8 is not fully initialized by the time redraw() is called, you could encounter unexpected display states. Consider safeguards (e.g., if (!initDone) return;) in loop() or redraw().
There was a problem hiding this comment.
This is outside the scope of this PR, as above
There was a problem hiding this comment.
@willmmiles Understood. I'll respect the scope boundaries of your PR. Thanks for the clarification.
Fix incorrect module name in library.json Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fix incorrect json Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fix typo in json Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Must've been a bad search-and-replace on my part somewhere..
Fix up some more usermods that I missed in #4480. This is a replacement for #4601.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores