From 15e1ac2500101025242960bf2ef80a8f629905a1 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 16 Aug 2026 17:28:16 -0500 Subject: [PATCH] [ld2420] Simplify startup state machine and tests Rebuild startup frames on demand instead of keeping a frame member, share the ack timeout and retry constants with the blocking engine, guard the restart button during startup, drop dead code, and factor the duplicated integration test bodies into a shared helper. --- esphome/components/ld2420/ld2420.cpp | 98 +++++++++----- esphome/components/ld2420/ld2420.h | 16 +-- tests/integration/test_uart_mock_ld2420.py | 150 ++++++--------------- 3 files changed, 115 insertions(+), 149 deletions(-) diff --git a/esphome/components/ld2420/ld2420.cpp b/esphome/components/ld2420/ld2420.cpp index 5afe218aa5..dc54debcc0 100644 --- a/esphome/components/ld2420/ld2420.cpp +++ b/esphome/components/ld2420/ld2420.cpp @@ -69,11 +69,11 @@ 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 match the blocking command engine. +// timeout and command retry count are shared with the blocking command engine. static constexpr uint32_t STARTUP_LISTEN_TIMEOUT_MS = 10000; static constexpr uint32_t STARTUP_RETRY_LISTEN_MS = 3000; -static constexpr uint32_t STARTUP_ACK_TIMEOUT_MS = 1000; -static constexpr uint8_t STARTUP_CMD_MAX_RETRIES = 3; +static constexpr uint32_t CMD_ACK_TIMEOUT_MS = 1000; +static constexpr uint8_t CMD_MAX_RETRIES = 3; static constexpr uint8_t STARTUP_SEQUENCE_MAX_RETRIES = 3; // Command sets @@ -94,6 +94,7 @@ static constexpr uint16_t CMD_SYSTEM_MODE_GR = 0x0003; static constexpr uint16_t CMD_SYSTEM_MODE_MTT = 0x0001; static constexpr uint16_t CMD_SYSTEM_MODE_SIMPLE = 0x0064; static constexpr uint16_t CMD_SYSTEM_MODE_DEBUG = 0x0000; +static constexpr uint16_t CMD_SYSTEM_MODE_ENERGY = 0x0004; static constexpr uint16_t CMD_SYSTEM_MODE_VS = 0x0002; static constexpr uint16_t CMD_WRITE_ABD_PARAM = 0x0007; static constexpr uint16_t CMD_WRITE_REGISTER = 0x0001; @@ -226,17 +227,19 @@ void LD2420Component::dump_config() { } } -void LD2420Component::setup() { this->begin_startup_(STARTUP_LISTEN_TIMEOUT_MS); } +void LD2420Component::setup() { this->begin_startup_(); } -void LD2420Component::begin_startup_(uint32_t listen_timeout_ms) { +void LD2420Component::begin_startup_() { + // Default to energy mode so the stream parser can frame data from a module + // 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->begin_listen_(listen_timeout_ms); + this->begin_listen_(); } -void LD2420Component::begin_listen_(uint32_t listen_timeout_ms) { +void LD2420Component::begin_listen_() { this->rx_seen_ = false; this->buffer_pos_ = 0; - this->listen_timeout_ms_ = listen_timeout_ms; this->phase_start_ms_ = millis(); this->startup_state_ = StartupState::STARTUP_STATE_LISTEN; } @@ -248,39 +251,68 @@ void LD2420Component::drain_rx_() { this->buffer_pos_ = 0; } -void LD2420Component::send_cmd_async_(const CmdFrameT &frame) { +// Builds the command frame for the current startup state +void LD2420Component::build_startup_frame_(CmdFrameT &frame) { + switch (this->startup_state_) { + case StartupState::STARTUP_STATE_ENTER_CONFIG: + this->build_config_mode_frame_(frame, true); + break; + case StartupState::STARTUP_STATE_READ_LIMITS: + this->build_min_max_timeout_frame_(frame); + break; + case StartupState::STARTUP_STATE_READ_VERSION: + this->build_version_frame_(frame); + break; + case StartupState::STARTUP_STATE_READ_GATES: + this->build_gate_threshold_frame_(frame, this->startup_gate_); + break; + case StartupState::STARTUP_STATE_SET_MODE: + this->build_system_mode_frame_(frame, this->system_mode_); + break; + case StartupState::STARTUP_STATE_EXIT_CONFIG: + this->build_config_mode_frame_(frame, false); + break; + default: + break; + } +} + +void LD2420Component::send_startup_cmd_() { + CmdFrameT frame; + this->build_startup_frame_(frame); + this->startup_cmd_ = (uint8_t) frame.command; this->cmd_reply_.ack = false; this->write_cmd_frame_(frame); this->phase_start_ms_ = millis(); } void LD2420Component::start_startup_cmd_(StartupState state) { - this->startup_cmd_retries_ = 1; this->startup_state_ = state; - this->send_cmd_async_(this->startup_frame_); + this->startup_cmd_retries_ = 1; + this->send_startup_cmd_(); } // 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 == (uint8_t) this->startup_frame_.command) { + if (this->cmd_reply_.ack && this->cmd_reply_.command == this->startup_cmd_) { return true; } - if (millis() - this->phase_start_ms_ <= STARTUP_ACK_TIMEOUT_MS) { + if (millis() - this->phase_start_ms_ <= CMD_ACK_TIMEOUT_MS) { return false; } - if (this->startup_cmd_retries_ < STARTUP_CMD_MAX_RETRIES) { + if (this->startup_cmd_retries_ < CMD_MAX_RETRIES) { this->startup_cmd_retries_++; - ESP_LOGV(TAG, "No reply to startup command %2X; resending", this->startup_frame_.command); - this->send_cmd_async_(this->startup_frame_); + ESP_LOGV(TAG, "No reply to startup command %2X; resending", this->startup_cmd_); + this->send_startup_cmd_(); return false; } if (this->startup_sequence_retries_ < STARTUP_SEQUENCE_MAX_RETRIES) { this->startup_sequence_retries_++; ESP_LOGW(TAG, "Module setup attempt %u failed; retrying", this->startup_sequence_retries_); this->status_set_warning(ESP_LOG_MSG_COMM_FAIL); - this->begin_listen_(STARTUP_RETRY_LISTEN_MS); + this->begin_listen_(); return false; } // Give up on configuration but keep parsing the stream; a module that is @@ -293,28 +325,29 @@ bool LD2420Component::startup_ack_check_() { void LD2420Component::loop_startup_() { switch (this->startup_state_) { - case StartupState::STARTUP_STATE_LISTEN: + case StartupState::STARTUP_STATE_LISTEN: { // The module locks up until power cycled if it receives data before it // has sent its first frame after powering on, so wait until it has // provably transmitted before sending anything. A module stuck in some // other state stays quiet, so fall through after the listen window. if (!this->rx_seen_) { - if (millis() - this->phase_start_ms_ < this->listen_timeout_ms_) { + const uint32_t listen_timeout_ms = + this->startup_sequence_retries_ == 0 ? STARTUP_LISTEN_TIMEOUT_MS : STARTUP_RETRY_LISTEN_MS; + if (millis() - this->phase_start_ms_ < listen_timeout_ms) { return; } ESP_LOGW(TAG, "No data received from the module; attempting configuration anyway"); } // Drop any partial frame so the ack parser starts clean this->drain_rx_(); - this->build_config_mode_frame_(this->startup_frame_, true); this->start_startup_cmd_(StartupState::STARTUP_STATE_ENTER_CONFIG); return; + } case StartupState::STARTUP_STATE_ENTER_CONFIG: if (!this->startup_ack_check_()) { return; } - this->build_min_max_timeout_frame_(this->startup_frame_); this->start_startup_cmd_(StartupState::STARTUP_STATE_READ_LIMITS); return; @@ -325,7 +358,6 @@ void LD2420Component::loop_startup_() { this->current_config.min_gate = (uint16_t) this->cmd_reply_.data[0]; this->current_config.max_gate = (uint16_t) this->cmd_reply_.data[1]; this->current_config.timeout = (uint16_t) this->cmd_reply_.data[2]; - this->build_version_frame_(this->startup_frame_); this->start_startup_cmd_(StartupState::STARTUP_STATE_READ_VERSION); return; @@ -338,7 +370,6 @@ void LD2420Component::loop_startup_() { listener->on_fw_version(fw_str); } this->startup_gate_ = 0; - this->build_gate_threshold_frame_(this->startup_frame_, this->startup_gate_); this->start_startup_cmd_(StartupState::STARTUP_STATE_READ_GATES); return; } @@ -350,7 +381,6 @@ void LD2420Component::loop_startup_() { this->current_config.move_thresh[this->startup_gate_] = this->cmd_reply_.data[0]; this->current_config.still_thresh[this->startup_gate_] = this->cmd_reply_.data[1]; if (++this->startup_gate_ < TOTAL_GATES) { - this->build_gate_threshold_frame_(this->startup_frame_, this->startup_gate_); this->start_startup_cmd_(StartupState::STARTUP_STATE_READ_GATES); return; } @@ -375,7 +405,6 @@ void LD2420Component::loop_startup_() { #ifdef USE_NUMBER this->init_gate_config_numbers(); #endif - this->build_system_mode_frame_(this->startup_frame_, this->system_mode_); this->start_startup_cmd_(StartupState::STARTUP_STATE_SET_MODE); return; @@ -383,7 +412,6 @@ void LD2420Component::loop_startup_() { if (!this->startup_ack_check_()) { return; } - this->build_config_mode_frame_(this->startup_frame_, false); this->start_startup_cmd_(StartupState::STARTUP_STATE_EXIT_CONFIG); return; @@ -464,12 +492,16 @@ void LD2420Component::factory_reset_action() { } void LD2420Component::restart_module_action() { + if (this->startup_state_ != StartupState::STARTUP_STATE_RUNNING) { + ESP_LOGW(TAG, "Module is still starting up; ignoring"); + return; + } ESP_LOGD(TAG, "Restarting"); this->send_module_restart(); // The module is silent while it boots and locks up if it receives data // before it has sent its first frame, so re-run the listen-first startup // sequence instead of transmitting into the boot window. - this->begin_startup_(STARTUP_LISTEN_TIMEOUT_MS); + this->begin_startup_(); } void LD2420Component::revert_config_action() { @@ -485,8 +517,11 @@ void LD2420Component::loop() { if (this->cmd_active_) { return; } - this->read_batch_(this->buffer_data_); + const bool got_data = this->read_batch_(this->buffer_data_); if (this->startup_state_ != StartupState::STARTUP_STATE_RUNNING) { + if (got_data) { + this->rx_seen_ = true; + } this->loop_startup_(); } } @@ -695,12 +730,10 @@ void LD2420Component::handle_simple_mode_(const uint8_t *inbuf, int len) { } } -void LD2420Component::read_batch_(std::span buffer) { +bool LD2420Component::read_batch_(std::span buffer) { // Read all available bytes in batches to reduce UART call overhead. size_t avail = this->available(); - if (avail > 0) { - this->rx_seen_ = true; - } + const bool got_data = avail > 0; uint8_t buf[MAX_LINE_LENGTH]; while (avail > 0) { size_t to_read = std::min(avail, sizeof(buf)); @@ -713,6 +746,7 @@ void LD2420Component::read_batch_(std::span buffer) { this->readline_(buf[i], buffer.data(), buffer.size()); } } + return got_data; } void LD2420Component::handle_ack_data_(uint8_t *buffer, int len) { diff --git a/esphome/components/ld2420/ld2420.h b/esphome/components/ld2420/ld2420.h index 3b200a3faf..a8821f732f 100644 --- a/esphome/components/ld2420/ld2420.h +++ b/esphome/components/ld2420/ld2420.h @@ -25,7 +25,6 @@ static constexpr uint8_t CALIBRATE_SAMPLES = 64; // inside the buffer during footer-based resynchronization after losing sync. static constexpr uint8_t MAX_LINE_LENGTH = 50; static constexpr uint8_t TOTAL_GATES = 16; -static constexpr uint16_t CMD_SYSTEM_MODE_ENERGY = 0x0004; enum OpMode : uint8_t { OP_NORMAL_MODE = 1, @@ -168,21 +167,21 @@ class LD2420Component final : public Component, public uart::UARTDevice { STARTUP_STATE_RUNNING, }; - void begin_startup_(uint32_t listen_timeout_ms); - void begin_listen_(uint32_t listen_timeout_ms); + void begin_startup_(); + void begin_listen_(); void loop_startup_(); void start_startup_cmd_(StartupState state); + void send_startup_cmd_(); bool startup_ack_check_(); void drain_rx_(); void write_cmd_frame_(const CmdFrameT &frame); - void send_cmd_async_(const CmdFrameT &frame); + void build_startup_frame_(CmdFrameT &frame); void build_config_mode_frame_(CmdFrameT &frame, bool enable); void build_min_max_timeout_frame_(CmdFrameT &frame); void build_gate_threshold_frame_(CmdFrameT &frame, uint8_t gate); void build_version_frame_(CmdFrameT &frame); void build_system_mode_frame_(CmdFrameT &frame, uint16_t mode); - void get_reg_value_(uint16_t reg); uint16_t get_mode_() { return this->system_mode_; }; void set_mode_(uint16_t mode) { this->system_mode_ = mode; }; bool get_presence_() { return this->presence_; }; @@ -193,7 +192,7 @@ class LD2420Component final : public Component, public uart::UARTDevice { void handle_energy_mode_(uint8_t *buffer, int len); void handle_ack_data_(uint8_t *buffer, int len); void readline_(int rx_data, uint8_t *buffer, int len); - void read_batch_(std::span buffer); + bool read_batch_(std::span buffer); void set_calibration_(bool state) { this->calibration_ = state; }; bool get_calibration_() { return this->calibration_; }; @@ -209,12 +208,11 @@ class LD2420Component final : public Component, public uart::UARTDevice { #endif uint16_t distance_{0}; - uint16_t system_mode_{CMD_SYSTEM_MODE_ENERGY}; + uint16_t system_mode_{0}; // Set to the energy mode default in begin_startup_() uint16_t gate_energy_[TOTAL_GATES]; uint32_t phase_start_ms_{0}; - uint32_t listen_timeout_ms_{0}; - CmdFrameT startup_frame_; StartupState startup_state_{StartupState::STARTUP_STATE_LISTEN}; + uint8_t startup_cmd_{0}; // Command byte of the in-flight startup command, for ack matching uint8_t startup_cmd_retries_{0}; uint8_t startup_sequence_retries_{0}; uint8_t startup_gate_{0}; diff --git a/tests/integration/test_uart_mock_ld2420.py b/tests/integration/test_uart_mock_ld2420.py index fc3f033d68..5da44d8e31 100644 --- a/tests/integration/test_uart_mock_ld2420.py +++ b/tests/integration/test_uart_mock_ld2420.py @@ -172,20 +172,21 @@ async def test_uart_mock_ld2420( ) -@pytest.mark.asyncio -async def test_uart_mock_ld2420_warm_restart( +async def _run_listen_first_test( yaml_config: str, run_compiled: RunCompiledFunction, api_client_connected: APIClientConnectedFactory, + *, + post_setup_distance: float | None = None, ) -> None: - """Module streams from boot; component must listen first, then set up.""" - external_components_path = str( - Path(__file__).parent / "fixtures" / "external_components" - ) - yaml_config = yaml_config.replace( - "EXTERNAL_COMPONENT_PATH", external_components_path - ) + """Shared body for the listen-first startup tests. + Asserts the component never transmits before the module has sent data + (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. + """ loop = asyncio.get_running_loop() setup_complete = loop.create_future() @@ -217,6 +218,15 @@ async def test_uart_mock_ld2420_warm_restart( binary_sensor_names=["has_target"], ) + post_setup_received = None + if post_setup_distance is not None: + post_setup_received = collector.add_waiter( + lambda: ( + pytest.approx(post_setup_distance) + in collector.sensor_states["moving_distance"] + ) + ) + async with ( run_compiled(yaml_config, line_callback=line_callback), api_client_connected() as client, @@ -234,7 +244,7 @@ async def test_uart_mock_ld2420_warm_restart( except TimeoutError: pytest.fail("Timeout waiting for initial states") - # Setup handshake must complete against the live stream + # Setup handshake must complete once the module has talked try: await asyncio.wait_for(setup_complete, timeout=10.0) except TimeoutError: @@ -256,6 +266,16 @@ async def test_uart_mock_ld2420_warm_restart( assert collector.sensor_states["moving_distance"][0] == pytest.approx(100.0) assert collector.binary_states["has_target"][0] is True + if post_setup_received is not None: + try: + await asyncio.wait_for(post_setup_received, timeout=5.0) + except TimeoutError: + pytest.fail( + f"Timeout waiting for post-setup frame " + f"(distance={post_setup_distance}). Received:\n" + f" moving_distance: {collector.sensor_states['moving_distance']}" + ) + # The component must never transmit before the module has talked; # real hardware locks up until power cycled if it does. assert not tx_before_rx, ( @@ -266,6 +286,16 @@ async def test_uart_mock_ld2420_warm_restart( assert not failure_lines, f"Unexpected failure log lines: {failure_lines}" +@pytest.mark.asyncio +async def test_uart_mock_ld2420_warm_restart( + yaml_config: str, + run_compiled: RunCompiledFunction, + 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) + + @pytest.mark.asyncio async def test_uart_mock_ld2420_delayed_boot( yaml_config: str, @@ -273,105 +303,9 @@ async def test_uart_mock_ld2420_delayed_boot( api_client_connected: APIClientConnectedFactory, ) -> None: """Module silent for 2 s; component must not transmit into the boot window.""" - external_components_path = str( - Path(__file__).parent / "fixtures" / "external_components" + await _run_listen_first_test( + yaml_config, run_compiled, api_client_connected, post_setup_distance=50.0 ) - yaml_config = yaml_config.replace( - "EXTERNAL_COMPONENT_PATH", external_components_path - ) - - loop = asyncio.get_running_loop() - - setup_complete = loop.create_future() - rx_seen = False - tx_before_rx = False - failure_lines: list[str] = [] - - def line_callback(line: str) -> None: - nonlocal rx_seen, tx_before_rx - if "uart_mock" in line: - if "RX inject" in line or "Injecting" in line: - rx_seen = True - elif "TX " in line and not rx_seen: - tx_before_rx = 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 "No data received from the module" in line - ): - failure_lines.append(line) - - collector = SensorStateCollector( - sensor_names=["moving_distance"], - binary_sensor_names=["has_target"], - ) - - # The second injected frame (distance=50) proves streaming works post-setup - post_setup_received = collector.add_waiter( - lambda: pytest.approx(50.0) in collector.sensor_states["moving_distance"] - ) - - 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") - - # Module's first frame arrives at t=2000ms; handshake follows - try: - await asyncio.wait_for(setup_complete, timeout=10.0) - except TimeoutError: - pytest.fail( - "Timeout waiting for 'Module setup complete' log line. " - "The startup state machine did not finish its handshake." - ) - - 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}\n" - f" binary_states: {collector.binary_states}" - ) - - # First frame (t=2000ms) publishes distance=100 - assert collector.sensor_states["moving_distance"][0] == pytest.approx(100.0) - assert collector.binary_states["has_target"][0] is True - - # Second frame (t=3300ms, after setup) publishes distance=50 - try: - await asyncio.wait_for(post_setup_received, timeout=5.0) - except TimeoutError: - pytest.fail( - f"Timeout waiting for post-setup frame (distance=50). Received:\n" - f" moving_distance: {collector.sensor_states['moving_distance']}" - ) - - # The component must have stayed quiet for the module's whole 2 s boot - # window; transmitting into it locks up real LD2420 hardware. - assert not tx_before_rx, ( - "Component transmitted on the UART before the module sent its " - "first frame; this locks up real LD2420 hardware" - ) - - assert not failure_lines, f"Unexpected failure log lines: {failure_lines}" @pytest.mark.asyncio