Open
Conversation
…own battery type got removed
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFactory-based battery instantiation added with LiPo, Li‑Ion and LiFePO4 support; voltage-to‑SoC mapping moved to LUT interpolation. Charging detection (voltage-trend) and estimated runtime (coulomb counting with optional INA226) implemented. MQTT/Home Assistant autodiscovery and UM data inter-mod exposure added. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
usermods/Battery/UMBattery.h (1)
45-53: Add a zero-span guard in LUT interpolation.If two adjacent LUT entries have equal voltage, Line 51 divides by zero and produces invalid percentages. This is easy to hit in custom battery definitions.
🛡️ Defensive interpolation guard
- if (v >= lo.voltage) { - float ratio = (v - lo.voltage) / (hi.voltage - lo.voltage); + if (v >= lo.voltage) { + const float span = hi.voltage - lo.voltage; + if (fabsf(span) < 1e-6f) return hi.percent; + float ratio = (v - lo.voltage) / span; return lo.percent + ratio * (hi.percent - lo.percent); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/UMBattery.h` around lines 45 - 53, The LUT interpolation loop in UMBattery.h can divide by zero when adjacent LutEntry voltages are equal; inside the loop that iterates over lut (using variables hi, lo, v, size and type LutEntry) add a zero-span guard: detect when hi.voltage == lo.voltage and handle it (e.g., return lo.percent or the average of lo.percent and hi.percent) instead of computing ratio = (v - lo.voltage) / (hi.voltage - lo.voltage); ensure the guard is placed before calculating ratio so the method that performs interpolation returns a valid percentage for equal-voltage entries.
🤖 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 299-345: When getINA226Current() returns a negative value
(current_A < 0) the code path exits without clearing runtime telemetry, leaving
estimatedTimeLeft and smoothedCurrent stale; update the block that checks
estimatedRuntimeEnabled/getINA226Current (referencing getINA226Current,
estimatedRuntimeEnabled, current_A, estimatedTimeLeft, smoothedCurrent) so that
whenever current_A is invalid (current_A < 0) or estimatedRuntimeEnabled is
false you explicitly reset estimatedTimeLeft = -1 and smoothedCurrent = -1.0f
(and avoid doing Coulomb updates) so no stale runtime value is published.
- Around line 397-398: The JSON "Next update" countdown uses (nextReadTime -
millis())/1000 which can be negative when the read time has passed; change the
code that builds infoNextUpdate (the calls around
infoNextUpdate.add((nextReadTime - millis()) / 1000); infoNextUpdate.add(F("
sec")); ) to compute a non-negative remainingSeconds first (e.g.
remainingSeconds = (nextReadTime > millis()) ? (nextReadTime - millis())/1000 :
0) and then add remainingSeconds and " sec" to infoNextUpdate so the displayed
countdown is clamped to 0.
- Around line 104-121: The low-power logic calls bat->getLevel() before a valid
battery reading exists; modify lowPowerIndicator() to first verify the battery
has a valid/initialized level (e.g., with an existing isValid/hasLevel method or
by checking getLevel() against a clear sentinel like <0 or NaN) and early-return
if the level is invalid, then use the validated value in the existing checks
(lowPowerIndicationDone, lowPowerIndicatorReactivationThreshold,
lowPowerIndicatorThreshold) so
applyPreset(lastPreset)/applyPreset(lowPowerIndicatorPreset) cannot be triggered
until a real battery level is available; update references in
lowPowerIndicator(), lowPowerIndicationDone, lowPowerIndicatorActive,
lowPowerActivationTime, lastPreset, currentPreset and lowPowerIndicatorDuration
accordingly.
- Around line 586-594: The runtime sensor registration is currently skipped in
onMqttConnect() because it checks estimatedRuntimeEnabled which is only set
after the INA226 probe runs in loop(), causing discovery to be missed; remove
the estimatedRuntimeEnabled gate from onMqttConnect() (always call
registerMqttSensor("runtime", F("Runtime"), "sensor", "duration", "min") when
HomeAssistantDiscovery is true) and instead perform the actual enablement/state
publishing when the probe completes — i.e., after the INA226 probe success code
where estimatedRuntimeEnabled is set (or in the probe callback), publish the
runtime value or availability. Ensure references: onMqttConnect,
registerMqttSensor, estimatedRuntimeEnabled, and the INA226 probe completion
path are updated accordingly.
- Around line 132-136: In getINA226Current(), before dereferencing
data->u_data[0], add defensive checks that data->u_size >= 1, data->u_data is
non-null, and data->u_data[0] is non-null (and optionally ensure the pointed
value is a float via proper cast) and return -1.0f on failure; update the
function (which currently calls UsermodManager::getUMData(&data,
USERMOD_ID_INA226)) to perform these validations and only then cast and return
*(float*)data->u_data[0].
In `@usermods/Battery/readme.md`:
- Around line 298-301: Update the changelog date string "2025-02-25" to the
correct PR creation date "2026-02-25" in the Battery readme (search for the
exact token "2025-02-25" in usermods/Battery/readme.md) so the entry aligns with
the PR timestamp.
In `@usermods/Battery/types/UnknownUMBattery.h`:
- Around line 24-25: Add the missing guarded macro definitions for
USERMOD_BATTERY_UNKNOWN_MIN_VOLTAGE and USERMOD_BATTERY_UNKNOWN_MAX_VOLTAGE into
battery_defaults.h so UnknownUMBattery.h can compile; specifically, define
USERMOD_BATTERY_UNKNOWN_MIN_VOLTAGE as 2.6f and
USERMOD_BATTERY_UNKNOWN_MAX_VOLTAGE as 4.2f using `#ifndef/`#define/#endif guards
to match the style of the other battery type macros referenced by
UnknownUMBattery.h.
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Around line 347-351: getUMData currently returns true based on _lastLoopCheck
which is set when sampling starts, allowing stale defaults to be exposed; add a
definitive "measurement complete" guard (e.g. a boolean _measurementReady or a
timestamp _lastMeasurementComplete) that is set only after raw sensor values are
actually read and parsed in the sampling code, and cleared when a new
measurement is triggered, then update getUMData to check that _measurementReady
(or that _lastMeasurementComplete is newer than the measurement start time) in
addition to _settingEnabled before returning um_data; ensure the sampling path
sets/clears that flag/timestamp in the functions that start sampling and the
function that consumes sensor data so getUMData only returns true after a
finished measurement.
---
Nitpick comments:
In `@usermods/Battery/UMBattery.h`:
- Around line 45-53: The LUT interpolation loop in UMBattery.h can divide by
zero when adjacent LutEntry voltages are equal; inside the loop that iterates
over lut (using variables hi, lo, v, size and type LutEntry) add a zero-span
guard: detect when hi.voltage == lo.voltage and handle it (e.g., return
lo.percent or the average of lo.percent and hi.percent) instead of computing
ratio = (v - lo.voltage) / (hi.voltage - lo.voltage); ensure the guard is placed
before calculating ratio so the method that performs interpolation returns a
valid percentage for equal-voltage entries.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
usermods/Battery/Battery.cppusermods/Battery/UMBattery.husermods/Battery/battery_defaults.husermods/Battery/readme.mdusermods/Battery/types/Lifepo4UMBattery.husermods/Battery/types/LionUMBattery.husermods/Battery/types/LipoUMBattery.husermods/Battery/types/UnknownUMBattery.husermods/Battery/types/UnkownUMBattery.husermods/INA226_v2/INA226_v2.cpp
💤 Files with no reviewable changes (1)
- usermods/Battery/types/UnkownUMBattery.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
usermods/Battery/Battery.cpp (2)
260-266:⚠️ Potential issue | 🟠 MajorRuntime HA discovery can still be missed until MQTT reconnect.
If INA226 becomes available after
onMqttConnect(), Line 608’s gate means runtime entity discovery won’t happen in that session. Register runtime when probe succeeds in loop as well.📡 Suggested fix
if (!ina226Probed) { um_data_t *data = nullptr; if (UsermodManager::getUMData(&data, USERMOD_ID_INA226) && data) { estimatedRuntimeEnabled = true; ina226Probed = true; +#ifndef WLED_DISABLE_MQTT + if (HomeAssistantDiscovery && WLED_MQTT_CONNECTED) { + registerMqttSensor("runtime", F("Runtime"), "sensor", "duration", "min"); + } +#endif } }Also applies to: 593-610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` around lines 260 - 266, When the INA226 probe succeeds in the loop (where ina226Probed and estimatedRuntimeEnabled are set), also invoke the same runtime/entity registration that onMqttConnect() performs so discovery is registered immediately instead of waiting for an MQTT reconnect; update the probe-success block (where ina226Probed and estimatedRuntimeEnabled are set) to call the runtime registration function used by onMqttConnect() (the same routine that publishes/registers runtime entities) so runtime discovery occurs right away.
134-138:⚠️ Potential issue | 🟡 MinorHarden
getINA226Current()before dereference.Line 137 dereferences
data->u_data[0]without validating slot availability. Add minimal shape checks to avoid UB if contract ever shifts.🛡️ Suggested fix
float getINA226Current() { um_data_t *data = nullptr; if (!UsermodManager::getUMData(&data, USERMOD_ID_INA226) || !data) return -1.0f; + if (data->u_size < 1 || !data->u_data || !data->u_data[0]) return -1.0f; return *(float*)data->u_data[0]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` around lines 134 - 138, getINA226Current currently dereferences data->u_data[0] without validating the u_data array or its first slot; update getINA226Current to first ensure UsermodManager::getUMData returned true, then verify data is non-null, data->u_data is non-null, and data->u_data[0] is non-null (and if um_data_t exposes a count/length field check it is > 0) before casting to float* and returning the value, otherwise return -1.0f to preserve the existing error path.
🤖 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 269-277: The first-voltage and config-save paths call
readVoltage() even when batteryPin == -1; before any call to readVoltage()
(e.g., in the isFirstVoltageReading block where bat->setVoltage(readVoltage())
and in the config-save path), add a guard that checks batteryPin >= 0 and skip
the read/set when batteryPin is invalid (return early or bypass the ADC read),
so no ADC access occurs when batteryPin is -1.
In `@usermods/Battery/readme.md`:
- Around line 89-90: Replace the typo "Lithium Ionen" with "Lithium Ion" in the
battery chemistry table entry so it matches the rest of the documentation and
UI; update the row that maps to the `USERMOD_BATTERY_LION_MIN_VOLTAGE` symbol
(the table line for lion (Li-Ion)) accordingly.
---
Duplicate comments:
In `@usermods/Battery/Battery.cpp`:
- Around line 260-266: When the INA226 probe succeeds in the loop (where
ina226Probed and estimatedRuntimeEnabled are set), also invoke the same
runtime/entity registration that onMqttConnect() performs so discovery is
registered immediately instead of waiting for an MQTT reconnect; update the
probe-success block (where ina226Probed and estimatedRuntimeEnabled are set) to
call the runtime registration function used by onMqttConnect() (the same routine
that publishes/registers runtime entities) so runtime discovery occurs right
away.
- Around line 134-138: getINA226Current currently dereferences data->u_data[0]
without validating the u_data array or its first slot; update getINA226Current
to first ensure UsermodManager::getUMData returned true, then verify data is
non-null, data->u_data is non-null, and data->u_data[0] is non-null (and if
um_data_t exposes a count/length field check it is > 0) before casting to float*
and returning the value, otherwise return -1.0f to preserve the existing error
path.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/Battery/Battery.cppusermods/Battery/UMBattery.husermods/Battery/readme.mdusermods/INA226_v2/INA226_v2.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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)
142-179:⚠️ Potential issue | 🟡 Minor
StaticJsonDocument<600>may be too small for HA discovery payloads, silently truncating fields.The discovery document includes a nested
devicesub-object (5 fields with string values) on top of the sensor-level fields. ArduinoJson v6 stores string copies in the static buffer, so the effective capacity can be consumed faster than the serialised byte count suggests. When the document is full, subsequentoperator[]assignments are silently dropped.
json_str[1024]already allocates enough stack for a larger document — increase the document capacity to match:♻️ Proposed fix
- StaticJsonDocument<600> doc; + StaticJsonDocument<1024> doc;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` around lines 142 - 179, The StaticJsonDocument<600> in addMqttSensor is too small and can silently drop assignments when building the Home Assistant discovery payload (strings are stored in the JSON buffer); increase the StaticJsonDocument capacity to match the available json_str buffer (e.g., ~1024 or slightly larger) so the nested device object and sensor fields fit, then recompile and verify serializeJson returns the full payload size before calling mqtt->publish; reference the addMqttSensor function, the StaticJsonDocument<600> declaration, and json_str[1024] when making the change.
🧹 Nitpick comments (1)
usermods/Battery/Battery.cpp (1)
20-20: Unnecessary default-constructedLipoUMBatteryallocation at field initialization.
batis unconditionally deleted at the start ofsetup()and replaced by the factory, so the inlinenew LipoUMBattery()allocation is wasted on every boot.♻️ Proposed refactor
- UMBattery* bat = new LipoUMBattery(); + UMBattery* bat = nullptr;Then guard
bat->update(cfg)and any other pre-setup()accesses accordingly (they already are, sincesetup()is always called first).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` at line 20, The field-level allocation "UMBattery* bat = new LipoUMBattery();" wastes a default-constructed LipoUMBattery because setup() immediately deletes and replaces it; remove the inline new and initialize bat to nullptr (or equivalent) instead, update any pre-setup usages by guarding calls like bat->update(cfg) behind a nullptr check, and ensure setup() still constructs the correct battery via the factory and handles deletion appropriately in the setup() method.
🤖 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 48-49: Replace the magic literal in the array declaration with the
constant so the array size stays in sync: change the declaration of
voltageHistory to use VOLTAGE_HISTORY_SIZE (referring to the existing static
const uint8_t VOLTAGE_HISTORY_SIZE) instead of the hardcoded 5, ensuring
voltageHistory is sized using the symbolic constant.
- Around line 362-364: Auto-off currently triggers on startup because
bat->getLevel() returns -1 until the first ADC read; modify the auto-off check
in the Battery class so it only runs when a valid battery level exists (same
guard used for lowPowerIndicator()). Concretely, in the block referencing
autoOffEnabled, autoOffThreshold, bat->getLevel() and turnOff(), add a condition
that bat->getLevel() >= 0 (or reuse the existing validity check used by
lowPowerIndicator()) before comparing to autoOffThreshold so turnOff() is not
called on the initial invalid reading.
- Around line 334-338: When performing the rest recalibration in the block that
checks current_A against REST_CURRENT_THRESHOLD, guard against the
bat->getLevel() sentinel value (−1) before assigning coulombSoC: read int level
= bat->getLevel() and only set coulombSoC = level / 100.0f when level >= 0;
otherwise skip the recalibration so the existing constrained SoC logic
(constrain(...) / Coulomb counter) remains intact. Ensure you still update
atRest and restStartTime as currently done, but avoid using the −1 result to
overwrite coulombSoC.
- Around line 238-239: The umLevel field is declared as int8_t but is being
advertised as UMT_BYTE which treats it as unsigned, causing the -1 sentinel to
appear as 255; fix by either changing the advertised type to a signed container
(set um_data->u_type[1] = UMT_INT16 and ensure um_data->u_data[1] points to a
matching int16_t storage) or change the umLevel storage to uint8_t and use an
unsigned sentinel (e.g., 0xFF) and keep u_type[1] = UMT_BYTE; update any related
sentinel checks to match the chosen representation (refer to umLevel,
um_data->u_data[1], and um_data->u_type[1]).
In `@usermods/Battery/readme.md`:
- Line 194: Replace the ambiguous phrase "no extra configuration needed" in the
Battery readme with a clarification that the INA226 usermod must still be added
to custom_usermods (as shown in the Setup section) but that no additional build
flags or compile-time configuration are required; reference the INA226 usermod
name (INA226_v2) and the custom_usermods entry so readers know where to look.
- Around line 138-145: The header currently defines the static data
MyBattery::dischargeLut and MyBattery::dischargeLutSize which causes an
ODR/multiple-definition linker error if MyBattery.h is included from multiple
translation units; fix it by making those definitions inline (C++17) or moving
their non-inline definitions into a single .cpp (e.g., Battery.cpp) so only one
translation unit emits them—update the declarations/definitions for
MyBattery::dischargeLut and MyBattery::dischargeLutSize accordingly (or add a
comment instructing users to include the header only from Battery.cpp if you
choose the .cpp-only approach).
---
Outside diff comments:
In `@usermods/Battery/Battery.cpp`:
- Around line 142-179: The StaticJsonDocument<600> in addMqttSensor is too small
and can silently drop assignments when building the Home Assistant discovery
payload (strings are stored in the JSON buffer); increase the StaticJsonDocument
capacity to match the available json_str buffer (e.g., ~1024 or slightly larger)
so the nested device object and sensor fields fit, then recompile and verify
serializeJson returns the full payload size before calling mqtt->publish;
reference the addMqttSensor function, the StaticJsonDocument<600> declaration,
and json_str[1024] when making the change.
---
Nitpick comments:
In `@usermods/Battery/Battery.cpp`:
- Line 20: The field-level allocation "UMBattery* bat = new LipoUMBattery();"
wastes a default-constructed LipoUMBattery because setup() immediately deletes
and replaces it; remove the inline new and initialize bat to nullptr (or
equivalent) instead, update any pre-setup usages by guarding calls like
bat->update(cfg) behind a nullptr check, and ensure setup() still constructs the
correct battery via the factory and handles deletion appropriately in the
setup() method.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 325-327: The code currently integrates Coulombs using dt_hours =
(now - lastCoulombTime)/3600000.0f (variable lastCoulombTime) even when previous
current samples were invalid, causing a huge erroneous integration after a gap;
modify the path that handles invalid current samples (the block around the logic
at lines 358-362) to update/reset lastCoulombTime to the current timestamp (now)
whenever you drop or mark current data invalid so that the next valid sample
uses a zero dt interval; ensure the same reset is applied wherever current is
ignored to prevent integrating over stale gaps.
- Around line 382-387: Update the block comment describing Battery data exported
by getUMData() so slot 1's documented type matches the actual exported type:
change the description of "slot 1" from "int8_t, 0-100%" to "int16_t, 0-100%"
(and optionally note UMT_INT16), ensuring the comment in Battery.cpp near the
getUMData() / UM-data slots list accurately reflects that slot 1 uses int16_t
rather than int8_t.
- Line 39: The code computes lowPowerIndicatorReactivationThreshold by adding 10
to lowPowerIndicatorThreshold without clamping, which can exceed 100 or wrap for
uint8_t; update all places that set lowPowerIndicatorReactivationThreshold (the
occurrences that use lowPowerIndicatorThreshold + 10) to first clamp
lowPowerIndicatorThreshold into a safe range (0..100) and compute the
reactivation threshold using a wider integer then cap to 100, e.g. temp =
std::min(100, std::max(0, int(lowPowerIndicatorThreshold) + 10));
lowPowerIndicatorReactivationThreshold = static_cast<uint8_t>(temp); apply the
same clamp logic wherever lowPowerIndicatorReactivationThreshold is computed.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.claude/settings.local.jsonusermods/Battery/Battery.cppusermods/Battery/readme.md
✅ Files skipped from review due to trivial changes (1)
- .claude/settings.local.json
| uint8_t lowPowerIndicatorPreset = USERMOD_BATTERY_LOW_POWER_INDICATOR_PRESET; | ||
| uint8_t lowPowerIndicatorThreshold = USERMOD_BATTERY_LOW_POWER_INDICATOR_THRESHOLD; | ||
| uint8_t lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold+10; | ||
| uint8_t lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold + 10; |
Contributor
There was a problem hiding this comment.
Clamp low-power thresholds to keep reactivation reachable.
Line 39 and Line 578 compute lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold + 10 without capping. With thresholds >90, reactivation can become impossible (>100), and high config values can also overflow behavior.
🔧 Suggested fix
- uint8_t lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold + 10;
+ uint8_t lowPowerIndicatorReactivationThreshold = min<uint8_t>(100, lowPowerIndicatorThreshold + 10);
@@
- lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold + 10;
+ lowPowerIndicatorReactivationThreshold = min<uint8_t>(100, lowPowerIndicatorThreshold + 10);
@@
void setLowPowerIndicatorThreshold(int8_t threshold) {
- lowPowerIndicatorThreshold = threshold;
- lowPowerIndicatorThreshold = autoOffEnabled /*&& lowPowerIndicatorEnabled*/ ? max(autoOffThreshold + 1, (int)lowPowerIndicatorThreshold) : max(5, (int)lowPowerIndicatorThreshold);
+ lowPowerIndicatorThreshold = constrain((int)threshold, 0, 100);
+ lowPowerIndicatorThreshold = autoOffEnabled
+ ? max((int)autoOffThreshold + 1, (int)lowPowerIndicatorThreshold)
+ : max(5, (int)lowPowerIndicatorThreshold);
+ lowPowerIndicatorThreshold = min((int)lowPowerIndicatorThreshold, 100);
+ lowPowerIndicatorReactivationThreshold = min(100, (int)lowPowerIndicatorThreshold + 10);
}Also applies to: 578-579, 650-653
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/Battery.cpp` at line 39, The code computes
lowPowerIndicatorReactivationThreshold by adding 10 to
lowPowerIndicatorThreshold without clamping, which can exceed 100 or wrap for
uint8_t; update all places that set lowPowerIndicatorReactivationThreshold (the
occurrences that use lowPowerIndicatorThreshold + 10) to first clamp
lowPowerIndicatorThreshold into a safe range (0..100) and compute the
reactivation threshold using a wider integer then cap to 100, e.g. temp =
std::min(100, std::max(0, int(lowPowerIndicatorThreshold) + 10));
lowPowerIndicatorReactivationThreshold = static_cast<uint8_t>(temp); apply the
same clamp logic wherever lowPowerIndicatorReactivationThreshold is computed.
Comment on lines
+325
to
+327
| float dt_hours = (now - lastCoulombTime) / 3600000.0f; | ||
| lastCoulombTime = now; | ||
|
|
Contributor
There was a problem hiding this comment.
Reset the Coulomb integration timebase when current data is invalid.
When execution hits Line 358-Line 362, lastCoulombTime is left stale. On the next valid sample, Line 325 integrates over the entire gap using one instantaneous current reading, skewing SoC/runtime.
🔧 Suggested fix
} else {
estimatedTimeLeft = -1;
smoothedCurrent = -1.0f;
atRest = false;
+ if (coulombInitialized) lastCoulombTime = millis();
}Also applies to: 358-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/Battery.cpp` around lines 325 - 327, The code currently
integrates Coulombs using dt_hours = (now - lastCoulombTime)/3600000.0f
(variable lastCoulombTime) even when previous current samples were invalid,
causing a huge erroneous integration after a gap; modify the path that handles
invalid current samples (the block around the logic at lines 358-362) to
update/reset lastCoulombTime to the current timestamp (now) whenever you drop or
mark current data invalid so that the next valid sample uses a zero dt interval;
ensure the same reset is applied wherever current is ignored to prevent
integrating over stale gaps.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
usermods/Battery/Battery.cpp (2)
356-357:⚠️ Potential issue | 🟠 MajorReset Coulomb integration timebase when current data is invalid.
When current becomes invalid,
lastCoulombTimestays stale. The next valid sample integrates over the full outage interval using a single instantaneous current reading.🔧 Proposed fix
} else { estimatedTimeLeft = -1; smoothedCurrent = -1.0f; atRest = false; + if (coulombInitialized) lastCoulombTime = millis(); }Also applies to: 389-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` around lines 356 - 357, When current data is detected invalid, lastCoulombTime is left stale and the next valid sample will integrate over the outage; update the code handling invalid current to reset the Coulomb integration timebase by setting lastCoulombTime = now (the same timestamp used to compute dt_hours) whenever you skip integration due to invalid current so dt_hours will be computed from the next valid sample only; apply the same fix in the other similar block (lines referencing dt_hours/lastCoulombTime around the second integration, e.g., the block at 389-393).
39-39:⚠️ Potential issue | 🟠 MajorClamp low-power thresholds and derive reactivation threshold in one place.
lowPowerIndicatorReactivationThresholdcan exceed 100, which makes reactivation impossible for valid level values. Clamp and recompute in the setter to keep behavior consistent.🔧 Proposed fix
- uint8_t lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold + 10; + uint8_t lowPowerIndicatorReactivationThreshold = min<uint8_t>(100, lowPowerIndicatorThreshold + 10); ... - lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold + 10; ... - lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold + 10; ... void setLowPowerIndicatorThreshold(int8_t threshold) { - lowPowerIndicatorThreshold = threshold; - lowPowerIndicatorThreshold = autoOffEnabled /*&& lowPowerIndicatorEnabled*/ ? max(autoOffThreshold + 1, (int)lowPowerIndicatorThreshold) : max(5, (int)lowPowerIndicatorThreshold); + lowPowerIndicatorThreshold = constrain((int)threshold, 0, 100); + lowPowerIndicatorThreshold = autoOffEnabled + ? max((int)autoOffThreshold + 1, (int)lowPowerIndicatorThreshold) + : max(5, (int)lowPowerIndicatorThreshold); + lowPowerIndicatorThreshold = min((int)lowPowerIndicatorThreshold, 100); + lowPowerIndicatorReactivationThreshold = min(100, (int)lowPowerIndicatorThreshold + 10); }Also applies to: 126-127, 628-629, 723-726
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/Battery/Battery.cpp` at line 39, Clamp lowPowerIndicatorThreshold to the 0–100 range and recompute lowPowerIndicatorReactivationThreshold there instead of ad-hoc additions: ensure the setter that assigns lowPowerIndicatorThreshold (e.g., setLowPowerIndicatorThreshold or the function that currently writes lowPowerIndicatorThreshold) enforces lowPowerIndicatorThreshold = min(max(value, 0), 100) and then sets lowPowerIndicatorReactivationThreshold = min(lowPowerIndicatorThreshold + 10, 100); replace other direct computations of lowPowerIndicatorReactivationThreshold (the occurrences around the existing lines with lowPowerIndicatorReactivationThreshold and any duplicates at the other noted spots) to use the clamped value from that single setter so reactivation never exceeds 100.
🤖 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 664-667: Replace the unsafe strcpy/strcat_P assembly when building
the subscription topic with a bounded snprintf_P call: instead of using subuf,
strcpy(subuf, mqttDeviceTopic) and strcat_P(subuf, PSTR("/battery/set")), call
snprintf_P(subuf, sizeof(subuf), PSTR("%s/battery/set"), mqttDeviceTopic) and
then pass subuf to mqtt->subscribe; this keeps the same topic (matching the
existing strncmp_P logic) but prevents buffer overruns and follows the mqtt.cpp
pattern used elsewhere.
In `@usermods/Battery/readme.md`:
- Line 197: The README references the INA226 usermod inconsistently—update the
setup instructions and the custom_usermods example so they use the exact same
usermod name (either "INA226" or "INA226_v2") throughout (including the
paragraph that mentions "INA226 usermod" and the example block referenced around
lines 197 and 210-212) to prevent copy/paste misconfiguration; pick the
canonical module name used by the repository (e.g., the directory name or module
identifier) and replace the mismatched occurrences so all mentions (setup text
and custom_usermods example) are identical.
---
Duplicate comments:
In `@usermods/Battery/Battery.cpp`:
- Around line 356-357: When current data is detected invalid, lastCoulombTime is
left stale and the next valid sample will integrate over the outage; update the
code handling invalid current to reset the Coulomb integration timebase by
setting lastCoulombTime = now (the same timestamp used to compute dt_hours)
whenever you skip integration due to invalid current so dt_hours will be
computed from the next valid sample only; apply the same fix in the other
similar block (lines referencing dt_hours/lastCoulombTime around the second
integration, e.g., the block at 389-393).
- Line 39: Clamp lowPowerIndicatorThreshold to the 0–100 range and recompute
lowPowerIndicatorReactivationThreshold there instead of ad-hoc additions: ensure
the setter that assigns lowPowerIndicatorThreshold (e.g.,
setLowPowerIndicatorThreshold or the function that currently writes
lowPowerIndicatorThreshold) enforces lowPowerIndicatorThreshold = min(max(value,
0), 100) and then sets lowPowerIndicatorReactivationThreshold =
min(lowPowerIndicatorThreshold + 10, 100); replace other direct computations of
lowPowerIndicatorReactivationThreshold (the occurrences around the existing
lines with lowPowerIndicatorReactivationThreshold and any duplicates at the
other noted spots) to use the clamped value from that single setter so
reactivation never exceeds 100.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
usermods/Battery/Battery.cppusermods/Battery/battery_defaults.husermods/Battery/readme.md
Comment on lines
+664
to
+667
| char subuf[64]; | ||
| strcpy(subuf, mqttDeviceTopic); | ||
| strcat_P(subuf, PSTR("/battery/set")); | ||
| mqtt->subscribe(subuf, 0); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the Battery.cpp file
find . -name "Battery.cpp" -type f | head -5Repository: wled/WLED
Length of output: 83
🏁 Script executed:
# Read the relevant sections of Battery.cpp
if [ -f "usermods/Battery/Battery.cpp" ]; then
echo "=== Lines 660-670 (context for lines 664-667) ==="
sed -n '660,670p' usermods/Battery/Battery.cpp | cat -n
echo ""
echo "=== Lines 688-698 (context for lines 692-693) ==="
sed -n '688,698p' usermods/Battery/Battery.cpp | cat -n
fiRepository: wled/WLED
Length of output: 1021
🏁 Script executed:
# Get full context around subscription and topic matching
if [ -f "usermods/Battery/Battery.cpp" ]; then
echo "=== Full context around MQTT subscription (lines 650-700) ==="
sed -n '650,700p' usermods/Battery/Battery.cpp | cat -n
fiRepository: wled/WLED
Length of output: 2254
🏁 Script executed:
# Search for mqttDeviceTopic definition and size constraints
rg "mqttDeviceTopic" --max-count=15 -A 2 -B 2 | head -80Repository: wled/WLED
Length of output: 5121
🏁 Script executed:
# Check how WLED passes topics to onMqttMessage
rg "onMqttMessage" wled00/ -A 5 -B 2 | head -100Repository: wled/WLED
Length of output: 3946
🏁 Script executed:
# Look for MQTT topic length defines
rg "MQTT.*MAX|MAX.*MQTT|MQTT.*LEN" --max-count=20Repository: wled/WLED
Length of output: 3299
🏁 Script executed:
# Look at the full mqtt message processing flow
sed -n '200,350p' wled00/mqtt.cpp | cat -nRepository: wled/WLED
Length of output: 1991
🏁 Script executed:
# Find the exact topic processing logic
rg "topicPrefixLen" wled00/mqtt.cpp -B 5 -A 10Repository: wled/WLED
Length of output: 748
🏁 Script executed:
# Get more context on how usermods receive MQTT messages
sed -n '260,290p' wled00/mqtt.cpp | cat -nRepository: wled/WLED
Length of output: 35
🏁 Script executed:
# Check if there's any path where usermods get full topic with prefix
rg "UsermodManager::onMqttMessage" wled00/mqtt.cpp -B 10 -A 5Repository: wled/WLED
Length of output: 1084
🏁 Script executed:
# Check the exact prefix stripping logic in mqtt.cpp around line 250-330
sed -n '240,330p' wled00/mqtt.cpp | cat -nRepository: wled/WLED
Length of output: 97
🏁 Script executed:
# Look at the exact topic format used in main WLED mqtt code
rg "sTopicFormat" wled00/mqtt.cpp -B 2 -A 2 | head -20Repository: wled/WLED
Length of output: 834
🏁 Script executed:
# Get the full onMqttMessage handler implementation
sed -n '230,320p' wled00/mqtt.cpp | cat -nRepository: wled/WLED
Length of output: 571
🏁 Script executed:
# Search for the static onMqttMessage function definition
rg "static void onMqttMessage" wled00/mqtt.cpp -A 80 | head -100Repository: wled/WLED
Length of output: 2740
🏁 Script executed:
# Let me verify the exact buffer size calculation
python3 << 'EOF'
# MQTT_MAX_TOPIC_LEN default
max_topic_len = 32
# Battery usermod buffer
buffer_size = 64
# Concatenation needs: mqttDeviceTopic + "/battery/set"
# Max: 32 + strlen("/battery/set") + null terminator
# strlen("/battery/set") = 12
required = max_topic_len + 12 + 1
print(f"MQTT_MAX_TOPIC_LEN: {max_topic_len}")
print(f"Buffer size: {buffer_size}")
print(f"Required for concat: {required}")
print(f"Sufficient: {required <= buffer_size}")
EOFRepository: wled/WLED
Length of output: 132
🏁 Script executed:
# Check if there are other usermods that handle MQTT to see the pattern
rg "onMqttMessage.*override" usermods/ -A 10 | head -60Repository: wled/WLED
Length of output: 4320
🏁 Script executed:
# Look at the complete flow when topic doesn't match deviceTopic or groupTopic
sed -n '265,315p' wled00/mqtt.cpp | cat -nRepository: wled/WLED
Length of output: 35
🏁 Script executed:
# Check if there are any other usermods implementing onMqttMessage to see the pattern
rg "bool onMqttMessage" usermods/ -A 3 | grep -A 3 "onMqttMessage.*override"Repository: wled/WLED
Length of output: 1798
🏁 Script executed:
# Let me trace through an actual example to confirm the topic stripping behavior
# Check the exact sequence: subscription, MQTT message, and topic processing
# First verify the multi_relay implementation to see how other usermods handle this
rg "onMqttMessage.*override" usermods/multi_relay/ -A 15Repository: wled/WLED
Length of output: 1162
🏁 Script executed:
# Find the multi_relay implementation
rg "bool onMqttMessage" usermods/multi_relay/multi_relay.cpp -A 20Repository: wled/WLED
Length of output: 823
🏁 Script executed:
# Search for the actual implementation of onMqttMessage in multi_relay
fd "multi_relay.cpp" usermods/ -x cat {} | grep -A 20 "^bool.*onMqttMessage"Repository: wled/WLED
Length of output: 738
🏁 Script executed:
# Let me verify the exact topic flow by examining if topics are always stripped before reaching usermods
# Check if there's a case where a usermod gets an un-stripped topic
sed -n '253,310p' wled00/mqtt.cpp | cat -nRepository: wled/WLED
Length of output: 35
🏁 Script executed:
# Get the complete onMqttMessage function
rg "static void onMqttMessage" wled00/mqtt.cpp -A 100 | head -110Repository: wled/WLED
Length of output: 3162
The topic matching logic is correct as-is; use snprintf_P instead of strcpy/strcat_P to avoid unsafe buffer operations.
WLED strips the mqttDeviceTopic prefix before passing topics to usermods for WLED-subscribed topics, so the usermod receives the stripped topic "/battery/set" rather than the full prefixed topic. The current strncmp_P check is appropriate. However, using strcpy and strcat_P is unnecessarily unsafe; replace with snprintf_P for clarity and consistency with WLED's codebase (see mqtt.cpp for the standard pattern).
🔧 Suggested fix
if (mqttDeviceTopic[0] != 0) {
char subuf[64];
- strcpy(subuf, mqttDeviceTopic);
- strcat_P(subuf, PSTR("/battery/set"));
+ snprintf_P(subuf, sizeof(subuf), PSTR("%s/battery/set"), mqttDeviceTopic);
mqtt->subscribe(subuf, 0);
}📝 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.
Suggested change
| char subuf[64]; | |
| strcpy(subuf, mqttDeviceTopic); | |
| strcat_P(subuf, PSTR("/battery/set")); | |
| mqtt->subscribe(subuf, 0); | |
| if (mqttDeviceTopic[0] != 0) { | |
| char subuf[64]; | |
| snprintf_P(subuf, sizeof(subuf), PSTR("%s/battery/set"), mqttDeviceTopic); | |
| mqtt->subscribe(subuf, 0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/Battery.cpp` around lines 664 - 667, Replace the unsafe
strcpy/strcat_P assembly when building the subscription topic with a bounded
snprintf_P call: instead of using subuf, strcpy(subuf, mqttDeviceTopic) and
strcat_P(subuf, PSTR("/battery/set")), call snprintf_P(subuf, sizeof(subuf),
PSTR("%s/battery/set"), mqttDeviceTopic) and then pass subuf to mqtt->subscribe;
this keeps the same topic (matching the existing strncmp_P logic) but prevents
buffer overruns and follows the mqtt.cpp pattern used elsewhere.
|
|
||
| ## ⏱️ Estimated Runtime | ||
|
|
||
| The battery usermod can estimate the remaining runtime of your project. This feature is **automatically enabled** when the [INA226 usermod](../INA226_v2/) is detected at runtime — no additional build flags or compile-time configuration are required beyond adding the INA226 usermod to `custom_usermods` (see [Setup](#setup) below). |
Contributor
There was a problem hiding this comment.
Use the exact INA226 usermod name consistently in setup instructions.
The docs point to INA226_v2, but the custom_usermods example uses INA226. Please keep these aligned to avoid copy/paste misconfiguration.
✏️ Suggested edit
-custom_usermods = Battery INA226
+custom_usermods = Battery INA226_v2Also applies to: 210-212
🧰 Tools
🪛 LanguageTool
[grammar] ~197-~197: Ensure spelling is correct
Context: ...> ## ⏱️ Estimated Runtime The battery usermod can estimate the remaining runtime of y...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/readme.md` at line 197, The README references the INA226
usermod inconsistently—update the setup instructions and the custom_usermods
example so they use the exact same usermod name (either "INA226" or "INA226_v2")
throughout (including the paragraph that mentions "INA226 usermod" and the
example block referenced around lines 197 and 210-212) to prevent copy/paste
misconfiguration; pick the canonical module name used by the repository (e.g.,
the directory name or module identifier) and replace the mismatched occurrences
so all mentions (setup text and custom_usermods example) are identical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Major update to the Battery usermod with new features, improved accuracy, and better code organization.
New features
INA226 integration
The battery usermod now discovers INA226. Just list both in
custom_usermodsand it works — no build flags needed. If INA226 isn't present, the features stay hidden and everything else works as before.Charging detection
added charging detection logic.
Code cleanup
readFromConfig()Documentation
Summary by CodeRabbit
New Features
Changes
Documentation