Conversation
WalkthroughImplements defensive fixes and indexing corrections in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/display/core/predictive.h (1)
4-5: Include what you use.This header uses
std::vectorandstd::powbut doesn’t include their headers. Avoid relying on transitive includes.#include <Arduino.h> +#include <vector> +#include <cmath>
🧹 Nitpick comments (4)
src/display/core/predictive.h (4)
45-47: Indexing fix is correct; avoid pow in the hot loop.Use precomputed deltas and multiplication to reduce floating-point overhead on MCU targets.
- tdev_vdev += (measurementTimes[j] - t_mean) * (measurements[j] - v_mean); - tdev2 += pow(measurementTimes[j] - t_mean, 2.0); + const double dt = measurementTimes[j] - t_mean; + const double dv = measurements[j] - v_mean; + tdev_vdev += dt * dv; + tdev2 += dt * dt;
48-53: Near-zero denominator guard is good; name the threshold.Minor: give the magic number a name for readability and easier tuning.
- // Prevent division by zero - if (tdev2 < 1e-10) { + // Prevent division by near-zero denominator (units: ms^2) + constexpr double kTdev2Epsilon = 1e-10; + if (tdev2 < kTdev2Epsilon) { return 0.0; }
54-56: Comment mismatch on units.The code returns volume/ms; the comment suggests seconds. Align the comment to avoid confusion.
- double volumePerMilliSecond = tdev_vdev / tdev2; // the slope (volume per millisecond) of the linear best fit - return volumePerMilliSecond > 0 ? volumePerMilliSecond : 0.0; // return 0 if it is not positive, convert to seconds + double volumePerMilliSecond = tdev_vdev / tdev2; // slope (volume per millisecond) of the linear best fit + return volumePerMilliSecond > 0 ? volumePerMilliSecond : 0.0; // return 0 if it is not positive (units: volume/ms)
62-70: Division-by-zero guard is correct; clarify negative overshoot behavior.If actual < expected, result is negative ms. Is that intended? Consider clamping to 0 for “overshoot” semantics.
double rate = getRate(measurementTimes.back()); // Prevent division by zero if (rate < 1e-10) { return 0.0; } - - return overshoot / rate; + // If actual <= expected, there is no overshoot to adjust. + if (overshoot <= 0.0) { + return 0.0; + } + return overshoot / rate;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/display/core/predictive.h(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (1)
src/display/core/predictive.h (1)
25-27: Good underflow safeguard in cutoff scan.The added
i > 0guard prevents size_t wraparound and out-of-bounds access when the window excludes all points. LGTM.
… when encountered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/display/core/predictive.h (2)
42-53: Indexing fix and tdev2 guard look good; consider tiny micro-optimizationUsing
jconsistently in the accumulation loop fixes the earlier indexing bug, and thetdev2 < 1e-10guard avoids division by (near) zero in degenerate cases, which is the right defensive behavior here.If you care about tight performance on MCU targets, you could avoid
powand reuse the time delta:- for (size_t j = i; j < measurements.size(); j++) { - tdev_vdev += (measurementTimes[j] - t_mean) * (measurements[j] - v_mean); - tdev2 += pow(measurementTimes[j] - t_mean, 2.0); - } + for (size_t j = i; j < measurements.size(); j++) { + const double dt = measurementTimes[j] - t_mean; + tdev_vdev += dt * (measurements[j] - v_mean); + tdev2 += dt * dt; + }Purely optional, but a bit lighter than
powin a tight loop.
57-79: Robust overshoot adjustment; confirm undershoot behavior and header dependenciesThe new early return on
measurementTimes.size() < 2, the cachedrate, and guards onrateandadjust(including NaN/Inf and negativity) make this method much safer against bad data and arithmetic issues.Two points to double‑check:
Undershoot semantics: When
actualVolume < expectedVolume,overshootbecomes negative,adjustbecomes negative, and you now treat that as invalid and return0.0(with a warning). If the intended behavior is “only correct positive overshoot”, this is fine; if you ever want to compensate undershoot (e.g., extend run time), you’ll need a separate code path instead of discarding it.Logging / math dependencies: This header now relies on
ESP_LOGW,isnan, andisinf. Please ensure every translation unit includingpredictive.hhas the appropriate logging and math headers included (or include them here) so the header remains self‑contained and consistently compilable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/display/core/predictive.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy
- GitHub Check: test
🔇 Additional comments (1)
src/display/core/predictive.h (1)
23-31: Boundary check in cutoff loop correctly prevents underflow/OOB accessAdding
i > 0to the loop condition makes the backward scan safe when all timestamps are newer thancutoff, and the subsequentmeasurements.size() - i < 2guard ensures you only fit when there are at least two in-window points. The window selection logic still matches the comment about “first entry after the cutoff”.
* Fix incorrenct indexing * fix * Added checks for invalid rate and adjustment values, logging warnings when encountered. --------- Co-authored-by: Jochen Niebuhr <kontakt@ju-hh.de>
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.