[ld6002b] Add target sensors (2/5) - #17820
Conversation
|
To use the changes from this PR as an external component, add the following to your ESPHome configuration YAML file: external_components:
- source: github://pr#17820
components: [ld6002b]
refresh: 1h(Added by the PR bot) |
There was a problem hiding this comment.
📦 Pull Request Size
Hey @hepter, thanks for the contribution! Just a heads up, this PR is on the large side (1104 line changes excluding tests), which makes it harder for maintainers to review.
Smaller, focused PRs tend to be reviewed much faster since they fit into the short gaps between other maintainer work; large ones often have to wait for a rare long uninterrupted block of time. If you can break this up into smaller pieces that can be reviewed independently, it will almost certainly land faster overall.
Before putting more time in, it's also worth popping into #devs on Discord so we can help you scope things and flag anything already in flight.
For more details (including how to split the work up), see: https://developers.esphome.io/contributing/submitting-your-work/#how-to-approach-large-submissions
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #17820 +/- ##
=======================================
Coverage 87.28% 87.28%
=======================================
Files 64 64
Lines 14697 14697
Branches 2217 2217
=======================================
Hits 12828 12828
Misses 1560 1560
Partials 309 309 🚀 New features to boost your workflow:
|
Protocol (per the HLK-LD6002B communication protocol V1.2): - Rework command ACK correlation. The module acknowledges with a frame carrying its own counter (frame IDs are per-peer and every message type is unidirectional), so matching our TX id could never succeed. ACKs are matched by type in transmission order: an ACK arriving while its command is still active completes it, and leftover ACKs from superseded retry attempts are absorbed so they cannot complete the next command. - Give the first attempt a wider ACK budget (600 ms): the first frame sent to a sleeping module doubles as its wake-up and is consumed unanswered, so the wake must not cascade into extra retries. - Verify the header checksum before acting on the length field when discarding oversized frames; a corrupted length byte now costs 8 bytes of resync instead of up to 65537. Reliability and RAM: - Allocate the RX buffer once in setup() via RAMAllocator with a real failure check. - Stop heap-allocating per command send in the wake-up path; stage untracked payloads in a fixed scratch buffer. - Right-size command slots to the largest protocol payload (28 bytes) and document why the queue depth stays at 16. - Drop manual publish de-duplication (binary_sensor already does it), clamp the wire-reported target count, and bounds-check indexed setters. Config and logging: - Rename wakeup_pulse_ms to wakeup_pulse and require wakeup_pin when auto_wake or wakeup_pulse is configured. - Move max_data_len (and helpers only used by later parts) into the parts that use them; cap max_data_len at 8192. - Cap low_power_sleep_time at 500 ms per the datasheet recommendation. - Remove the boot log line; expand dump_config with the wake pin, pulse, resolved buffer size and per-target sensors. - Log retries for non-control command types too. Tests: - Include common.yaml via dict-style packages in the test fixtures. All of the above validated on real hardware (ESP32 intercom + LD6002B): full settings matrix, area write/readback flows, reboot soak with the boot sequence sending each command exactly once, and zero component warnings or errors in the captured logs.
Move the DISCARD decrement out of the condition and apply De Morgan to the stale-ACK guard. Behaviour unchanged.
Co-authored-by: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com>
Review dismissed: All requirements have been met
|
@esphbot review |
Memory Impact AnalysisComponents:
📊 Component Memory Breakdown
🔍 Symbol-Level Changes (click to expand)Changed Symbols
New Symbols (top 15)
Removed Symbols (top 15)
This analysis runs automatically when components change. Memory usage is measured from a representative test configuration. |
There was a problem hiding this comment.
Pull request overview
Adds the sensor platform and supporting hub plumbing for the new ld6002b UART presence radar component, enabling per-target coordinate-related sensors plus target/point counts and point-cloud stream handling.
Changes:
- Add
ld6002bsensor platform schema/codegen for target sensors (x/y/z/doppler/cluster) andtarget_count/point_count. - Extend the hub C++ implementation to publish the new sensors, handle point-cloud report frames, and adjust UART frame buffer sizing based on point-cloud usage.
- Update component test YAML to exercise the new sensors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/components/ld6002b/common.yaml | Adds sensor platform configuration to the component’s test setup. |
| esphome/components/ld6002b/sensor.py | New sensor platform schema and codegen wiring to the hub setters. |
| esphome/components/ld6002b/ld6002b.h | Adds sensor pointers/state tracking, throttling fields, and point-cloud handler declarations. |
| esphome/components/ld6002b/ld6002b.cpp | Implements publish de-dup/throttling, buffer sizing, target sensor publishes, and point-cloud count handling. |
| esphome/components/ld6002b/const.py | Adds component-local config keys used by the new sensor platform and hub options. |
| esphome/components/ld6002b/init.py | Adds max_data_len and throttle hub options and passes them into the C++ component. |
| cv.Optional(CONF_MAX_DATA_LEN): cv.int_range(min=256, max=8192), | ||
| cv.Optional( | ||
| CONF_THROTTLE, default="1000ms" | ||
| ): cv.positive_time_period_milliseconds, | ||
| } |
| if (this->should_throttle_stream_(this->last_point_publish_)) { | ||
| return; | ||
| } | ||
| this->handle_point_cloud_(data, len); | ||
| break; |
|
|
There was a problem hiding this comment.
Warning
Important issues found.
- Hub-level
throttlere-implements sensor filters the project removed from ld2450 - Configuring
point_countturns on a full point-cloud stream (4 KB frames on a 115200 link) to publish one integer
|
Reworked, throttle is gone in favor of the ld2450 style filter defaults and point cloud moved to part 3 behind its switch. Kept KEY_TARGET_COUNT as is, the CONF_ rename trips the repo lint since ld2450 and rd03d already define the same constant, moving all three to the shared const file is a separate PR. |
Rework confirmed. Checked On Remaining items all non-blocking. One worth doing before release: rename |
|
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
Lets create that PR then. |
|
@esphbot review |
Done, landed already. #18120 moved One housekeeping note: part 1 (#17819) also merged, so the "stacked on #17819" line in the body reads stale now. A rebase onto |
PR Review — [ld6002b] Add target sensors (2/5)Merge-ready. Every finding from the previous round is fixed, and the new code holds up under a fresh trace of the frame path. What is genuinely well done here:
Remaining points, all non-blocking:
✅ Resolved since last review (4)Previously-flagged issues verified fixed
🟢 Suggestions
1. Per-target publish/clear block is 54 lines of repetition with an empty if/else in non-sensor builds
|
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
What does this implement/fix?
Part 2 of 5 of the HLK-LD6002B 60GHz presence radar component (split from #13532). Depends on #17819.
Adds the
sensorplatform on top of the hub:target_counttimeout+throttle_with_priorityfilters on the value sensors, matchingld2450(NaN transitions publish immediately)Types of changes
Pull request in esphome.io with documentation (if applicable):
Test Environment
Example entry for
config.yaml:Checklist:
tests/folder).If user exposed functionality or configuration variables are added/changed: