INA228 Usermod#5236
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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)
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. |
|
@DedeHai Thanks for taking a look. The INA228 and INA226 are different ICs with the INA228 having more features and higher precision ADCs. The main difference for this case is that the INA228 can monitor/alert-on temperature (die temp or external therm). I did reference the existing INA226 usermod a bit and they are similar. The differences mostly come down to the underlying device implementation. Like different register values and calibration constants and such.
Just to be clear, everything except the PWM fan control and piezo buzzer is bone stock INA228. The buzzer is part of the emergency shutdown process when the alert pin of the INA228 goes low. So it seems logical to include. But I should simplify the settings a bit. The PWM fan control should probably be split out. I saw another usermod for something similar but didn't look too deeply. Ideally there is one PWM fan control usermod to rule them all. Maybe something that just subscribes to a "Temperature" topic and interconnects via MQTT. I will come back to this soon when I have more time and implement the fixes suggested by the robot and also think about what to do with the "extra" stuff. Thanks again for giving it some eyes |
|
Might be related to |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
usermods/INA228_v2/INA228_v2.cpp (1)
601-617: ⚡ Quick winInconsistent indentation.
The code block has inconsistent indentation - lines 606-616 are indented with tabs or extra spaces relative to the surrounding block. Per coding guidelines, use 2-space indentation consistently.
♻️ 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; + } +}🤖 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/INA228_v2/INA228_v2.cpp` around lines 601 - 617, The addToJsonState(JsonObject& root) method has inconsistent indentation for the nested usermod assignment block; reformat the body of addToJsonState so all lines use 2-space indentation consistently (including the usermod creation, assignments to _lastBusVoltage/_lastCurrent/_lastPower/_lastTemperature, the fan block using _fanEnabled/_fanPwmPin and fan fields, and the shutdown block using _shutdownEnabled/_shutdownTriggered) and ensure opening/closing braces align with the function's indentation level.
🤖 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.
Inline comments:
In `@platformio.ini`:
- Around line 13-33: The default_envs list in platformio.ini has been commented
out, altering PlatformIO's default build environments and requiring maintainer
approval; either restore the original default_envs block (uncomment those lines
in platformio.ini so the commented-out list is back to its prior state) if this
change was only for local testing, or add a maintainer-approved note and include
explicit approval in the PR description before merging; target the default_envs
section in platformio.ini to locate and revert or document the change.
In `@usermods/INA228_v2/INA228_v2.cpp`:
- Around line 277-282: The interpolation block using range = _fanMaxTemp -
_fanStartTemp can divide by zero or be negative if _fanMaxTemp <= _fanStartTemp;
update the code around that block (references: _fanMaxTemp, _fanStartTemp,
tempAboveMin, dutyCycle) to first check if range <= 0 and handle it
deterministically (for example, if range <= 0 then set dutyCycle to 0 when temp
<= _fanStartTemp else 255), otherwise perform the existing interpolation; also
ensure the computed dutyCycle is clamped to 0–255 to avoid out-of-range values.
- Around line 305-320: The ADC range is set to 164 mV in resetAlert() via
_ina228->setADCRange(false) but initializeINA228() never sets the ADC range,
causing inconsistent ADC configuration after reset; update initializeINA228() to
call _ina228->setADCRange(false) during setup (or remove the call from
resetAlert() if you prefer the default) so both initialization and reset use the
same ADC range—modify the initializeINA228() function to include the same
setADCRange(false) call to ensure consistent behavior with resetAlert().
---
Nitpick comments:
In `@usermods/INA228_v2/INA228_v2.cpp`:
- Around line 601-617: The addToJsonState(JsonObject& root) method has
inconsistent indentation for the nested usermod assignment block; reformat the
body of addToJsonState so all lines use 2-space indentation consistently
(including the usermod creation, assignments to
_lastBusVoltage/_lastCurrent/_lastPower/_lastTemperature, the fan block using
_fanEnabled/_fanPwmPin and fan fields, and the shutdown block using
_shutdownEnabled/_shutdownTriggered) and ensure opening/closing braces align
with the function's indentation level.
🪄 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: ecc984cf-d896-4210-9252-87645006728e
📒 Files selected for processing (6)
platformio.iniusermods/INA228_v2/INA228_v2.cppusermods/INA228_v2/library.jsonusermods/INA228_v2/readme.mdwled00/const.hwled00/pin_manager.h
✅ Files skipped from review due to trivial changes (1)
- usermods/INA228_v2/library.json
| } else { | ||
| // Between thresholds: linear interpolation | ||
| float range = _fanMaxTemp - _fanStartTemp; | ||
| float tempAboveMin = temp - _fanStartTemp; | ||
| dutyCycle = (uint8_t)((tempAboveMin / range) * 255.0f); | ||
| } |
There was a problem hiding this comment.
Potential division by zero in fan speed interpolation.
If _fanMaxTemp <= _fanStartTemp (misconfigured or equal values), range will be zero or negative, causing undefined behavior or incorrect duty cycle calculation at line 281.
🛡️ Proposed fix
} else {
// Between thresholds: linear interpolation
float range = _fanMaxTemp - _fanStartTemp;
+ if (range <= 0.0f) {
+ dutyCycle = 255; // Fail-safe: full speed if misconfigured
+ } else {
float tempAboveMin = temp - _fanStartTemp;
dutyCycle = (uint8_t)((tempAboveMin / range) * 255.0f);
+ }
}🤖 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/INA228_v2/INA228_v2.cpp` around lines 277 - 282, The interpolation
block using range = _fanMaxTemp - _fanStartTemp can divide by zero or be
negative if _fanMaxTemp <= _fanStartTemp; update the code around that block
(references: _fanMaxTemp, _fanStartTemp, tempAboveMin, dutyCycle) to first check
if range <= 0 and handle it deterministically (for example, if range <= 0 then
set dutyCycle to 0 when temp <= _fanStartTemp else 255), otherwise perform the
existing interpolation; also ensure the computed dutyCycle is clamped to 0–255
to avoid out-of-range values.
| // Reconfigure shutdown thresholds if enabled | ||
| if (_shutdownEnabled && (_tempLimit > 0 || _powerLimit > 0)) { | ||
| _ina228->setADCRange(false); // 164 mV range | ||
| float current_LSB = _maxCurrent * 1.9073486328125e-6; | ||
|
|
||
| if (_tempLimit > 0) { | ||
| uint16_t tempThreshold = (uint16_t)(_tempLimit / 7.8125e-3); | ||
| _ina228->setTemperatureOverLimitTH(tempThreshold); | ||
| } | ||
|
|
||
| if (_powerLimit > 0) { | ||
| float powerLSB = 3.2 * current_LSB; | ||
| uint16_t powerThreshold = (uint16_t)(_powerLimit / (256.0 * powerLSB)); | ||
| _ina228->setPowerOverLimitTH(powerThreshold); | ||
| } | ||
| } |
There was a problem hiding this comment.
ADC range configuration inconsistency between initializeINA228() and resetAlert().
resetAlert() calls setADCRange(false) at line 307 to set 164mV range, but initializeINA228() never calls setADCRange(). This means after a reset, the sensor operates with different ADC configuration than after initial setup.
Either add setADCRange(false) to initializeINA228() for consistency, or remove it from resetAlert() if the default range is intended.
♻️ Proposed fix (add to initializeINA228)
// Configure the sensor
+_ina228->setADCRange(false); // 164 mV range for higher resolution
_ina228->setMaxCurrentShunt(_maxCurrent, _shuntResistor);
_ina228->setAverage(_averageSamples);🤖 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/INA228_v2/INA228_v2.cpp` around lines 305 - 320, The ADC range is
set to 164 mV in resetAlert() via _ina228->setADCRange(false) but
initializeINA228() never sets the ADC range, causing inconsistent ADC
configuration after reset; update initializeINA228() to call
_ina228->setADCRange(false) during setup (or remove the call from resetAlert()
if you prefer the default) so both initialization and reset use the same ADC
range—modify the initializeINA228() function to include the same
setADCRange(false) call to ensure consistent behavior with resetAlert().
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
usermods/INA228_v2/readme.md (2)
1-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
custom_usermodsshould reference the friendly key, not the_v2folder name.Line 37 uses
custom_usermods = INA228_v2. Based on learnings,_v2in a usermod directory name denotes the second-generation usermod framework, not a hardware version, and installation instructions should reference the friendly key (INA228) since WLED's build scripts resolve it to the_v2folder via a fallback mechanism.📝 Proposed fix
-- ```custom_usermods = INA228_v2``` +- ```custom_usermods = INA228```🤖 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/INA228_v2/readme.md` around lines 1 - 38, The installation instructions in the INA228_v2 README use the internal folder-style value for custom_usermods instead of the friendly usermod key. Update the Installation section to reference the resolved key used by WLED build scripts, and make sure the guidance around custom_usermods points to INA228 rather than INA228_v2 so users copy the correct setting.Source: Learnings
16-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive link text instead of "Link".
Flagged by markdownlint (MD059). Minor doc polish.
🤖 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/INA228_v2/readme.md` around lines 16 - 17, The markdown in the INA228_v2 readme uses generic “Link” anchor text for the Adafruit INA228 and Mateksys I2C-INA-BM entries, which triggers markdownlint MD059. Update the link labels in this section to descriptive text that matches each product name, keeping the same URLs and surrounding bullet formatting in readme.md.Source: Linters/SAST tools
usermods/INA228_v2/INA228_v2.cpp (2)
339-346: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardware alert is only polled once per
_checkInterval, not "instant".readme.md advertises the ALERT pin as providing "instant notification" / "faster response" (readme lines 72, 83), but
checkEmergencyShutdown()is only invoked fromloop()inside the same_checkInterval-gated block as the rest of the sensor read cycle — so hardware and software detection have identical latency (up to_checkIntervalms, default 1000ms). Consider pollingdigitalRead(_alertPin)everyloop()iteration (cheap) instead of gating it behind_checkInterval, or attach a real interrupt on_alertPin(as already done for the tach pin) to fulfill the "instant" claim.🤖 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/INA228_v2/INA228_v2.cpp` around lines 339 - 346, The ALERT pin handling in checkEmergencyShutdown() is currently delayed by the _checkInterval gate inside loop(), so it does not provide the advertised instant response. Update the INA228_v2 flow so digitalRead(_alertPin) is checked on every loop iteration, or wire _alertPin to a real interrupt like the tach path, and keep the existing criticalCondition/reason handling in checkEmergencyShutdown() intact.
679-731: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFan/alert/buzzer pin config changes are silently ignored until reboot.
hasSettingsChanged()(and the reinit call it gates) only tracks I2C address, shunt resistor, max current, average samples, and the shutdown limits. If a user changesPWM Pin,Tach Pin,Alert Pin, orBuzzer Pinon the settings page,readFromConfig()updates the member variables but never re-runs pin allocation/deallocation — the old pins stay allocated and the new ones are never initialized until WLED is rebooted, with no warning to the user that a reboot is required for these fields.🤖 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/INA228_v2/INA228_v2.cpp` around lines 679 - 731, Pin changes for fan/alert/buzzer are being loaded in readFromConfig() but never trigger reinitialization, so the new values in PWM Pin, Tach Pin, Alert Pin, and Buzzer Pin are ignored until reboot. Update hasSettingsChanged() (or add a dedicated config-change check in readFromConfig()) to include the hardware pin fields used by the PWM fan, emergency shutdown, and piezo buzzer sections, and ensure initializeINA228() is called after deallocating any previously configured pins so the new allocations take effect immediately.
🤖 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.
Inline comments:
In `@usermods/INA228_v2/INA228_v2.cpp`:
- Around line 332-391: Emergency shutdown in checkEmergencyShutdown() is only
enforced while the current reading still trips criticalCondition, so
firmware-threshold shutdowns can stop being applied after values recover. Update
the logic in checkEmergencyShutdown() to drive enforcement from the latched
_shutdownTriggered flag (not just the instantaneous temperature/power check),
while still using criticalCondition to detect the first trigger. Keep the
existing first-trigger bookkeeping (_shutdownReason, _shutdownTemp,
_shutdownPower, getTimeString, initiateBuzzerAlarm) in the trigger path, but
ensure the bri=0/stateUpdated and fan full-speed actions continue whenever
_shutdownTriggered is true until the reset/clear path explicitly releases it.
- Around line 410-477: The tachometer setup path in setup() leaves _fanTachoPin
configured even when PinManager::allocatePin() fails, unlike the PWM, alert, and
buzzer branches. Update the fan tach allocation failure handling in
INA228_v2::setup() to reset _fanTachoPin to -1 (and keep _fanEnabled consistent
if needed) so later logic in calculateRPM(), the loop() debug output, and
addToJsonInfo() does not treat a failed tach input as a valid sensor and report
a stale 0 RPM value.
---
Nitpick comments:
In `@usermods/INA228_v2/INA228_v2.cpp`:
- Around line 339-346: The ALERT pin handling in checkEmergencyShutdown() is
currently delayed by the _checkInterval gate inside loop(), so it does not
provide the advertised instant response. Update the INA228_v2 flow so
digitalRead(_alertPin) is checked on every loop iteration, or wire _alertPin to
a real interrupt like the tach path, and keep the existing
criticalCondition/reason handling in checkEmergencyShutdown() intact.
- Around line 679-731: Pin changes for fan/alert/buzzer are being loaded in
readFromConfig() but never trigger reinitialization, so the new values in PWM
Pin, Tach Pin, Alert Pin, and Buzzer Pin are ignored until reboot. Update
hasSettingsChanged() (or add a dedicated config-change check in
readFromConfig()) to include the hardware pin fields used by the PWM fan,
emergency shutdown, and piezo buzzer sections, and ensure initializeINA228() is
called after deallocating any previously configured pins so the new allocations
take effect immediately.
In `@usermods/INA228_v2/readme.md`:
- Around line 1-38: The installation instructions in the INA228_v2 README use
the internal folder-style value for custom_usermods instead of the friendly
usermod key. Update the Installation section to reference the resolved key used
by WLED build scripts, and make sure the guidance around custom_usermods points
to INA228 rather than INA228_v2 so users copy the correct setting.
- Around line 16-17: The markdown in the INA228_v2 readme uses generic “Link”
anchor text for the Adafruit INA228 and Mateksys I2C-INA-BM entries, which
triggers markdownlint MD059. Update the link labels in this section to
descriptive text that matches each product name, keeping the same URLs and
surrounding bullet formatting in readme.md.
🪄 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: 2a409ca1-2faf-40ac-99fb-7cc933654bc7
📒 Files selected for processing (5)
usermods/INA228_v2/INA228_v2.cppusermods/INA228_v2/library.jsonusermods/INA228_v2/readme.mdwled00/const.hwled00/pin_manager.h
💤 Files with no reviewable changes (1)
- wled00/pin_manager.h
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/INA228_v2/library.json
| // Check for emergency shutdown conditions | ||
| void checkEmergencyShutdown() { | ||
| if (!_shutdownEnabled) return; | ||
|
|
||
| bool criticalCondition = false; | ||
| const char* reason = nullptr; | ||
|
|
||
| // Check alert pin if configured (hardware alert - latched until cleared) | ||
| if (_alertConfigured && _alertPin >= 0) { | ||
| // INA228 ALERT pin is active LOW | ||
| if (digitalRead(_alertPin) == LOW) { | ||
| criticalCondition = true; | ||
| reason = "INA228 Alert Pin"; | ||
| } | ||
| } | ||
|
|
||
| // Software checks as additional protection (always check, independent of alert pin) | ||
| if (!criticalCondition) { | ||
| if (_tempLimit > 0 && _lastTemperature >= _tempLimit) { | ||
| criticalCondition = true; | ||
| reason = "Firmware Check"; | ||
| } else if (_powerLimit > 0 && _lastPower >= _powerLimit) { | ||
| criticalCondition = true; | ||
| reason = "Firmware Check"; | ||
| } | ||
| } | ||
|
|
||
| if (criticalCondition) { | ||
| // Store shutdown info on first trigger | ||
| if (!_shutdownTriggered) { | ||
| _shutdownTriggered = true; | ||
| _shutdownReason = reason; | ||
| _shutdownTemp = _lastTemperature; | ||
| _shutdownPower = _lastPower; | ||
| getTimeString(_shutdownTime); // Store formatted time string | ||
| DEBUG_PRINTLN(F("INA228: EMERGENCY SHUTDOWN TRIGGERED!")); | ||
| DEBUG_PRINT(F("INA228: Reason: ")); | ||
| DEBUG_PRINTLN(reason); | ||
| initiateBuzzerAlarm(); // Initiate the buzzer (non-blocking) | ||
| } | ||
|
|
||
| // Always enforce shutdown when critical condition exists | ||
| if (bri > 0) { | ||
| DEBUG_PRINTLN(F("INA228: Enforcing emergency shutdown (LEDs were turned back on)")); | ||
| briLast = bri; | ||
| bri = 0; | ||
| stateUpdated(CALL_MODE_DIRECT_CHANGE); | ||
| } | ||
|
|
||
| // Turn fan to full speed if available | ||
| if (_fanEnabled && _fanPwmPin >= 0) { | ||
| analogWrite(_fanPwmPin, 255); | ||
| _lastFanDutyCycle = 255; | ||
| _lastFanSpeedPercent = 100; | ||
| } | ||
| } | ||
| // Note: Shutdown is NOT automatically cleared when conditions improve. | ||
| // The ALERT pin is latched and must be cleared via the Reset Alert button. | ||
| // This ensures the user acknowledges the event before resuming operation. | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Emergency shutdown enforcement is not actually latched for software-threshold triggers.
criticalCondition is recomputed fresh on every call, and the bri = 0 / full-fan-speed enforcement lives inside if (criticalCondition). When shutdown is triggered by temperature/power threshold (no _alertPin configured, or alert not asserted), once the reading drops back below the limit, criticalCondition becomes false again and enforcement silently stops — even though _shutdownTriggered remains true and the UI still shows "TRIGGERED". A user could then turn LEDs back on and they would stay on, contradicting the documented behavior that shutdown "is NOT automatically cleared" and persists until the alert is cleared (readme.md line 90).
Only the hardware ALERT-pin path is truly latched (since the IC keeps the pin low until reset); the firmware-check path is not.
🛡️ Proposed fix: enforce based on the latched flag, not the instantaneous condition
if (criticalCondition) {
// Store shutdown info on first trigger
if (!_shutdownTriggered) {
_shutdownTriggered = true;
_shutdownReason = reason;
_shutdownTemp = _lastTemperature;
_shutdownPower = _lastPower;
getTimeString(_shutdownTime); // Store formatted time string
DEBUG_PRINTLN(F("INA228: EMERGENCY SHUTDOWN TRIGGERED!"));
DEBUG_PRINT(F("INA228: Reason: "));
DEBUG_PRINTLN(reason);
initiateBuzzerAlarm(); // Initiate the buzzer (non-blocking)
}
-
- // Always enforce shutdown when critical condition exists
- if (bri > 0) {
- DEBUG_PRINTLN(F("INA228: Enforcing emergency shutdown (LEDs were turned back on)"));
- briLast = bri;
- bri = 0;
- stateUpdated(CALL_MODE_DIRECT_CHANGE);
- }
-
- // Turn fan to full speed if available
- if (_fanEnabled && _fanPwmPin >= 0) {
- analogWrite(_fanPwmPin, 255);
- _lastFanDutyCycle = 255;
- _lastFanSpeedPercent = 100;
- }
}
+ // Enforce as long as shutdown is latched (not just while the instantaneous
+ // condition holds) so software-threshold triggers stay enforced until
+ // explicitly cleared via resetAlert().
+ if (_shutdownTriggered) {
+ if (bri > 0) {
+ DEBUG_PRINTLN(F("INA228: Enforcing emergency shutdown (LEDs were turned back on)"));
+ briLast = bri;
+ bri = 0;
+ stateUpdated(CALL_MODE_DIRECT_CHANGE);
+ }
+ if (_fanEnabled && _fanPwmPin >= 0) {
+ analogWrite(_fanPwmPin, 255);
+ _lastFanDutyCycle = 255;
+ _lastFanSpeedPercent = 100;
+ }
+ }📝 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.
| // Check for emergency shutdown conditions | |
| void checkEmergencyShutdown() { | |
| if (!_shutdownEnabled) return; | |
| bool criticalCondition = false; | |
| const char* reason = nullptr; | |
| // Check alert pin if configured (hardware alert - latched until cleared) | |
| if (_alertConfigured && _alertPin >= 0) { | |
| // INA228 ALERT pin is active LOW | |
| if (digitalRead(_alertPin) == LOW) { | |
| criticalCondition = true; | |
| reason = "INA228 Alert Pin"; | |
| } | |
| } | |
| // Software checks as additional protection (always check, independent of alert pin) | |
| if (!criticalCondition) { | |
| if (_tempLimit > 0 && _lastTemperature >= _tempLimit) { | |
| criticalCondition = true; | |
| reason = "Firmware Check"; | |
| } else if (_powerLimit > 0 && _lastPower >= _powerLimit) { | |
| criticalCondition = true; | |
| reason = "Firmware Check"; | |
| } | |
| } | |
| if (criticalCondition) { | |
| // Store shutdown info on first trigger | |
| if (!_shutdownTriggered) { | |
| _shutdownTriggered = true; | |
| _shutdownReason = reason; | |
| _shutdownTemp = _lastTemperature; | |
| _shutdownPower = _lastPower; | |
| getTimeString(_shutdownTime); // Store formatted time string | |
| DEBUG_PRINTLN(F("INA228: EMERGENCY SHUTDOWN TRIGGERED!")); | |
| DEBUG_PRINT(F("INA228: Reason: ")); | |
| DEBUG_PRINTLN(reason); | |
| initiateBuzzerAlarm(); // Initiate the buzzer (non-blocking) | |
| } | |
| // Always enforce shutdown when critical condition exists | |
| if (bri > 0) { | |
| DEBUG_PRINTLN(F("INA228: Enforcing emergency shutdown (LEDs were turned back on)")); | |
| briLast = bri; | |
| bri = 0; | |
| stateUpdated(CALL_MODE_DIRECT_CHANGE); | |
| } | |
| // Turn fan to full speed if available | |
| if (_fanEnabled && _fanPwmPin >= 0) { | |
| analogWrite(_fanPwmPin, 255); | |
| _lastFanDutyCycle = 255; | |
| _lastFanSpeedPercent = 100; | |
| } | |
| } | |
| // Note: Shutdown is NOT automatically cleared when conditions improve. | |
| // The ALERT pin is latched and must be cleared via the Reset Alert button. | |
| // This ensures the user acknowledges the event before resuming operation. | |
| } | |
| // Check for emergency shutdown conditions | |
| void checkEmergencyShutdown() { | |
| if (!_shutdownEnabled) return; | |
| bool criticalCondition = false; | |
| const char* reason = nullptr; | |
| // Check alert pin if configured (hardware alert - latched until cleared) | |
| if (_alertConfigured && _alertPin >= 0) { | |
| // INA228 ALERT pin is active LOW | |
| if (digitalRead(_alertPin) == LOW) { | |
| criticalCondition = true; | |
| reason = "INA228 Alert Pin"; | |
| } | |
| } | |
| // Software checks as additional protection (always check, independent of alert pin) | |
| if (!criticalCondition) { | |
| if (_tempLimit > 0 && _lastTemperature >= _tempLimit) { | |
| criticalCondition = true; | |
| reason = "Firmware Check"; | |
| } else if (_powerLimit > 0 && _lastPower >= _powerLimit) { | |
| criticalCondition = true; | |
| reason = "Firmware Check"; | |
| } | |
| } | |
| if (criticalCondition) { | |
| // Store shutdown info on first trigger | |
| if (!_shutdownTriggered) { | |
| _shutdownTriggered = true; | |
| _shutdownReason = reason; | |
| _shutdownTemp = _lastTemperature; | |
| _shutdownPower = _lastPower; | |
| getTimeString(_shutdownTime); // Store formatted time string | |
| DEBUG_PRINTLN(F("INA228: EMERGENCY SHUTDOWN TRIGGERED!")); | |
| DEBUG_PRINT(F("INA228: Reason: ")); | |
| DEBUG_PRINTLN(reason); | |
| initiateBuzzerAlarm(); // Initiate the buzzer (non-blocking) | |
| } | |
| } | |
| // Enforce as long as shutdown is latched (not just while the instantaneous | |
| // condition holds) so software-threshold triggers stay enforced until | |
| // explicitly cleared via resetAlert(). | |
| if (_shutdownTriggered) { | |
| if (bri > 0) { | |
| DEBUG_PRINTLN(F("INA228: Enforcing emergency shutdown (LEDs were turned back on)")); | |
| briLast = bri; | |
| bri = 0; | |
| stateUpdated(CALL_MODE_DIRECT_CHANGE); | |
| } | |
| if (_fanEnabled && _fanPwmPin >= 0) { | |
| analogWrite(_fanPwmPin, 255); | |
| _lastFanDutyCycle = 255; | |
| _lastFanSpeedPercent = 100; | |
| } | |
| } | |
| // Note: Shutdown is NOT automatically cleared when conditions improve. | |
| // The ALERT pin is latched and must be cleared via the Reset Alert button. | |
| // This ensures the user acknowledges the event before resuming operation. | |
| } |
🤖 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/INA228_v2/INA228_v2.cpp` around lines 332 - 391, Emergency shutdown
in checkEmergencyShutdown() is only enforced while the current reading still
trips criticalCondition, so firmware-threshold shutdowns can stop being applied
after values recover. Update the logic in checkEmergencyShutdown() to drive
enforcement from the latched _shutdownTriggered flag (not just the instantaneous
temperature/power check), while still using criticalCondition to detect the
first trigger. Keep the existing first-trigger bookkeeping (_shutdownReason,
_shutdownTemp, _shutdownPower, getTimeString, initiateBuzzerAlarm) in the
trigger path, but ensure the bri=0/stateUpdated and fan full-speed actions
continue whenever _shutdownTriggered is true until the reset/clear path
explicitly releases it.
| void setup() override { | ||
| // Only initialize if enabled | ||
| if (!_settingEnabled) { | ||
| DEBUG_PRINTLN(F("INA228: Usermod is disabled, skipping initialization")); | ||
| return; | ||
| } | ||
|
|
||
| // INA228 will use the global I2C bus initialized by WLED | ||
| // Check if I2C pins are configured (i2c_sda and i2c_scl globals) | ||
| if (i2c_sda < 0 || i2c_scl < 0) { | ||
| DEBUG_PRINTLN(F("INA228: Global I2C pins not configured. Set them in Usermods setup.")); | ||
| return; | ||
| } | ||
|
|
||
| initializeINA228(); | ||
|
|
||
| // Initialize fan control if enabled | ||
| if (_fanEnabled && _fanPwmPin >= 0) { | ||
| if (PinManager::allocatePin(_fanPwmPin, true, PinOwner::UM_INA228)) { | ||
| pinMode(_fanPwmPin, OUTPUT); | ||
| analogWrite(_fanPwmPin, 0); // Start with fan off | ||
| DEBUG_PRINTLN(F("INA228: Fan PWM pin initialized")); | ||
| } else { | ||
| DEBUG_PRINTLN(F("INA228: Failed to allocate fan PWM pin")); | ||
| _fanPwmPin = -1; | ||
| _fanEnabled = false; | ||
| } | ||
| } | ||
|
|
||
| // Initialize tachometer pin if specified | ||
| if (_fanEnabled && _fanTachoPin >= 0) { | ||
| if (PinManager::allocatePin(_fanTachoPin, false, PinOwner::UM_INA228)) { | ||
| pinMode(_fanTachoPin, INPUT_PULLUP); | ||
| g_tachPulseCount = 0; // Reset global counter | ||
| attachInterrupt(_fanTachoPin, tachISR, FALLING); | ||
| DEBUG_PRINT(F("INA228: Fan tachometer pin initialized on GPIO ")); | ||
| DEBUG_PRINTLN(_fanTachoPin); | ||
| } else { | ||
| DEBUG_PRINT(F("INA228: Failed to allocate fan tachometer pin GPIO ")); | ||
| DEBUG_PRINTLN(_fanTachoPin); | ||
| } | ||
| } | ||
|
|
||
| // Initialize alert pin if configured | ||
| if (_shutdownEnabled && _alertPin >= 0) { | ||
| if (PinManager::allocatePin(_alertPin, false, PinOwner::UM_INA228)) { | ||
| pinMode(_alertPin, INPUT_PULLUP); // ALERT is active LOW, needs pullup | ||
| DEBUG_PRINTLN(F("INA228: Alert pin initialized")); | ||
| } else { | ||
| DEBUG_PRINTLN(F("INA228: Failed to allocate alert pin")); | ||
| _alertPin = -1; | ||
| } | ||
| } | ||
|
|
||
| // Initialize buzzer pin if configured | ||
| if (_buzzerPin >= 0) { | ||
| if (PinManager::allocatePin(_buzzerPin, true, PinOwner::UM_INA228)) { | ||
| pinMode(_buzzerPin, OUTPUT); | ||
| digitalWrite(_buzzerPin, LOW); // Ensure buzzer is off | ||
| DEBUG_PRINTLN(F("INA228: Buzzer pin initialized")); | ||
| } else { | ||
| DEBUG_PRINTLN(F("INA228: Failed to allocate buzzer pin")); | ||
| _buzzerPin = -1; | ||
| } | ||
| } | ||
|
|
||
| _initDone = true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Init failure gate — already flagged (see above); new: tach pin allocation failure leaves _fanTachoPin unreset.
Unlike the PWM pin (line 434), alert pin (line 460), and buzzer pin (line 472) — which all reset their pin member to -1 on PinManager::allocatePin() failure — the tachometer branch (lines 447-450) does not reset _fanTachoPin. Since attachInterrupt() was never called, g_tachPulseCount never increments, so calculateRPM(), the loop() debug block, and addToJsonInfo() will all treat the tach as configured and report a bogus/static 0 RPM instead of omitting the field.
🔧 Proposed fix
} else {
DEBUG_PRINT(F("INA228: Failed to allocate fan tachometer pin GPIO "));
DEBUG_PRINTLN(_fanTachoPin);
+ _fanTachoPin = -1;
}📝 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.
| void setup() override { | |
| // Only initialize if enabled | |
| if (!_settingEnabled) { | |
| DEBUG_PRINTLN(F("INA228: Usermod is disabled, skipping initialization")); | |
| return; | |
| } | |
| // INA228 will use the global I2C bus initialized by WLED | |
| // Check if I2C pins are configured (i2c_sda and i2c_scl globals) | |
| if (i2c_sda < 0 || i2c_scl < 0) { | |
| DEBUG_PRINTLN(F("INA228: Global I2C pins not configured. Set them in Usermods setup.")); | |
| return; | |
| } | |
| initializeINA228(); | |
| // Initialize fan control if enabled | |
| if (_fanEnabled && _fanPwmPin >= 0) { | |
| if (PinManager::allocatePin(_fanPwmPin, true, PinOwner::UM_INA228)) { | |
| pinMode(_fanPwmPin, OUTPUT); | |
| analogWrite(_fanPwmPin, 0); // Start with fan off | |
| DEBUG_PRINTLN(F("INA228: Fan PWM pin initialized")); | |
| } else { | |
| DEBUG_PRINTLN(F("INA228: Failed to allocate fan PWM pin")); | |
| _fanPwmPin = -1; | |
| _fanEnabled = false; | |
| } | |
| } | |
| // Initialize tachometer pin if specified | |
| if (_fanEnabled && _fanTachoPin >= 0) { | |
| if (PinManager::allocatePin(_fanTachoPin, false, PinOwner::UM_INA228)) { | |
| pinMode(_fanTachoPin, INPUT_PULLUP); | |
| g_tachPulseCount = 0; // Reset global counter | |
| attachInterrupt(_fanTachoPin, tachISR, FALLING); | |
| DEBUG_PRINT(F("INA228: Fan tachometer pin initialized on GPIO ")); | |
| DEBUG_PRINTLN(_fanTachoPin); | |
| } else { | |
| DEBUG_PRINT(F("INA228: Failed to allocate fan tachometer pin GPIO ")); | |
| DEBUG_PRINTLN(_fanTachoPin); | |
| } | |
| } | |
| // Initialize alert pin if configured | |
| if (_shutdownEnabled && _alertPin >= 0) { | |
| if (PinManager::allocatePin(_alertPin, false, PinOwner::UM_INA228)) { | |
| pinMode(_alertPin, INPUT_PULLUP); // ALERT is active LOW, needs pullup | |
| DEBUG_PRINTLN(F("INA228: Alert pin initialized")); | |
| } else { | |
| DEBUG_PRINTLN(F("INA228: Failed to allocate alert pin")); | |
| _alertPin = -1; | |
| } | |
| } | |
| // Initialize buzzer pin if configured | |
| if (_buzzerPin >= 0) { | |
| if (PinManager::allocatePin(_buzzerPin, true, PinOwner::UM_INA228)) { | |
| pinMode(_buzzerPin, OUTPUT); | |
| digitalWrite(_buzzerPin, LOW); // Ensure buzzer is off | |
| DEBUG_PRINTLN(F("INA228: Buzzer pin initialized")); | |
| } else { | |
| DEBUG_PRINTLN(F("INA228: Failed to allocate buzzer pin")); | |
| _buzzerPin = -1; | |
| } | |
| } | |
| _initDone = true; | |
| } | |
| void setup() override { | |
| // Only initialize if enabled | |
| if (!_settingEnabled) { | |
| DEBUG_PRINTLN(F("INA228: Usermod is disabled, skipping initialization")); | |
| return; | |
| } | |
| // INA228 will use the global I2C bus initialized by WLED | |
| // Check if I2C pins are configured (i2c_sda and i2c_scl globals) | |
| if (i2c_sda < 0 || i2c_scl < 0) { | |
| DEBUG_PRINTLN(F("INA228: Global I2C pins not configured. Set them in Usermods setup.")); | |
| return; | |
| } | |
| initializeINA228(); | |
| // Initialize fan control if enabled | |
| if (_fanEnabled && _fanPwmPin >= 0) { | |
| if (PinManager::allocatePin(_fanPwmPin, true, PinOwner::UM_INA228)) { | |
| pinMode(_fanPwmPin, OUTPUT); | |
| analogWrite(_fanPwmPin, 0); // Start with fan off | |
| DEBUG_PRINTLN(F("INA228: Fan PWM pin initialized")); | |
| } else { | |
| DEBUG_PRINTLN(F("INA228: Failed to allocate fan PWM pin")); | |
| _fanPwmPin = -1; | |
| _fanEnabled = false; | |
| } | |
| } | |
| // Initialize tachometer pin if specified | |
| if (_fanEnabled && _fanTachoPin >= 0) { | |
| if (PinManager::allocatePin(_fanTachoPin, false, PinOwner::UM_INA228)) { | |
| pinMode(_fanTachoPin, INPUT_PULLUP); | |
| g_tachPulseCount = 0; // Reset global counter | |
| attachInterrupt(_fanTachoPin, tachISR, FALLING); | |
| DEBUG_PRINT(F("INA228: Fan tachometer pin initialized on GPIO ")); | |
| DEBUG_PRINTLN(_fanTachoPin); | |
| } else { | |
| DEBUG_PRINT(F("INA228: Failed to allocate fan tachometer pin GPIO ")); | |
| DEBUG_PRINTLN(_fanTachoPin); | |
| _fanTachoPin = -1; | |
| } | |
| } | |
| // Initialize alert pin if configured | |
| if (_shutdownEnabled && _alertPin >= 0) { | |
| if (PinManager::allocatePin(_alertPin, false, PinOwner::UM_INA228)) { | |
| pinMode(_alertPin, INPUT_PULLUP); // ALERT is active LOW, needs pullup | |
| DEBUG_PRINTLN(F("INA228: Alert pin initialized")); | |
| } else { | |
| DEBUG_PRINTLN(F("INA228: Failed to allocate alert pin")); | |
| _alertPin = -1; | |
| } | |
| } | |
| // Initialize buzzer pin if configured | |
| if (_buzzerPin >= 0) { | |
| if (PinManager::allocatePin(_buzzerPin, true, PinOwner::UM_INA228)) { | |
| pinMode(_buzzerPin, OUTPUT); | |
| digitalWrite(_buzzerPin, LOW); // Ensure buzzer is off | |
| DEBUG_PRINTLN(F("INA228: Buzzer pin initialized")); | |
| } else { | |
| DEBUG_PRINTLN(F("INA228: Failed to allocate buzzer pin")); | |
| _buzzerPin = -1; | |
| } | |
| } | |
| _initDone = true; | |
| } |
🤖 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/INA228_v2/INA228_v2.cpp` around lines 410 - 477, The tachometer
setup path in setup() leaves _fanTachoPin configured even when
PinManager::allocatePin() fails, unlike the PWM, alert, and buzzer branches.
Update the fan tach allocation failure handling in INA228_v2::setup() to reset
_fanTachoPin to -1 (and keep _fanEnabled consistent if needed) so later logic in
calculateRPM(), the loop() debug output, and addToJsonInfo() does not treat a
failed tach input as a valid sensor and report a stale 0 RPM value.
3 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
New Features
Documentation