Add INA219 usermod support and update platformio.ini#361
Open
Topchris18 wants to merge 3 commits into
Open
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. ChangesINA219 Usermod Implementation
Build Integration & ID
Project Configuration
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/INA219_v2/usermod_ina219.h (1)
198-201:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark config as incomplete when clamping invalid shunt values.
When the shunt value is corrected,
configCompleteshould be set tofalseso the sanitized value can be persisted.🛠️ Suggested patch
if (shuntResistor_mOhm < 1.0f) { USER_PRINTLN(F("[INA219]: shuntResistor-mOhm clamped to minimum 1 mOhm.")); shuntResistor_mOhm = 1.0f; + configComplete = false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/INA219_v2/usermod_ina219.h` around lines 198 - 201, The code clamps shuntResistor_mOhm to 1.0f but doesn't mark the configuration as needing persistence; inside the branch that adjusts shuntResistor_mOhm (the if (shuntResistor_mOhm < 1.0f) block) set configComplete = false so the sanitized value will be saved; update the same block where USER_PRINTLN is called (referencing shuntResistor_mOhm and configComplete) to ensure the corrected value is persisted.
🧹 Nitpick comments (1)
usermods/INA219_v2/usermod_ina219.h (1)
80-83: ⚡ Quick winRelease INA219 state when the usermod is disabled.
This early return leaves
ina219allocated andsensorFoundstale while disabled. Clearing them here keeps state consistent and frees RAM.♻️ Suggested patch
void setup() override { if (!enabled) { + if (ina219) { + delete ina219; + ina219 = nullptr; + } + sensorFound = false; initDone = true; // mark as initialized so re-enabling via UI triggers (re)setup return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/INA219_v2/usermod_ina219.h` around lines 80 - 83, When handling the early-return branch in the enabled check, release the INA219 state: if enabled is false, delete or free the ina219 object (or call its destructor/cleanup) and set sensorFound to false (and any related flags) before setting initDone = true and returning; update the block around the enabled check that currently contains initDone = true; return; to explicitly clear ina219 and reset sensor-related state so RAM is freed and state remains consistent when the usermod is re-enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@usermods/INA219_v2/usermod_ina219.h`:
- Around line 198-201: The code clamps shuntResistor_mOhm to 1.0f but doesn't
mark the configuration as needing persistence; inside the branch that adjusts
shuntResistor_mOhm (the if (shuntResistor_mOhm < 1.0f) block) set configComplete
= false so the sanitized value will be saved; update the same block where
USER_PRINTLN is called (referencing shuntResistor_mOhm and configComplete) to
ensure the corrected value is persisted.
---
Nitpick comments:
In `@usermods/INA219_v2/usermod_ina219.h`:
- Around line 80-83: When handling the early-return branch in the enabled check,
release the INA219 state: if enabled is false, delete or free the ina219 object
(or call its destructor/cleanup) and set sensorFound to false (and any related
flags) before setting initDone = true and returning; update the block around the
enabled check that currently contains initDone = true; return; to explicitly
clear ina219 and reset sensor-related state so RAM is freed and state remains
consistent when the usermod is re-enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87b858a7-5069-4c9a-9e87-458fc3a679f9
📒 Files selected for processing (1)
usermods/INA219_v2/usermod_ina219.h
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