Conversation
_settingEnabled was uninitialized, defaulting to false for static objects, which silently disabled the usermod. Default it to true and explicitly initialize all other bit fields.
Log configuration parameters and operating mode on init, and print each measurement reading to serial when WLED_DEBUG is enabled.
With low-value shunt resistors the voltage drop rounds to 0.00V. Read in mV, rename label to "Shunt Voltage Drop", and update the MQTT topic and HA discovery accordingly.
Add appendConfigData() to show units (seconds, mΩ, mA, μs, etc.) next to each config field. Store ShuntResistor as float milliohms so values like 4.8 mΩ can be entered directly. Old integer configs remain backward compatible.
WalkthroughAdded compile-time defaults and unit changes for INA226 (μΩ, mA); replaced interval tracking with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Line 100: The _checkIntervalMs field must be widened to 32-bit to avoid
overflow when the user-configured seconds are multiplied by 1000; change the
declaration of _checkIntervalMs from uint16_t to uint32_t and update any
assignments/calculations that set it (the code that multiplies the seconds value
by 1000) to use the new type, ensuring any callers, setters, and comparisons
(including the logic that selects mode based on _checkIntervalMs) handle
uint32_t and that any format specifiers or explicit casts are adjusted
accordingly.
- Around line 561-569: The code must reject non-positive shunt and range values
before they get used by setResistorRange; after reading ShuntResistor with
getJsonValue into shuntMilliOhms, verify shuntMilliOhms > 0 before assigning
_shuntResistorUOhm and otherwise set configComplete = false; similarly, when
reading CurrentRange into _currentRangeMa treat a missing or non-positive value
as invalid (set configComplete = false) instead of accepting 0; keep the
existing fallback for CurrentOffset using INA226_CURRENT_OFFSET_MA. Reference
symbols: getJsonValue, shuntMilliOhms, _shuntResistorUOhm, _currentRangeMa,
setResistorRange, INA226_CURRENT_OFFSET_MA.
In `@usermods/INA226_v2/README.md`:
- Around line 69-93: Update the Compile-time Defaults (or a newly added
"MQTT/topic migration" subsection) to document that the MQTT field/topic
formerly named "shunt_voltage" has been renamed to "shunt_voltage_drop" and its
unit changed from V to mV; explicitly call out that existing dashboards,
automations, and integrations must be updated to use the new name and unit, show
the old → new mapping (shunt_voltage → shunt_voltage_drop) and unit conversion
(V → mV) so users can adjust thresholds/graphs, and mention any retained
backward-compatibility behavior (if none, state that topics/payloads are
breaking changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4297ef6-36c3-4aa6-8917-cd201bd3f615
📒 Files selected for processing (2)
usermods/INA226_v2/INA226_v2.cppusermods/INA226_v2/README.md
| ### Compile-time Defaults | ||
|
|
||
| Several parameters can be overridden at compile time via `-D` build flags. This is useful for setting board-specific defaults so the device works correctly on first boot without manual configuration. | ||
|
|
||
| | Build Flag | Default | Unit | Description | | ||
| |---|---|---|---| | ||
| | `INA226_ADDRESS` | `0x40` | — | I2C address of the INA226 | | ||
| | `INA226_SHUNT_MICRO_OHMS` | `1000000` | μΩ | Shunt resistor value (1 000 000 μΩ = 1 Ω) | | ||
| | `INA226_DEFAULT_CURRENT_RANGE` | `1000` | mA | Expected maximum current (1000 mA = 1 A) | | ||
| | `INA226_CURRENT_OFFSET_MA` | `0` | mA | Current offset subtracted from readings | | ||
|
|
||
| Example for a board with a 4.8 mΩ shunt, 10 A range, and 53 mA offset: | ||
|
|
||
| ```ini | ||
| [env:my_board] | ||
| extends = env:esp32dev | ||
| custom_usermods = ${env:esp32dev.custom_usermods} INA226 | ||
| build_flags = ${env:esp32dev.build_flags} | ||
| -D USERMOD_INA226 | ||
| -D INA226_SHUNT_MICRO_OHMS=4800 | ||
| -D INA226_DEFAULT_CURRENT_RANGE=10000 | ||
| -D INA226_CURRENT_OFFSET_MA=53 | ||
| ``` | ||
|
|
||
| All compile-time defaults can still be changed at runtime through the Usermod settings page. No newline at end of file |
There was a problem hiding this comment.
Document the MQTT/topic migration in this section.
This PR intentionally renames shunt_voltage to shunt_voltage_drop and changes the unit from V to mV, but the README still does not call out that existing dashboards and automations need to be updated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/INA226_v2/README.md` around lines 69 - 93, Update the Compile-time
Defaults (or a newly added "MQTT/topic migration" subsection) to document that
the MQTT field/topic formerly named "shunt_voltage" has been renamed to
"shunt_voltage_drop" and its unit changed from V to mV; explicitly call out that
existing dashboards, automations, and integrations must be updated to use the
new name and unit, show the old → new mapping (shunt_voltage →
shunt_voltage_drop) and unit conversion (V → mV) so users can adjust
thresholds/graphs, and mention any retained backward-compatibility behavior (if
none, state that topics/payloads are breaking changes).
There was a problem hiding this comment.
I don't think this belongs in the README.md, but rather in the release notes. Anyone have an opinion?
There was a problem hiding this comment.
@daggerrz, that's a fair point — release notes are the natural place to call out breaking changes for a specific version. However, there's a case for a small note in the README as well: release notes tend to get buried over time, while the README is what a user sees when they first look at the module or when they're troubleshooting a broken dashboard months after upgrading. Many projects (e.g., Home Assistant integrations, Mosquitto-based tools) keep a short "Upgrading / Breaking Changes" section in the README alongside the main changelog entry, so the information stays discoverable long-term.
A lightweight middle ground could be:
- Release notes – the primary call-out, with the full old → new mapping.
- README – a brief, permanent note (one or two lines) in the configuration section under
ShuntResistor/ MQTT settings, e.g.:
⚠️ Breaking change (v<X>): The MQTT topicshunt_voltage(V) has been renamed toshunt_voltage_drop(mV). Existing dashboards and automations must be updated.
That way users who land on the README without reading the release notes are still warned. But if the project's convention is to keep only release notes for such notices, that's perfectly reasonable too — ultimately it's your call as the maintainer.
🧠 Learnings used
Learnt from: netmindz
Repo: wled/WLED PR: 5311
File: platformio.ini:395-395
Timestamp: 2026-01-31T18:19:33.055Z
Learning: In WLED, usermods can be referenced as out-of-tree library dependencies (e.g., via lib_deps pointing to external repositories) while the `custom_usermods` setting in platformio.ini controls which in-tree usermods are enabled. Setting `custom_usermods = ` (empty) only disables in-tree usermods; out-of-tree usermods pulled via library dependencies remain active.
Learnt from: KrX3D
Repo: wled/WLED PR: 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.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:612-617
Timestamp: 2026-02-18T00:41:15.185Z
Learning: In the seven_segment_display_reloaded_v2 usermod (PR `#4585`), the `addToConfig()` method intentionally calls `_updateMQTT()` to publish MQTT state on every config save. This behavior is by design and should not be flagged as an issue.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/INA226_v2/INA226_v2.cpp (1)
561-568:⚠️ Potential issue | 🟠 MajorReject non-positive calibration inputs here.
ShuntResistor <= 0andCurrentRange == 0are still accepted, so one bad cfg entry can leave the module calibrated with nonsensical values until the setting is fixed. Please fall back to the compile-time defaults and mark the config incomplete when either field is missing or non-positive.Proposed fix
float shuntMilliOhms; - if (getJsonValue(top[F("ShuntResistor")], shuntMilliOhms)) + if (getJsonValue(top[F("ShuntResistor")], shuntMilliOhms) && shuntMilliOhms > 0.0f) _shuntResistorUOhm = static_cast<uint32_t>(shuntMilliOhms * 1000.0f + 0.5f); - else + else { + _shuntResistorUOhm = INA226_SHUNT_MICRO_OHMS; configComplete = false; + } - configComplete &= getJsonValue(top[F("CurrentRange")], _currentRangeMa); + if (!getJsonValue(top[F("CurrentRange")], _currentRangeMa) || _currentRangeMa == 0) { + _currentRangeMa = INA226_DEFAULT_CURRENT_RANGE; + configComplete = false; + } if (!getJsonValue(top[F("CurrentOffset")], _currentOffsetMa)) _currentOffsetMa = INA226_CURRENT_OFFSET_MA; // Use compile-time default if missing from config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/INA226_v2/INA226_v2.cpp` around lines 561 - 568, The code currently accepts non-positive ShuntResistor and CurrentRange values; modify the logic around getJsonValue for top[F("ShuntResistor")] and top[F("CurrentRange")] so that if the parsed shuntMilliOhms <= 0 or _currentRangeMa == 0 you restore the compile-time defaults (reset _shuntResistorUOhm to the constant default and _currentRangeMa to its default), set configComplete = false, and do not apply the invalid value; keep using getJsonValue and update the configComplete boolean accordingly (also ensure the branch that sets _currentOffsetMa remains unchanged except for relying on the validated range).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Around line 344-350: The default conversion time is being stored in raw
microseconds but the code expects the shifted representation (µs >> 2); update
the initialization so _settingInaConversionTimeUs is assigned the shifted value
(use DEFAULT_INACONVERSIONTIME >> 2 or equivalent) to match the rest of the code
and avoid initializeINA226() clamping from altering the intended 1100µs default.
---
Duplicate comments:
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Around line 561-568: The code currently accepts non-positive ShuntResistor and
CurrentRange values; modify the logic around getJsonValue for
top[F("ShuntResistor")] and top[F("CurrentRange")] so that if the parsed
shuntMilliOhms <= 0 or _currentRangeMa == 0 you restore the compile-time
defaults (reset _shuntResistorUOhm to the constant default and _currentRangeMa
to its default), set configComplete = false, and do not apply the invalid value;
keep using getJsonValue and update the configComplete boolean accordingly (also
ensure the branch that sets _currentOffsetMa remains unchanged except for
relying on the validated range).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec90716c-4d12-40ec-b2f7-730c57dccb56
📒 Files selected for processing (1)
usermods/INA226_v2/INA226_v2.cpp
…er as we always init the struct later.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Around line 164-166: The debug printf uses the wrong format specifier for a
uint32_t: in the DEBUG_PRINTF_P call that prints _checkIntervalMs (and related
fields) replace the `%u` used for _checkIntervalMs with `%lu` (or cast
_checkIntervalMs to unsigned long and use `%lu`) so the format matches the
uint32_t type; update the format string in the DEBUG_PRINTF_P invocation that
references _isTriggeredOperationMode, _checkIntervalMs, _settingInaSamples, and
_settingInaConversionTimeUs accordingly to avoid undefined behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9eee7385-d83a-4cb6-9106-0b67b35fac23
📒 Files selected for processing (1)
usermods/INA226_v2/INA226_v2.cpp
| DEBUG_PRINTF_P(PSTR("INA226: mode=%s interval=%ums samples=%u convTime=%uμs\n"), | ||
| _isTriggeredOperationMode ? "triggered" : "continuous", | ||
| _checkIntervalMs, _settingInaSamples, _settingInaConversionTimeUs << 2); |
There was a problem hiding this comment.
Use %lu format specifier for uint32_t.
_checkIntervalMs is uint32_t, but %u is for unsigned int. While this works on ESP32/ESP8266 where both are 32-bit, %lu is the correct portable specifier for uint32_t.
Proposed fix
- DEBUG_PRINTF_P(PSTR("INA226: mode=%s interval=%ums samples=%u convTime=%uμs\n"),
+ DEBUG_PRINTF_P(PSTR("INA226: mode=%s interval=%lums samples=%u convTime=%uμs\n"),
_isTriggeredOperationMode ? "triggered" : "continuous",
_checkIntervalMs, _settingInaSamples, _settingInaConversionTimeUs << 2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/INA226_v2/INA226_v2.cpp` around lines 164 - 166, The debug printf
uses the wrong format specifier for a uint32_t: in the DEBUG_PRINTF_P call that
prints _checkIntervalMs (and related fields) replace the `%u` used for
_checkIntervalMs with `%lu` (or cast _checkIntervalMs to unsigned long and use
`%lu`) so the format matches the uint32_t type; update the format string in the
DEBUG_PRINTF_P invocation that references _isTriggeredOperationMode,
_checkIntervalMs, _settingInaSamples, and _settingInaConversionTimeUs
accordingly to avoid undefined behavior.
I have not reviewed any code - is there a specific area you want feedback on? Contributions to the INA226 mod for WLED to make it better, easier to approach, all around improvements are a win in my book any day. :) I'm not a C/C++/MCU expert by any measure, so what I've made might also reflect that. |
|
@LordMike no worries, I can take care of the review but since this is your usermod, you get a veto in any changes. Usually we try to keep things backwards compatible as much as possible so existing setups do still work even when using latest code (please take that into accountt @daggerrz). I do not use this sensor nor any other sensors or MQTT - changing the MQTT topic seems like a bad idea, also units of measurements etc. |
|
I would definitely not enable it by default. It needs I2C set up, it acts on a specific address (which could be used elsewhere), it takes up space (both ROM, and compute time), .. don't do that. But that's more of a general thing imo. Dunno about changing mqtt topics.. that seems risky. Although I can understand wanting to move from a decimal to an integer (V v. mV). To be honest though, I don't recall adding MQTT output - I only recall adding to the info output (shown in wled UI under info for the server). :O EDIT: Just checked, I did add MQTT stuff. Hot damn. I forgot all about it :D |
Of course, I don't mind at all to revert the renaming of |
|
I'm fine with either name. Don't worry about me there. |
|
@DedeHai, anything else your want me to take care of here? |
|
@coderabbitai please check the changes for backwards compatibility. list potential issues. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me examine the code changes in detail. 🏁 Script executed: Length of output: 21503 Let me also look at the git history to compare with the original version. 🏁 Script executed: Length of output: 22450 Let me get the original version from the upstream main branch for a proper comparison. 🏁 Script executed: Length of output: 18669
Backwards Compatibility Analysis — INA226 v2 (PR
|
| Item | Details |
|---|---|
ShuntResistor config key |
Still named "ShuntResistor". Old saved integer mΩ values (e.g. 100) parse correctly as float 100.0 mΩ → 100 000 μΩ. ✅ |
CurrentRange config key |
Still named "CurrentRange", still in mA. ✅ |
CheckInterval config key |
Still named "CheckInterval", still in seconds, same valid range (1–600). ✅ |
New CurrentOffset field |
Gracefully defaults to 0 when absent from old config. ✅ |
INA226_ADDRESS guard |
Wrapped in #ifndef — boards already defining this won't conflict. ✅ |
_checkInterval → _checkIntervalMs |
Internal rename only; config key and unit are unchanged. ✅ |
| Triggered mode threshold | Logic >= 20000 ms is unchanged; purely internal variable rename. ✅ |
Summary
| # | Severity | Issue |
|---|---|---|
| 1 | 🔴 Breaking | MQTT topic shunt_voltage → shunt_voltage_drop breaks all subscribers |
| 2 | 🔴 Breaking | Shunt value unit V → mV (1000× scale change) in MQTT and JSON info |
| 3 | 🔴 Breaking | HA discovery: stale retained sensor not cleaned up automatically |
| 4 | 🟡 Behavioral | Default poll interval 60 s → 1 s; changes operating mode default (triggered → continuous) |
| 5 | 🟡 Behavioral | Usermod now enabled by default (_settingEnabled = true) — should be false |
🧠 Learnings used
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: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:612-617
Timestamp: 2026-02-18T00:41:15.185Z
Learning: In the seven_segment_display_reloaded_v2 usermod (PR `#4585`), the `addToConfig()` method intentionally calls `_updateMQTT()` to publish MQTT state on every config save. This behavior is by design and should not be flagged as an issue.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: KrX3D
Repo: wled/WLED PR: 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.
|
there are still breaking changes, this may annoy a lot of people using this usermod (it is unknonw to us how many that are, especially if this UM is used in products, this is a problem) |
|
If reverting breaking changes is not an option for you, try to get your changes into the other usermod I linked which is still unreleased and more flexible. |
softhack007
added
discussion
waiting for feedback
Sorry about the delay. The other PR looks interesting. I'll close this PR for now, check out the other one and add my changes to the other one if applicable. |
4 participants
Improves the INA226 usermod to work correctly out of the box and be easier to configure. Previously, the usermod was silently disabled by default, had no serial debug output, couldn't represent common low-value shunt resistors, and showed no units in the config UI.
so board profiles can ship with correct values without requiring runtime configuration
Breaking changes
ShuntResistor config value: Previously stored as integer milliohms, now stored as float milliohms. Old integer values (e.g. 1000 for 1Ω) will still parse
correctly. However, sub-milliohm shunt values that were previously truncated to 0 will need to be re-entered. This is an acceptable tradeoff because:
Shunt voltage MQTT topic: Changed from shunt_voltage to shunt_voltage_drop, unit changed from V to mV. This breaks existing HA dashboards or automations referencing the old topic. I think it's worth it because:
Summary by CodeRabbit
New Features
Improvements
Documentation