Usermod INA219 Current, Voltage & Power Sensor #4237
Conversation
|
since you keep adding commits, is this work in progress? if so, mark it as such or better yet: commit to this PR once you are finished. |
Hi, no its finsied. I just did some small formatting changes while waiting for a reply. But now im happy with it ;) EDIT: im also using it for like 2 weeks now without any problems |
|
ok nice. did you test it working on different ESPs? |
|
i tested it on an ESP32 S2 (lolin_s2_mini) and an C3 (esp32c3dev) where it is working. like i said its already running for around 2 weeks |
|
Hi, i tested it now also with an ESp32 Wroom-32 (esp32dev) and it also worked. did i forget something so it will be added to version 0,15? |
|
looks fine, needs more testing and review, there are a few usermods in pipeline so please be patient. |
changed uint8_t to int8_t for _sdaPin and _sclPin ---> to be able to pick "unused" in pin selection which is -1,
Several optimizations for WLED_MQTT_CONNECTED and if Usermod is enabled/disabled
fixed endif to #endif
- Moved configurable settings to the top of the file for better readability. - Defined default values for each configuration option using #ifndef directives. - Updated default I2C pins to WLED ESP32 defaults (SDA: 21, SCL: 22). - Cleaned up variable initialization to use pre-defined constants for configuration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/INA219_v2/INA219_v2.cpp (2)
44-46: Missing<cmath>include.The code uses math functions like
roundf,powf, andfabsfbut doesn't include the necessary header.#include "wled.h" #include <INA219_WE.h> +#include <cmath> // roundf, powf, fabsf
142-154:⚠️ Potential issuePotential memory leak if initialization fails.
If
_ina219->init()fails, the allocated object is not deleted, causing memory leaks._ina219 = new INA219_WE(_i2cAddress); if (!_ina219) { DEBUG_PRINTLN(F("Failed to allocate memory for INA219 sensor!")); enabled = false; return false; } if (!_ina219->init()) { DEBUG_PRINTLN(F("INA219 initialization failed!")); enabled = false; + delete _ina219; + _ina219 = nullptr; return false; }
🧹 Nitpick comments (2)
usermods/INA219_v2/INA219_v2.cpp (2)
126-136: Consider adding a null check before accessing_ina219.When
updateINA219Settings()is called, you check for valid I2C pins but don't verify that_ina219is properly allocated before dereference in the initialization or other functions.// Validate I2C pins; if invalid, disable usermod and log message if (i2c_scl < 0 || i2c_sda < 0) { enabled = false; DEBUG_PRINTLN(F("INA219 disabled: Invalid I2C pins. Check global I2C settings.")); return false; }
400-404: Consider adding overflow protection for power calculation.The energy calculation assumes the power value is valid. Consider adding a check for overflow or out-of-range values before updating energy consumption.
// Update energy consumption if (lastPublishTime != 0) { + // Only update energy if power reading is valid and not in overflow state + if (!overflow && power >= 0) { updateEnergy(power, lastCheck - lastPublishTime); + } } lastPublishTime = lastCheck;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/INA219_v2/INA219_v2.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
usermods/INA219_v2/INA219_v2.cpp (1)
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
🔇 Additional comments (11)
usermods/INA219_v2/INA219_v2.cpp (11)
101-107: Good use ofpowfwith float arguments.The function correctly uses
powfwith float arguments to avoid unnecessary type conversions, which saves flash memory on ESP devices.
162-201: Efficient UTF-8 character handling for MQTT client ID sanitization.The function to sanitize MQTT client IDs handles UTF-8 encoded characters like umlauts and ß efficiently by checking for the UTF-8 lead byte and mapping them to ASCII equivalents.
206-238: Energy calculation implementation with time-based resets.The energy calculation logic handles daily and monthly resets appropriately based on date/time changes. This approach ensures accurate energy tracking over time.
273-275: Fixed potential buffer overflow risk by passing buffer size.By using the buffer size parameter in
serializeJson(), the code mitigates the risk of buffer overflows when serializing JSON.
333-335: Fixed potential buffer overflow risk by passing buffer size.The code correctly uses
serializeJson(doc, buffer, sizeof(buffer))to prevent potential buffer overflows when serializing JSON for Home Assistant discovery.
379-381: Good early-return pattern for optimization.The loop function efficiently uses the early-return pattern to avoid unnecessary processing when conditions aren't met, which is a good optimization for microcontrollers.
479-480: Properly usingstrstrfor MQTT topic matching.The code correctly uses
strstrto check if the topic contains the path rather than requiring an exact match, which is essential for MQTT topic handling with prefixes.
615-616: Correctly storing the check interval in seconds in the config.The code divides the millisecond value by 1000 to store it as seconds in the config JSON, which is the expected format.
670-680: Correct handling of interval values with safe conversion.The code properly validates the check interval value from the config and uses a safe conversion with
static_cast<uint32_t>to avoid overflow issues.
702-704: Standard usermod ID implementation.The
getId()method returns the appropriate constant defined in WLED's constant definitions, which is necessary for proper usermod registration.
707-710: Proper usermod registration using the REGISTER_USERMOD macro.The code correctly creates a static instance and registers it with the WLED framework using the standard registration macro.
got the numbers mixed up, yes I mean INA226 which should work very much the same way, so would be good to support both using a #define since you already put in a lot of work and this UM is much more versatile than the old one. |
ok i will give it a try soon, but it will take some time, since i also want to buy an ina226 so i can try it out EDIT: also the library.json looks like this now: how can i splitt is so that only the correct lib will be used, like this? (cant test it currently) with library.json: setup_ina_deps.py: |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
usermods/INA219_v2/INA219_v2.cpp (1)
44-46:⚠️ Potential issueMissing
<cmath>include for mathematical functionsThe code uses mathematical functions (
roundf,powf,fabsf) but doesn't include the<cmath>header explicitly. While it might compile due to indirect inclusions, this is fragile and platform-dependent.#include "wled.h" #include <INA219_WE.h> +#include <cmath>
🧹 Nitpick comments (2)
usermods/INA219_v2/INA219_v2.cpp (2)
209-211: Consider handling edge cases in energy calculationThe
updateEnergyfunction doesn't handle very small power values which might lead to rounding errors when calculating extremely low energy usage.void updateEnergy(float power, unsigned long durationMs) { float durationHours = durationMs / 3600000.0; // Milliseconds to hours + // Skip negligible power values to avoid accumulating rounding errors + if (power < 0.01) return; float energy_kWh = (power / 1000.0) * durationHours; // Watts to kilowatt-hours (kWh)
388-402: Missing error handling for sensor communication failuresThe sensor reading code doesn't include error handling for potential I2C communication failures which might occur during operation, especially in environments with electrical noise.
Consider adding error detection and recovery:
// Fetch sensor data + bool readSuccess = true; + shuntVoltage = truncateDecimals(_ina219->getShuntVoltage_mV()); busVoltage = truncateDecimals(_ina219->getBusVoltage_V()); float rawCurrent_mA = _ina219->getCurrent_mA(); + // Simple error check - unrealistic values might indicate communication issues + if (rawCurrent_mA < -10000 || rawCurrent_mA > 10000) { + DEBUG_PRINTLN(F("INA219 unrealistic current reading detected")); + readSuccess = false; + } current_mA = truncateDecimals(rawCurrent_mA); current = truncateDecimals(rawCurrent_mA / 1000.0); // Convert from mA to A float rawPower_mW = _ina219->getBusPower(); + // Skip energy calculation on suspicious readings + if (!readSuccess) { + DEBUG_PRINTLN(F("INA219 skipping energy calculation due to suspicious readings")); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/INA219_v2/INA219_v2.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
usermods/INA219_v2/INA219_v2.cpp (1)
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
🔇 Additional comments (10)
usermods/INA219_v2/INA219_v2.cpp (10)
521-522: MQTT topic construction follows good practicesThe MQTT subscription topic construction correctly uses the device's MQTT topic prefix, which will properly route incoming messages to this usermod.
481-485: Proper MQTT topic matching implementationGood job using
strstr()instead ofstrcmp_P()for topic matching. This correctly handles the device topic prefix in MQTT messages, ensuring proper functionality.
675-685: Well-implemented interval validationThe code properly validates the check interval and uses appropriate type conversions to prevent overflow. The use of
static_cast<uint32_t>and1000ULensures correct handling of larger intervals.
277-278: Buffer overflow protection correctly implementedGood job protecting against buffer overflow by passing the buffer size to
serializeJson(). This ensures that the serialization won't write beyond the allocated buffer.
127-136: Good I2C pin validationThe code properly validates the I2C pins before proceeding with sensor initialization, preventing potential issues on platforms where I2C might not be available.
150-156: Proper memory cleanup during initialization failureThe initialization code properly handles failure cases by deleting the allocated sensor object and setting it to nullptr when initialization fails, preventing memory leaks.
224-228: Energy tracking with daily resetThe daily energy reset logic correctly handles both midnight resets and day changes, ensuring energy tracking is properly compartmentalized by day.
232-240: Energy tracking with monthly resetThe monthly energy reset logic correctly identifies month changes using a clever calculation (
year*12 + month), ensuring proper monthly energy tracking.
458-472: Clean cleanup of Home Assistant entitiesThe code properly removes all Home Assistant entities when the discovery feature is disabled, which is an important cleanup step to avoid orphaned entities in Home Assistant.
142-149: 🛠️ Refactor suggestionMemory allocation check improvement needed
The
newoperator allocation result is checked at line 144, but it's actually safer to check the result immediately after allocation to avoid any potential dereference of a null pointer.- _ina219 = new INA219_WE(_i2cAddress); - - if (!_ina219) { + auto* tmp = new INA219_WE(_i2cAddress); + if (!tmp) { DEBUG_PRINTLN(F("Failed to allocate memory for INA219 sensor!")); enabled = false; return false; } + _ina219 = tmp;Likely an incorrect or invalid review comment.
|
good question. what happens if you add in both libraries in the dependencies? does that eat up flash even if only one of them is used? |
ok i tried it with: and before and after the ram and flash size didnt change from: the bin file is also the same size 1074kb, so i will keep it that way
ok i will rename it that way. i think when i add the ina226 the current once can be removed, since i also started with that usermod to create this one.
hmm i need to look that up, but i think mostly the mqtt topics are the same. i think only the overflow changed since it wasnt working for me. |
…bility - Fixed issue with energy values being overwritten on startup or reflash - Restored energy values from MQTT are now added to current totals instead of replacing them - Added additional configuration options for INA sensor behavior
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/INA219_v2/INA219_v2.cpp (1)
60-62:⚠️ Potential issueMissing
<cmath>includeFunctions like
roundf,pow, andfabsffrom<cmath>are used but the header is not included. The code may compile due to indirect inclusions, but this is fragile.#include "wled.h" #include <INA219_WE.h> +#include <cmath> // roundf, pow, fabsf
🧹 Nitpick comments (1)
usermods/INA219_v2/INA219_v2.cpp (1)
869-1021: Configuration handling could be simplifiedThe
readFromConfigmethod is quite long (150+ lines) and follows a repetitive pattern. Consider refactoring to use a helper function for each setting type to reduce code duplication.Example pattern that could be extracted into a helper:
template<typename T> bool updateConfigValue(JsonObject& obj, const char* key, T& current, const char* logName) { T temp = current; if (getJsonValue(obj[key], temp)) { if (temp != current) { _logUsermodInaSensor("%s updated to: %d", logName, temp); current = temp; } return true; } return false; }This would reduce the repetitive pattern seen throughout the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
usermods/INA219_v2/img/homeassistant.pngis excluded by!**/*.pngusermods/INA219_v2/img/info.pngis excluded by!**/*.pngusermods/INA219_v2/img/usermod_settings.pngis excluded by!**/*.png
📒 Files selected for processing (3)
usermods/INA219_v2/INA219_v2.cpp(1 hunks)usermods/INA219_v2/library.json(1 hunks)wled00/const.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/const.h
🧰 Additional context used
🧠 Learnings (1)
usermods/INA219_v2/INA219_v2.cpp (1)
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
🔇 Additional comments (12)
usermods/INA219_v2/library.json (1)
1-7: LGTM - Dependencies properly definedThe library manifest correctly defines dependencies on both INA219_WE and INA226_WE libraries with appropriate version constraints using the tilde notation (~), which allows patch-level updates but locks the minor version.
usermods/INA219_v2/INA219_v2.cpp (11)
1-58: Well-structured configuration section with appropriate defaultsThe configuration section is well-organized with clear default values and descriptive comments for each parameter. The use of
#ifndef/#defineblocks allows for easy customization while providing sensible defaults.
123-130: Well-implemented decimal truncation functionThe function correctly uses
powfwith float arguments as recommended in previous reviews, which helps avoid unnecessary type conversions and saves flash memory on ESP devices.
157-217: Memory management in sensor initialization is now saferThe function properly cleans up resources if initialization fails, preventing memory leaks. The function checks for required conditions (valid I2C pins) before attempting to initialize the sensor.
220-260: Thorough MQTT client ID sanitizationGood handling of character replacements, including UTF-8 accented characters. The function ensures MQTT client IDs meet specifications by replacing invalid characters with underscores.
265-314: Well-implemented energy calculation with data validationThe energy calculation includes several safeguards:
- Skips negligible power values to prevent cumulative rounding errors
- Rejects unrealistic energy values
- Properly handles daily and monthly resets
- Uses appropriate time conversion for energy calculation
The approach makes the energy calculation robust against outliers and calculation errors.
320-369: MQTT publishing correctly handles buffer size limitsThe code now properly passes the buffer size to
serializeJson()as recommended in previous reviews to prevent potential buffer overflows.
374-438: Home Assistant discovery implementation with proper buffer handlingThe code correctly checks for potential buffer overflow in topic string construction, addressing earlier review feedback. The discovery configuration follows Home Assistant MQTT discovery standards.
440-457: Home Assistant sensor removal with proper buffer handlingSimilar to the discovery implementation, this code now correctly checks for potential buffer overflow when constructing topic strings.
599-643: MQTT message handling correctly uses topic substring matchingThe code now properly uses
strstr()instead ofstrcmp_P()for checking MQTT topics as recommended in previous reviews, allowing it to correctly handle device-prefixed topics.
894-908: Improved handling of check_interval configurationThe code now correctly handles the conversion from seconds to milliseconds with proper type casting and validation, avoiding potential overflow issues.
1033-1034: Clear and proper usermod registrationThe usermod is correctly registered with a static instance and the
REGISTER_USERMODmacro, following the WLED convention.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
usermods/ina2xx_v2/ina2xx_v2.cpp (1)
123-130:truncateDecimals()rounds, re-computespowf()every call, and hurts performance
- The function name says truncate but the logic uses
roundf, i.e. it
rounds – that can be misleading when somebody expects a pure truncation.powf(10, _decimalFactor)is evaluated each sensor read, although
_decimalFactorrarely changes.
A cached factor (or a simpleswitchon 0-3) saves several floating-point
operations per loop.- float truncateDecimals(float val) { - … - float factor = powf(10.0f, _decimalFactor); - return roundf(val * factor) / factor; - } + float roundDecimals(float val) { + static float factorLUT[4] = {1.f, 10.f, 100.f, 1000.f}; + float factor = (_decimalFactor <= 3) ? factorLUT[_decimalFactor] + : powf(10.0f, _decimalFactor); + return roundf(val * factor) / factor; + }Please also rename the caller(s) for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
usermods/ina2xx_v2/img/INA219-Pin-Outs.pngis excluded by!**/*.pngusermods/ina2xx_v2/img/homeassistant.pngis excluded by!**/*.pngusermods/ina2xx_v2/img/info.pngis excluded by!**/*.pngusermods/ina2xx_v2/img/usermod_settings.pngis excluded by!**/*.pngusermods/ina2xx_v2/img/usermod_settings_pins_selection.pngis excluded by!**/*.png
📒 Files selected for processing (3)
usermods/ina2xx_v2/Readme.md(1 hunks)usermods/ina2xx_v2/ina2xx_v2.cpp(1 hunks)usermods/ina2xx_v2/library.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- usermods/ina2xx_v2/library.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
usermods/ina2xx_v2/Readme.md
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Hard tabs
Column: 1
(MD010, no-hard-tabs)
55-55: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
58-58: Hard tabs
Column: 1
(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1
(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1
(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1
(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1
(MD010, no-hard-tabs)
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
67-67: Hard tabs
Column: 1
(MD010, no-hard-tabs)
68-68: Hard tabs
Column: 1
(MD010, no-hard-tabs)
69-69: Hard tabs
Column: 1
(MD010, no-hard-tabs)
70-70: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
77-77: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Hard tabs
Column: 1
(MD010, no-hard-tabs)
81-81: Hard tabs
Column: 1
(MD010, no-hard-tabs)
82-82: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
103-103: Hard tabs
Column: 1
(MD010, no-hard-tabs)
104-104: Hard tabs
Column: 1
(MD010, no-hard-tabs)
105-105: Hard tabs
Column: 1
(MD010, no-hard-tabs)
106-106: Hard tabs
Column: 1
(MD010, no-hard-tabs)
107-107: Hard tabs
Column: 1
(MD010, no-hard-tabs)
108-108: Hard tabs
Column: 1
(MD010, no-hard-tabs)
109-109: Hard tabs
Column: 1
(MD010, no-hard-tabs)
110-110: Hard tabs
Column: 1
(MD010, no-hard-tabs)
111-111: Hard tabs
Column: 1
(MD010, no-hard-tabs)
112-112: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (3)
usermods/ina2xx_v2/ina2xx_v2.cpp (2)
320-369: Possible MQTT payload truncation – 1 kB static buffer is not guaranteed to fit
serializeJson(jsonDoc, buffer, sizeof(buffer))silently truncates when the
payload does not fit into 1024 bytes, leading to corrupted JSON and Home
Assistant discovery failures.Please measure the exact size first or use
serializeJson(doc, mqttClient);
with a dynamically sizedString, e.g.size_t len = measureJson(jsonDoc); if (len >= sizeof(buffer)) { _logUsermodInaSensor("JSON size %u exceeds buffer", len); }or switch to
StaticJsonDocument<512>+ stream.
266-314: 🛠️ Refactor suggestionDaily / monthly reset relies on hitting exactly 00:00 – can easily miss and drift
With a 5 s default sampling interval it is quite possible that the
loop()
iteration never lands on minute = 0 (especially if the MCU is busy or Wi-Fi
blocks). When that happens the counters will never reset and start drifting.Consider resetting solely on day / month change instead of the timestamp
edge:- if ((hour(localTime) == 0 && minute(localTime) == 0 && dailyResetTime != currentDay) || - (currentDay > dailyResetTime && dailyResetTime > 0)) { + if (currentDay != dailyResetTime) { … }The same applies to the monthly logic below.
Likely an incorrect or invalid review comment.
usermods/ina2xx_v2/Readme.md (1)
86-91: Build instruction points to a non-existing identifier
custom_usermods = … INA219_v2
but the folder and theREGISTER_USERMODmacro register ina2xx_v2.
Copy-pasting these lines will therefore fail to compile.- custom_usermods = ${env:d1_mini.custom_usermods} INA219_v2 + custom_usermods = ${env:d1_mini.custom_usermods} ina2xx_v2🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
87-87: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/ina2xx_v2/ina2xx_v2.cpp (1)
195-196: Implement INA226 sensor supportCurrently, the code only instantiates INA219_WE objects despite the more generic INA2xx naming.
I see from the learnings that you plan to add INA226 support in a future update. When implementing it, consider using a compile-time switch approach as previously suggested:
- _ina2xx = new INA219_WE(_i2cAddress); + #if defined(USE_INA226) + _ina2xx = new INA226_WE(_i2cAddress); + #else + _ina2xx = new INA219_WE(_i2cAddress); + #endif
🧹 Nitpick comments (4)
usermods/ina2xx_v2/ina2xx_v2.cpp (4)
149-155: Consider using named constants instead of magic numbersThe threshold value (0.01f) is hardcoded, which makes it harder to understand its significance and makes future adjustments more difficult.
-bool hasSignificantChange(float oldValue, float newValue, float threshold = 0.01f) { +// Define at class level +static constexpr float DEFAULT_CHANGE_THRESHOLD = 0.01f; + +bool hasSignificantChange(float oldValue, float newValue, float threshold = DEFAULT_CHANGE_THRESHOLD) {
287-296: Replace magic numbers with named constantsThe code uses hardcoded thresholds (0.01 and 10.0) for filtering negligible power values and unrealistic energy spikes.
+// Define at class level +static constexpr float MIN_POWER_THRESHOLD_W = 0.01f; // Minimum power in watts to consider for energy calculation +static constexpr float MAX_ENERGY_SPIKE_KWH = 10.0f; // Maximum realistic energy spike in kWh // Skip negligible power values to avoid accumulating rounding errors -if (power < 0.01) { +if (power < MIN_POWER_THRESHOLD_W) { _logUsermodInaSensor("SKIPPED: Power too low (%.3f W) — skipping to avoid rounding errors.", power); return; } // Skip negative values or unrealistic spikes -if (energy_kWh < 0 || energy_kWh > 10.0) { // 10 kWh in a few seconds is unrealistic +if (energy_kWh < 0 || energy_kWh > MAX_ENERGY_SPIKE_KWH) { // Unrealistic energy spike _logUsermodInaSensor("SKIPPED: Energy value out of range (%.6f kWh)", energy_kWh); return; }
530-537: Handle millis() overflow in time calculationsWhen calculating time differences with millis(), overflow can occur after approximately 49.7 days of uptime, which could affect energy calculations.
// Update energy consumption if (lastPublishTime != 0) { if (power >= 0) { - updateEnergy(power, lastCheck - lastPublishTime); + // Handle millis() overflow when calculating duration + unsigned long duration = (lastCheck >= lastPublishTime) + ? (lastCheck - lastPublishTime) + : (0xFFFFFFFF - lastPublishTime + lastCheck + 1); + updateEnergy(power, duration); } else { _logUsermodInaSensor("Skipping energy update due to negative power: %.3f W", power); } } else { _logUsermodInaSensor("First reading - establishing baseline for energy calculation"); } lastPublishTime = lastCheck;
61-61: Consider preparing for future INA226 supportThe code only includes the INA219_WE library but plans to support INA226 in the future.
#include "wled.h" #include <INA219_WE.h> +// Uncomment when adding INA226 support +// #include <INA226_WE.h> + +// Uncomment and define when implementing INA226 support +// #define USE_INA226
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/ina2xx_v2/ina2xx_v2.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
usermods/ina2xx_v2/ina2xx_v2.cpp (1)
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/ina2xx_v2/ina2xx_v2.cpp:178-185
Timestamp: 2025-05-06T20:31:05.167Z
Learning: KrX3D plans to add INA226 support to the INA2xx usermod in a future update. Currently, the code only instantiates INA219_WE objects despite the more generic naming.
🔇 Additional comments (4)
usermods/ina2xx_v2/ina2xx_v2.cpp (4)
173-234: Robust sensor initialization flowThe sensor initialization code is well-implemented with proper error handling and comprehensive logging. It validates I2C pins, ensures memory allocation is successful, and checks for successful sensor initialization.
282-331: Well-structured energy calculation logicThe energy calculation implementation is thorough with:
- Proper conversion from power and duration to energy
- Filtering of negligible or unrealistic values to avoid accumulating errors
- Intelligent day/month boundary detection for counter resets
- Detailed logging at each step
This creates a reliable foundation for accurate energy tracking over time.
391-455: Comprehensive Home Assistant integrationThe Home Assistant MQTT discovery implementation is excellent, with:
- Proper device and entity configuration
- Unique IDs to prevent duplicates
- Appropriate device classes and units
- Well-structured value templates
- Buffer overflow checks for topic generation
This will provide a seamless integration experience for Home Assistant users.
84-99: Well-organized configuration variablesThe configuration variables are well-structured with clear naming and appropriate use of static casting for enum types. This creates a maintainable foundation for the module's configuration.
|
what is the status? |
|
Hi, |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usermods/ina2xx_v2/Readme.md (2)
20-20: Remove hard tabs in markdown
Line 20 is triggering a markdownlint MD010 error (no-hard-tabs). Convert any hard tabs to spaces for consistent formatting.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Hard tabs
Column: 144(MD010, no-hard-tabs)
133-161: Fix numbering in Usage steps
Steps are numbered 1, 2, 3, 3. Please renumber to maintain a single ascending sequence (e.g., 1–4) to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
usermods/ina2xx_v2/img/usermod_settings_ina219.pngis excluded by!**/*.pngusermods/ina2xx_v2/img/usermod_settings_ina226.pngis excluded by!**/*.png
📒 Files selected for processing (1)
usermods/ina2xx_v2/Readme.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
usermods/ina2xx_v2/Readme.md (1)
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/ina2xx_v2/ina2xx_v2.cpp:178-185
Timestamp: 2025-05-06T20:31:05.208Z
Learning: KrX3D plans to add INA226 support to the INA2xx usermod in a future update. Currently, the code only instantiates INA219_WE objects despite the more generic naming.
🪛 markdownlint-cli2 (0.17.2)
usermods/ina2xx_v2/Readme.md
20-20: Hard tabs
Column: 144
(MD010, no-hard-tabs)
🔇 Additional comments (2)
usermods/ina2xx_v2/Readme.md (2)
1-4: Title and intro claim INA226 support before it’s implemented
The README states support for both INA219 and INA226, but the current code only instantiates INA219_WE objects (per retrieved learnings). Please confirm whether INA226 is fully supported in this release or adjust the documentation accordingly.
120-130: Verifycustom_usermodsidentifier matches code
The example usesINA2XX_v2—please confirm that this matches the actual folder name, class name, and#define USERMOD_ID_INA2XXentry so that PlatformIO will pick it up correctly.
|
Hi @DedeHai i updated the usermod to support the INA226. I tested it in series with my Ina219 sensor which is running since months on a project and the values reported were nearly the same (0,002 difference) I also updated the readme and some pictures |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/ina2xx_v2/Readme.md (2)
24-29: Update screenshots to show the combined INA2XX usermod interfaceThe screenshots still show separate INA219 and INA226 settings pages, but this is now a unified usermod with a sensor type selector.
181-181: Update instruction to reference the unified INA2XX usermodThe text should refer to "the INA2XX usermod" instead of separately mentioning INA219 or INA226.
🧹 Nitpick comments (2)
usermods/ina2xx_v2/ina2xx_v2.h (1)
80-92: Macro has implicit dependency onconfigCompletevariableThe
UPDATE_CONFIGmacro requiresconfigCompleteto be in scope wherever it's used. Consider documenting this requirement or passing it as a parameter to make the dependency explicit.-#define UPDATE_CONFIG(obj, key, var, fmt) \ +// Note: Requires 'configComplete' boolean variable in scope +#define UPDATE_CONFIG(obj, key, var, fmt) \usermods/ina2xx_v2/Readme.md (1)
3-3: Fix grammar: "sensor" should be "sensors"Since you're referring to both INA219 and INA226, use the plural form.
-This Usermod integrates the INA219 **and** INA226 sensor with WLED to monitor energy consumption. It can read voltage, current, power, and calculate daily, monthly and total energy usage for either sensor. +This Usermod integrates the INA219 **and** INA226 sensors with WLED to monitor energy consumption. It can read voltage, current, power, and calculate daily, monthly and total energy usage for either sensor.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...od integrates the INA219 and INA226 sensor with WLED to monitor energy consumption...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/ina2xx_v2/Readme.md(1 hunks)usermods/ina2xx_v2/ina2xx_v2.cpp(1 hunks)usermods/ina2xx_v2/ina2xx_v2.h(1 hunks)usermods/ina2xx_v2/library.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/ina2xx_v2/library.json
- usermods/ina2xx_v2/ina2xx_v2.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
usermods/ina2xx_v2/Readme.md (1)
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/ina2xx_v2/ina2xx_v2.cpp:178-185
Timestamp: 2025-05-06T20:31:05.208Z
Learning: KrX3D plans to add INA226 support to the INA2xx usermod in a future update. Currently, the code only instantiates INA219_WE objects despite the more generic naming.
🧬 Code Graph Analysis (1)
usermods/ina2xx_v2/ina2xx_v2.h (1)
usermods/ina2xx_v2/ina2xx_v2.cpp (1)
UsermodINA2xx(434-440)
🪛 LanguageTool
usermods/ina2xx_v2/Readme.md
[uncategorized] ~3-~3: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...od integrates the INA219 and INA226 sensor with WLED to monitor energy consumption...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
🪛 markdownlint-cli2 (0.17.2)
usermods/ina2xx_v2/Readme.md
20-20: Hard tabs
Column: 152
(MD010, no-hard-tabs)
🔇 Additional comments (4)
usermods/ina2xx_v2/ina2xx_v2.h (4)
1-5: LGTM!Standard header structure with appropriate includes.
6-20: Well-designed sensor type selection mechanismThe compile-time sensor selection using type alias provides a clean abstraction. Good error handling for invalid sensor types.
93-159: Well-structured class member variablesGood organization with clear separation of configuration, state, and sensor-specific variables. The change detection mechanism using last sent values is a nice optimization.
178-179: Proper resource cleanup in destructorThe destructor correctly handles cleanup of the dynamically allocated sensor object as shown in the implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/ina2xx_v2/Readme.md (2)
26-29: Screenshots not reflecting unified UI.
The Settings section still shows separate INA219 and INA226 pages, but this is a single INA2XX usermod with a sensor-type selector. Please update these images to display the combined UI.
181-181: Update final enable instruction.
The callout refers to enabling the "INA219" or "INA226" usermod, but the unified module should be referred to as "INA2XX" (with sensor-type selector).
🧹 Nitpick comments (3)
usermods/ina2xx_v2/Readme.md (3)
3-3: Grammar: Use plural for sensors.
Replace "sensor" with "sensors" to match the plural integration scope.- This Usermod integrates the INA219 **and** INA226 sensor with WLED to monitor energy consumption. + This Usermod integrates the INA219 **and** INA226 sensors with WLED to monitor energy consumption.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...od integrates the INA219 and INA226 sensor with WLED to monitor energy consumption...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
20-22: Replace hard tabs with spaces.
Static analysis flagged a hard tab in the table header (line 20). Convert any hard tabs to spaces to comply with markdownlint MD010.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Hard tabs
Column: 152(MD010, no-hard-tabs)
34-38: Consolidate sensor type definition instructions.
The README duplicates the#define INA_SENSOR_TYPEsnippet in two places. Consider merging into a single section and referring back to avoid redundancy.Also applies to: 133-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
usermods/ina2xx_v2/img/INA226-Pin-Outs.pngis excluded by!**/*.png
📒 Files selected for processing (1)
usermods/ina2xx_v2/Readme.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
usermods/ina2xx_v2/Readme.md (1)
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/ina2xx_v2/ina2xx_v2.cpp:178-185
Timestamp: 2025-05-06T20:31:05.208Z
Learning: KrX3D plans to add INA226 support to the INA2xx usermod in a future update. Currently, the code only instantiates INA219_WE objects despite the more generic naming.
🪛 LanguageTool
usermods/ina2xx_v2/Readme.md
[uncategorized] ~3-~3: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...od integrates the INA219 and INA226 sensor with WLED to monitor energy consumption...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
🪛 markdownlint-cli2 (0.17.2)
usermods/ina2xx_v2/Readme.md
20-20: Hard tabs
Column: 152
(MD010, no-hard-tabs)
Hi,
i created a usermod for the ina219 Current, Voltage & Power Sensor:
I took a liking to the INA226 Usermod here:
https://github.com/Aircoookie/WLED/tree/0_15/usermods/INA226_v2
and took some parts and added my 3 cents to it.
I hope the readme states everything needed to get what can be done.
the only think i couldnt solved is to add the sensors data to the wled home assistant integration page (if thats even possible) so instead it is added just to mqtt. if there is a way to have the autodiscover to add it directly to the wled integrations page, i would be more than happy to change the usermod.
also i hope i added everything correctly in git, since its my first time working like this in git
Summary by CodeRabbit
New Features
Documentation