Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Awesome! I am slowly working through my backlog - I'll hopefully be able to do a real review next week. |
|
I like the idea |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@usermods/UM_SensorDummy/UM_SensorDummy.cpp`:
- Around line 30-37: The getSensor implementation has inverted logic and lacks
bounds checking: change getSensor to first validate index (< getSensorCount())
and return nullptr for out-of-range indices, then map index 0 to the SEF sensor
(this) and index 1 to the weather sensor (_sensorArray); update the method
(getSensor) to use explicit checks or a switch so indices >=2 cannot incorrectly
return this.
In `@wled00/Sensor.cpp`:
- Around line 119-127: The switch in Sensor::something handling SensorValueType
has a fall-through: after executing visitor.visit(*_struct) in the
SensorValueType::Struct case it omits a break and unintentionally falls into
SensorValueType::Whatever; add a break statement after the
visitor.visit(*_struct) call in the SensorValueType::Struct case so execution
stops there and does not call visitor.visit(_whatever).
🧹 Nitpick comments (5)
usermods/Internal_Temperature_v2/Internal_Temperature_v2.cpp (1)
182-183: Consider validating the index parameter ingetSensor.The
getSensor(uint8_t)override ignores the index parameter and always returnsthis. WhilegetSensorCount()returns 1, a consumer passing an invalid index would still get a valid sensor pointer.For consistency with the pattern in
UM_SensorDummy(which checks index), consider returningnullptrfor invalid indices:♻️ Suggested fix
- Sensor *getSensor(uint8_t) override { return this; } + Sensor *getSensor(uint8_t index) override { return index == 0 ? this : nullptr; }usermods/UM_SensorInfo/UM_SensorInfo.cpp (1)
101-103: Remove unusedpropsvariable.The
propsvariable on line 101 is fetched but never used; the actual property access happens via a separate call on line 103. This pattern repeats inmode_SEF_all()(lines 136-138) andmode_Fluctuations()(lines 178-180).♻️ Suggested cleanup
const bool isReady = channel.isReady(); if (!isReady) continue; - const auto &props = channel.getProps(); const float sensorValue = channel.getValue(); const float sensorValueMax = channel.getProps().rangeMax;Apply similar changes to lines 136 and 178.
wled00/Sensor.h (3)
311-318: Silent index clamping to channel 0 may hide bugs.When an invalid
channelIndexis passed (≥channelCount()), the code silently uses channel 0 instead. This behavior inset(),do_isSensorChannelReady(),do_getSensorChannelValue(), anddo_getSensorChannelProperties()could mask programming errors.Consider whether an assertion or returning a sentinel/error would be more appropriate for a debug/development context:
♻️ Alternative approach for debugging
void set(uint8_t channelIndex, SensorValue val) { - if (channelIndex >= channelCount()) - channelIndex = 0; + if (channelIndex >= channelCount()) { + // Consider: DEBUG_PRINTF("EasySensorBase: invalid channelIndex %u\n", channelIndex); + return; // or assert, depending on desired behavior + } _channelValues[channelIndex] = val;This would make integration bugs more visible during development.
54-57: Implicitoperator bool()may cause unexpected behavior.The implicit
operator bool()callsas_bool(), which returnsfalsefor any non-BoolSensorValue(includingFloat(1.0f)orInt32(42)). This could lead to confusing behavior:SensorValue v(42); // Int32 if (v) { /* never executed - v converts to false */ }Consider making the bool conversion explicit, or documenting this behavior prominently:
♻️ Suggested fix
- operator bool() const { return as_bool(); } + explicit operator bool() const { return as_bool(); }Or alternatively, have
operator bool()return whether the value is "truthy" based on its actual type.
385-388: Default-initialized_valbecomesWhatever(nullptr).The
SensorValue _val;member is default-initialized viaexplicit SensorValue(const void *val = nullptr), resulting in typeWhateverwith a null pointer. This is fine if sensors always callset()before being read, but consumers might get unexpectedWhatevervalues from uninitialized sensors.Consider initializing with a sensible default matching the expected channel type, or documenting this behavior.
| uint8_t getSensorCount() override { return 2; } | ||
|
|
||
| Sensor *getSensor(uint8_t index) override | ||
| { | ||
| if (index != 0) | ||
| return this; | ||
| return &_sensorArray; | ||
| } |
There was a problem hiding this comment.
The getSensor index handling is inverted and potentially buggy.
The current logic returns:
_sensorArray(Weather Station) whenindex == 0this(SEF sensor) whenindex != 0(including 1, 2, 3, etc.)
This means any index ≥ 2 also returns the SEF sensor, which is inconsistent with getSensorCount() returning 2. Consider explicit bounds checking:
🐛 Proposed fix
Sensor *getSensor(uint8_t index) override
{
- if (index != 0)
- return this;
- return &_sensorArray;
+ if (index == 0)
+ return &_sensorArray;
+ if (index == 1)
+ return this;
+ return nullptr;
}🤖 Prompt for AI Agents
In `@usermods/UM_SensorDummy/UM_SensorDummy.cpp` around lines 30 - 37, The
getSensor implementation has inverted logic and lacks bounds checking: change
getSensor to first validate index (< getSensorCount()) and return nullptr for
out-of-range indices, then map index 0 to the SEF sensor (this) and index 1 to
the weather sensor (_sensorArray); update the method (getSensor) to use explicit
checks or a switch so indices >=2 cannot incorrectly return this.
There was a problem hiding this comment.
@coderabbitai Added comments that invalid indices result in undefined behaviour (just like out of bound array access) -- thus anything can happen. In those cases, I prefer "the UB" to be a valid (although unexpected) sensor over a nullptr to avoid crashes.
So unexperienced developers can recognize that something is not working as expected, which is more helpful and less frustrating than crashes. And it avoids range checks that are not really beneficial.
Aside from that, do we have adequate assert macros in WLED? Those would be appropriate here, assuming that debug builds are a relevant thing at all.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for assert macros and debug assertions in WLED codebase
echo "=== Searching for assert definitions and usage ==="
rg -n --type cpp --type c -C2 '#define.*assert' | head -50
echo -e "\n=== Searching for assert usage patterns ==="
rg -n --type cpp --type c -C1 '\bassert\s*\(' | head -30
echo -e "\n=== Searching for DEBUG or debug build patterns ==="
rg -n --type cpp --type c -C2 '#ifdef.*DEBUG|#if.*DEBUG' | head -40
echo -e "\n=== Checking for WLED-specific assertion patterns ==="
rg -n --type cpp --type c 'WLED_ASSERT|DEBUG_PRINT.*assert|USER_PRINT.*assert' | head -20
Length of output: 2844
🏁 Script executed:
#!/bin/bash
# Check for standard assert.h includes and usage patterns
echo "=== Checking for assert.h includes ==="
rg -n --type cpp --type c '#include.*assert\.h'
echo -e "\n=== Looking at the assert usage in um_manager.cpp ==="
cat -n wled00/um_manager.cpp | sed -n '90,105p'
echo -e "\n=== Checking for DEBUG_PRINT and other debug macros ==="
rg -n --type cpp --type c '#define.*DEBUG_PRINT' | head -20
echo -e "\n=== Checking const.h for debug-related defines ==="
fd -e h const.h --exec cat {} | head -100
Length of output: 6503
@JoaDick, I completely understand your rationale—returning a valid sensor provides more graceful degradation than nullptr crashes, especially helpful for learning developers who can see "wrong behavior" rather than hitting a segfault.
Based on my search of the codebase, here's what's available for assertions:
Standard C assert(): Available and already used in WLED. For example, in wled00/um_manager.cpp:96:
assert(!oappend_shim);Debug builds: WLED supports debug builds via the WLED_DEBUG flag, which enables:
DEBUG_PRINT(x),DEBUG_PRINTLN(x),DEBUG_PRINTF(x...)macros (defined inwled00/wled.h)- These compile to no-ops when
WLED_DEBUGis not defined - Additional specialized flags like
WLED_DEBUG_MATHfor specific subsystems
No WLED-specific assert macro currently exists. If you want consistent assertion behavior across the codebase, you could consider adding something like:
`#ifdef` WLED_DEBUG
`#define` WLED_ASSERT(condition) assert(condition)
`#else`
`#define` WLED_ASSERT(condition) ((void)0)
`#endif`However, given your preference for graceful degradation and the minimal use of assertions in the current codebase, documenting the UB behavior in comments (as you've done) seems like a pragmatic approach that aligns well with WLED's design philosophy.
✏️ Learnings added
Learnt from: JoaDick
Repo: wled/WLED PR: 5344
File: usermods/UM_SensorDummy/UM_SensorDummy.cpp:30-37
Timestamp: 2026-02-14T22:28:29.696Z
Learning: In WLED sensor implementations (like UM_SensorDummy.cpp), when getSensor() receives out-of-bounds indices, the preferred behavior is to return a valid (though unexpected) sensor rather than nullptr. This prevents crashes and helps unexperienced developers recognize incorrect behavior rather than encountering frustrating segfaults. Bounds checking can be skipped in favor of graceful degradation, with UB documented in comments.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Learnt from: JoaDick
Repo: wled/WLED PR: 5319
File: usermods/UM_PluginDemo/UM_PluginDemo.cpp:62-86
Timestamp: 2026-01-26T00:04:16.470Z
Learning: In WLED (wled00/FX_fcn.cpp), Segment::setPixelColor() performs internal bounds checking, returning early if the index is negative or exceeds vLength(). No additional guards are needed when calling this function, even with potentially out-of-range indices.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:146-148
Timestamp: 2026-01-01T07:19:40.244Z
Learning: In the WLED Ants effect (usermods/user_fx/user_fx.cpp), the color alternation logic `antIndex % 3 == 1` is intentional and creates a 1/3 vs 2/3 distribution between SEGCOLOR(0) and SEGCOLOR(2). This unbalanced alternation is by design and should not be changed to a balanced mod 2 alternation.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/data/index.js:2406-2413
Timestamp: 2025-09-28T09:53:42.670Z
Learning: In WLED, code paths that appear unreachable may actually be safety fallbacks for low-heap conditions. These fallbacks are intentional defensive programming measures to ensure UI functionality is preserved when system resources are constrained.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:22:20.525Z
Learning: In WLED's image_loader.cpp and similar contexts, when suggesting cleanup or de-allocation code in error handlers, first verify whether the underlying library or function already handles cleanup after failure. For example, `new` expressions that throw exceptions do not assign partial pointers and automatically clean up storage, making explicit dealloc() calls in catch blocks unnecessary. Only suggest "safety" cleanup when the library/function's documented semantics do not already provide graceful failure handling.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
|
From a quick architectural overview:
|
3 participants
@willmmiles Good news: The PoC for a generic handling of sensor readings that we discussed in #5319 turned out to be less complicated than initially expected!
Please try out the examples in
UM_SensorInfoandUM_SensorDummyto see if this is going in the right direction.I've also adjusted
DHT,TemperatureandInternal_Temperature_v2, but haven't been able to test it in-depth (my boards seem to be a bit shaky).Concept & Features
Sensor...SensorChannel...SensorValue, which can represent one specific datatype (float,int,bool, ...).SensorChannelPropertiesconsist of:SensorValue's (typical) minimum range of operation.SensorValue's (typical) maximum range of operation.Temperature,Voltage, "NotKnownYet", ...°C,V, "WTF", ...SensorValuefor sensor readings:boolfloatint32_tuint32_tvoid*- although not recommended)ValueType==float, or quantity = "Temperature".And don't forget to look at the Info-Page!
Summary by CodeRabbit
New Features
Bug Fixes