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: 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)
- Fixed energy counter resets not working properly due to missing timestamp tracking - Added validation to prevent energy calculation during invalid time periods (before NTP sync) - Fixed missing `dailyResetTimestamp` and `monthlyResetTimestamp` initialization on first run - Improved daily and monthly reset logic to use day/month comparison instead of hour/minute checks ### Added - Added `dailyResetTimestamp` and `monthlyResetTimestamp` fields to track exact reset times - Added timestamp fields to MQTT payload for Home Assistant `last_reset_value_template` support - Added `state_class` configuration for Home Assistant sensors: - `total_increasing` for total energy (never resets) - `total` with `last_reset_value_template` for daily/monthly energy - `measurement` for instantaneous values (current, voltage, power) - Added timestamp restoration from MQTT retained messages
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/ina2xx_v2/Readme.md (1)
181-181: Align the callout with the unified INA2XX usermodThis still tells readers to enable the “INA219” or “INA226” usermod even though we now expose a single INA2XX module with compile-time sensor selection. Please reword it to “enable the INA2XX usermod” (and mention the build flag if needed).
🧹 Nitpick comments (1)
usermods/ina2xx_v2/Readme.md (1)
20-22: Replace hard tabs in the screenshots tablemarkdownlint (MD010) is flagging the hard tab characters in this row; swap them for spaces so the linter (and rendered layout) stay clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
usermods/ina2xx_v2/Readme.md(1 hunks)usermods/ina2xx_v2/ina2xx_v2.cpp(1 hunks)usermods/ina2xx_v2/ina2xx_v2.h(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-05-06T20:31:05.208Z
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.
Applied to files:
usermods/ina2xx_v2/Readme.mdusermods/ina2xx_v2/ina2xx_v2.cppusermods/ina2xx_v2/ina2xx_v2.h
🧬 Code graph analysis (1)
usermods/ina2xx_v2/ina2xx_v2.h (1)
usermods/ina2xx_v2/ina2xx_v2.cpp (43)
UsermodINA2xx(539-545)roundDecimals(9-20)roundDecimals(9-9)hasSignificantChange(22-28)hasSignificantChange(22-22)hasValueChanged(30-44)hasValueChanged(30-30)checkForI2cErrors(559-588)checkForI2cErrors(559-559)updateINA2xxSettings(47-119)updateINA2xxSettings(47-47)sanitizeMqttClientID(122-162)sanitizeMqttClientID(122-122)updateEnergy(167-261)updateEnergy(167-167)publishMqtt(267-329)publishMqtt(267-269)mqttCreateHassSensor(334-422)mqttCreateHassSensor(334-336)mqttRemoveHassSensor(424-441)mqttRemoveHassSensor(424-424)setup(548-556)setup(548-548)loop(592-713)loop(592-592)onMqttMessage(446-519)onMqttMessage(446-446)onMqttConnect(524-535)onMqttConnect(524-524)addToJsonInfo(718-753)addToJsonInfo(718-718)addToJsonState(758-788)addToJsonState(758-758)readFromJsonState(793-867)readFromJsonState(793-793)addToConfig(872-896)addToConfig(872-872)appendConfigData(901-983)appendConfigData(901-901)readFromConfig(988-1042)readFromConfig(988-988)getId(1047-1049)getId(1047-1047)
🪛 markdownlint-cli2 (0.18.1)
usermods/ina2xx_v2/Readme.md
20-20: Hard tabs
Column: 152
(MD010, no-hard-tabs)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
usermods/ina2xx_v2/ina2xx_v2.cpp (2)
167-261: Consider adding overflow protection for energy accumulation.The energy calculation accumulates floating-point values over potentially long periods (months/years). While the current bounds checking (line 181) catches unrealistic single readings, consider adding a sanity check on the accumulated totals to prevent corruption from sensor glitches or prolonged operation.
For example, add a check in the updateEnergy function:
totalEnergy_kWh += energy_kWh; // Update total energy consumed _logUsermodInaSensor("Total energy updated to: %.6f kWh", totalEnergy_kWh); + + // Sanity check on accumulated total (e.g., 100,000 kWh = ~11 years at 1kW continuous) + if (totalEnergy_kWh > 100000.0) { + _logUsermodInaSensor("WARNING: Total energy suspiciously high (%.6f kWh), possible corruption", totalEnergy_kWh); + }
121-162: Consider documenting the UTF-8 handling for special characters.The sanitizeMqttClientID function includes specialized handling for German umlauts (ü, ä, ö, ß). While the implementation looks correct, consider adding a comment explaining this UTF-8 byte sequence handling for future maintainers who might not be familiar with multi-byte character encoding.
+// Handle UTF-8 encoded German umlauts (ü/Ü, ä/Ä, ö/Ö, ß) +// UTF-8 sequences: 0xC3 0xBC = ü, 0xC3 0x9C = Ü, etc. if (c == '\xC3' && i + 1 < clientID.length()) { char next = clientID[i + 1];usermods/ina2xx_v2/Readme.md (1)
20-20: Fix hard tab in table.Static analysis detected a hard tab character in the table at line 20. Consider replacing with spaces for consistency.
-| Info screen | Home Assistant | Usermod Settings page - Pin Selection | +| Info screen | Home Assistant | Usermod Settings page - Pin Selection |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/ina2xx_v2/Readme.md(1 hunks)usermods/ina2xx_v2/ina2xx_v2.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-05-06T20:31:05.208Z
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.
Applied to files:
usermods/ina2xx_v2/ina2xx_v2.cppusermods/ina2xx_v2/Readme.md
🪛 markdownlint-cli2 (0.18.1)
usermods/ina2xx_v2/Readme.md
20-20: Hard tabs
Column: 152
(MD010, no-hard-tabs)
🔇 Additional comments (5)
usermods/ina2xx_v2/ina2xx_v2.cpp (3)
558-591: LGTM - I2C error handler properly fixed.The I2C error handler now correctly uses
%ufor numeric error codes and disables the usermod gracefully instead of spinning forever in a while loop. This properly addresses the critical issue raised in previous reviews.
317-319: LGTM - Buffer overflow checks added.Good safety checks for JSON serialization buffer overflow. This addresses the previous review comment about adding size checks.
Also applies to: 405-407
69-69: INA_SENSOR_CLASS alias verified. Header defines the alias for both INA219_WE and INA226_WE; no further changes needed.usermods/ina2xx_v2/Readme.md (2)
26-28: Screenshots reflect compile-time sensor selection.The separate INA219 and INA226 screenshots are actually appropriate given the current implementation uses compile-time sensor selection via the
INA_SENSOR_TYPEdefine (line 36), rather than a runtime UI dropdown. Each sensor type produces a different settings UI, so showing both is correct.
1-3: LGTM - Documentation properly reflects INA2XX family support.The README correctly documents support for both INA219 and INA226 sensors, explains the compile-time sensor selection via
INA_SENSOR_TYPE, and uses the unified "INA2XX" naming throughout. This addresses previous review comments about updating documentation for the combined usermod.Also applies to: 34-37, 181-181
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
usermods/ina2xx_v2/Readme.md (1)
26-28: Update screenshots to reflect combined usermod UI.The screenshots still display separate INA219 and INA226 settings pages. Since this is a unified INA2XX usermod, update these images to show the combined interface including the sensor type selector dropdown.
🧹 Nitpick comments (1)
usermods/ina2xx_v2/Readme.md (1)
20-20: Replace hard tabs with spaces in table formatting.The table contains hard tabs for alignment (column 152), which can cause inconsistent rendering across different markdown viewers. Replace tabs with spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/ina2xx_v2/Readme.md(1 hunks)usermods/ina2xx_v2/ina2xx_v2.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-05-06T20:31:05.208Z
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.
Applied to files:
usermods/ina2xx_v2/ina2xx_v2.cppusermods/ina2xx_v2/Readme.md
🪛 markdownlint-cli2 (0.18.1)
usermods/ina2xx_v2/Readme.md
20-20: Hard tabs
Column: 152
(MD010, no-hard-tabs)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/ina2xx_v2/Readme.md (1)
26-28: Update screenshots to reflect compile-time sensor selection.The screenshots show separate INA219 and INA226 settings pages, but the usermod uses compile-time selection via the
INA_SENSOR_TYPEmacro. The UI will only show settings for the compiled sensor type. Please update the screenshots to show the actual UI for each compiled configuration.Based on past review comments.
usermods/ina2xx_v2/ina2xx_v2.cpp (1)
1015-1015: Correct format specifier for float currentRange.The
currentRangevariable is a float (see README line 83: default1.3, range0.1to10.0), butUPDATE_CONFIGuses%dwhich expects an integer. This will cause incorrect logging.Apply this diff:
- UPDATE_CONFIG(top, "currentRange", currentRange, "%d"); + UPDATE_CONFIG(top, "currentRange", currentRange, "%.1f");Based on past review comments.
🧹 Nitpick comments (1)
usermods/ina2xx_v2/Readme.md (1)
20-20: Replace hard tab with spaces.Line 20 contains a hard tab character. Please replace it with spaces for consistent formatting.
As per static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/ina2xx_v2/Readme.md(1 hunks)usermods/ina2xx_v2/ina2xx_v2.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-05-06T20:31:05.208Z
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.
Applied to files:
usermods/ina2xx_v2/ina2xx_v2.cppusermods/ina2xx_v2/Readme.md
🪛 markdownlint-cli2 (0.18.1)
usermods/ina2xx_v2/Readme.md
20-20: Hard tabs
Column: 152
(MD010, no-hard-tabs)
🔇 Additional comments (13)
usermods/ina2xx_v2/Readme.md (1)
1-14: LGTM!The title and feature list accurately reflect the unified INA2XX usermod supporting both INA219 and INA226 sensors.
usermods/ina2xx_v2/ina2xx_v2.cpp (12)
1-7: LGTM!Standard WLED usermod initialization pattern is correctly implemented.
8-44: LGTM!The utility functions are well-implemented with appropriate optimizations (LUT for common decimal factors) and clear change detection logic.
46-119: INA226 support properly implemented!The sensor instantiation now uses the
INA_SENSOR_CLASSmacro (line 69), which resolves to eitherINA219_WEorINA226_WEbased on theINA_SENSOR_TYPEcompile-time flag. The sensor-specific configuration is properly handled via conditional compilation (lines 89-115). This addresses the previous concern about being locked to INA219 only.Based on learnings.
121-162: LGTM!The MQTT client ID sanitization handles UTF-8 encoded special characters appropriately and ensures valid MQTT client IDs.
164-266: LGTM!The energy calculation logic is robust with comprehensive validation:
- Negligible power filtering to avoid rounding errors
- Energy range validation
- Time validity checks before resets
- Proper initialization and timestamp handling
- Calendar-based daily/monthly resets
The implementation correctly handles edge cases and maintains data integrity.
268-544: LGTM!The MQTT implementation is comprehensive and addresses previous review concerns:
- JSON serialization includes buffer size checks (lines 322-324)
- State restoration uses proper integer validation instead of incorrect
isnan()calls (lines 493-520)- Home Assistant discovery properly configures state classes for energy sensors
- Idempotent state restoration ensures values are merged only once
The implementation is solid and handles edge cases appropriately.
547-565: LGTM!The destructor properly cleans up resources, and the setup function correctly initializes the sensor.
567-600: I2C error handling fixed!The I2C error handler now correctly uses numeric format specifier
%u(line 573) and gracefully disables the usermod on error (lines 593-597) instead of freezing the system with a blocking loop. This addresses the critical issue from previous reviews.Based on past review comments.
602-878: LGTM!The main loop and JSON state management functions are well-implemented:
- Proper sensor reading with conditional handling for INA219 vs INA226 overflow detection
- Energy calculation with overflow protection
- MQTT publishing with change detection
- Home Assistant discovery management
- Comprehensive state persistence
The code correctly handles both sensor types via conditional compilation.
912-992: LGTM!The UI dropdown generation is properly conditionally compiled:
- INA219-specific dropdowns (lines 926-949) are wrapped in
#if INA_SENSOR_TYPE == 219- INA226-specific dropdowns (lines 950-991) are wrapped in
#elif INA_SENSOR_TYPE == 226This ensures only the relevant sensor's configuration options appear in the UI.
997-1051: LGTM!The configuration reading function is comprehensive with proper validation and sensor reinitialization after config changes (lines 1043-1048).
1053-1058: LGTM!The
getId()function correctly returns the unique usermod identifier.
2 participants
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
Integrations
Documentation
Chores