Add configurable update interval for LTR390 sensor#72
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdded interval-driven update control for the LTR390 sensor: a global uint32_t timestamp tracks last update, a public template number configures the update cadence (1–300s, default 60s), the sensor's automatic updates are disabled, and a 1s interval block conditionally triggers sensor refreshes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
Integrations/ESPHome/Core.yaml (1)
154-167: Please confirmdisabled_by_defaultis intentional for this new user-facing control.Line 157 disables the new interval entity by default, which can make the feature harder to discover.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Integrations/ESPHome/Core.yaml` around lines 154 - 167, Review whether the template sensor "LTR390 Update Interval" (id: ltr390_update_interval) should be user-visible by default; if disabling by default was unintended, remove the disabled_by_default: true line (or set it to false) so the new entity is enabled and discoverable, otherwise leave it and add a comment in the YAML documenting the intentional reason for keeping disabled_by_default on this entity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Integrations/ESPHome/Core.yaml`:
- Around line 729-745: The interval parsing is brittle because
id(ltr390_update_interval).state may be NAN at boot and the first-poll check
uses an unnecessary current_time > 0 guard; update the lambda to read the float
configured_interval = id(ltr390_update_interval).state, use
isnan(configured_interval) and configured_interval >= 1.0f to decide the
interval (fallback to a safe default like 60s), cast that to uint32_t for
interval, and remove the current_time > 0 check so the immediate boot update
triggers when id(ltr390_last_update) == 0; ensure you still call
id(ltr_390).update() and set id(ltr390_last_update) = current_time as before.
---
Nitpick comments:
In `@Integrations/ESPHome/Core.yaml`:
- Around line 154-167: Review whether the template sensor "LTR390 Update
Interval" (id: ltr390_update_interval) should be user-visible by default; if
disabling by default was unintended, remove the disabled_by_default: true line
(or set it to false) so the new entity is enabled and discoverable, otherwise
leave it and add a comment in the YAML documenting the intentional reason for
keeping disabled_by_default on this entity.
2 participants
Version: 26.3.2.1
What does this implement/fix?
Ports the configurable LTR390 update interval from MSR-2 (PR #56) to MSR-1.
ltr390_last_updateglobal to track last poll timestampLTR390 Update Intervalnumber entity (1–300s, default 60s, CONFIG category, persists across reboots)update_interval: neveron the LTR390 sensor to disable auto-pollingTypes of changes
Checklist / Checklijst:
If user-visible functionality or configuration variables are added/modified:
Summary by CodeRabbit