Integrate um_data structure to battery usermod#5521
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds cached battery state (voltage and level), exposes prepared Changes
Sequence Diagram(s)sequenceDiagram
participant Setup as Setup
participant Usermod as Usermod(Battery)
participant BAT as BatterySensor
participant UM as UM_Framework
participant MQTT as MQTT_Broker
Setup->>Usermod: allocate um_data_t, point exports to cached fields
Usermod->>BAT: request stabilized voltage/read
BAT-->>Usermod: voltage
Usermod->>Usermod: calculate level, refreshExportedBatteryData()
Usermod->>UM: getUMData() returns pointer
Usermod->>MQTT: publish "battery" using String(bat->getLevel())
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/Battery/Battery.cpp (1)
273-277:⚠️ Potential issue | 🟠 MajorRefresh the exported cache when the battery reading changes.
getUMData()exposesbatteryVoltageandbatteryLevel, but those fields are only updated as getter side effects. With MQTT disabled and auto-off/indicator disabled, other usermods can keep reading the initial0values even afterbathas been updated.Proposed fix
if (isFirstVoltageReading) { - bat->setVoltage(readVoltage()); + batteryVoltage = readVoltage(); + bat->setVoltage(batteryVoltage); + bat->calculateAndSetLevel(batteryVoltage); + batteryLevel = bat->getLevel(); isFirstVoltageReading = false; } @@ bat->setVoltage(filteredVoltage); // translate battery voltage into percentage bat->calculateAndSetLevel(filteredVoltage); + batteryVoltage = bat->getVoltage(); + batteryLevel = bat->getLevel(); @@ - float getVoltage() + float getVoltage() const { - batteryVoltage = bat->getVoltage(); return batteryVoltage; } @@ - int8_t getBatteryLevel() + int8_t getBatteryLevel() const { - batteryLevel = bat->getLevel(); return batteryLevel; }As per coding guidelines, “Mark getter/query methods as
constand usestaticfor methods not accessing instance state”.Also applies to: 294-304, 687-700
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` around lines 273 - 277, When you set the first voltage value in the isFirstVoltageReading block (where you call bat->setVoltage(readVoltage())), also refresh the exported/UM cache so external callers see the updated batteryVoltage and batteryLevel immediately; add a call there to the module's export-refresh helper (the same mechanism used elsewhere to refresh UM-exported values) and apply the same change in the other battery-update sites (the blocks covering lines similar to 294-304 and 687-700). Also make any getter/query methods for battery data const and mark methods that do not access instance state as static (e.g., the getUMData-related getters that expose batteryVoltage/batteryLevel) to follow the coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/Battery/Battery.cpp`:
- Around line 867-871: getUMData currently returns true even when the internal
um_data hasn't been allocated, which yields a null payload; update the
getUMData(um_data_t **data) override to check both that data is non-null and
that um_data is initialized (um_data != nullptr) before assigning and returning
true, otherwise return false so callers/fallback logic can detect uninitialized
payloads.
- Line 26: The file declares a private um_data pointer that shadows the
inherited Usermod::um_data, causing allocated memory in the Battery class
(allocation in the setup/init block around where lines 221-230 are) to never be
cleaned up by the base destructor; remove the private declaration "um_data_t*
um_data = nullptr" and instead assign to the inherited member (use this->um_data
or plain um_data) when creating the struct in the Battery::setup/init code, and
also remove any manual delete of the shadowed pointer in Battery::~Battery so
ownership is solely via Usermod::um_data.
- Line 303: The call publishMqtt("battery", String(getBatteryLevel(),
0).c_str()) uses String(int8_t, 0) which passes base=0 to the itoa path and
yields an empty string; update the String construction in Battery.cpp to use a
decimal representation (either String(getBatteryLevel()) or
String(getBatteryLevel(), DEC) / base 10) so publishMqtt receives the actual
battery numeric value; change the argument where publishMqtt is called and
ensure getBatteryLevel() remains the value source.
---
Outside diff comments:
In `@usermods/Battery/Battery.cpp`:
- Around line 273-277: When you set the first voltage value in the
isFirstVoltageReading block (where you call bat->setVoltage(readVoltage())),
also refresh the exported/UM cache so external callers see the updated
batteryVoltage and batteryLevel immediately; add a call there to the module's
export-refresh helper (the same mechanism used elsewhere to refresh UM-exported
values) and apply the same change in the other battery-update sites (the blocks
covering lines similar to 294-304 and 687-700). Also make any getter/query
methods for battery data const and mark methods that do not access instance
state as static (e.g., the getUMData-related getters that expose
batteryVoltage/batteryLevel) to follow the coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c87e7c4-c039-4818-9635-04a1b8840a2a
📒 Files selected for processing (1)
usermods/Battery/Battery.cpp
|
Thanks for your reply. I have a completed usermod relying on this. What is the timeline for the new interface? I suggest to merge this as long as the new interface is not ready. https://github.com/PSi86/RaceLink_WLED |
|
the new interface is on the horizon for 17.0, certainly not for 16.0. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usermods/Battery/Battery.cpp (2)
109-110: Consider using the cached values here for consistency.Now that
batteryVoltage/batteryLevelare the exported source-of-truth and are refreshed synchronously with measurements, the remaining directbat->getLevel()/bat->getVoltage()reads inside this class (low-power indicator, auto-off threshold check, JSON info) will momentarily diverge from whatum_dataexposes if any of them is called betweenbat->calculateAndSetLevel()andrefreshExportedBatteryData(). In practice the window is tiny, but for consistency (and to match the reviewer suggestion in PR discussion to update um_data from the main loop and leave existing call sites alone, or fully switch to the cached values), consider picking one source within this class and using it uniformly.Also applies to: 307-308, 348-358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` around lines 109 - 110, The checks reading bat->getLevel()/bat->getVoltage() should use the class's cached exported values (batteryLevel/batteryVoltage) for consistency; replace direct calls to bat->getLevel() and bat->getVoltage() in low-power indicator logic (e.g., the conditions around lowPowerIndicationDone, lowPowerIndicatorReactivationThreshold, lowPowerIndicatorThreshold), the auto-off threshold check, and JSON/info generation with the cached batteryLevel/batteryVoltage values that are set by calculateAndSetLevel()/refreshExportedBatteryData(); ensure you only use one source (batteryLevel/batteryVoltage) throughout this class so reads won’t transiently diverge from um_data.
226-235:um_datais not re-allocated whensetup()re-runs after a pin change.
readFromConfig()callssetup()again whenbatteryPinchanges (line 591), but theif (!initDone)guard here skips the allocation block. That's safe for the existing allocation (no leak), but worth confirming that is the intended design: if the pin change path is ever extended to resetinitDone, the current block would leak the previously allocatedum_data_t(the base class destructor only runs on module destruction). A small comment noting the guard is intentional — and/or a null check around the allocation instead of!initDone— would make the invariant explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` around lines 226 - 235, The allocation block for um_data inside setup() is skipped when initDone is true which can hide a potential leak if setup() is ever re-run after a pin change; update the code to either (a) add a clear comment next to the initDone guard explaining that um_data is intentionally allocated only once and will not be reallocated on subsequent setup() calls (referencing um_data, initDone, setup(), readFromConfig(), batteryPin), or (b) change the guard to check for nullptr and, if um_data is non-null, delete/free the previous um_data_t (and its u_type/u_data arrays) before re-allocating to prevent leaks when setup() is invoked again; choose one approach and apply it around the allocation block so the invariant is explicit and safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/Battery/Battery.cpp`:
- Around line 23-24: The field batteryLevel is declared as int8_t but exported
as UMT_BYTE (unsigned), causing negative sentinel values (from Bat::getLevel())
to appear as large unsigned values; change batteryLevel to uint8_t and, where
you read bat->getLevel() (e.g. in the code path that currently emits "invalid"
when bat->getLevel() < 0), map negative/invalid results to an explicit sentinel
like 0xFF and clamp valid levels to 0–100 before assigning to batteryLevel;
ensure any export/serialization code that references UMT_BYTE uses that sentinel
and document the sentinel value so consumers interpret 0xFF as invalid rather
than 255% battery.
- Around line 495-498: When updating the cached voltage in addToConfig (calling
bat->setVoltage(readVoltage())), also invoke the battery level recomputation on
the same bat instance by calling bat->calculateAndSetLevel() before
refreshExportedBatteryData(); this ensures batteryLevel is recalculated from the
new voltage and prevents stale batteryLevel/batteryVoltage mismatch when
calibration/voltage multiplier/min-max values change. Locate the sequence around
bat->setVoltage(readVoltage()) and insert the calculateAndSetLevel() call on
that bat object prior to refreshExportedBatteryData().
---
Nitpick comments:
In `@usermods/Battery/Battery.cpp`:
- Around line 109-110: The checks reading bat->getLevel()/bat->getVoltage()
should use the class's cached exported values (batteryLevel/batteryVoltage) for
consistency; replace direct calls to bat->getLevel() and bat->getVoltage() in
low-power indicator logic (e.g., the conditions around lowPowerIndicationDone,
lowPowerIndicatorReactivationThreshold, lowPowerIndicatorThreshold), the
auto-off threshold check, and JSON/info generation with the cached
batteryLevel/batteryVoltage values that are set by
calculateAndSetLevel()/refreshExportedBatteryData(); ensure you only use one
source (batteryLevel/batteryVoltage) throughout this class so reads won’t
transiently diverge from um_data.
- Around line 226-235: The allocation block for um_data inside setup() is
skipped when initDone is true which can hide a potential leak if setup() is ever
re-run after a pin change; update the code to either (a) add a clear comment
next to the initDone guard explaining that um_data is intentionally allocated
only once and will not be reallocated on subsequent setup() calls (referencing
um_data, initDone, setup(), readFromConfig(), batteryPin), or (b) change the
guard to check for nullptr and, if um_data is non-null, delete/free the previous
um_data_t (and its u_type/u_data arrays) before re-allocating to prevent leaks
when setup() is invoked again; choose one approach and apply it around the
allocation block so the invariant is explicit and safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e699d33-131c-475c-aa5c-44531dc30855
📒 Files selected for processing (1)
usermods/Battery/Battery.cpp
| int8_t batteryLevel = 0; // current battery level in % | ||
| float batteryVoltage = 0.0f; // current battery voltage in V |
There was a problem hiding this comment.
Signed/unsigned mismatch: batteryLevel (int8_t) exposed as UMT_BYTE.
batteryLevel can be negative (the code emits "invalid" when bat->getLevel() < 0 at line 348), but it is exported with UMT_BYTE, which consumers will dereference as uint8_t (see e.g. the AudioReactive pattern in usermods/audioreactive/audio_reactive.cpp:1342-1370). A -1 sentinel will appear as 255 to consumers, which is indistinguishable from an almost-full battery. Consider storing batteryLevel as uint8_t and using a distinct sentinel (e.g. 0xFF only when explicitly documented), or adding a new typed entry, so the exported type matches the exported payload.
Also applies to: 233-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/Battery.cpp` around lines 23 - 24, The field batteryLevel is
declared as int8_t but exported as UMT_BYTE (unsigned), causing negative
sentinel values (from Bat::getLevel()) to appear as large unsigned values;
change batteryLevel to uint8_t and, where you read bat->getLevel() (e.g. in the
code path that currently emits "invalid" when bat->getLevel() < 0), map
negative/invalid results to an explicit sentinel like 0xFF and clamp valid
levels to 0–100 before assigning to batteryLevel; ensure any
export/serialization code that references UMT_BYTE uses that sentinel and
document the sentinel value so consumers interpret 0xFF as invalid rather than
255% battery.
There was a problem hiding this comment.
In UMBattery.h the battery level is also defined as int8_t and its range is 0 to 100. Please confirm if it should be changed to uint8_t as suggested by coderabbit.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check UMBattery.h for level type and getLevel definition
fd -e h UMBattery --exec cat {}
# Check concrete battery types for level calculation
fd -e h -e cpp --search-path usermods/Battery --exec grep -n "getLevel\|setLevel\|level\|int8_t\|uint8_t" {}
Length of output: 10410
@PSi86, after checking the full source of UMBattery.h and all concrete battery types, here is a precise answer:
What the code actually does today
| Location | Field / return type | Effective range |
|---|---|---|
UMBattery.h |
int8_t level = 100 |
setLevel() calls constrain(level, 0.0f, 110.0f) → always 0–110, never negative |
UMBattery::getLevel() |
returns float |
same 0.0–110.0 |
Battery.cpp |
int8_t batteryLevel |
assigned from getLevel() (float→int8_t truncation), so 0–110 |
| Export | UMT_BYTE |
consumers read it as uint8_t |
The check bat->getLevel() < 0 in addToJsonInfo is dead code — the base class setLevel() uses constrain(..., 0.0f, 110.0f), so a negative level cannot be produced through the normal API.
Should you change to uint8_t?
Yes, it is safe and semantically correct, because:
- The level is provably non-negative (0–110 range).
uint8_t(max 255) comfortably holds 110.- It aligns with the
UMT_BYTEexport type so consumers aren't misled.
For full consistency, both places should change together:
UMBattery.h — change the stored field:
// Before
int8_t level = 100;
// After
uint8_t level = 100;Battery.cpp — change the cached export variable:
// Before
int8_t batteryLevel = 0; // current battery level in %
// After
uint8_t batteryLevel = 0; // current battery level in %Battery.cpp — remove the now-confirmed dead < 0 guard (or convert it to a range check):
// Before
if (bat->getLevel() < 0) {
infoPercentage.add(F("invalid"));
} else {
infoPercentage.add(bat->getLevel());
}
// After (dead code removed; level is always 0–110)
infoPercentage.add(bat->getLevel());Battery.cpp — update the getter signature to match:
// Before
int8_t getBatteryLevel() { return bat->getLevel(); }
// After
uint8_t getBatteryLevel() { return bat->getLevel(); }Note: If you ever want to support a true "not yet measured" sentinel,
0xFF(255) is the idiomaticuint8_tchoice — just document it so consumers can distinguish it from a real 100% reading.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:3-5
Timestamp: 2026-02-18T00:11:02.403Z
Learning: In PR `#4585`, the seven_segment_display_reloaded_v2 usermod intentionally uses `#error` to require MQTT (`#ifdef WLED_DISABLE_MQTT `#error` "This user mod requires MQTT to be enabled." `#endif``) because the integration flow has not been tested or validated without MQTT, and failing fast at compile time is preferred over allowing potentially broken builds until a full MQTT-optional refactor is completed.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-10T01:13:40.736Z
Learning: In WLED, the general rule is that usermod PRs must NOT modify the main `platformio.ini` and should use `platformio_override.ini` instead. However, there is a legitimate exception: experimental/maintainer-driven PRs that introduce hybrid build environments (dual `framework = espidf, arduino`) requiring CMakeLists.txt integration, custom SCons extra_scripts, and IDF component manager setup (idf_component.yml) MAY modify the main `platformio.ini`. These changes cannot be fully expressed in `platformio_override.ini` alone. Example: PR `#5456` (Matter over WiFi usermod by netmindz/softhack007) adding `[env:esp32s3_matter_wifi]` with dual-framework build infrastructure. Regular usermod authors (non-maintainers, pure Arduino builds) still must not touch `platformio.ini`.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
There was a problem hiding this comment.
good analysis. what about the change of "String(bat->getLevel(), 0).c_str()" to "String(static_cast(bat->getLevel())).c_str()", what are the advantages and disadvantages?
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
Refactor battery voltage handling and data export.
|
|
||
| #ifndef WLED_DISABLE_MQTT | ||
| publishMqtt("battery", String(bat->getLevel(), 0).c_str()); | ||
| publishMqtt("battery", String(static_cast<int>(bat->getLevel())).c_str()); |
There was a problem hiding this comment.
This change is based on a note from coderabbitai on my first proposed version of the code.
The original String(bat->getLevel(), 0) was problematic because for integer values the second String argument is the numeric base, not the number of decimal places. So 0 is not “zero decimals” here.
The static_cast<int> was intended to force normal decimal integer formatting and avoid small-integer / char-like overload ambiguity. However, I agree that it is more verbose than necessary here. I will change this to String(bat->getLevel()), or alternatively String(bat->getLevel(), DEC) if you prefer the decimal base to be explicit.
There was a problem hiding this comment.
I am not too deep into this usermod. I suspect that a float was once changed to an int (if it is an int now) but doing a static cast of an int into an int only makes sense if String class requires that (I dont think it does) so its more confusing than helping here.
There was a problem hiding this comment.
I agree with you. But the old notation with 0 as numeric base is not right either. Which of the two proposed fixes should I commit? String(bat->getLevel()), or alternatively String(bat->getLevel(), DEC)?
There was a problem hiding this comment.
I would expect String(bat->getLevel()) to use base 10 by default? the String(bat->getLevel()), 0) may be a left-over from the time where it was a float and this was required. WLED code has a lot of contributors and history and not everything in it makes perfect sense or is well aligned :)
There was a problem hiding this comment.
Absolutely. To my knowledge you are right about the base 10 default. Ok, then lets use the simplest form of the code: publishMqtt("battery", String(bat->getLevel()).c_str());
3 participants
Enable other usermods to access the battery voltage and percentage using the um_data interface.
Summary by CodeRabbit
Refactor
Bug Fixes