-
Notifications
You must be signed in to change notification settings - Fork 9
Merge beta to main which brings in fixes for ld2412 and ld2450 stuck targets #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… id names discord user Mistar Muffin pointed out the names were not specific on ld2450 presence target entities. I edited the names to match the same as LD2412 names and edited each entity ID for both ld2412/ld2450 to more accurately reflect the name of the entities. https://discord.com/channels/1126966963206361199/1354841614178062417/1418203837310963712
…me-fixes update ld2412 and ld2450 target entities with better names and better id names
Removed throttle filters from various target counts and coordinates for LD2450. Matches bdraco's suggested fix here: bdraco#1 likely needs to be done on LD2412 entities as well as an esphome issue raised - will test and open issue with esphome first.
fix copy paste mistake changing the ld2450 radar entitiy id and name
Remove throttle filters from LD2450 targets
Removed throttle filters from various LD2412 metrics.
…xes are not present before this version
…xes are not present before this version
Remove throttle filters from LD2412 metrics
WalkthroughESPHome configuration files updated with version 25.11.20.1, LD2450 binary sensor identifiers and names standardized with explicit ID prefixes, throttle filters removed for performance optimization, and minimum ESPHome version requirement increased to 2025.11.0 across device variants. Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Integrations/ESPHome/Core.yaml (1)
1-884: PR description is empty; missing migration guide and test results.The PR body uses the repository's default template with no additional context. To safely merge a breaking change, provide:
- Detailed description of the fixes for LD2412 and LD2450 "stuck targets."
- Test results confirming the fixes work as intended.
- Migration guide for users affected by entity ID changes (see related comments above).
- Changelog entry documenting the breaking changes and version requirements.
- Rationale for throttle removal and performance implications.
Would you like help drafting a migration guide or changelog entry documenting these breaking changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Integrations/ESPHome/Core.yaml(4 hunks)Integrations/ESPHome/R_PRO-1_ETH.yaml(1 hunks)Integrations/ESPHome/R_PRO-1_W.yaml(1 hunks)
🔇 Additional comments (5)
Integrations/ESPHome/R_PRO-1_ETH.yaml (1)
14-14: Version bump aligns with R_PRO-1_W.yaml; pending upstream validation.This change mirrors the version bump in R_PRO-1_W.yaml. Once the ESPHome 2025.11.0 version is confirmed, both device configs will be consistent.
Integrations/ESPHome/Core.yaml (3)
569-569: Clarify scope and rationale for throttle filter removal.The AI summary indicates that throttle filters have been removed from various sensor definitions (including LD2450 presence/target counts and LD2412 gate energies), but the provided code snippet only shows a blank line at line 569. The extent of throttle removals is not fully visible.
Questions:
- How many throttle filters were removed? (all LD2450 and LD2412 sensors, or specific ones?)
- What is the rationale? Is this to fix the "stuck targets" issue, or a separate performance optimization?
- What are the performance/resource implications? (more frequent updates = higher CPU/memory usage)
Please clarify the throttle removal strategy and confirm it directly addresses the "stuck targets" issue mentioned in the PR title.
340-340: Theid: ink_ha_connectedfield is for internal ESPHome component references, not Home Assistant entity references. Home Assistant automations and templates reference entities by theirentity_id(e.g.,binary_sensor.online), which is derived from the sensor's platform and name—not the ESPHomeid. Adding or modifying an explicit ESPHome ID does not break existing HA automations or templates.Likely an incorrect or invalid review comment.
2-2: Clarify internal firmware versioning — 25.11.20.1 is not an official ESPHome release.The version string "25.11.20.1" is ApolloAutomation internal device firmware versioning, not an ESPHome release. Official ESPHome versions follow the format 2025.9.0, 2025.10.0, etc., and 25.11.20.1 does not exist in ESPHome's official changelog. Add documentation to Core.yaml explaining this is a device firmware build version and its relationship to the underlying ESPHome version (currently targeting ESPHome 2025.8.0 based on recent fixes).
Likely an incorrect or invalid review comment.
Integrations/ESPHome/R_PRO-1_W.yaml (1)
14-14: No action required. ESPHome version 2025.11.0 is a stable, officially released version with published changelog and migration documentation. The version constraint update is valid.
Version:
What does this implement/fix?
Types of changes
Checklist / Checklijst:
If user-visible functionality or configuration variables are added/modified:
Summary by CodeRabbit
Chores
Updates
✏️ Tip: You can customize this high-level summary in your review settings.