[ld2420] Address third review round

Guarantee a drain pass in every listen phase so bytes that arrived
before it can never satisfy the settle window even when the main loop
stalls, publish the number entities on the give-up path, zero the
reply data before each startup send so short acks cannot store stale
values, zero initialize the config threshold arrays, refuse config
writes when the configuration was never read, guard the revert button
during startup, gate the apply and factory reset warning clear on the
final ack, report the firmware version as unknown in dump_config until
it is read, and add tests for the command resend and the sequence
retry and give-up paths.
This commit is contained in:
J. Nick Koston
2026-08-16 21:12:58 -05:00
parent 73a95c411d
commit d6179b6d56
5 changed files with 485 additions and 10 deletions
+46 -8
View File
@@ -197,10 +197,13 @@ static int32_t get_firmware_int(const char *version_string) {
}
void LD2420Component::dump_config() {
// Setup no longer blocks, so the config dump usually runs before the
// version is read; do not present the "v0.0.0" placeholder as real
const int32_t firmware = ld2420::get_firmware_int(this->firmware_ver_);
ESP_LOGCONFIG(TAG,
"LD2420:\n"
" Firmware version: %7s",
this->firmware_ver_);
firmware > 0 ? this->firmware_ver_ : "unknown");
#ifdef USE_NUMBER
ESP_LOGCONFIG(TAG, "Number:");
LOG_NUMBER(" ", "Gate Timeout:", this->gate_timeout_number_);
@@ -222,7 +225,6 @@ void LD2420Component::dump_config() {
ESP_LOGCONFIG(TAG, "Select:");
LOG_SELECT(" ", "Operating Mode", this->operating_selector_);
#endif
const int32_t firmware = ld2420::get_firmware_int(this->firmware_ver_);
if (firmware > 0 && firmware < CALIBRATE_VERSION_MIN) {
ESP_LOGW(TAG, "Firmware version %s and older supports Simple Mode only", this->firmware_ver_);
}
@@ -240,6 +242,7 @@ void LD2420Component::begin_startup_() {
void LD2420Component::begin_listen_() {
this->rx_seen_ = false;
this->listen_drained_ = false;
this->buffer_pos_ = 0;
this->phase_start_ms_ = millis();
this->startup_state_ = StartupState::STARTUP_STATE_LISTEN;
@@ -293,6 +296,9 @@ void LD2420Component::send_startup_cmd_() {
this->startup_cmd_ = (uint8_t) frame.command;
this->cmd_reply_.ack = false;
this->cmd_reply_.error = 0;
// A short reply acks without filling every data word; zero them so stale
// values from the previous command cannot be stored as this command's data
memset(this->cmd_reply_.data, 0, sizeof(this->cmd_reply_.data));
this->write_cmd_frame_(frame);
this->phase_start_ms_ = millis();
}
@@ -342,6 +348,11 @@ bool LD2420Component::startup_ack_check_() {
// will not publish sensor data either.
ESP_LOGE(TAG, "Firmware version and operating mode were never read");
}
#ifdef USE_NUMBER
// Publish whatever was read before giving up so the number entities show
// values next to the warning status instead of staying unknown forever
this->init_gate_config_numbers();
#endif
this->status_set_warning(ESP_LOG_MSG_COMM_FAIL);
this->startup_state_ = StartupState::STARTUP_STATE_RUNNING;
return false;
@@ -368,13 +379,17 @@ void LD2420Component::loop_startup_() {
// stale data buffered before setup, or the ack to the blind config mode
// exit. Ignore reception during a short settle window (clearing the rx
// flag and the frame parser) so only data the module sends afterwards
// counts as proof that it is up and streaming. (A full-frame check
// counts as proof that it is up and streaming. The listen_drained_ flag
// guarantees at least one such drain pass even when the main loop
// stalls past the whole window, so bytes that arrived before the listen
// phase can never be mistaken for fresh data. (A full-frame check
// cannot serve as that proof here: old-firmware text frames are only
// recognized once the operating mode is known, which requires the very
// handshake this phase gates.)
if (elapsed < STARTUP_LISTEN_SETTLE_MS) {
if (elapsed < STARTUP_LISTEN_SETTLE_MS || !this->listen_drained_) {
this->drain_rx_();
this->rx_seen_ = false;
this->listen_drained_ = true;
return;
}
// The module locks up until power cycled if it receives data before it
@@ -485,6 +500,12 @@ void LD2420Component::apply_config_action() {
ESP_LOGW(TAG, "Module is still starting up; ignoring");
return;
}
if (ld2420::get_firmware_int(this->firmware_ver_) == 0) {
// Setup gave up before the configuration was ever read; writing the
// unread config to the module's NVM would wipe its stored thresholds
ESP_LOGW(TAG, "Module configuration was never read; ignoring");
return;
}
const uint8_t checksum = calc_checksum(&this->new_config, sizeof(this->new_config));
if (checksum == calc_checksum(&this->current_config, sizeof(this->current_config))) {
ESP_LOGD(TAG, "No configuration change detected");
@@ -506,9 +527,15 @@ void LD2420Component::apply_config_action() {
this->init_gate_config_numbers();
#endif
this->set_system_mode(this->system_mode_);
this->set_config_mode(false); // Disable config mode to save new values in LD2420 nvm
// Disable config mode to save new values in LD2420 nvm. The individual
// write commands do not report errors, so use the final ack as the best
// available signal before reporting the reconfiguration as healthy.
if (this->set_config_mode(false) == LD2420_ERROR_NONE) {
this->status_clear_warning();
} else {
this->status_set_warning(ESP_LOG_MSG_COMM_FAIL);
}
this->set_operating_mode(OP_NORMAL_MODE_STRING);
this->status_clear_warning();
}
void LD2420Component::factory_reset_action() {
@@ -516,6 +543,10 @@ void LD2420Component::factory_reset_action() {
ESP_LOGW(TAG, "Module is still starting up; ignoring");
return;
}
if (ld2420::get_firmware_int(this->firmware_ver_) == 0) {
ESP_LOGW(TAG, "Module configuration was never read; ignoring");
return;
}
ESP_LOGD(TAG, "Setting factory defaults");
if (this->set_config_mode(true) == LD2420_ERROR_TIMEOUT) {
ESP_LOGE(TAG, ESP_LOG_MSG_COMM_FAIL);
@@ -536,12 +567,15 @@ void LD2420Component::factory_reset_action() {
}
memcpy(&this->current_config, &this->new_config, sizeof(this->new_config));
this->set_system_mode(this->system_mode_);
this->set_config_mode(false);
if (this->set_config_mode(false) == LD2420_ERROR_NONE) {
this->status_clear_warning();
} else {
this->status_set_warning(ESP_LOG_MSG_COMM_FAIL);
}
#ifdef USE_NUMBER
this->init_gate_config_numbers();
this->refresh_gate_config_numbers();
#endif
this->status_clear_warning();
}
void LD2420Component::restart_module_action() {
@@ -558,6 +592,10 @@ void LD2420Component::restart_module_action() {
}
void LD2420Component::revert_config_action() {
if (this->startup_state_ != StartupState::STARTUP_STATE_RUNNING) {
ESP_LOGW(TAG, "Module is still starting up; ignoring");
return;
}
memcpy(&this->new_config, &this->current_config, sizeof(this->current_config));
#ifdef USE_NUMBER
this->init_gate_config_numbers();
+3 -2
View File
@@ -51,8 +51,8 @@ class LD2420Component final : public Component, public uart::UARTDevice {
};
struct RegConfigT {
uint32_t move_thresh[TOTAL_GATES];
uint32_t still_thresh[TOTAL_GATES];
uint32_t move_thresh[TOTAL_GATES]{};
uint32_t still_thresh[TOTAL_GATES]{};
uint16_t min_gate{0};
uint16_t max_gate{0};
uint16_t timeout{0};
@@ -218,6 +218,7 @@ class LD2420Component final : public Component, public uart::UARTDevice {
uint8_t startup_sequence_retries_{0};
uint8_t startup_gate_{0};
bool rx_seen_{false};
bool listen_drained_{false};
uint8_t buffer_pos_{0}; // where to resume processing/populating buffer
uint8_t buffer_data_[MAX_LINE_LENGTH];
char firmware_ver_[8]{"v0.0.0"};
@@ -0,0 +1,131 @@
esphome:
name: uart-mock-ld2420-retry-test
host:
api:
batch_delay: 0ms # Disable batching to receive all state updates
logger:
level: VERBOSE
external_components:
- source:
type: local
path: EXTERNAL_COMPONENT_PATH
# Dummy uart entry to satisfy ld2420's DEPENDENCIES = ["uart"]
uart:
baud_rate: 115200
port: /dev/null
# Exercises the per-command retry path: the module ignores the first config
# mode enable command and only answers the resend, so the startup handshake
# must time out once, resend, and then complete normally.
uart_mock:
id: mock_uart
baud_rate: 115200
auto_start: true
injections:
# Wake-up frame (t=700ms): energy frame (presence=1, distance=100).
# Delay=700ms keeps it outside the component's 500ms listen settle window.
- delay: 700ms
inject_rx:
[
0xF4, 0xF3, 0xF2, 0xF1,
0x23, 0x00,
0x01,
0x64, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0xF8, 0xF7, 0xF6, 0xF5,
]
# The config mode enable command is answered from the on_tx hook below so
# that the first attempt can be ignored; it must not have a responder here.
responses:
# Version response: returns "v2.0.0" → 200 >= 154 → energy mode
- expect_tx:
[0xFD, 0xFC, 0xFB, 0xFA, 0x02, 0x00, 0x00, 0x00, 0x04, 0x03, 0x02, 0x01]
inject_rx:
[
0xFD, 0xFC, 0xFB, 0xFA,
0x0C, 0x00,
0x00, 0x01,
0x00, 0x00,
0x06, 0x00,
0x76, 0x32, 0x2E, 0x30, 0x2E, 0x30,
0x04, 0x03, 0x02, 0x01,
]
# System mode write: CMD_WRITE_SYS_PARAM (0x0012), mode = energy (0x0004)
- expect_tx:
[0xFD, 0xFC, 0xFB, 0xFA, 0x08, 0x00, 0x12, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x04, 0x03, 0x02, 0x01]
inject_rx:
[
0xFD, 0xFC, 0xFB, 0xFA,
0x04, 0x00,
0x12, 0x01,
0x00, 0x00,
0x04, 0x03, 0x02, 0x01,
]
# Config mode disable: CMD_DISABLE_CONF (0x00FE)
- expect_tx: [0xFD, 0xFC, 0xFB, 0xFA, 0x02, 0x00, 0xFE, 0x00, 0x04, 0x03, 0x02, 0x01]
inject_rx:
[
0xFD, 0xFC, 0xFB, 0xFA,
0x04, 0x00,
0xFE, 0x01,
0x00, 0x00,
0x04, 0x03, 0x02, 0x01,
]
# Catch-all for the CMD_READ_ABD_PARAM (0x0008) reads: limits and the 16
# gate threshold reads. Three zeroed uint32 data values.
- expect_tx: [0x04, 0x03, 0x02, 0x01]
inject_rx:
[
0xFD, 0xFC, 0xFB, 0xFA,
0x10, 0x00,
0x08, 0x01,
0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x04, 0x03, 0x02, 0x01,
]
# Ignore the first config mode enable command; ack every one after it
on_tx:
- lambda: |-
static int enable_count = 0;
if (data.size() == 14 && data[6] == 0xFF) {
enable_count++;
if (enable_count >= 2) {
id(mock_uart).inject_to_rx_buffer(std::vector<uint8_t>{
0xFD, 0xFC, 0xFB, 0xFA, 0x04, 0x00, 0xFF, 0x01, 0x00, 0x00, 0x04, 0x03, 0x02, 0x01});
}
}
ld2420:
id: ld2420_dev
uart_id: mock_uart
sensor:
- platform: ld2420
ld2420_id: ld2420_dev
moving_distance:
name: "Moving Distance"
filters:
- timeout:
timeout: 50ms
value: last
- throttle_with_priority: 50ms
binary_sensor:
- platform: ld2420
ld2420_id: ld2420_dev
has_target:
name: "Has Target"
filters:
- settle: 50ms
@@ -0,0 +1,128 @@
esphome:
name: uart-mock-ld2420-giveup-test
host:
api:
batch_delay: 0ms # Disable batching to receive all state updates
logger:
level: VERBOSE
external_components:
- source:
type: local
path: EXTERNAL_COMPONENT_PATH
# Dummy uart entry to satisfy ld2420's DEPENDENCIES = ["uart"]
uart:
baud_rate: 115200
port: /dev/null
# Exercises the sequence retry and give-up path: the module streams energy
# frames and answers every command except the firmware version read. The
# startup handshake must retry the whole sequence, eventually give up with a
# warning instead of marking the component failed, and keep parsing the
# stream afterwards. Runs for roughly 16 seconds of retry cadence.
uart_mock:
id: mock_uart
baud_rate: 115200
auto_start: true
# Module streams a valid energy frame (presence=1, distance=100) continuously
periodic_rx:
- interval: 250ms
data:
[
0xF4, 0xF3, 0xF2, 0xF1,
0x23, 0x00,
0x01,
0x64, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0xF8, 0xF7, 0xF6, 0xF5,
]
injections:
# Post-give-up parser probe (t=22s): 50 bytes of 0xFF overflow the frame
# buffer, which the parser answers with a "Max command length exceeded"
# warning. The give-up happens around t=16s, so seeing that warning after
# the give-up proves the stream parser is still running in the degraded
# state. (A distinct sensor value cannot serve as the probe: the 1s
# publish throttle races the constant periodic stream, and the API
# deduplicates repeated identical states.)
- delay: 22000ms
inject_rx:
[
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
]
responses:
# Version read: matched so the catch-all cannot answer it, but never
# replied to; this is the command the handshake gives up on
- expect_tx:
[0xFD, 0xFC, 0xFB, 0xFA, 0x02, 0x00, 0x00, 0x00, 0x04, 0x03, 0x02, 0x01]
inject_rx: []
# Config mode enable: CMD_ENABLE_CONF (0x00FF)
- expect_tx:
[0xFD, 0xFC, 0xFB, 0xFA, 0x04, 0x00, 0xFF, 0x00, 0x02, 0x00, 0x04, 0x03, 0x02, 0x01]
inject_rx:
[
0xFD, 0xFC, 0xFB, 0xFA,
0x04, 0x00,
0xFF, 0x01,
0x00, 0x00,
0x04, 0x03, 0x02, 0x01,
]
# Config mode disable: CMD_DISABLE_CONF (0x00FE), sent blind before each
# sequence retry and on the final give-up
- expect_tx: [0xFD, 0xFC, 0xFB, 0xFA, 0x02, 0x00, 0xFE, 0x00, 0x04, 0x03, 0x02, 0x01]
inject_rx:
[
0xFD, 0xFC, 0xFB, 0xFA,
0x04, 0x00,
0xFE, 0x01,
0x00, 0x00,
0x04, 0x03, 0x02, 0x01,
]
# Catch-all for the CMD_READ_ABD_PARAM (0x0008) reads
- expect_tx: [0x04, 0x03, 0x02, 0x01]
inject_rx:
[
0xFD, 0xFC, 0xFB, 0xFA,
0x10, 0x00,
0x08, 0x01,
0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x04, 0x03, 0x02, 0x01,
]
ld2420:
id: ld2420_dev
uart_id: mock_uart
sensor:
- platform: ld2420
ld2420_id: ld2420_dev
moving_distance:
name: "Moving Distance"
filters:
- timeout:
timeout: 50ms
value: last
- throttle_with_priority: 50ms
binary_sensor:
- platform: ld2420
ld2420_id: ld2420_dev
has_target:
name: "Has Target"
filters:
- settle: 50ms
+177
View File
@@ -35,6 +35,16 @@ test_uart_mock_ld2420_restart_button (module restart action):
boots. The component must not treat the tail bytes as proof the module is
up and must only re-run its handshake after the module's first post-boot
frame; transmitting into the boot window locks up real hardware.
test_uart_mock_ld2420_cmd_retry (per-command resend):
The module ignores the first config mode enable command and only answers
the resend. The handshake must time out once, resend, and complete.
test_uart_mock_ld2420_give_up (sequence retry and give-up):
The module streams and answers everything except the firmware version
read. The handshake must retry the whole sequence, eventually give up with
a warning instead of marking the component failed, and keep publishing
sensor data from the stream afterwards.
"""
from __future__ import annotations
@@ -316,6 +326,173 @@ async def test_uart_mock_ld2420_delayed_boot(
)
@pytest.mark.asyncio
async def test_uart_mock_ld2420_cmd_retry(
yaml_config: str,
run_compiled: RunCompiledFunction,
api_client_connected: APIClientConnectedFactory,
) -> None:
"""First config command gets no reply; the resend must recover."""
loop = asyncio.get_running_loop()
setup_complete = loop.create_future()
resend_seen = loop.create_future()
failure_lines: list[str] = []
def line_callback(line: str) -> None:
if "No reply to startup command" in line and not resend_seen.done():
resend_seen.set_result(True)
if (
"Module setup complete; firmware v2.0.0" in line
and not setup_complete.done()
):
setup_complete.set_result(True)
if (
"marked FAILED" in line
or "Communication failed" in line
or "Module setup attempt" in line
):
failure_lines.append(line)
collector = SensorStateCollector(
sensor_names=["moving_distance"],
binary_sensor_names=["has_target"],
)
async with (
run_compiled(yaml_config, line_callback=line_callback),
api_client_connected() as client,
):
entities, _ = await client.list_entities_services()
collector.build_key_mapping(entities)
initial_state_helper = InitialStateHelper(entities)
client.subscribe_states(
initial_state_helper.on_state_wrapper(collector.on_state)
)
try:
await initial_state_helper.wait_for_initial_states()
except TimeoutError:
pytest.fail("Timeout waiting for initial states")
# The first enable command is ignored, so a resend must happen
try:
await asyncio.wait_for(resend_seen, timeout=10.0)
except TimeoutError:
pytest.fail("Timeout waiting for the startup command resend log line")
# The resend gets an ack and the handshake completes normally
try:
await asyncio.wait_for(setup_complete, timeout=10.0)
except TimeoutError:
pytest.fail("Timeout waiting for 'Module setup complete' after the resend")
try:
await collector.wait_for_all(timeout=5.0)
except TimeoutError:
pytest.fail(
f"Timeout waiting for sensor data. Received:\n"
f" sensor_states: {collector.sensor_states}"
)
assert collector.sensor_states["moving_distance"][0] == pytest.approx(100.0)
# A single command resend must not burn a whole sequence retry or
# produce any failure log line
assert not failure_lines, f"Unexpected failure log lines: {failure_lines}"
@pytest.mark.asyncio
async def test_uart_mock_ld2420_give_up(
yaml_config: str,
run_compiled: RunCompiledFunction,
api_client_connected: APIClientConnectedFactory,
) -> None:
"""Version read never answers; retries then give-up, stream keeps working."""
loop = asyncio.get_running_loop()
sequence_retry_seen = loop.create_future()
give_up_seen = loop.create_future()
parser_alive_after_give_up = loop.create_future()
marked_failed_lines: list[str] = []
def line_callback(line: str) -> None:
if "Module setup attempt 1 failed; retrying" in line and (
not sequence_retry_seen.done()
):
sequence_retry_seen.set_result(True)
if "Firmware version and operating mode were never read" in line and (
not give_up_seen.done()
):
give_up_seen.set_result(True)
# The overflow probe injected at t=22s (after the give-up) makes the
# parser log this warning only if it is still running
if (
"Max command length exceeded" in line
and give_up_seen.done()
and not parser_alive_after_give_up.done()
):
parser_alive_after_give_up.set_result(True)
if "marked FAILED" in line:
marked_failed_lines.append(line)
collector = SensorStateCollector(
sensor_names=["moving_distance"],
binary_sensor_names=["has_target"],
)
async with (
run_compiled(yaml_config, line_callback=line_callback),
api_client_connected() as client,
):
entities, _ = await client.list_entities_services()
collector.build_key_mapping(entities)
initial_state_helper = InitialStateHelper(entities)
client.subscribe_states(
initial_state_helper.on_state_wrapper(collector.on_state)
)
try:
await initial_state_helper.wait_for_initial_states()
except TimeoutError:
pytest.fail("Timeout waiting for initial states")
# The version read times out three times, then the sequence retries
try:
await asyncio.wait_for(sequence_retry_seen, timeout=15.0)
except TimeoutError:
pytest.fail("Timeout waiting for the sequence retry log line")
# After all sequence retries the component gives up with a warning
try:
await asyncio.wait_for(give_up_seen, timeout=30.0)
except TimeoutError:
pytest.fail("Timeout waiting for the give-up log line")
# The stream must still be parsed after giving up; the overflow probe
# injected at t=22s only produces its warning if the parser runs
try:
await asyncio.wait_for(parser_alive_after_give_up, timeout=20.0)
except TimeoutError:
pytest.fail(
"No parser activity after the give-up; the stream parser "
"must keep running in the degraded state"
)
# The stream published sensor data while the handshake was failing
assert pytest.approx(100.0) in collector.sensor_states["moving_distance"], (
f"Expected the stream to publish distance=100, "
f"got: {collector.sensor_states['moving_distance']}"
)
# The whole point of the degraded state: the component keeps running
assert not marked_failed_lines, (
f"Component was marked failed: {marked_failed_lines}"
)
@pytest.mark.asyncio
async def test_uart_mock_ld2420_restart_button(
yaml_config: str,