Skip to content

Integrate um_data structure to battery usermod #5521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PSi86 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Integrate um_data structure to battery usermod #5521

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
efb86a6
Integrate um_data structure
PSi86 Apr 21, 2026
60d9f8e
Improve data export
PSi86 Apr 22, 2026
7320b72
Stick closer to original code
PSi86 Apr 23, 2026
6d0085c
Change battery level publishing to avoid static cast
PSi86 Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 55 additions & 16 deletions usermods/Battery/Battery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@ class UsermodBattery : public Usermod
private:
// battery pin can be defined in my_config.h
int8_t batteryPin = USERMOD_BATTERY_MEASUREMENT_PIN;

UMBattery* bat = new UnkownUMBattery();
batteryConfig cfg;

int8_t batteryLevel = 0; // current battery level in %
float batteryVoltage = 0.0f; // current battery voltage in V
Comment on lines +23 to +24
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Copy Markdown
Author

@PSi86 PSi86 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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:

  1. The level is provably non-negative (0–110 range).
  2. uint8_t (max 255) comfortably holds 110.
  3. It aligns with the UMT_BYTE export 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 idiomatic uint8_t choice — 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.

Copy link
Copy Markdown
Author

@PSi86 PSi86 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.



// Initial delay before first reading to allow voltage stabilization
unsigned long initialDelay = USERMOD_BATTERY_INITIAL_DELAY;
bool initialDelayComplete = false;
Expand Down Expand Up @@ -80,6 +84,12 @@ class UsermodBattery : public Usermod
return str;
}

void refreshExportedBatteryData()
{
batteryVoltage = bat->getVoltage();
batteryLevel = bat->getLevel();
}

/**
* Turn off all leds
*/
Expand Down Expand Up @@ -213,6 +223,21 @@ class UsermodBattery : public Usermod
pinMode(batteryPin, INPUT);
#endif

// Export snapshot for um_data is allocated only once.
// setup() may be called again after a pin change via readFromConfig(),
// but the exported pointers remain valid because they still point to
// the same member variables of this usermod instance.
if (!this->um_data) {
this->um_data = new um_data_t;
this->um_data->u_size = 2;
this->um_data->u_type = new um_types_t[this->um_data->u_size];
this->um_data->u_data = new void*[this->um_data->u_size];
this->um_data->u_data[0] = &batteryVoltage;
this->um_data->u_type[0] = UMT_FLOAT;
this->um_data->u_data[1] = &batteryLevel;
this->um_data->u_type[1] = UMT_BYTE;
}

// First voltage reading is delayed to allow voltage stabilization after powering up
nextReadTime = millis() + initialDelay;
lastReadTime = millis();
Expand Down Expand Up @@ -256,7 +281,10 @@ class UsermodBattery : public Usermod
// Make the first voltage reading after the initial delay has elapsed
if (isFirstVoltageReading)
{
bat->setVoltage(readVoltage());
float firstVoltage = readVoltage();
bat->setVoltage(firstVoltage);
bat->calculateAndSetLevel(firstVoltage);
refreshExportedBatteryData();
isFirstVoltageReading = false;
}

Expand All @@ -277,13 +305,14 @@ class UsermodBattery : public Usermod
bat->setVoltage(filteredVoltage);
// translate battery voltage into percentage
bat->calculateAndSetLevel(filteredVoltage);
refreshExportedBatteryData();

// Auto off -- Master power off
if (autoOffEnabled && (autoOffThreshold >= bat->getLevel()))
turnOff();

#ifndef WLED_DISABLE_MQTT
publishMqtt("battery", String(bat->getLevel(), 0).c_str());
publishMqtt("battery", String(static_cast<int>(bat->getLevel())).c_str());
Copy link
Copy Markdown
Collaborator

@DedeHai DedeHai Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? this is worse.

Copy link
Copy Markdown
Author

@PSi86 PSi86 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@DedeHai DedeHai Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@PSi86 PSi86 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Copy Markdown
Collaborator

@DedeHai DedeHai Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Copy Markdown
Author

@PSi86 PSi86 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

publishMqtt("voltage", String(bat->getVoltage()).c_str());
#endif

Expand Down Expand Up @@ -313,7 +342,7 @@ class UsermodBattery : public Usermod

infoNextUpdate.add((nextReadTime - millis()) / 1000);
infoNextUpdate.add(F(" sec"));

if (initializing) {
infoPercentage.add(FPSTR(_init));
infoVoltage.add(FPSTR(_init));
Expand Down Expand Up @@ -376,7 +405,7 @@ class UsermodBattery : public Usermod
setLowPowerIndicatorThreshold(lp[FPSTR(_threshold)] | lowPowerIndicatorThreshold);
lowPowerIndicatorReactivationThreshold = lowPowerIndicatorThreshold+10;
setLowPowerIndicatorDuration(lp[FPSTR(_duration)] | lowPowerIndicatorDuration);

if(initDone)
bat->update(cfg);
}
Expand All @@ -391,9 +420,9 @@ class UsermodBattery : public Usermod

if (battery.isNull())
battery = root.createNestedObject(FPSTR(_name));

addBatteryToJsonObject(battery, true);

DEBUG_PRINTLN(F("Battery state exposed in JSON API."));
}

Expand All @@ -411,7 +440,7 @@ class UsermodBattery : public Usermod

if (!battery.isNull()) {
getUsermodConfigFromJsonObject(battery);

DEBUG_PRINTLN(F("Battery state read from JSON API."));
}
}
Expand Down Expand Up @@ -456,19 +485,22 @@ class UsermodBattery : public Usermod
void addToConfig(JsonObject& root)
{
JsonObject battery = root.createNestedObject(FPSTR(_name));

if (battery.isNull()) {
battery = root.createNestedObject(FPSTR(_name));
}

#ifdef ARDUINO_ARCH_ESP32
battery[F("pin")] = batteryPin;
#endif

addBatteryToJsonObject(battery, false);

// read voltage in case calibration or voltage multiplier changed to see immediate effect
bat->setVoltage(readVoltage());
float configVoltage = readVoltage();
bat->setVoltage(configVoltage);
bat->calculateAndSetLevel(configVoltage);
refreshExportedBatteryData();

Comment thread
coderabbitai[bot] marked this conversation as resolved.
DEBUG_PRINTLN(F("Battery config saved."));
}
Expand All @@ -488,7 +520,7 @@ class UsermodBattery : public Usermod
oappend(F("addInfo('Battery:auto-off:threshold',1,'%');")); // 45 Bytes
oappend(F("addInfo('Battery:indicator:threshold',1,'%');")); // 46 Bytes
oappend(F("addInfo('Battery:indicator:duration',1,'s');")); // 45 Bytes

// this option list would exeed the oappend() buffer
// a list of all presets to select one from
// oappend(F("bd=addDropdown('Battery:low-power-indicator', 'preset');"));
Expand Down Expand Up @@ -523,7 +555,7 @@ class UsermodBattery : public Usermod
#ifdef ARDUINO_ARCH_ESP32
int8_t newBatteryPin = batteryPin;
#endif

JsonObject battery = root[FPSTR(_name)];
if (battery.isNull())
{
Expand Down Expand Up @@ -652,7 +684,7 @@ class UsermodBattery : public Usermod
{
return bat->getMaxVoltage();
}

/**
* Set highest battery voltage
* can't be below minBatteryVoltage
Expand Down Expand Up @@ -733,7 +765,7 @@ class UsermodBattery : public Usermod
{
autoOffEnabled = enabled;
}

/**
* Get auto-off threshold in percent (0-100)
*/
Expand Down Expand Up @@ -844,6 +876,13 @@ class UsermodBattery : public Usermod
{
return HomeAssistantDiscovery;
}

bool getUMData(um_data_t **data) override
{
if (!data || !this->um_data) return false; // no pointer provided or no data available
*data = this->um_data;
return true;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
};

// strings to reduce flash memory usage (used more than twice)
Expand All @@ -858,4 +897,4 @@ const char UsermodBattery::_haDiscovery[] PROGMEM = "HA-discovery";


static UsermodBattery battery;
REGISTER_USERMOD(battery);
REGISTER_USERMOD(battery);