INA228 Usermod #5236
INA228 Usermod #5236
Conversation
WalkthroughIntroduces a new INA228_v2 usermod for WLED integrating power monitoring via the RobTillaart INA228 library. Enables measurement of bus voltage, current, power, and temperature over I2C. Includes PWM fan control, tachometer RPM calculation, emergency shutdown with hardware/firmware triggers, and persistent configuration. Updates core WLED files for usermod registration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
usermods/INA228_v2/readme.md (1)
16-17: Consider using more descriptive link text.The link text "Link" is generic. Using the product/vendor name would improve accessibility and clarity.
🔎 Proposed fix
-- **Adafruit INA228**: 0.015Ω shunt, 10A max - [Link](https://www.adafruit.com/product/5832) -- **Mateksys I2C-INA-BM**: 0.0002Ω shunt, 204A max - [Link](https://www.mateksys.com/?portfolio=i2c-ina-bm) +- **Adafruit INA228**: 0.015Ω shunt, 10A max - [Adafruit Product Page](https://www.adafruit.com/product/5832) +- **Mateksys I2C-INA-BM**: 0.0002Ω shunt, 204A max - [Mateksys Product Page](https://www.mateksys.com/?portfolio=i2c-ina-bm)usermods/INA228_v2/INA228_v2.cpp (3)
601-617: Inconsistent indentation inaddToJsonState().The function body has mixed indentation - lines 606-616 appear to have different nesting than the function declaration and early lines.
🔎 Proposed fix
void addToJsonState(JsonObject& root) override { - if (!_initDone || !_settingEnabled) return; + if (!_initDone || !_settingEnabled) return; - JsonObject usermod = root[FPSTR(_name)]; - if (usermod.isNull()) usermod = root.createNestedObject(FPSTR(_name)); - usermod[F("voltage")] = _lastBusVoltage; - usermod[F("current")] = _lastCurrent; - usermod[F("power")] = _lastPower; - usermod[F("temperature")] = _lastTemperature; - if (_fanEnabled && _fanPwmPin >= 0) { - usermod[F("fanSpeed")] = _lastFanSpeedPercent; - usermod[F("fanDutyCycle")] = _lastFanDutyCycle; - } - if (_shutdownEnabled) { - usermod[F("shutdownTriggered")] = _shutdownTriggered; - } - } + JsonObject usermod = root[FPSTR(_name)]; + if (usermod.isNull()) usermod = root.createNestedObject(FPSTR(_name)); + usermod[F("voltage")] = _lastBusVoltage; + usermod[F("current")] = _lastCurrent; + usermod[F("power")] = _lastPower; + usermod[F("temperature")] = _lastTemperature; + if (_fanEnabled && _fanPwmPin >= 0) { + usermod[F("fanSpeed")] = _lastFanSpeedPercent; + usermod[F("fanDutyCycle")] = _lastFanDutyCycle; + } + if (_shutdownEnabled) { + usermod[F("shutdownTriggered")] = _shutdownTriggered; + } +}
106-113: Direct floating-point equality comparisons.Using
!=for floating-point comparisons (_shuntResistor,_maxCurrent,_tempLimit,_powerLimit) can be unreliable due to precision issues. In this context, since values come directly from JSON config rather than computation, the risk is low, but epsilon-based comparison would be more robust.🔎 Example using epsilon comparison
// Helper function bool floatChanged(float a, float b, float epsilon = 0.0001f) { return fabsf(a - b) > epsilon; } bool hasSettingsChanged() { return (_i2cAddress != _prevI2cAddress || floatChanged(_shuntResistor, _prevShuntResistor) || floatChanged(_maxCurrent, _prevMaxCurrent) || _averageSamples != _prevAverageSamples || floatChanged(_tempLimit, _prevTempLimit) || floatChanged(_powerLimit, _prevPowerLimit)); }
142-143: Missing blank line between functions.There's no blank line separating
calculateRPM()andinitiateBuzzerAlarm(). Similarly betweenupdateBuzzerState()andinitializeINA228()(lines 188-189). Adding blank lines improves readability.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
usermods/INA228_v2/INA228_v2.cppusermods/INA228_v2/library.jsonusermods/INA228_v2/readme.mdwled00/const.hwled00/pin_manager.h
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/const.hwled00/pin_manager.h
🧠 Learnings (5)
📓 Common learnings
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: willmmiles
Repo: wled/WLED PR: 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.
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
usermods/INA228_v2/library.json
📚 Learning: 2025-05-06T20:31:05.208Z
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.
Applied to files:
usermods/INA228_v2/library.jsonusermods/INA228_v2/readme.mdusermods/INA228_v2/INA228_v2.cpp
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED PR: 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.
Applied to files:
wled00/const.hwled00/pin_manager.h
📚 Learning: 2025-04-27T10:06:22.545Z
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
Applied to files:
wled00/pin_manager.h
🧬 Code graph analysis (2)
wled00/const.h (1)
usermods/INA228_v2/INA228_v2.cpp (1)
USERMOD_ID_INA228(810-812)
wled00/pin_manager.h (2)
usermods/pixels_dice_tray/pixels_dice_tray.cpp (1)
USERMOD_ID_PIXELS_DICE_TRAY(528-528)usermods/INA228_v2/INA228_v2.cpp (1)
USERMOD_ID_INA228(810-812)
🪛 Cppcheck (2.19.0)
usermods/INA228_v2/INA228_v2.cpp
[error] 830-830: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
🪛 LanguageTool
usermods/INA228_v2/readme.md
[grammar] ~31-~31: Ensure spelling is correct
Context: ...your power supply. Important: This usermod uses the global I2C GPIO pins configure...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~108-~108: Ensure spelling is correct
Context: ...raw. - The temperature reported by this usermod is the die temp of the INA228. If using...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~108-~108: Ensure spelling is correct
Context: ...n still be used to estimate LED temp by coorelation. For LED matricies with an INA228 on th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~108-~108: Ensure spelling is correct
Context: ...timate LED temp by coorelation. For LED matricies with an INA228 on the same PCB, the tem...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~110-~110: Ensure spelling is correct
Context: ...INA228 devices and are included in this usermod for convienence. Noctua PWM fans (like ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~110-~110: Ensure spelling is correct
Context: ...es and are included in this usermod for convienence. Noctua PWM fans (like the NF-A4x10 5V ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~110-~110: Ensure spelling is correct
Context: ...PWM fans (like the NF-A4x10 5V PWM) are recomended and known to work directly with ESP32 l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~111-~111: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...this version, but could be added later. Feel free to submit a pull request. ## Credits Dep...
(FEEL_FREE_TO_STYLE_ME)
🪛 markdownlint-cli2 (0.18.1)
usermods/INA228_v2/readme.md
16-16: Link text should be descriptive
(MD059, descriptive-link-text)
17-17: Link text should be descriptive
(MD059, descriptive-link-text)
🔇 Additional comments (7)
wled00/const.h (1)
210-210: LGTM!The new usermod ID follows the sequential numbering convention. Minor note: the comment references
INA228_v2.hbut the actual implementation file isINA228_v2.cpp. This is a trivial inconsistency that doesn't affect functionality.wled00/pin_manager.h (1)
72-73: LGTM!The new
UM_INA228enum value correctly referencesUSERMOD_ID_INA228and follows the existing pattern. The hex comment (0x3B = 59) is accurate.usermods/INA228_v2/INA228_v2.cpp (4)
20-26: LGTM!The ISR implementation correctly uses
IRAM_ATTRfor the function andDRAM_ATTRwithvolatilefor the counter. The ISR body is minimal, which is appropriate.
296-299: Blocking delay in resetAlert().The
delay(10)is a blocking call. While acceptable for a user-initiated reset action, consider if this could cause issues during time-sensitive operations. The current usage appears safe since it's only called fromreadFromJsonState().
332-391: LGTM!The emergency shutdown logic is well-designed with proper latching behavior, continuous enforcement, and comprehensive state capture. The decision to require manual reset is documented and appropriate for safety-critical functionality.
829-834: LGTM!The static usermod registration follows WLED conventions. The Cppcheck warning about
PROGMEMis a false positive - it's a standard Arduino macro for storing data in flash memory.usermods/INA228_v2/library.json (1)
21-27: Dependency declaration is correct.The INA228 library version 0.4.0 is available in the PlatformIO Registry, and the caret versioning allows compatible updates as intended.
| // Calculate RPM: 2 pulses per rotation, multiply by intervals per minute | ||
| // RPM = (pulses / 2) * (60000 / _checkInterval) | ||
| _fanRPM = (pulses * 60000UL) / (_checkInterval * 2); |
There was a problem hiding this comment.
Potential division by zero if _checkInterval is 0.
If a user configures Check Interval to 0, the division at line 137 will cause undefined behavior. Consider adding validation in readFromConfig() or guarding here.
🔎 Proposed fix in readFromConfig()
configComplete &= getJsonValue(top[F("Check Interval")], _checkInterval, (uint16_t)1000);
+if (_checkInterval == 0) _checkInterval = 1000; // Prevent division by zeroCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In usermods/INA228_v2/INA228_v2.cpp around lines 135-137, the RPM calculation
divides by _checkInterval which can be zero; add a guard to prevent division by
zero by validating _checkInterval when reading config or right before the
calculation: if _checkInterval is zero, set it to a sensible default (e.g.,
1000) or treat the measurement as invalid and set _fanRPM to 0/skip calculation,
otherwise perform the existing calculation; ensure any change updates related
comments and keeps behavior deterministic.
| // Initialize the INA228 sensor | ||
| void initializeINA228() { | ||
| if (_ina228 != nullptr) { | ||
| delete _ina228; | ||
| _ina228 = nullptr; | ||
| } | ||
|
|
||
| // Create new INA228 instance | ||
| _ina228 = new INA228(_i2cAddress, &Wire); | ||
|
|
||
| if (!_ina228->begin()) { | ||
| DEBUG_PRINTLN(F("INA228: Initialization failed!")); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Initialization failure doesn't prevent _initDone from being set.
When _ina228->begin() fails, the function returns early but setup() still sets _initDone = true at line 476. This allows loop() and other methods to proceed with a null/failed sensor.
🔎 Proposed fix
void initializeINA228() {
if (_ina228 != nullptr) {
delete _ina228;
_ina228 = nullptr;
}
// Create new INA228 instance
_ina228 = new INA228(_i2cAddress, &Wire);
if (!_ina228->begin()) {
DEBUG_PRINTLN(F("INA228: Initialization failed!"));
+ delete _ina228;
+ _ina228 = nullptr;
return;
}Then in setup(), guard _initDone:
initializeINA228();
+ if (_ina228 == nullptr) {
+ DEBUG_PRINTLN(F("INA228: Sensor init failed, usermod disabled"));
+ return;
+ }
// Initialize fan control if enabledCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In usermods/INA228_v2/INA228_v2.cpp around lines 189-202, initializeINA228
currently returns early on _ina228->begin() failure but setup() still
unconditionally sets _initDone = true later; change initializeINA228 to
propagate success/failure (return bool) or set a member flag so failures clear
_initDone, then update setup() to set _initDone = true only when
initializeINA228 reports success (and ensure on failure _initDone remains false
and further loop()/methods abort). Ensure any existing call sites are updated to
use the boolean result or check the member flag.
| ~UsermodINA228() { | ||
| // Detach interrupt if tachometer was configured | ||
| if (_fanTachoPin >= 0) { | ||
| detachInterrupt(_fanTachoPin); | ||
| } | ||
| if (_ina228 != nullptr) { | ||
| delete _ina228; | ||
| _ina228 = nullptr; | ||
| } | ||
| } |
There was a problem hiding this comment.
Destructor does not deallocate pins.
Pins allocated in setup() via PinManager::allocatePin() are not deallocated in the destructor. This causes a resource leak if the usermod is destroyed or reinitialized.
🔎 Proposed fix
~UsermodINA228() {
// Detach interrupt if tachometer was configured
if (_fanTachoPin >= 0) {
detachInterrupt(_fanTachoPin);
+ PinManager::deallocatePin(_fanTachoPin, PinOwner::UM_INA228);
}
+ if (_fanPwmPin >= 0) {
+ PinManager::deallocatePin(_fanPwmPin, PinOwner::UM_INA228);
+ }
+ if (_alertPin >= 0) {
+ PinManager::deallocatePin(_alertPin, PinOwner::UM_INA228);
+ }
+ if (_buzzerPin >= 0) {
+ PinManager::deallocatePin(_buzzerPin, PinOwner::UM_INA228);
+ }
if (_ina228 != nullptr) {
delete _ina228;
_ina228 = nullptr;
}
}🤖 Prompt for AI Agents
In usermods/INA228_v2/INA228_v2.cpp around lines 817-826, the destructor
currently frees the INA228 object and detaches interrupts but does not
deallocate pins that were allocated in setup() via PinManager::allocatePin(),
causing a resource leak; update the destructor to call the
PinManager::deallocatePin() (or pinManager.deallocatePin()) for every pin the
usermod allocated (at minimum _fanTachoPin if it was allocated), ensure you
detach the interrupt first, then deallocate the pin and set the pin member(s) to
-1 to mark them free; keep the existing _ina228 deletion after pin cleanup.
| - The temperature reported by this usermod is the die temp of the INA228. If using an INA228 breakout board, the temperature will not reflect the actual temperature of the LEDs, but can still be used to estimate LED temp by coorelation. For LED matricies with an INA228 on the same PCB, the temperature reading is more valuable. The INA228 also supports an external temperature sensor, but that is not implemented in this usermod. | ||
| - The INA228 IC also supports battery charge calculation. This is supported by the library developed by robtillaart but not implemented in this user mod. Users that need this feature should feel free to add support with a pull request. | ||
| - PWM fan control and piezo buzzers are not standard on INA228 devices and are included in this usermod for convienence. Noctua PWM fans (like the NF-A4x10 5V PWM) are recomended and known to work directly with ESP32 logic levels so no breakout board is needed. Active piezo buzzers are commonly available on breakout boards. | ||
| - MQTT support is not included in this version, but could be added later. Feel free to submit a pull request. |
There was a problem hiding this comment.
Fix spelling errors in documentation.
Several typos were detected:
- Line 108: "coorelation" → "correlation", "matricies" → "matrices"
- Line 110: "convienence" → "convenience", "recomended" → "recommended"
🔎 Proposed fixes
-- The temperature reported by this usermod is the die temp of the INA228. If using an INA228 breakout board, the temperature will not reflect the actual temperature of the LEDs, but can still be used to estimate LED temp by coorelation. For LED matricies with an INA228 on the same PCB, the temperature reading is more valuable. The INA228 also supports an external temperature sensor, but that is not implemented in this usermod.
+- The temperature reported by this usermod is the die temp of the INA228. If using an INA228 breakout board, the temperature will not reflect the actual temperature of the LEDs, but can still be used to estimate LED temp by correlation. For LED matrices with an INA228 on the same PCB, the temperature reading is more valuable. The INA228 also supports an external temperature sensor, but that is not implemented in this usermod.
-- PWM fan control and piezo buzzers are not standard on INA228 devices and are included in this usermod for convienence. Noctua PWM fans (like the NF-A4x10 5V PWM) are recomended and known to work directly with ESP32 logic levels so no breakout board is needed. Active piezo buzzers are commonly available on breakout boards.
+- PWM fan control and piezo buzzers are not standard on INA228 devices and are included in this usermod for convenience. Noctua PWM fans (like the NF-A4x10 5V PWM) are recommended and known to work directly with ESP32 logic levels so no breakout board is needed. Active piezo buzzers are commonly available on breakout boards.🧰 Tools
🪛 LanguageTool
[grammar] ~108-~108: Ensure spelling is correct
Context: ...raw. - The temperature reported by this usermod is the die temp of the INA228. If using...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~108-~108: Ensure spelling is correct
Context: ...n still be used to estimate LED temp by coorelation. For LED matricies with an INA228 on th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~108-~108: Ensure spelling is correct
Context: ...timate LED temp by coorelation. For LED matricies with an INA228 on the same PCB, the tem...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~110-~110: Ensure spelling is correct
Context: ...INA228 devices and are included in this usermod for convienence. Noctua PWM fans (like ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~110-~110: Ensure spelling is correct
Context: ...es and are included in this usermod for convienence. Noctua PWM fans (like the NF-A4x10 5V ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~110-~110: Ensure spelling is correct
Context: ...PWM fans (like the NF-A4x10 5V PWM) are recomended and known to work directly with ESP32 l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~111-~111: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...this version, but could be added later. Feel free to submit a pull request. ## Credits Dep...
(FEEL_FREE_TO_STYLE_ME)
🤖 Prompt for AI Agents
In usermods/INA228_v2/readme.md around lines 108 to 111, the documentation
contains several spelling mistakes; update "coorelation" to "correlation",
"matricies" to "matrices", "convienence" to "convenience", and "recomended" to
"recommended" so the README reads correctly and professionally.
There was a problem hiding this comment.
Didn't expect to get roasted on my grammar with this one 😄
There was a problem hiding this comment.
@AdamJSchofield Haha, sorry about that! 😅 Just trying to help polish up the documentation. The content itself is really well-written and comprehensive—those are just minor typos. Feel free to address them whenever convenient, or not at all if you prefer! The README does a great job explaining the usermod's features and usage.
🧠 Learnings used
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: willmmiles
Repo: wled/WLED PR: 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.
|
thanks for contributin. How different is the INA228 from INA226? see #4237 since this is highly specific for your hardware you can also leave it as-is and make it an out-of-tree usermod, although it is not clear to me on how to do that, a tutorial on it is pending. |
2 participants
Adds a v2 usermod with support for most features of the INA228 current/temp monitor. Also adds a few peripherals like temperature-based PWM fan control, piezo buzzer alarm, and emergency shutoff.
I made this usermod to support the features included on a custom WLED-compatible LED matrix that I am working on. If the additional features beyond those of the INA228 IC itself would be better suited in separate usermods, let me know and Ill break it apart.
Dependencies:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.