From 6a0e5ffce8e356788a6fe574e7e6f6bc24b09480 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 16 Aug 2026 21:59:17 -0500 Subject: [PATCH] [ld2420] Address fourth review round Sync new_config with current_config on the give-up path and gate the config writing actions on an explicit config read complete flag so a partial read can never be written to the module's NVM, raise the retry listen window above the module boot silence, treat replies shorter than the expected data length as silence, apply the parser mode only after the mode write is acknowledged, propagate write errors in the apply and factory reset actions before adopting the new config or clearing the warning, log drain failures and unbuildable startup frames, watch the runtime marked as failed log string in the tests, give the first enable command in the retry fixture genuine silence, and alternate the warm restart stream values so state deduplication cannot starve a late subscriber. --- esphome/components/ld2420/ld2420.cpp | 94 ++++++++++++------- esphome/components/ld2420/ld2420.h | 12 ++- .../fixtures/uart_mock_ld2420_cmd_retry.yaml | 10 +- .../uart_mock_ld2420_warm_restart.yaml | 20 +++- tests/integration/test_uart_mock_ld2420.py | 29 ++++-- 5 files changed, 118 insertions(+), 47 deletions(-) diff --git a/esphome/components/ld2420/ld2420.cpp b/esphome/components/ld2420/ld2420.cpp index b471008e417..76c03467151 100644 --- a/esphome/components/ld2420/ld2420.cpp +++ b/esphome/components/ld2420/ld2420.cpp @@ -68,12 +68,18 @@ static constexpr uint32_t CMD_ACK_TIMEOUT_MS = 1000; static constexpr uint8_t CMD_MAX_RETRIES = 3; // Startup state machine timing. The module starts transmitting ~3.5 s after a -// power cycle; the listen window is three times that to be safe. The ack -// timeout and command retry count are shared with the blocking command engine. +// power cycle; the first listen window is roughly three times that to be +// safe, and the shorter retry window still stays above the boot silence in +// case the module reset itself between attempts. static constexpr uint32_t STARTUP_LISTEN_TIMEOUT_MS = 10000; -static constexpr uint32_t STARTUP_RETRY_LISTEN_MS = 3000; +static constexpr uint32_t STARTUP_RETRY_LISTEN_MS = 5000; static constexpr uint32_t STARTUP_LISTEN_SETTLE_MS = 500; static constexpr uint8_t STARTUP_SEQUENCE_MAX_RETRIES = 3; +// Minimum reply data lengths for the startup reads: the limits read returns +// three values and each gate read returns two, four bytes each plus the four +// status bytes counted in the reply length field +static constexpr uint8_t REPLY_MIN_LEN_LIMITS = 16; +static constexpr uint8_t REPLY_MIN_LEN_GATE = 12; // Command sets static constexpr uint16_t CMD_DISABLE_CONF = 0x00FE; @@ -235,6 +241,7 @@ void LD2420Component::begin_startup_() { // that kept streaming across a soft restart, before the mode is negotiated. this->system_mode_ = CMD_SYSTEM_MODE_ENERGY; this->startup_sequence_retries_ = 0; + this->config_read_complete_ = false; this->begin_listen_(); } @@ -248,6 +255,7 @@ void LD2420Component::drain_rx_() { size_t avail; while ((avail = this->available()) > 0) { if (!this->read_array(buf, std::min(avail, sizeof(buf)))) { + ESP_LOGV(TAG, "Failed to drain the receive buffer"); break; } } @@ -271,7 +279,7 @@ bool LD2420Component::build_startup_frame_(CmdFrameT &frame) { this->build_gate_threshold_frame_(frame, this->startup_gate_); return true; case StartupState::STARTUP_STATE_SET_MODE: - this->build_system_mode_frame_(frame, this->system_mode_); + this->build_system_mode_frame_(frame, this->startup_target_mode_); return true; case StartupState::STARTUP_STATE_EXIT_CONFIG: this->build_config_mode_frame_(frame, false); @@ -284,6 +292,9 @@ bool LD2420Component::build_startup_frame_(CmdFrameT &frame) { void LD2420Component::send_startup_cmd_() { CmdFrameT frame; if (!this->build_startup_frame_(frame)) { + // Programming error: a command state without a frame would otherwise look + // exactly like a module timeout + ESP_LOGE(TAG, "No command frame for startup state %u", (unsigned) this->startup_state_); return; } // Discard anything still buffered (including a late reply to a previous @@ -311,8 +322,11 @@ void LD2420Component::start_startup_cmd_(StartupState state) { // Common ack handling for the startup commands: returns true once the reply to // the current startup frame arrived; resends on timeout, and after too many // failed sends either restarts the whole sequence or gives up with a warning. -bool LD2420Component::startup_ack_check_() { - if (this->cmd_reply_.ack && this->cmd_reply_.command == this->startup_cmd_) { +// A reply shorter than min_data_len is treated like silence so a truncated +// read cannot be stored as zeroed configuration. +bool LD2420Component::startup_ack_check_(uint8_t min_data_len) { + if (this->cmd_reply_.ack && this->cmd_reply_.command == this->startup_cmd_ && + this->cmd_reply_.length >= min_data_len) { return true; } if (this->cmd_reply_.error > 0) { @@ -351,7 +365,12 @@ void LD2420Component::abandon_startup_() { // without a version read the mode was never negotiated, so such a module // will not publish sensor data either. ESP_LOGE(TAG, "Firmware version and operating mode were never read"); + } else if (this->startup_state_ == StartupState::STARTUP_STATE_SET_MODE) { + ESP_LOGE(TAG, "Operating mode write was not acknowledged; sensor data may not be parsed"); } + // Keep the editable config in sync with what was actually read so a later + // Apply Config cannot write values that were never read from the module + memcpy(&this->new_config, &this->current_config, sizeof(this->current_config)); #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 @@ -420,7 +439,7 @@ void LD2420Component::loop_startup_(bool got_data) { return; case StartupState::STARTUP_STATE_READ_LIMITS: - if (!this->startup_ack_check_()) { + if (!this->startup_ack_check_(REPLY_MIN_LEN_LIMITS)) { return; } this->current_config.min_gate = (uint16_t) this->cmd_reply_.data[0]; @@ -443,7 +462,7 @@ void LD2420Component::loop_startup_(bool got_data) { } case StartupState::STARTUP_STATE_READ_GATES: - if (!this->startup_ack_check_()) { + if (!this->startup_ack_check_(REPLY_MIN_LEN_GATE)) { return; } this->current_config.move_thresh[this->startup_gate_] = this->cmd_reply_.data[0]; @@ -452,6 +471,7 @@ void LD2420Component::loop_startup_(bool got_data) { this->start_startup_cmd_(StartupState::STARTUP_STATE_READ_GATES); return; } + this->config_read_complete_ = true; memcpy(&this->new_config, &this->current_config, sizeof(this->current_config)); if (ld2420::get_firmware_int(this->firmware_ver_) < CALIBRATE_VERSION_MIN) { this->set_operating_mode(OP_SIMPLE_MODE_STRING); @@ -460,10 +480,10 @@ void LD2420Component::loop_startup_(bool got_data) { this->operating_selector_->publish_state(OP_SIMPLE_MODE_STRING); } #endif - this->set_mode_(CMD_SYSTEM_MODE_SIMPLE); + this->startup_target_mode_ = CMD_SYSTEM_MODE_SIMPLE; ESP_LOGW(TAG, "Firmware version %s and older supports Simple Mode only", this->firmware_ver_); } else { - this->set_mode_(CMD_SYSTEM_MODE_ENERGY); + this->startup_target_mode_ = CMD_SYSTEM_MODE_ENERGY; #ifdef USE_SELECT if (this->operating_selector_ != nullptr) { this->operating_selector_->publish_state(OP_NORMAL_MODE_STRING); @@ -480,6 +500,9 @@ void LD2420Component::loop_startup_(bool got_data) { if (!this->startup_ack_check_()) { return; } + // Switch the parser only after the module acknowledged the mode write, + // so both sides stay in the same mode when the write is never acked + this->set_mode_(this->startup_target_mode_); this->start_startup_cmd_(StartupState::STARTUP_STATE_EXIT_CONFIG); return; @@ -498,17 +521,17 @@ void LD2420Component::loop_startup_(bool got_data) { } // Common precondition for the button actions: the startup handshake must have -// finished, and actions that write configuration additionally require that the -// configuration was actually read (setup may have given up before the version -// read; writing the unread config to the module's NVM would wipe its stored -// thresholds). +// finished, and actions that write configuration additionally require that +// every limit and gate threshold was actually read (setup may have given up +// partway through; writing the unread config to the module's NVM would wipe +// its stored thresholds). bool LD2420Component::action_allowed_(bool needs_config) { if (this->startup_state_ != StartupState::STARTUP_STATE_RUNNING) { ESP_LOGW(TAG, "Module is still starting up; ignoring"); return false; } - if (needs_config && ld2420::get_firmware_int(this->firmware_ver_) == 0) { - ESP_LOGW(TAG, "Module configuration was never read; ignoring"); + if (needs_config && !this->config_read_complete_) { + ESP_LOGW(TAG, "Module configuration was never fully read; ignoring"); return false; } return true; @@ -529,22 +552,25 @@ void LD2420Component::apply_config_action() { this->status_set_warning(ESP_LOG_MSG_COMM_FAIL); return; } - this->set_min_max_distances_timeout(this->new_config.max_gate, this->new_config.min_gate, this->new_config.timeout); + uint8_t error = this->set_min_max_distances_timeout(this->new_config.max_gate, this->new_config.min_gate, + this->new_config.timeout); for (uint8_t gate = 0; gate < TOTAL_GATES; gate++) { delay_microseconds_safe(125); - this->set_gate_threshold(gate); + error |= this->set_gate_threshold(gate); + } + if (error == LD2420_ERROR_NONE) { + // Only adopt the new values as current once every write was acknowledged + memcpy(¤t_config, &new_config, sizeof(new_config)); } - memcpy(¤t_config, &new_config, sizeof(new_config)); #ifdef USE_NUMBER this->init_gate_config_numbers(); #endif this->set_system_mode(this->system_mode_); - // 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) { + // Disable config mode to save the new values in the LD2420 nvm + if (this->set_config_mode(false) == LD2420_ERROR_NONE && error == LD2420_ERROR_NONE) { this->status_clear_warning(); } else { + ESP_LOGE(TAG, ESP_LOG_MSG_COMM_FAIL); this->status_set_warning(ESP_LOG_MSG_COMM_FAIL); } this->set_operating_mode(OP_NORMAL_MODE_STRING); @@ -560,7 +586,7 @@ void LD2420Component::factory_reset_action() { this->status_set_warning(ESP_LOG_MSG_COMM_FAIL); return; } - this->set_min_max_distances_timeout(FACTORY_MAX_GATE, FACTORY_MIN_GATE, FACTORY_TIMEOUT); + uint8_t error = this->set_min_max_distances_timeout(FACTORY_MAX_GATE, FACTORY_MIN_GATE, FACTORY_TIMEOUT); #ifdef USE_NUMBER this->gate_timeout_number_->state = FACTORY_TIMEOUT; this->min_gate_distance_number_->state = FACTORY_MIN_GATE; @@ -570,13 +596,16 @@ void LD2420Component::factory_reset_action() { this->new_config.move_thresh[gate] = FACTORY_MOVE_THRESH[gate]; this->new_config.still_thresh[gate] = FACTORY_STILL_THRESH[gate]; delay_microseconds_safe(125); - this->set_gate_threshold(gate); + error |= this->set_gate_threshold(gate); + } + if (error == LD2420_ERROR_NONE) { + memcpy(&this->current_config, &this->new_config, sizeof(this->new_config)); } - memcpy(&this->current_config, &this->new_config, sizeof(this->new_config)); this->set_system_mode(this->system_mode_); - if (this->set_config_mode(false) == LD2420_ERROR_NONE) { + if (this->set_config_mode(false) == LD2420_ERROR_NONE && error == LD2420_ERROR_NONE) { this->status_clear_warning(); } else { + ESP_LOGE(TAG, ESP_LOG_MSG_COMM_FAIL); this->status_set_warning(ESP_LOG_MSG_COMM_FAIL); } #ifdef USE_NUMBER @@ -1088,8 +1117,9 @@ void LD2420Component::build_version_frame_(CmdFrameT &frame) { ESP_LOGV(TAG, "Sending read firmware version command: %2X", frame.command); } -void LD2420Component::set_min_max_distances_timeout(uint32_t max_gate_distance, uint32_t min_gate_distance, // NOLINT - uint32_t timeout) { +uint8_t LD2420Component::set_min_max_distances_timeout(uint32_t max_gate_distance, + uint32_t min_gate_distance, // NOLINT + uint32_t timeout) { // Header H, Length L, Register R, Value V, Footer F // |Min Gate |Max Gate |Timeout | // HH HH HH HH LL LL CC CC RR RR VV VV VV VV RR RR VV VV VV VV RR RR VV VV VV VV FF FF FF FF @@ -1118,10 +1148,10 @@ void LD2420Component::set_min_max_distances_timeout(uint32_t max_gate_distance, cmd_frame.footer = CMD_FRAME_FOOTER; ESP_LOGV(TAG, "Sending write gate min max and timeout command: %2X", cmd_frame.command); - this->send_cmd_from_array(cmd_frame); + return this->send_cmd_from_array(cmd_frame); } -void LD2420Component::set_gate_threshold(uint8_t gate) { +uint8_t LD2420Component::set_gate_threshold(uint8_t gate) { // Header H, Length L, Command C, Register R, Value V, Footer F // HH HH HH HH LL LL CC CC RR RR VV VV VV VV RR RR VV VV VV VV FF FF FF FF // FD FC FB FA 14 00 07 00 10 00 00 FF 00 00 00 01 00 0F 00 00 04 03 02 01 @@ -1144,7 +1174,7 @@ void LD2420Component::set_gate_threshold(uint8_t gate) { cmd_frame.data_length += sizeof(this->new_config.still_thresh[gate]); cmd_frame.footer = CMD_FRAME_FOOTER; ESP_LOGV(TAG, "Sending set gate %4X sensitivity command: %2X", gate, cmd_frame.command); - this->send_cmd_from_array(cmd_frame); + return this->send_cmd_from_array(cmd_frame); } #ifdef USE_NUMBER diff --git a/esphome/components/ld2420/ld2420.h b/esphome/components/ld2420/ld2420.h index e94013412d1..a2a68e37b1b 100644 --- a/esphome/components/ld2420/ld2420.h +++ b/esphome/components/ld2420/ld2420.h @@ -111,8 +111,8 @@ class LD2420Component final : public Component, public uart::UARTDevice { void auto_calibrate_sensitivity(); void update_radar_data(uint16_t const *gate_energy, uint8_t sample_number); uint8_t set_config_mode(bool enable); - void set_min_max_distances_timeout(uint32_t max_gate_distance, uint32_t min_gate_distance, uint32_t timeout); - void set_gate_threshold(uint8_t gate); + uint8_t set_min_max_distances_timeout(uint32_t max_gate_distance, uint32_t min_gate_distance, uint32_t timeout); + uint8_t set_gate_threshold(uint8_t gate); void set_reg_value(uint16_t reg, uint16_t value); void set_system_mode(uint16_t mode); void ld2420_restart(); @@ -175,7 +175,7 @@ class LD2420Component final : public Component, public uart::UARTDevice { void send_startup_cmd_(); void abort_startup_cmd_(); void abandon_startup_(); - bool startup_ack_check_(); + bool startup_ack_check_(uint8_t min_data_len = 0); bool action_allowed_(bool needs_config); void drain_rx_(); void write_cmd_frame_(const CmdFrameT &frame); @@ -212,7 +212,8 @@ class LD2420Component final : public Component, public uart::UARTDevice { #endif uint16_t distance_{0}; - uint16_t system_mode_{0}; // Set to the energy mode default in begin_startup_() + uint16_t system_mode_{0}; // Set to the energy mode default in begin_startup_() + uint16_t startup_target_mode_{0}; // Mode the startup handshake writes; applied to system_mode_ once acked uint16_t gate_energy_[TOTAL_GATES]; uint32_t phase_start_ms_{0}; StartupState startup_state_{StartupState::STARTUP_STATE_LISTEN_SETTLE}; @@ -220,7 +221,8 @@ class LD2420Component final : public Component, public uart::UARTDevice { uint8_t startup_cmd_attempts_{0}; uint8_t startup_sequence_retries_{0}; uint8_t startup_gate_{0}; - uint8_t buffer_pos_{0}; // where to resume processing/populating buffer + bool config_read_complete_{false}; // All limits and gate thresholds were read from the module + 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"}; bool cmd_active_{false}; diff --git a/tests/integration/fixtures/uart_mock_ld2420_cmd_retry.yaml b/tests/integration/fixtures/uart_mock_ld2420_cmd_retry.yaml index 78a6a64b4cd..8f9e223ab79 100644 --- a/tests/integration/fixtures/uart_mock_ld2420_cmd_retry.yaml +++ b/tests/integration/fixtures/uart_mock_ld2420_cmd_retry.yaml @@ -42,9 +42,15 @@ uart_mock: 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. + # The config mode enable command is matched by an empty responder so the + # catch-all cannot answer it (responders match on the TX suffix and every + # command ends with the frame footer); the on_tx hook below acks it from + # the second attempt on, so the first attempt is genuine silence. responses: + - expect_tx: + [0xFD, 0xFC, 0xFB, 0xFA, 0x04, 0x00, 0xFF, 0x00, 0x02, 0x00, 0x04, 0x03, 0x02, 0x01] + inject_rx: [] + # Version response: returns "v2.0.0" → 200 >= 154 → energy mode - expect_tx: [0xFD, 0xFC, 0xFB, 0xFA, 0x02, 0x00, 0x00, 0x00, 0x04, 0x03, 0x02, 0x01] diff --git a/tests/integration/fixtures/uart_mock_ld2420_warm_restart.yaml b/tests/integration/fixtures/uart_mock_ld2420_warm_restart.yaml index d318a7e576c..86d4a0b4bec 100644 --- a/tests/integration/fixtures/uart_mock_ld2420_warm_restart.yaml +++ b/tests/integration/fixtures/uart_mock_ld2420_warm_restart.yaml @@ -26,7 +26,12 @@ uart_mock: baud_rate: 115200 auto_start: true - # Module streams a valid energy frame (presence=1, distance=100) continuously + # Module streams valid energy frames continuously. Two alternating frames + # are used (presence=1/distance=100 and presence=0/distance=75) so states + # keep changing: with a constant frame the API deduplicates the repeated + # identical states, and a client that subscribes after the first publish + # would swallow the only transition as the initial state and never see an + # update. periodic_rx: - interval: 250ms data: @@ -41,6 +46,19 @@ uart_mock: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xF8, 0xF7, 0xF6, 0xF5, ] + - interval: 1050ms + data: + [ + 0xF4, 0xF3, 0xF2, 0xF1, + 0x23, 0x00, + 0x00, + 0x4B, 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, + ] responses: # Version response: returns "v2.0.0" → 200 >= 154 → energy mode diff --git a/tests/integration/test_uart_mock_ld2420.py b/tests/integration/test_uart_mock_ld2420.py index fecec2b23bb..3a794ca6777 100644 --- a/tests/integration/test_uart_mock_ld2420.py +++ b/tests/integration/test_uart_mock_ld2420.py @@ -287,6 +287,7 @@ async def _run_listen_first_test( api_client_connected: APIClientConnectedFactory, *, post_setup_distance: float | None = None, + strict_first: bool = True, ) -> None: """Shared body for the listen-first startup tests. @@ -294,7 +295,9 @@ async def _run_listen_first_test( (real hardware locks up until power cycled if it does), that the setup handshake completes, and that sensor data publishes. When post_setup_distance is given, additionally waits for that value to prove - streaming still works after the handshake. + streaming still works after the handshake. strict_first asserts on the + first collected state; pass False for fixtures whose stream alternates + values, where the first collected state depends on subscribe timing. """ loop = asyncio.get_running_loop() @@ -314,6 +317,7 @@ async def _run_listen_first_test( setup_complete.set_result(True) if ( "marked FAILED" in line + or "was marked as failed" in line or "Communication failed" in line or "No data received from the module" in line ): @@ -358,8 +362,12 @@ async def _run_listen_first_test( ), ) - assert collector.sensor_states["moving_distance"][0] == pytest.approx(100.0) - assert collector.binary_states["has_target"][0] is True + if strict_first: + assert collector.sensor_states["moving_distance"][0] == pytest.approx(100.0) + assert collector.binary_states["has_target"][0] is True + else: + assert pytest.approx(100.0) in collector.sensor_states["moving_distance"] + assert True in collector.binary_states["has_target"] if post_setup_received is not None: await _wait_or_fail( @@ -389,7 +397,9 @@ async def test_uart_mock_ld2420_warm_restart( api_client_connected: APIClientConnectedFactory, ) -> None: """Module streams from boot; component must listen first, then set up.""" - await _run_listen_first_test(yaml_config, run_compiled, api_client_connected) + await _run_listen_first_test( + yaml_config, run_compiled, api_client_connected, strict_first=False + ) @pytest.mark.asyncio @@ -414,7 +424,12 @@ async def test_uart_mock_ld2420_cmd_retry( watcher = _LogWatcher() resend_seen = watcher.watch("No reply to startup command") setup_complete = watcher.watch(SETUP_COMPLETE_LOG) - watcher.collect("marked FAILED", "Communication failed", "Module setup attempt") + watcher.collect( + "marked FAILED", + "was marked as failed", + "Communication failed", + "Module setup attempt", + ) collector = SensorStateCollector( sensor_names=["moving_distance"], @@ -472,7 +487,7 @@ async def test_uart_mock_ld2420_give_up( parser_alive_after_give_up = watcher.watch( "Max command length exceeded", after=give_up_seen ) - watcher.collect("marked FAILED") + watcher.collect("marked FAILED", "was marked as failed") collector = SensorStateCollector( sensor_names=["moving_distance"], @@ -535,7 +550,7 @@ async def test_uart_mock_ld2420_restart_button( after=restart_seen, until=module_frame_after_restart, ) - watcher.collect("marked FAILED", "Communication failed") + watcher.collect("marked FAILED", "was marked as failed", "Communication failed") async with ( run_compiled(yaml_config, line_callback=watcher),