Add INA219 usermod support and update platformio.ini#361
Open
Topchris18 wants to merge 2 commits intoMoonModules:mdevfrom
Open
Add INA219 usermod support and update platformio.ini#361Topchris18 wants to merge 2 commits intoMoonModules:mdevfrom
Topchris18 wants to merge 2 commits intoMoonModules:mdevfrom
Conversation
WalkthroughThis PR adds INA219 I2C current/power sensor support to WLED via a new usermod, updates build registration and usermod ID, and adds an optional Adafruit INA219 library entry in platformio.ini. Changes
Sequence Diagram(s)sequenceDiagram
actor System as WLED
participant Usermod as INA219 Usermod
participant I2C as I2C Bus
participant Sensor as INA219 Sensor
participant Config as Config/JSON
System->>Usermod: setup()
Usermod->>I2C: joinWire()
Usermod->>Sensor: instantiate & begin(address)
Usermod->>Sensor: applyCalibration(busVoltage,maxCurrent)
Sensor-->>Usermod: ready / not found
loop every readInterval
System->>Usermod: loop()
Usermod->>Sensor: read shunt voltage
Sensor-->>Usermod: V_shunt
Usermod->>Sensor: read bus voltage
Sensor-->>Usermod: V_bus
Usermod->>Usermod: calculate current & power
Usermod-->>Config: addToJsonInfo(readings)
end
System->>Usermod: readFromConfig()
Config-->>Usermod: settings
Usermod->>Usermod: reinit or reapply calibration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 `@platformio.ini`:
- Around line 297-298: The new global dependency line "adafruit/Adafruit INA219
@ 1.2.1" was added under the global [env] and therefore affects all build
environments; revert this change from the global section and either move the
dependency into the specific environment(s) that need the INA219 or leave it
commented with a clear TODO requiring explicit maintainer/WLED org approval
before enabling. Ensure you reference the dependency string "adafruit/Adafruit
INA219 @ 1.2.1" when making the change and add a brief sign-off note (e.g.,
"Requires maintainer approval") so reviewers can approve before it becomes
active.
In `@usermods/INA219_v2/usermod_ina219.h`:
- Around line 119-120: The computation of current (current_mA = shuntVoltage_mV
/ (shuntResistor_mOhm / 1000.0f)) can divide by zero or a tiny value if
shuntResistor_mOhm is misconfigured; validate and clamp shuntResistor_mOhm when
loading config (where it’s set) to a safe minimum (e.g. >0 and not near-zero)
and add a runtime guard before computing current in the measurement path (check
shuntResistor_mOhm against the minimum, skip or set current_mA/power_mW to 0 and
log/warn if invalid). Ensure you update symbols: shuntResistor_mOhm validation
at config load and the check around current_mA/power_mW calculation that uses
shuntVoltage_mV and loadVoltage_V.
- Around line 62-69: applyCalibration currently favors 32V calibrations whenever
maxCurrentRange_A > 0.4, ignoring a configured busVoltageRange_V of 16V; update
applyCalibration to first branch on busVoltageRange_V (e.g., if
busVoltageRange_V <= 16) and then choose the correct current-based calibration
(call ina219->setCalibration_16V_400mA for <=0.4A else a 16V+higher-current
routine if available), otherwise for busVoltageRange_V > 16 choose the 32V
calibrations (use ina219->setCalibration_32V_1A or ina219->setCalibration_32V_2A
based on maxCurrentRange_A); make the same voltage-first decision wherever the
settings from busVoltageRange_V/maxCurrentRange_A are applied (references:
function applyCalibration, variables busVoltageRange_V and maxCurrentRange_A,
and methods ina219->setCalibration_16V_400mA, ina219->setCalibration_32V_1A,
ina219->setCalibration_32V_2A).
🪄 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: 9ca8f889-7b42-4cb8-858e-7c6fa6fed515
📒 Files selected for processing (4)
platformio.iniusermods/INA219_v2/usermod_ina219.hwled00/const.hwled00/usermods_list.cpp
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usermods/INA219_v2/usermod_ina219.h (2)
43-43:lastMeasureduplicates base-classlastTime.
Usermodalready providesunsigned long lastTime. Reusing it would slightly shrink the footprint and match convention across usermods. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/INA219_v2/usermod_ina219.h` at line 43, The field lastMeasure duplicates the base-class Usermod's lastTime; remove the unsigned long lastMeasure declaration from usermod_ina219.h and update all usages in the INA219 usermod (references to lastMeasure in methods like setup(), loop(), read/measure routines) to use lastTime instead so the mod reuses the inherited timer field and reduces footprint while preserving behavior.
217-222: PROGMEM static member definitions in header — potential ODR risk if header is included elsewhere.Defining
UsermodINA219::_readInterval[]et al. in the header is currently safe because this header is included only once (viausermods_list.cppwith#ifdef USERMOD_INA219guard). However, if the header is ever included from a second translation unit (e.g., tests, a future companion.cpp, or another usermod), you'll get multiple-definition linker errors.Consider marking them
inline(C++17) to allow safe multi-inclusion, or move them to a companion.cpp. Other usermods in the repo use this pattern, so this is optional.♻️ Proposed change (C++17)
-const char UsermodINA219::_readInterval[] PROGMEM = "readInterval-ms"; -const char UsermodINA219::_i2cAddress[] PROGMEM = "i2cAddress"; -const char UsermodINA219::_shuntResistor[] PROGMEM = "shuntResistor-mOhm"; -const char UsermodINA219::_maxCurrentRange[] PROGMEM = "maxCurrentRange-A"; -const char UsermodINA219::_busVoltageRange[] PROGMEM = "busVoltageRange-V"; +inline const char UsermodINA219::_readInterval[] PROGMEM = "readInterval-ms"; +inline const char UsermodINA219::_i2cAddress[] PROGMEM = "i2cAddress"; +inline const char UsermodINA219::_shuntResistor[] PROGMEM = "shuntResistor-mOhm"; +inline const char UsermodINA219::_maxCurrentRange[] PROGMEM = "maxCurrentRange-A"; +inline const char UsermodINA219::_busVoltageRange[] PROGMEM = "busVoltageRange-V";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/INA219_v2/usermod_ina219.h` around lines 217 - 222, The PROGMEM static definitions (UsermodINA219::_readInterval, _i2cAddress, _shuntResistor, _maxCurrentRange, _busVoltageRange) in the header can cause ODR multiple-definition errors if the header is included in more than one TU; fix by either marking these static data definitions inline (C++17) or moving them into a companion .cpp file where they are defined once; update the header to declare them (if moving) or add the inline specifier to each definition and keep the PROGMEM storage so UsermodINA219 still references the same symbols.
🤖 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/INA219_v2/usermod_ina219.h`:
- Around line 201-210: readFromConfig() currently only calls setup() when the
I2C address changes, which misses the case where enabled transitions from
false→true; update readFromConfig() to track the previous enabled state (e.g.
oldEnabled) and if oldEnabled != enabled and enabled == true call setup() to
force sensor initialization (use same setup() that handles I2C init and sets
sensorFound/ina219), and when enabled transitions to false clear or reset
sensorFound and ina219 to avoid stale state; keep the existing address-change
logic but ensure both paths handle the enabled flag consistently.
- Around line 190-199: After reading JSON into i2cAddress, readInterval and
busVoltageRange_V via getJsonValue, add validation and clamping: validate
i2cAddress against the supported INA219 addresses (0x40,0x41,0x44,0x45) and if
not one of those set a safe default (e.g. 0x40) and log the correction; ensure
readInterval is >= 1 (or a chosen minimum) and if zero or too small set it to
the minimum and log; for busVoltageRange_V only accept 16 or 32 (clamp or
default to 32) and log adjustments. Place these checks immediately after the
getJsonValue lines (near variables i2cAddress, readInterval, busVoltageRange_V)
so begin() will receive valid values. Ensure configComplete remains accurate if
values were corrected and keep the existing shuntResistor_mOhm min clamp logic.
---
Nitpick comments:
In `@usermods/INA219_v2/usermod_ina219.h`:
- Line 43: The field lastMeasure duplicates the base-class Usermod's lastTime;
remove the unsigned long lastMeasure declaration from usermod_ina219.h and
update all usages in the INA219 usermod (references to lastMeasure in methods
like setup(), loop(), read/measure routines) to use lastTime instead so the mod
reuses the inherited timer field and reduces footprint while preserving
behavior.
- Around line 217-222: The PROGMEM static definitions
(UsermodINA219::_readInterval, _i2cAddress, _shuntResistor, _maxCurrentRange,
_busVoltageRange) in the header can cause ODR multiple-definition errors if the
header is included in more than one TU; fix by either marking these static data
definitions inline (C++17) or moving them into a companion .cpp file where they
are defined once; update the header to declare them (if moving) or add the
inline specifier to each definition and keep the PROGMEM storage so
UsermodINA219 still references the same symbols.
🪄 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: f446a9fc-73ad-45d1-bc7d-bee6245ef75e
📒 Files selected for processing (2)
platformio.iniusermods/INA219_v2/usermod_ina219.h
✅ Files skipped from review due to trivial changes (1)
- platformio.ini
Comment on lines
+190
to
+199
| configComplete &= getJsonValue(top[F("enabled")], enabled, false); | ||
| configComplete &= getJsonValue(top[FPSTR(_readInterval)], readInterval, (uint32_t)5000); | ||
| configComplete &= getJsonValue(top[FPSTR(_i2cAddress)], i2cAddress, (uint8_t)0x40); | ||
| configComplete &= getJsonValue(top[FPSTR(_shuntResistor)], shuntResistor_mOhm, 100.0f); | ||
| if (shuntResistor_mOhm < 1.0f) { | ||
| USER_PRINTLN(F("[INA219]: shuntResistor-mOhm clamped to minimum 1 mOhm.")); | ||
| shuntResistor_mOhm = 1.0f; | ||
| } | ||
| configComplete &= getJsonValue(top[FPSTR(_maxCurrentRange)], maxCurrentRange_A, 2.0f); | ||
| configComplete &= getJsonValue(top[FPSTR(_busVoltageRange)], busVoltageRange_V, (uint8_t)32); |
There was a problem hiding this comment.
Validate i2cAddress and readInterval.
No bounds checks on user-supplied config:
i2cAddress: the INA219 only supports 0x40/0x41/0x44/0x45 (and combinations). Arbitrary addresses will just causebegin()to silently fail on reload.readInterval: a user-entered0makesnow - lastMeasure < 0always false and triggers a read everyloop()call, hammering the I²C bus.busVoltageRange_V: any value other than 16/32 will fall into the 32V branch at Line 68; either document or clamp.
🛡️ Proposed validation
configComplete &= getJsonValue(top[FPSTR(_readInterval)], readInterval, (uint32_t)5000);
+ if (readInterval < 50) { readInterval = 50; configComplete = false; }
configComplete &= getJsonValue(top[FPSTR(_i2cAddress)], i2cAddress, (uint8_t)0x40);
+ if (i2cAddress != 0x40 && i2cAddress != 0x41 && i2cAddress != 0x44 && i2cAddress != 0x45) {
+ USER_PRINTLN(F("[INA219]: invalid i2cAddress, reset to 0x40."));
+ i2cAddress = 0x40;
+ configComplete = false;
+ }
configComplete &= getJsonValue(top[FPSTR(_shuntResistor)], shuntResistor_mOhm, 100.0f);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| configComplete &= getJsonValue(top[F("enabled")], enabled, false); | |
| configComplete &= getJsonValue(top[FPSTR(_readInterval)], readInterval, (uint32_t)5000); | |
| configComplete &= getJsonValue(top[FPSTR(_i2cAddress)], i2cAddress, (uint8_t)0x40); | |
| configComplete &= getJsonValue(top[FPSTR(_shuntResistor)], shuntResistor_mOhm, 100.0f); | |
| if (shuntResistor_mOhm < 1.0f) { | |
| USER_PRINTLN(F("[INA219]: shuntResistor-mOhm clamped to minimum 1 mOhm.")); | |
| shuntResistor_mOhm = 1.0f; | |
| } | |
| configComplete &= getJsonValue(top[FPSTR(_maxCurrentRange)], maxCurrentRange_A, 2.0f); | |
| configComplete &= getJsonValue(top[FPSTR(_busVoltageRange)], busVoltageRange_V, (uint8_t)32); | |
| configComplete &= getJsonValue(top[F("enabled")], enabled, false); | |
| configComplete &= getJsonValue(top[FPSTR(_readInterval)], readInterval, (uint32_t)5000); | |
| if (readInterval < 50) { readInterval = 50; configComplete = false; } | |
| configComplete &= getJsonValue(top[FPSTR(_i2cAddress)], i2cAddress, (uint8_t)0x40); | |
| if (i2cAddress != 0x40 && i2cAddress != 0x41 && i2cAddress != 0x44 && i2cAddress != 0x45) { | |
| USER_PRINTLN(F("[INA219]: invalid i2cAddress, reset to 0x40.")); | |
| i2cAddress = 0x40; | |
| configComplete = false; | |
| } | |
| configComplete &= getJsonValue(top[FPSTR(_shuntResistor)], shuntResistor_mOhm, 100.0f); | |
| if (shuntResistor_mOhm < 1.0f) { | |
| USER_PRINTLN(F("[INA219]: shuntResistor-mOhm clamped to minimum 1 mOhm.")); | |
| shuntResistor_mOhm = 1.0f; | |
| } | |
| configComplete &= getJsonValue(top[FPSTR(_maxCurrentRange)], maxCurrentRange_A, 2.0f); | |
| configComplete &= getJsonValue(top[FPSTR(_busVoltageRange)], busVoltageRange_V, (uint8_t)32); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/INA219_v2/usermod_ina219.h` around lines 190 - 199, After reading
JSON into i2cAddress, readInterval and busVoltageRange_V via getJsonValue, add
validation and clamping: validate i2cAddress against the supported INA219
addresses (0x40,0x41,0x44,0x45) and if not one of those set a safe default (e.g.
0x40) and log the correction; ensure readInterval is >= 1 (or a chosen minimum)
and if zero or too small set it to the minimum and log; for busVoltageRange_V
only accept 16 or 32 (clamp or default to 32) and log adjustments. Place these
checks immediately after the getJsonValue lines (near variables i2cAddress,
readInterval, busVoltageRange_V) so begin() will receive valid values. Ensure
configComplete remains accurate if values were corrected and keep the existing
shuntResistor_mOhm min clamp logic.
Comment on lines
+201
to
+210
| if (!initDone) { | ||
| DEBUG_PRINTLN(F("[INA219] config loaded.")); | ||
| } else { | ||
| DEBUG_PRINTLN(F("[INA219] config (re)loaded.")); | ||
| if (oldAddress != i2cAddress) { | ||
| setup(); // reinitialize sensor with new I2C address | ||
| } else { | ||
| applyCalibration(); // update PGA gain for new voltage/current range | ||
| } | ||
| } |
There was a problem hiding this comment.
Re-entering setup() from readFromConfig() — verify behavior when disabled.
At Line 206, setup() is invoked on I²C-address change. If the user simultaneously toggled enabled to false, setup() short-circuits at Line 80, but sensorFound/ina219 keep their previous values. That's benign, but toggling enabled from false→true via config edit won't re-run setup() at all (no address change path) — the sensor will stay uninitialized until reboot. Consider forcing re-init when enabled transitions to true as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/INA219_v2/usermod_ina219.h` around lines 201 - 210, readFromConfig()
currently only calls setup() when the I2C address changes, which misses the case
where enabled transitions from false→true; update readFromConfig() to track the
previous enabled state (e.g. oldEnabled) and if oldEnabled != enabled and
enabled == true call setup() to force sensor initialization (use same setup()
that handles I2C init and sets sensorFound/ina219), and when enabled transitions
to false clear or reset sensorFound and ina219 to avoid stale state; keep the
existing address-change logic but ensure both paths handle the enabled flag
consistently.
Collaborator
@coderabbitai this pattern is "normal" for usermods in WLED-MM. Each usermod is included exactly once from usermods_list.cpp. the PROGMEM constants are intended to reduce flash size, especially on esp8266. Defining them 'inline' (c++17) creates a risk of duplicating strings again, which would counter the original intention. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit