Conversation
Update INA226 default check interval to 1 second Initialize all INA226 bit fields in constructor _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. Add more serial debug output to INA226 usermod Log configuration parameters and operating mode on init, and print each measurement reading to serial when WLED_DEBUG is enabled. Show shunt voltage drop in mV instead of V 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 units to INA226 config UI and support decimal mΩ shunt values 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. USe uint32_t for checkIntervalMs to avoid overflow Add basic config validation for INA226 Shift conversion time into 12-bit field. This _actually_ doesn't matter as we always init the struct later. Make INA226 enabled by default be a build flag Revert to backwards compatible MQTT topic Increae check frequency to 1/s
📝 WalkthroughWalkthroughReworks INA226 usermod internals to use explicit units (ms, µΩ, mA), adds a runtime CurrentOffset (mA) applied to readings, switches shunt voltage reads to mV (publishes V for MQTT), moves timing to milliseconds, expands config serialization/deserialization, introduces compile-time defaults, and adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 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: 2
🧹 Nitpick comments (1)
usermods/INA226_v2/INA226_v2.cpp (1)
502-511: Use 2-space indentation in this new block.This override is indented with 4 spaces. Please reindent the touched usermod code to the repo’s 2-space style.
As per coding guidelines:
usermods/**/*.{cpp,h,hpp}: Use spaces (2 per level) for C++ indentation in usermod files.🤖 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 502 - 511, The appendConfigData() override is indented with 4 spaces; reformat this function in INA226_v2.cpp to use the repo's 2-space indentation style: change the indentation of the function signature, its body and each oappend(...) call (symbols: appendConfigData, oappend) to use 2 spaces per level so the entire block matches other usermods C++ files.
🤖 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 28-30: readFromConfig()/deserializeConfig() leaves constructor
defaults intact when cfg.json lacks an INA226 block, so
INA226_ENABLED_DEFAULT=false and DEFAULT_CHECKINTERVAL (60000) persist on fresh
installs; update deserializeConfig()/readFromConfig() to explicitly supply the
intended defaults when keys are missing (e.g., use obj.getBool("enabled", true)
and obj.getInt("checkIntervalMs", 1000) or equivalent) so _enabled and
_checkIntervalMs are set to the desired enabled-by-default/1s values on first
boot; adjust handling in the deserializeConfig() / readFromConfig()
implementation (and similarly update the logic referenced around lines 350-359)
rather than relying on the constructor constants INA226_ENABLED_DEFAULT and
DEFAULT_CHECKINTERVAL.
- Around line 16-22: The current defaults (INA226_SHUNT_MICRO_OHMS = 1000000 and
INA226_DEFAULT_CURRENT_RANGE = 1000) produce a 1V drop at full scale and break
calibration; update the defaults to a realistic shunt/current pair (e.g.,
INA226_SHUNT_MICRO_OHMS = 100000 for 100 mΩ and keep
INA226_DEFAULT_CURRENT_RANGE = 1000 mA) and/or add a safety clamp inside
setResistorRange() to ensure computed Vshunt = (shunt_micro_ohms *
current_range_mA)/1e9 stays within a sane max (e.g., <= 0.1 V) before applying
calibration, adjusting the effective range or emitting a warning if the pair is
out of bounds so fresh installs no longer start with invalid calibration.
---
Nitpick comments:
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Around line 502-511: The appendConfigData() override is indented with 4
spaces; reformat this function in INA226_v2.cpp to use the repo's 2-space
indentation style: change the indentation of the function signature, its body
and each oappend(...) call (symbols: appendConfigData, oappend) to use 2 spaces
per level so the entire block matches other usermods C++ files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: abb4077f-e18e-4845-98e6-b84912325d40
📒 Files selected for processing (2)
usermods/INA226_v2/INA226_v2.cppusermods/INA226_v2/README.md
| #ifndef INA226_SHUNT_MICRO_OHMS | ||
| #define INA226_SHUNT_MICRO_OHMS 1000000 // 1 Ohm = 1,000,000 μΩ | ||
| #endif | ||
|
|
||
| #ifndef INA226_DEFAULT_CURRENT_RANGE | ||
| #define INA226_DEFAULT_CURRENT_RANGE 1000 // 1000 mA = 1 A | ||
| #endif |
There was a problem hiding this comment.
The default shunt/range pair calibrates to a broken operating point.
INA226_SHUNT_MICRO_OHMS=1000000 and INA226_DEFAULT_CURRENT_RANGE=1000 mean a 1Ω shunt at 1A, and this pair is fed straight into setResistorRange(). That implies a 1V drop across the shunt at full-scale, so fresh installs will start with bogus current/power calibration unless every board profile overrides both flags.
Also applies to: 166-166
🤖 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 16 - 22, The current defaults
(INA226_SHUNT_MICRO_OHMS = 1000000 and INA226_DEFAULT_CURRENT_RANGE = 1000)
produce a 1V drop at full scale and break calibration; update the defaults to a
realistic shunt/current pair (e.g., INA226_SHUNT_MICRO_OHMS = 100000 for 100 mΩ
and keep INA226_DEFAULT_CURRENT_RANGE = 1000 mA) and/or add a safety clamp
inside setResistorRange() to ensure computed Vshunt = (shunt_micro_ohms *
current_range_mA)/1e9 stays within a sane max (e.g., <= 0.1 V) before applying
calibration, adjusting the effective range or emitting a warning if the pair is
out of bounds so fresh installs no longer start with invalid calibration.
| #ifndef INA226_ENABLED_DEFAULT | ||
| #define INA226_ENABLED_DEFAULT false | ||
| #endif |
There was a problem hiding this comment.
First boot still defaults to “disabled + 60s”.
If cfg.json does not contain an INA226 block yet, readFromConfig() leaves these constructor defaults untouched. With INA226_ENABLED_DEFAULT still false and _checkIntervalMs still seeded from DEFAULT_CHECKINTERVAL (60000 ms), a fresh install misses the intended enabled-by-default / 1-second behavior.
Patch sketch
-#define DEFAULT_CHECKINTERVAL 60000
+#define DEFAULT_CHECKINTERVAL 1000
-#define INA226_ENABLED_DEFAULT false
+#define INA226_ENABLED_DEFAULT trueAlso applies to: 350-359
🤖 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 28 - 30,
readFromConfig()/deserializeConfig() leaves constructor defaults intact when
cfg.json lacks an INA226 block, so INA226_ENABLED_DEFAULT=false and
DEFAULT_CHECKINTERVAL (60000) persist on fresh installs; update
deserializeConfig()/readFromConfig() to explicitly supply the intended defaults
when keys are missing (e.g., use obj.getBool("enabled", true) and
obj.getInt("checkIntervalMs", 1000) or equivalent) so _enabled and
_checkIntervalMs are set to the desired enabled-by-default/1s values on first
boot; adjust handling in the deserializeConfig() / readFromConfig()
implementation (and similarly update the logic referenced around lines 350-359)
rather than relying on the constructor constants INA226_ENABLED_DEFAULT and
DEFAULT_CHECKINTERVAL.
|
@coderabbitai please re-review this PR. summarize both good practices and recommended improvements (prioritized). |
🧩 Analysis chain🏁 Script executed: Length of output: 27738
✅ Good Practices
🔧 Recommended Improvements (Prioritized)P1 — Bug / CorrectnessInconsistent MQTT preprocessor guard on member variables // ❌ current code — uses the WRONG define name:
`#ifndef` WLED_MQTT_DISABLE // <-- legacy / non-standard spelling
float _lastCurrentSent = 0;
...
`#endif`
// ✅ fix — must match every other guard in this file:
`#ifndef` WLED_DISABLE_MQTT // <-- WLED standard
float _lastCurrentSent = 0;
...
`#endif`Every method definition ( P2 — Discrepancy Between PR Description and Code
`#define` DEFAULT_CHECKINTERVAL 60000 // still 60 sIf the revert to 60 s was intentional, the PR description and README should document the final decision explicitly. If the 1 s target is still desired: `#define` DEFAULT_CHECKINTERVAL 1000 // 1 s
P3 — Code QualityMagic number if (currentTime - _lastTriggerTime >= 400) // ← no constant, no commentThis 400 ms busy-poll interval should be a named constant with a brief comment explaining the rationale (e.g., shorter than the shortest possible conversion, long enough not to spin-loop): static constexpr uint16_t TRIGGERED_POLL_INTERVAL_MS = 400; // poll faster than shortest conversion
// ❌ fragile — correct only because topic happens to be 128 bytes:
char topic[128];
snprintf_P(topic, 127, "%s/current", mqttDeviceTopic);
// ✅ safer:
snprintf_P(topic, sizeof(topic), "%s/current", mqttDeviceTopic);Appears in five places in Double literal in float context float power = truncateDecimals(_ina226->getBusPower() / 1000.0); // implicit double promotion
// ✅
float power = truncateDecimals(_ina226->getBusPower() / 1000.0f);No upper-bound validation on if (_currentRangeMa == 0)
_currentRangeMa = INA226_DEFAULT_CURRENT_RANGE;
// No check for unreasonably large values (e.g., 65535 mA with a 1 mΩ shunt)A plausible cap (e.g., 100 000 mA = 100 A) would prevent mis-calibration from a typo. P4 — Documentation / Minor
README compile example includes redundant flag Summary Table
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/INA226_v2/INA226_v2.cpp (1)
117-123:⚠️ Potential issue | 🔴 CriticalPreprocessor guard mismatch:
WLED_MQTT_DISABLEvsWLED_DISABLE_MQTT.This block uses
WLED_MQTT_DISABLE, but the rest of the file (lines 186, 243, 391, 493, 588, 609) usesWLED_DISABLE_MQTT. The WLED standard macro isWLED_DISABLE_MQTT. When MQTT is disabled via the standard flag, these member variables are still compiled, wasting RAM.🐛 Proposed fix
-#ifndef WLED_MQTT_DISABLE +#ifndef WLED_DISABLE_MQTT float _lastCurrentSent = 0; float _lastVoltageSent = 0; float _lastPowerSent = 0;🤖 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 117 - 123, The preprocessor guard around the MQTT-related member variables uses the wrong macro (WLED_MQTT_DISABLE) so those fields (_lastCurrentSent, _lastVoltageSent, _lastPowerSent, _lastShuntVoltageSent, _lastOverflowSent) are still compiled when the project uses the standard WLED_DISABLE_MQTT flag; change the guard to `#ifndef` WLED_DISABLE_MQTT (matching other checks in this file) so these variables are excluded when MQTT is disabled, and verify any corresponding `#endif` remains paired with that macro.
🧹 Nitpick comments (4)
usermods/INA226_v2/INA226_v2.cpp (4)
180-183: Minor: Prefer float literals for consistency.Line 182 uses
1000.0(double) while line 180 uses1000.0f(float). Using double literals causes implicit promotion on ESP32. Consider1000.0ffor consistency and to avoid unnecessary double arithmetic.♻️ Suggested fix
- float power = truncateDecimals(_ina226->getBusPower() / 1000.0); + float power = truncateDecimals(_ina226->getBusPower() / 1000.0f);🤖 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 180 - 183, The power calculation uses a double literal instead of a float, causing implicit promotion on ESP32; update the expression in the power assignment (variable power computed from _ina226->getBusPower()) to use a float literal (1000.0f) so the division is done in float precision consistently with current and other values and then passed to truncateDecimals.
249-263: Considersizeof(topic)instead of hardcoded size.Multiple
snprintf_Pcalls use127while the buffer ischar topic[128]. Usingsizeof(topic)reduces fragility if the buffer size changes.♻️ Suggested improvement
char topic[128]; - snprintf_P(topic, 127, "%s/current", mqttDeviceTopic); + snprintf_P(topic, sizeof(topic), PSTR("%s/current"), mqttDeviceTopic); mqttCreateHassSensor(F("Current"), topic, F("current"), F("A"));Apply similarly to other
snprintf_Pcalls in this function.🤖 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 249 - 263, The snprintf_P calls use a hardcoded 127 while the buffer is declared as char topic[128]; update each snprintf_P(...) call that builds topics (the topic buffer and mqttDeviceTopic usage in the function) to use sizeof(topic) (or sizeof topic) instead of 127 to avoid off-by-one/fragility; ensure all occurrences before calls to mqttCreateHassSensor and mqttCreateHassBinarySensor are changed consistently (e.g., the snprintf_P lines that produce "%s/current", "%s/voltage", "%s/power", "%s/shunt_voltage", "%s/overflow").
578-584: Consider adding upper-bound validation forCurrentRange.The code validates
_currentRangeMa != 0but doesn't cap unreasonably large values. A typo like100000000mA would pass validation but produce invalid INA226 calibration. Consider adding an upper limit (e.g., 20000 mA for common INA226 setups).♻️ Suggested improvement
if (getJsonValue(top[F("CurrentRange")], _currentRangeMa)) { - if (_currentRangeMa == 0) + if (_currentRangeMa == 0 || _currentRangeMa > 20000) _currentRangeMa = INA226_DEFAULT_CURRENT_RANGE; }🤖 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 578 - 584, The CurrentRange validation only checks for zero; update the getJsonValue(top[F("CurrentRange")], _currentRangeMa) handling in INA226_v2.cpp to also validate an upper bound (e.g., if _currentRangeMa > 20000) and either clamp it to INA226_DEFAULT_CURRENT_RANGE or a sane max (or set configComplete = false) to avoid producing invalid INA226 calibration values; adjust the branch that currently sets _currentRangeMa = INA226_DEFAULT_CURRENT_RANGE when zero to also handle out-of-range values and ensure the calling code uses the corrected/clamped value.
210-211: Consider naming the 400ms polling constant.The magic number
400represents the polling interval while waiting for triggered measurements. A named constant would clarify intent and make tuning easier.♻️ Suggested improvement
+// Poll for measurement completion at this interval (ms) in triggered mode +static constexpr unsigned long TRIGGERED_POLL_INTERVAL_MS = 400; + void handleTriggeredMode(unsigned long currentTime) { if (_measurementTriggered) { - // Test if we have a measurement every 400ms - if (currentTime - _lastTriggerTime >= 400) + if (currentTime - _lastTriggerTime >= TRIGGERED_POLL_INTERVAL_MS)🤖 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 210 - 211, The comparison "if (currentTime - _lastTriggerTime >= 400)" uses a magic number; introduce a clearly named constant (e.g., TRIGGER_POLL_INTERVAL_MS or PENDING_POLL_INTERVAL_MS) with the value 400 and use it in that condition to clarify intent and allow easy tuning; add the constant as a constexpr/static const in the INA226_v2 class or at the top of INA226_v2.cpp and replace the literal in the check (referencing the condition that uses currentTime and _lastTriggerTime) to use the new constant.
🤖 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/README.md`:
- Around line 83-93: Remove the unused build flag `-D USERMOD_INA226` from the
example build_flags block in README.md; edit the `build_flags` section shown
(the block containing `-D USERMOD_INA226`) so it no longer contains that flag,
leaving the other defines like `INA226_ENABLED_DEFAULT`,
`INA226_SHUNT_MICRO_OHMS`, `INA226_DEFAULT_CURRENT_RANGE`, and
`INA226_CURRENT_OFFSET_MA` intact, since `custom_usermods = ... INA226` is
sufficient to include the usermod.
---
Outside diff comments:
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Around line 117-123: The preprocessor guard around the MQTT-related member
variables uses the wrong macro (WLED_MQTT_DISABLE) so those fields
(_lastCurrentSent, _lastVoltageSent, _lastPowerSent, _lastShuntVoltageSent,
_lastOverflowSent) are still compiled when the project uses the standard
WLED_DISABLE_MQTT flag; change the guard to `#ifndef` WLED_DISABLE_MQTT (matching
other checks in this file) so these variables are excluded when MQTT is
disabled, and verify any corresponding `#endif` remains paired with that macro.
---
Nitpick comments:
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Around line 180-183: The power calculation uses a double literal instead of a
float, causing implicit promotion on ESP32; update the expression in the power
assignment (variable power computed from _ina226->getBusPower()) to use a float
literal (1000.0f) so the division is done in float precision consistently with
current and other values and then passed to truncateDecimals.
- Around line 249-263: The snprintf_P calls use a hardcoded 127 while the buffer
is declared as char topic[128]; update each snprintf_P(...) call that builds
topics (the topic buffer and mqttDeviceTopic usage in the function) to use
sizeof(topic) (or sizeof topic) instead of 127 to avoid off-by-one/fragility;
ensure all occurrences before calls to mqttCreateHassSensor and
mqttCreateHassBinarySensor are changed consistently (e.g., the snprintf_P lines
that produce "%s/current", "%s/voltage", "%s/power", "%s/shunt_voltage",
"%s/overflow").
- Around line 578-584: The CurrentRange validation only checks for zero; update
the getJsonValue(top[F("CurrentRange")], _currentRangeMa) handling in
INA226_v2.cpp to also validate an upper bound (e.g., if _currentRangeMa > 20000)
and either clamp it to INA226_DEFAULT_CURRENT_RANGE or a sane max (or set
configComplete = false) to avoid producing invalid INA226 calibration values;
adjust the branch that currently sets _currentRangeMa =
INA226_DEFAULT_CURRENT_RANGE when zero to also handle out-of-range values and
ensure the calling code uses the corrected/clamped value.
- Around line 210-211: The comparison "if (currentTime - _lastTriggerTime >=
400)" uses a magic number; introduce a clearly named constant (e.g.,
TRIGGER_POLL_INTERVAL_MS or PENDING_POLL_INTERVAL_MS) with the value 400 and use
it in that condition to clarify intent and allow easy tuning; add the constant
as a constexpr/static const in the INA226_v2 class or at the top of
INA226_v2.cpp and replace the literal in the check (referencing the condition
that uses currentTime and _lastTriggerTime) to use the new constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a817f7c-7663-466e-af79-2515042121e6
📒 Files selected for processing (2)
usermods/INA226_v2/INA226_v2.cppusermods/INA226_v2/README.md
There was a problem hiding this comment.
🧹 Nitpick comments (2)
usermods/INA226_v2/INA226_v2.cpp (2)
214-216: Consider extracting magic number 400 to a named constant.The polling interval for triggered mode completion lacks documentation. A named constant with a comment explaining the rationale (e.g., INA226 datasheet timing, empirical testing) would improve maintainability.
Example
+// Polling interval for triggered measurement completion check. +// Must be long enough to avoid I2C bus thrashing but short enough +// for responsive readings after the conversion completes. +static constexpr unsigned long TRIGGERED_POLL_INTERVAL_MS = 400; + void handleTriggeredMode(unsigned long currentTime) { if (_measurementTriggered) { - // Test if we have a measurement every 400ms - if (currentTime - _lastTriggerTime >= 400) + if (currentTime - _lastTriggerTime >= TRIGGERED_POLL_INTERVAL_MS) {🤖 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 214 - 216, Extract the magic number 400 used in the triggered-mode poll condition into a named constant (e.g., static const uint32_t TRIGGER_POLL_INTERVAL_MS = 400) and replace the literal in the condition "if (currentTime - _lastTriggerTime >= 400)" with that constant; place the constant near the top of INA226_v2.cpp or with other timing constants and add a short comment referring to the INA226 datasheet or empirical testing that justifies the 400ms interval so maintainers understand the rationale for _lastTriggerTime / currentTime checks.
278-291: Usesizeof(subuf)instead of hardcoded 127.The lambda in
mqttInitializenow correctly usessizeof(topic), but these functions still use the hardcoded value. This is fragile if the buffer size changes.Proposed fix
void mqttPublishIfChanged(const __FlashStringHelper *topic, float &lastState, float state, float minChange) { if (WLED_MQTT_CONNECTED && _mqttPublish && (_mqttPublishAlways || fabsf(lastState - state) > minChange)) { char subuf[128]; - snprintf_P(subuf, 127, PSTR("%s/%s"), mqttDeviceTopic, (const char *)topic); + snprintf_P(subuf, sizeof(subuf), PSTR("%s/%s"), mqttDeviceTopic, (const char *)topic); mqtt->publish(subuf, 0, false, String(state).c_str()); lastState = state; @@ -288,7 +288,7 @@ class UsermodINA226 : public Usermod if (WLED_MQTT_CONNECTED && _mqttPublish && (_mqttPublishAlways || lastState != state)) { char subuf[128]; - snprintf_P(subuf, 127, PSTR("%s/%s"), mqttDeviceTopic, (const char *)topic); + snprintf_P(subuf, sizeof(subuf), PSTR("%s/%s"), mqttDeviceTopic, (const char *)topic); mqtt->publish(subuf, 0, false, state ? "true" : "false");🤖 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 278 - 291, Replace the hardcoded buffer length 127 in the snprintf_P calls with sizeof(subuf) to avoid fragility if subuf's size changes; update both occurrences shown (the snprintf_P in the function handling const char* topic and the one in mqttPublishIfChanged that takes const __FlashStringHelper*) so they call snprintf_P(subuf, sizeof(subuf), PSTR("%s/%s"), mqttDeviceTopic, (const char*)topic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@usermods/INA226_v2/INA226_v2.cpp`:
- Around line 214-216: Extract the magic number 400 used in the triggered-mode
poll condition into a named constant (e.g., static const uint32_t
TRIGGER_POLL_INTERVAL_MS = 400) and replace the literal in the condition "if
(currentTime - _lastTriggerTime >= 400)" with that constant; place the constant
near the top of INA226_v2.cpp or with other timing constants and add a short
comment referring to the INA226 datasheet or empirical testing that justifies
the 400ms interval so maintainers understand the rationale for _lastTriggerTime
/ currentTime checks.
- Around line 278-291: Replace the hardcoded buffer length 127 in the snprintf_P
calls with sizeof(subuf) to avoid fragility if subuf's size changes; update both
occurrences shown (the snprintf_P in the function handling const char* topic and
the one in mqttPublishIfChanged that takes const __FlashStringHelper*) so they
call snprintf_P(subuf, sizeof(subuf), PSTR("%s/%s"), mqttDeviceTopic, (const
char*)topic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 377776eb-cda9-421c-9a77-3e65cb60a9a1
📒 Files selected for processing (2)
usermods/INA226_v2/INA226_v2.cppusermods/INA226_v2/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/INA226_v2/README.md
|
@softhack007 I believe have addressed all the issues that were relevant to my updates. Anything else you would like me to look into? |
2 participants
Reopening #5411 after a rebase on the current
main. The other PRs (#5236 and #4237) related to current sensing are more significant rewrites and I think it's better to just undo the breaking MQTT protocol changes in this version update instead.This 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
Summary by CodeRabbit
New Features
Documentation