Open
Conversation
…own battery type got removed
Contributor
WalkthroughFactory-based battery instantiation added; LiPo, Li-Ion and LiFePO4 supported. Voltage-to-SoC mapping refactored to LUT interpolation. Charging detection via voltage trends and estimated runtime (coulomb counting) with optional INA226 integration. MQTT/Home Assistant autodiscovery and inter-mod UM data exposure introduced. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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.
Comment on lines
+48
to
+49
| static const uint8_t VOLTAGE_HISTORY_SIZE = 5; // 5 × 30s = 2.5 min window | ||
| float voltageHistory[5] = {0}; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use VOLTAGE_HISTORY_SIZE for the array declaration instead of a magic literal.
voltageHistory[5] is not linked to VOLTAGE_HISTORY_SIZE = 5; changing the constant without updating the array declaration would silently produce a size mismatch.
♻️ Proposed fix
- float voltageHistory[5] = {0};
+ float voltageHistory[VOLTAGE_HISTORY_SIZE] = {0};Based on learnings: "look for 'magic numbers' (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR."
📝 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
| static const uint8_t VOLTAGE_HISTORY_SIZE = 5; // 5 × 30s = 2.5 min window | |
| float voltageHistory[5] = {0}; | |
| static const uint8_t VOLTAGE_HISTORY_SIZE = 5; // 5 × 30s = 2.5 min window | |
| float voltageHistory[VOLTAGE_HISTORY_SIZE] = {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 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.
Comment on lines
+238
to
+239
| um_data->u_data[1] = &umLevel; // int8_t, 0-100% | ||
| um_data->u_type[1] = UMT_BYTE; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find um_types_t enum definition
rg -n 'typedef.*um_types|enum.*um_types' -A 20 --type=hRepository: wled/WLED
Length of output: 35
🏁 Script executed:
#!/bin/bash
# Search for u_type assignments and definitions
rg -n 'u_type\[' -B 2 -A 1 --type=cpp | head -60
rg -n 'UMT_' --type=h --type=cpp | head -40
fd -e h -name '*usermod*' | head -10Repository: wled/WLED
Length of output: 7951
🏁 Script executed:
#!/bin/bash
# Search for consumers of Battery usermod um_data or umLevel
rg -n 'Battery.*umLevel|um_data\[1\]' --type=cpp | head -20
# Also check how Battery um_data is accessed in effects or other code
rg -n 'getUMData.*Battery|battery.*level' -i --type=cpp | head -20Repository: wled/WLED
Length of output: 894
🏁 Script executed:
#!/bin/bash
# Find getUMData function and its usage
rg -n 'getUMData' --type=cpp -B 2 -A 5 | head -40
# Also search for how effects read from usermod data
rg -n 'u_data\[1\]|u_type\[1\]' --type=cpp -B 1 -A 1 | grep -A 1 -B 1 BatteryRepository: wled/WLED
Length of output: 3513
Use UMT_INT16 instead of UMT_BYTE for umLevel, or change to uint8_t with a different sentinel value.
The current code declares umLevel as int8_t (signed) with a sentinel value of −1, but marks it as UMT_BYTE (unsigned). WLED has no UMT_INT8 type. When consumers read this data and interpret it according to u_type[1] = UMT_BYTE, the −1 value will be seen as 0xFF (255), breaking the sentinel check. Either use UMT_INT16 to properly represent the signed value, or refactor umLevel to uint8_t with a meaningful unsigned sentinel (e.g., 0xFF for invalid).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/Battery.cpp` around lines 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]).
Comment on lines
+334
to
+338
| if (current_A < REST_CURRENT_THRESHOLD) { | ||
| if (!atRest) { atRest = true; restStartTime = now; } | ||
| if (now - restStartTime >= REST_RECALIBRATE_MS) { | ||
| coulombSoC = bat->getLevel() / 100.0f; | ||
| } |
Contributor
There was a problem hiding this comment.
Rest recalibration can set coulombSoC to −0.01 if getLevel() returns the −1 sentinel.
bat->getLevel() returns −1 before a valid reading is available. Dividing by 100 gives −0.01, which skips the constrain() call on line 330 and corrupts the Coulomb counter for the next cycle (clamped to 0 the following update, briefly showing 0% SoC).
🐛 Proposed fix
if (now - restStartTime >= REST_RECALIBRATE_MS) {
- coulombSoC = bat->getLevel() / 100.0f;
+ const int8_t lvl = bat->getLevel();
+ if (lvl >= 0) coulombSoC = lvl / 100.0f;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/Battery.cpp` around lines 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.
Comment on lines
+362
to
364
| // auto-off | ||
| if (autoOffEnabled && (autoOffThreshold >= bat->getLevel())) | ||
| turnOff(); |
Contributor
There was a problem hiding this comment.
Auto-off can fire on startup before the first valid battery reading.
bat->getLevel() returns −1 until the first ADC reading is processed. Any non-negative autoOffThreshold satisfies autoOffThreshold >= -1, so turnOff() is called immediately if autoOffEnabled is true. The same guard applied to lowPowerIndicator() (Line 108) is missing here.
🐛 Proposed fix
- if (autoOffEnabled && (autoOffThreshold >= bat->getLevel()))
+ if (autoOffEnabled && bat->getLevel() >= 0 && (autoOffThreshold >= bat->getLevel()))
turnOff();📝 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
| // auto-off | |
| if (autoOffEnabled && (autoOffThreshold >= bat->getLevel())) | |
| turnOff(); | |
| // auto-off | |
| if (autoOffEnabled && bat->getLevel() >= 0 && (autoOffThreshold >= bat->getLevel())) | |
| turnOff(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/Battery.cpp` around lines 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.
Comment on lines
+138
to
+145
| const UMBattery::LutEntry MyBattery::dischargeLut[] PROGMEM = { | ||
| {4.20f, 100.0f}, | ||
| {3.90f, 75.0f}, | ||
| {3.60f, 25.0f}, | ||
| {3.00f, 0.0f}, | ||
| }; | ||
| const uint8_t MyBattery::dischargeLutSize = | ||
| sizeof(MyBattery::dischargeLut) / sizeof(MyBattery::dischargeLut[0]); |
Contributor
There was a problem hiding this comment.
ODR violation if MyBattery.h is included from more than one translation unit.
The static-member definitions of dischargeLut and dischargeLutSize placed in the header will cause a "multiple definition" linker error if MyBattery.h is ever #included from more than one .cpp file.
As long as users follow Step 4 and only include the header from Battery.cpp, this won't trigger — but the template gives no warning and it's easy to miss. The safest fix is to mark the definitions inline (C++17, which ESP-IDF/Arduino frameworks support):
✏️ Proposed fix
-const UMBattery::LutEntry MyBattery::dischargeLut[] PROGMEM = {
+inline const UMBattery::LutEntry MyBattery::dischargeLut[] PROGMEM = {
{4.20f, 100.0f},
{3.90f, 75.0f},
{3.60f, 25.0f},
{3.00f, 0.0f},
};
-const uint8_t MyBattery::dischargeLutSize =
+inline const uint8_t MyBattery::dischargeLutSize =
sizeof(MyBattery::dischargeLut) / sizeof(MyBattery::dischargeLut[0]);Alternatively, add a note near Step 4 reminding users to only include the new header from Battery.cpp.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/Battery/readme.md` around lines 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).
|
|
||
| ## ⏱️ 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 extra configuration needed. |
Contributor
There was a problem hiding this comment.
Clarify "no extra configuration needed" to avoid confusion.
"No extra configuration needed" may mislead readers into thinking INA226 requires zero setup. The Setup section right below it correctly shows that INA226 must be added to custom_usermods. The intended meaning (from the PR description) is no build flags are needed.
✏️ Suggested edit
-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 extra configuration needed.
+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 extra build flags needed (see [Setup](`#setup`) below).📝 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
| 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 extra configuration needed. | |
| 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 extra build flags needed (see [Setup](`#setup`) below). |
🧰 Tools
🪛 LanguageTool
[grammar] ~194-~194: 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 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.
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
1 participant
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