diff --git a/README.md b/README.md index 8352cea..e6bfe7b 100644 --- a/README.md +++ b/README.md @@ -164,11 +164,10 @@ For window-type devices (`io_device_type: "window_opener"` or `"ventilation_poin `silent: true` makes the motor travel more slowly and quietly, matching the "silent operation" toggle in the manufacturer apps. It selects the protocol's slow travel profile on position moves -and on the favorite ("My") command, both isolated by capturing the same command either side of that -toggle. STOP is excluded because stopping has no travel speed, and ventilation and tilt because -nothing has been captured for them yet — they keep their existing payloads rather than being -guessed at. Nothing on the wire reports a device's current profile, so — like `invert_position` — -this is a declared preference, not a readback. +and on the favorite ("My") command. STOP is excluded because stopping has no travel speed, and +ventilation and tilt because nothing has been captured for them yet — they keep their existing +payloads rather than being guessed at. Nothing on the wire reports a device's current profile, so — +like `invert_position` — this is a declared preference, not a readback. Declaring `silent:` on a cover also generates a ` Silent Operation` switch in Home Assistant, so the profile can be changed at runtime; the YAML value is the boot state. Omit the diff --git a/components/home_io_control/exchange_engine.cpp b/components/home_io_control/exchange_engine.cpp index 44b4352..7a04916 100644 --- a/components/home_io_control/exchange_engine.cpp +++ b/components/home_io_control/exchange_engine.cpp @@ -49,12 +49,11 @@ void ExchangeEngine::record_debug(const char *stage, uint8_t tries, bool saw_cha this->debug_.saw_challenge = this->debug_.saw_challenge || saw_challenge; const RadioCaptureInfo &capture = (*this->radio_ptr_)->get_last_capture(); - // Every wait_for_packet() clears the radio's capture before it starts listening, so a plain - // "latest wins" here meant a failure report always described the *final*, timed-out wait — which - // by construction saw nothing. `cap_valid=0` on a timeout was therefore tautological rather than - // evidence, and it hid the only distinction that matters when a device goes quiet: whether the - // radio never detected a frame at all, or received one this layer then threw away. Keep the - // first informative capture of the exchange instead of letting a later empty one erase it. + // wait_for_packet() clears the radio's capture before it starts listening, so the *last* call + // recorded here is always the final, timed-out wait — which by construction saw nothing. Keep the + // first informative capture of the exchange instead: it's the only one that can distinguish "the + // radio never detected a frame" from "it received one and this layer discarded it", which is the + // question a failure report actually needs to answer. if (!capture.valid && this->debug_.capture_valid) return; this->debug_.capture_valid = capture.valid; @@ -195,17 +194,17 @@ const char *inbound_stage_name(exchange::InboundAuthState state) { /// Check if frame is a 0x3D challenge response. bool frame_is_challenge_response(const IoFrame &frame) { return frame.cmd == CMD_CHALLENGE_RESP; } -/// Log a frame that arrived but could not be parsed. These were recorded into the debug snapshot -/// and never printed, which made "the radio heard nothing" and "we heard something and rejected -/// it" look identical in a failure report — the two need opposite fixes. Redacted through the same -/// helper as every other frame log, so an unparsable frame can't leak key material by being -/// unrecognisable (see ADR 0011). +/// Log a frame that arrived but could not be parsed. Printing it distinguishes "the radio heard +/// nothing" from "we heard something and rejected it" in a failure report — the two need opposite +/// fixes: a device that never transmitted needs a longer wait or a link check, while one that +/// transmits noise this layer can't decode needs the RX bandwidth or framing looked at. Redacted +/// through the same helper as every other frame log, so an unparsable frame can't leak key material +/// by being unrecognisable (see ADR 0011). void log_unparsable_frame(const char *stage, int tries, const RadioRxPacket &packet) { - // The *fact* stays unconditional: it means the receiver is demodulating traffic it cannot - // decode, which is a real fault worth surfacing to someone who never enables a debug flag — a - // misconfigured RX bandwidth showed up as several of these a minute. The bytes only help someone - // already debugging the PHY, and dumping them at that rate is what makes a log unreadable, so - // they sit behind the frame-log flag with the rest of that detail. + // The *fact* that a frame failed to parse stays unconditional — it is a real fault worth + // surfacing to someone who never enables a debug flag. The raw bytes only help someone already + // debugging the PHY, and a noisy channel can produce several of these a minute, so they sit + // behind the frame-log flag with the rest of that detail. ESP_LOGW(TAG, "%s try=%d: %u bytes did not parse as a frame on %" PRIu32 " Hz", stage, tries, packet.len, packet.freq_hz); #ifdef IOHOME_FRAME_LOG @@ -233,6 +232,7 @@ ExchangeOutcome ExchangeEngine::send_and_receive(const IoFrame &request, IoFrame this->reset_debug(request.cmd); const uint16_t request_preamble = is_start(request) ? LONG_PREAMBLE : (*this->radio_ptr_)->response_preamble(); const uint32_t exchange_begin_ms = millis(); + bool accepted_without_reply = false; for (uint8_t tries = 0; tries < EXCHANGE_RETRY_COUNT; tries++) { exchange::OutboundExchangeContext context; @@ -280,13 +280,15 @@ ExchangeOutcome ExchangeEngine::send_and_receive(const IoFrame &request, IoFrame auto final_disp = this->wait_for_final_response_(request, context); if (final_disp != decisions::ExchangeFinalResponseDisposition::ACCEPT) { // The device challenged us and accepted our answer, so it demonstrably received the request. - // Not every device closes the exchange with a synchronous reply (see ExchangeOutcome), and - // retrying here is actively harmful: the command is already executing, so the two remaining - // tries re-send a movement command to a device that is mid-move and, having already acted, - // ignores them — which is what turned a working command into a reported failure. + // Not every device closes the exchange with a synchronous reply (see ExchangeOutcome). A + // retry is safe only for a request with no side effect to repeat — CMD_EXECUTE is already + // acting on the first copy, so it stops here; everything else spends its full retry budget. context.state = exchange::OutboundExchangeState::SUCCESS; this->record_debug("success_auth_unconfirmed", context.try_index, true); - return ExchangeOutcome::SUCCESS_UNCONFIRMED; + accepted_without_reply = true; + if (!decisions::retry_after_unconfirmed_accept_is_safe(request.cmd)) + return ExchangeOutcome::SUCCESS_UNCONFIRMED; + continue; } context.state = exchange::OutboundExchangeState::SUCCESS; @@ -295,7 +297,10 @@ ExchangeOutcome ExchangeEngine::send_and_receive(const IoFrame &request, IoFrame return ExchangeOutcome::SUCCESS_WITH_RESPONSE; } - return ExchangeOutcome::FAILED; + // An exchange that authenticated on some try but never got a reply is not the same as one the + // device never answered at all: callers that only need "the request landed" can act on it, and + // callers that need the payload still cannot. + return accepted_without_reply ? ExchangeOutcome::SUCCESS_UNCONFIRMED : ExchangeOutcome::FAILED; } // ============================================================================ diff --git a/components/home_io_control/exchange_engine.h b/components/home_io_control/exchange_engine.h index 397dced..e6cab45 100644 --- a/components/home_io_control/exchange_engine.h +++ b/components/home_io_control/exchange_engine.h @@ -44,23 +44,25 @@ namespace home_io_control { /// Deliberately not a bool: "the device accepted the command" and "the device told us what /// happened" are different facts, and some devices only ever deliver the first. /// -/// A Somfy RS100 challenges a command, authenticates it, executes it — and then transmits nothing -/// for 3–12 seconds, reporting via an asynchronous status update instead (measured 2026-08-15 -/// across eight authenticated commands: next device frame at 3.4 s, 3.6 s, 5.3 s, 12.0 s, or -/// never, against a 500 ms window). A Somfy awning on the same protocol closes the exchange -/// properly with a synchronous 0x04 (tests/corpus/captures/somfy_awning/exchange_open_sx1276.yaml), -/// so the four-frame exchange is real — just not universal. +/// Some devices challenge a command, authenticate it, execute it, and then transmit nothing for +/// several seconds — up to a dozen — reporting via an asynchronous status update later instead of +/// closing the exchange with a synchronous reply, all well outside the exchange's own response +/// window. Other devices on the same protocol close the exchange properly with a synchronous 0x04 +/// (see tests/corpus/captures/somfy_awning/exchange_open_sx1276.yaml), so the four-frame exchange +/// is real — just not universal, and a caller cannot assume either shape from the command alone. /// -/// Treating the RS100's silence as failure made every command it *did* execute report as failed, -/// left the hub's position permanently stale, and re-sent two more copies of a movement command to -/// a shutter that was already moving. +/// SUCCESS_UNCONFIRMED exists so that silence after a real authentication is not treated the same +/// as a request the device may never have heard at all: the two need different retry rules (see +/// decisions::retry_after_unconfirmed_accept_is_safe()) and different reporting to the caller. enum class ExchangeOutcome : uint8_t { FAILED, ///< No usable reply; the device may never have heard the request. SUCCESS_WITH_RESPONSE, ///< Device replied; the caller's `response` frame is populated. SUCCESS_UNCONFIRMED, ///< Device authenticated the request — so it received and accepted it — ///< but sent no final response. `response` is NOT populated. Callers that ///< need payload (key exchange) must treat this as failure; callers that - ///< only need "the command landed" should treat it as success. + ///< only need "the command landed" should treat it as success. For every + ///< command but CMD_EXECUTE, this outcome is only returned after the full + ///< retry budget is spent — see retry_after_unconfirmed_accept_is_safe(). }; class ExchangeEngine { diff --git a/components/home_io_control/hub_core.cpp b/components/home_io_control/hub_core.cpp index 87540f1..a4cf028 100644 --- a/components/home_io_control/hub_core.cpp +++ b/components/home_io_control/hub_core.cpp @@ -329,19 +329,20 @@ void IOHomeControlComponent::loop() { // A blocking exchange makes the radio deaf for 1–3 s. When a linked remote's press schedules a // status poll, dispatching it while that same remote is still transmitting would blind the hub // to the rest of the press — so background polls yield for a moment. Control operations never do. - if (!this->busy_ && !this->defer_background_poll_()) + if (!this->busy_ && !this->defer_background_poll_()) { this->process_pending_operation_(); + } - // Frequency hopping — protocol specifies 2.7ms per channel, but ESPHome calls - // loop() every ~16-30ms. This is acceptable for a controller: we initiate all - // exchanges with a long preamble (1024 bytes ≈ 330ms airtime) so the device has - // time to detect us regardless of channel alignment. Precise hopping would only - // matter for a passive receiver scanning for unsolicited frames. - // Diagnostics build flag: park the receiver on one channel instead of hopping. A hopping monitor - // is on any given channel roughly a third of the time, so "the capture never shows frame X" is - // weak evidence — locking to the channel under study makes an absence mean something. Define it - // to the channel in Hz, e.g. -DIOHOME_LOCK_CHANNEL_HZ=868950000 for CH2, the command channel. - // Only useful for a passive monitor: a hub that cannot hop will miss replies on other channels. + // Frequency hopping — protocol specifies 2.7ms per channel, but ESPHome calls + // loop() every ~16-30ms. This is acceptable for a controller: we initiate all + // exchanges with a long preamble (1024 bytes ≈ 330ms airtime) so the device has + // time to detect us regardless of channel alignment. Precise hopping would only + // matter for a passive receiver scanning for unsolicited frames. + // Diagnostics build flag: park the receiver on one channel instead of hopping. A hopping monitor + // is on any given channel roughly a third of the time, so "the capture never shows frame X" is + // weak evidence — locking to the channel under study makes an absence mean something. Define it + // to the channel in Hz, e.g. -DIOHOME_LOCK_CHANNEL_HZ=868950000 for CH2, the command channel. + // Only useful for a passive monitor: a hub that cannot hop will miss replies on other channels. #ifdef IOHOME_LOCK_CHANNEL_HZ if (!this->busy_ && this->radio_ != nullptr && this->radio_->get_current_freq() != IOHOME_LOCK_CHANNEL_HZ) this->radio_->change_frequency(IOHOME_LOCK_CHANNEL_HZ); diff --git a/components/home_io_control/hub_core.h b/components/home_io_control/hub_core.h index 689ba77..5d63b5a 100644 --- a/components/home_io_control/hub_core.h +++ b/components/home_io_control/hub_core.h @@ -538,6 +538,16 @@ class IOHomeControlComponent : public Component, /// @param device_id ID of the device to poll. /// @param initial_delay_ms Delay before the first follow-up poll. void begin_status_poll_tracking_(const std::string &device_id, uint32_t initial_delay_ms); + /// Arm the confirming poll that follows a command, because a CMD_EXECUTE reply is never trusted + /// for position (see update_device_status_()'s trust_position parameter) and therefore leaves the + /// hub with no idea where the device actually is. Re-arms the bounded tracking window rather than + /// only setting a due time: the same untrusted reply clears that window whenever it claims the + /// device is stopped, and pop_due_device() discards a due poll that has no active window. An + /// already-scheduled earlier poll wins. + /// @param device_id Device the command was sent to. + /// @param for_stop True for STOP (and position POS_STOP), which settles under + /// STOP_SETTLE_POLL_CAP_MS instead of the normal settle cadence. + void arm_execute_confirmation_poll_(const std::string &device_id, bool for_stop); /// Schedule status polls for a fixed list of devices (shared by the id-linked and /// class-linked 1W paths, and by schedule_linked_remote_polls_()). /// @param device_ids Devices to poll. @@ -583,22 +593,26 @@ class IOHomeControlComponent : public Component, /// @param linked True if the sender is linked to at least one registered device. /// @param src_id Sender's node ID as a string (already computed by the caller). void maybe_fire_sender_event_(const OneWayFrameInfo &info, bool linked, const std::string &src_id); - /// Shared request/response helper for high-level operations. - /// @param device_id Target device ID. - /// @param request Outbound request frame. - /// @param warn_on_no_response If true, logs a warning when no response is received. - /// @param retry_after_fail_ms If non-zero, schedules next status poll after this delay on failure. - /// @return true if the device acknowledged *or* accepted the request without replying — see - /// @ref ExchangeOutcome, and the CMD_EXECUTE carve-out in the definition. - /// /// Handle an explicit CMD_ERROR_RESP refusal from the device: record the result code, stamp link /// health, and schedule the poll backoff. Split out of execute_request_and_update_() to keep that /// function's outcome dispatch readable — a refusal is a distinct concern from "what did the /// exchange achieve". + /// @param device_id Target device ID. + /// @param request Outbound request frame that drew the refusal. + /// @param response The CMD_ERROR_RESP frame. + /// @param retry_after_fail_ms If non-zero, schedules next status poll after this delay. /// @return Always false; a refusal is never a success. bool handle_error_response_(const std::string &device_id, const IoFrame &request, const IoFrame &response, uint32_t retry_after_fail_ms); + /// Shared request/response helper for high-level operations. + /// @param device_id Target device ID. + /// @param request Outbound request frame. + /// @param warn_on_no_response If true, logs a warning when no response is received. + /// @param retry_after_fail_ms If non-zero, schedules next status poll after this delay on failure. + /// @return true when the device replied, or when a CMD_EXECUTE was accepted without a reply — + /// every other command's unconfirmed acceptance is still a failure here (see + /// @ref ExchangeOutcome and decisions::retry_after_unconfirmed_accept_is_safe()). bool execute_request_and_update_(const std::string &device_id, const IoFrame &request, bool warn_on_no_response, uint32_t retry_after_fail_ms = 0); /// Execute a named device command (STOP, FAVORITE, VENT, FORCE_OPEN) via the authenticated exchange. diff --git a/components/home_io_control/hub_decisions.h b/components/home_io_control/hub_decisions.h index f65b53a..732f47b 100644 --- a/components/home_io_control/hub_decisions.h +++ b/components/home_io_control/hub_decisions.h @@ -105,6 +105,16 @@ inline ExchangeFinalResponseDisposition classify_exchange_final_response(const I : ExchangeFinalResponseDisposition::IGNORE_UNRELATED; } +/// Whether an authenticated-but-unanswered request may be sent again. +/// +/// CMD_EXECUTE is the only request the hub sends that moves something, so a retry there is a +/// second side effect on a device already acting on the first copy. Every other request (status +/// polls, name reads, management actions, config writes) is idempotent and keeps its full retry +/// budget when the device authenticates but never closes the exchange. +/// @param cmd Command byte of the outbound request. +/// @return true when the remaining retries should still be spent. +[[nodiscard]] inline bool retry_after_unconfirmed_accept_is_safe(uint8_t cmd) { return cmd != CMD_EXECUTE; } + // == Pairing discovery & key-challenge classification == /// Decide if a frame is a valid discovery response (0x29) during pairing. diff --git a/components/home_io_control/hub_operations.cpp b/components/home_io_control/hub_operations.cpp index 6a4d37f..c718081 100644 --- a/components/home_io_control/hub_operations.cpp +++ b/components/home_io_control/hub_operations.cpp @@ -77,6 +77,14 @@ const char *position_rejection_profile(const IoDevice &dev, uint8_t position) { } // namespace +void IOHomeControlComponent::arm_execute_confirmation_poll_(const std::string &device_id, bool for_stop) { + uint32_t const existing = this->poll_policy_.get_next_update(device_id); + uint32_t const delay_ms = settle_delay_ms(this->poll_policy_.get_interval(device_id), 0, for_stop); + this->begin_status_poll_tracking_(device_id, delay_ms); + if (existing != 0 && existing < millis() + delay_ms) + this->poll_policy_.set_next_update(device_id, existing); +} + // Execute an authenticated request on the standard command channel and, on success, feed the // device's reply back through the normal inbound status parser so all state normalization stays // in one place. @@ -190,9 +198,7 @@ bool IOHomeControlComponent::set_device_position(const std::string &device_id, u this->schedule_background_poll_backoff_(device_id, this->exchange_engine_.get_debug().saw_challenge); return false; } - if (position != POS_STOP && this->poll_policy_.get_interval(device_id) != 0 && - this->poll_policy_.get_next_update(device_id) == 0) - this->begin_status_poll_tracking_(device_id, this->poll_policy_.get_interval(device_id)); + this->arm_execute_confirmation_poll_(device_id, position == POS_STOP); return true; } @@ -227,19 +233,7 @@ bool IOHomeControlComponent::execute_device_command_(const std::string &device_i this->schedule_background_poll_backoff_(device_id, this->exchange_engine_.get_debug().saw_challenge); return false; } - if (cmd != CoverCommand::STOP && this->poll_policy_.get_interval(device_id) != 0 && - this->poll_policy_.get_next_update(device_id) == 0) - this->begin_status_poll_tracking_(device_id, this->poll_policy_.get_interval(device_id)); - // STOP: if the device is still moving (decelerating or settling to a rest position), cap the - // settle poll to STOP_SETTLE_POLL_CAP_MS. The private response is the shared reply to both polls - // and commands, so it cannot mark itself as a STOP; this is the one place that knows a STOP was - // sent and shortens the settle the response handler scheduled (which already folded in any hint). - if (cmd == CoverCommand::STOP && dev != nullptr && !dev->is_stopped) { - uint32_t const cap = millis() + STOP_SETTLE_POLL_CAP_MS; - uint32_t const existing = this->poll_policy_.get_next_update(device_id); - if (existing == 0 || cap < existing) - this->poll_policy_.set_next_update(device_id, cap); - } + this->arm_execute_confirmation_poll_(device_id, cmd == CoverCommand::STOP); return true; } @@ -268,8 +262,7 @@ bool IOHomeControlComponent::set_device_tilt(const std::string &device_id, uint8 this->schedule_background_poll_backoff_(device_id, this->exchange_engine_.get_debug().saw_challenge); return false; } - if (this->poll_policy_.get_interval(device_id) != 0 && this->poll_policy_.get_next_update(device_id) == 0) - this->begin_status_poll_tracking_(device_id, this->poll_policy_.get_interval(device_id)); + this->arm_execute_confirmation_poll_(device_id, false); return true; } @@ -299,8 +292,7 @@ bool IOHomeControlComponent::set_device_position_and_tilt(const std::string &dev this->schedule_background_poll_backoff_(device_id, this->exchange_engine_.get_debug().saw_challenge); return false; } - if (this->poll_policy_.get_interval(device_id) != 0 && this->poll_policy_.get_next_update(device_id) == 0) - this->begin_status_poll_tracking_(device_id, this->poll_policy_.get_interval(device_id)); + this->arm_execute_confirmation_poll_(device_id, false); return true; } diff --git a/components/home_io_control/platform_cover.cpp b/components/home_io_control/platform_cover.cpp index 60d6d70..c9c5ef2 100644 --- a/components/home_io_control/platform_cover.cpp +++ b/components/home_io_control/platform_cover.cpp @@ -158,23 +158,18 @@ void IOHomeCover::on_device_update_(const std::string &id, const IoDevice &dev) this->current_operation = operation_toward(invert, dev.position, dev.target); } else if (dev.target != UNKNOWN_POSITION && dev.position == UNKNOWN_POSITION) { // Moving, target known, live position withheld. Not every actuator publishes intermediate - // positions: a Somfy RS100 answers every poll mid-travel with current = POS_UNKNOWN (0xD4), - // flagging itself 0x61 while moving against 0x60 at rest, and only reports a real value once - // it settles. Both remaining branches below need a live position, so this used to fall off the - // end leaving current_operation at whatever it was before the command — Home Assistant showed - // the cover sitting idle for the entire travel. The last position we *did* see is enough to - // say which way it is going, and `this->position` keeps displaying that value meanwhile - // rather than blanking, because the assignment above is skipped for an unknown reading. + // positions: some report current = POS_UNKNOWN (0xD4) on every poll mid-travel (flagging + // themselves "moving" vs. "at rest" instead), and only report a real value once they settle. + // The last position we *did* see is enough to say which way the device is going, and + // `this->position` keeps displaying that value meanwhile rather than blanking, because the + // assignment above is skipped for an unknown reading. this->current_operation = operation_toward(invert, previous_io_position, dev.target); } else if (dev.position != UNKNOWN_POSITION) { // Either no target at all, or a target equal to the current position while the device says it - // is moving. The second case is not a standstill and it is emphatically not "closing": devices - // flag themselves as moving while still reporting their *pre-command* target, so a shutter - // resting closed at 100 and told to open reports `position=100 target=100 moving` for the - // first second or so. Comparing those two with `<` put that in the CLOSING bucket, and Home - // Assistant showed a closed cover briefly closing before it began to open — observed on a - // Somfy RS100, 16 such frames across the field logs. Fall through to the delta inference, - // which reports IDLE until an actual position change reveals the direction. + // is moving. The second case is not a standstill: a device flags itself moving while still + // reporting its *pre-command* target for roughly the first second, so equal endpoints here mean + // "no information yet", not "closing". Fall through to the delta inference, which reports IDLE + // until an actual position change reveals the direction. this->current_operation = this->infer_operation_from_position_delta_(invert, dev.position); } diff --git a/components/home_io_control/platform_cover.h b/components/home_io_control/platform_cover.h index 5a25486..2938651 100644 --- a/components/home_io_control/platform_cover.h +++ b/components/home_io_control/platform_cover.h @@ -71,7 +71,6 @@ class IOHomeCover : public cover::Cover, public Component, public DeviceBoundEnt /// @param invert Effective inversion mode for this device. /// @param current_io_position Latest reported IO position (0=open, 100=closed). /// @return OPENING, CLOSING, or IDLE when no direction can be inferred yet. - /// [[nodiscard]] cover::CoverOperation infer_operation_from_position_delta_(bool invert, float current_io_position) const; diff --git a/components/home_io_control/proto_commands.cpp b/components/home_io_control/proto_commands.cpp index 8cad17f..fe52f66 100644 --- a/components/home_io_control/proto_commands.cpp +++ b/components/home_io_control/proto_commands.cpp @@ -24,23 +24,15 @@ constexpr uint8_t POSITION_PERCENT_MAX = 100; constexpr uint8_t EXECUTE_ORIGINATOR = ORIGINATOR_USER_REMOTE; /// ACEI byte for execute commands — composed from priority and validity bits. /// -/// Level=3 (user_default), matching what a real 2W hub puts on the air. A 2026-08-14 third-party -/// capture of a Velux KIG300 commanding a Somfy RS100 shows its EXECUTE payload as -/// `01 63 C8 00 80 32 00 00` — ACEI 0x63, level 3 — and that command is obeyed, on the same -/// shutter and in the same minutes that this hub's own commands were being silently ignored. +/// Level=3 (user_default), matching what a real 2W hub puts on the air: a third-party capture of a +/// Velux KIG300 commanding a Somfy RS100 shows its EXECUTE payload as `01 63 C8 00 80 32 00 00` — +/// ACEI 0x63, level 3. The 0x43 (user_high) alternative comes from a 1W remote reference vector +/// (tests/corpus/captures/reference_1w_vectors/oneway_execute_iv_vector.yaml), not a 2W hub, and a +/// handheld remote claiming a higher priority than a home-automation hub is unsurprising. /// -/// This was level=2 (user_high, 0x43). Two things argued for that and neither survives contact -/// with the KIG300 capture: -/// - "matches real IO-homecontrol remotes": the 0x43 reference -/// (tests/corpus/captures/reference_1w_vectors/oneway_execute_iv_vector.yaml) is a *1W remote* -/// frame, and its origin is a documentation example whose own MAC field is a stated -/// placeholder — not a captured 2W hub. A handheld remote claiming a higher priority than a -/// home-automation hub is entirely plausible; we are the hub. -/// - "avoids RESULT_PRIORITY_LOCKED_NON_EXEC (0x38) rejections on devices locked at level 3": -/// still a real risk, and the reason to keep this in one named place. But the observed failure -/// mode is not a 0x38 rejection — it is no reply at all, while a level-3 controller on the same -/// network is obeyed. Watch for 0x38 in command results after this change; if it reappears, the -/// old value is one edit away and the tradeoff is genuine. +/// A device locked at level 3 rejects this with RESULT_PRIORITY_LOCKED_NON_EXEC (0x38) rather than +/// silence. Watch command results for 0x38 after touching this value; if it appears, level=2 +/// (0x43) is the fallback and the priority/reliability tradeoff is real. /// /// Composition: (ACEI_LEVEL_USER_DEFAULT << 5) | (0 << 3) | (1 << 1) | 1 = 0x63. constexpr uint8_t EXECUTE_ACEI = @@ -66,22 +58,15 @@ constexpr uint8_t EXECUTE_ACEI_FORCE_OPEN = constexpr size_t EXECUTE_PAYLOAD_SIZE = 8; /// Bit flag that marks the standard position payload layout after the encoded position byte. constexpr uint8_t EXECUTE_POSITION_LAYOUT_FLAG = 0x80; -/// Controller-capture matched helper byte used in normal execute payloads. +/// Travel-profile byte for a normal-speed move — the last field of the extended execute block. constexpr uint8_t EXECUTE_POSITION_PROFILE = 0x06; -/// Travel-profile byte — the last field of the extended block. Selects how fast the motor moves. +/// Travel-profile byte for a silent (slow) move — same field, selecting reduced motor speed. /// -/// Isolated 2026-08-15 by the cleanest experiment available: a Somfy hub commanding one RS100, -/// same command, same direction, with only the app's "silent operation" toggle flipped between the -/// two (monitor locked to CH2): -/// silent off: `01 67 00 00 80 D8 06 00` -/// silent on : `01 67 00 00 80 D8 05 00` -/// One byte. EXECUTE_POSITION_PROFILE (0x06) — carried here since long before this was understood, -/// as an unexplained "controller-capture matched helper byte" — turns out to *be* the normal -/// profile, so the payload this hub already sent was a correct normal-speed command all along. -/// -/// The same toggle on a Velux KIG300 produces a different encoding (`80 32 00 00` for silent, and -/// no extended block at all for normal). Two vendors filling an optional field their own way is -/// unremarkable; ours matches Somfy byte for byte, so Somfy's is the encoding to follow. +/// A Somfy hub commanding one RS100, same command and direction, with only the app's "silent +/// operation" toggle flipped, differs by exactly this one byte (`... D8 06 00` normal vs. +/// `... D8 05 00` silent); this hub follows Somfy's encoding rather than Velux's (which uses a +/// different byte, `80 32 00 00`, and omits the extended block entirely for a normal move) because +/// this hub's own hardware matches Somfy byte for byte. constexpr uint8_t EXECUTE_PROFILE_SILENT = 0x05; /// Short payload length for special execute commands such as stop/favorite. constexpr size_t EXECUTE_SPECIAL_PAYLOAD_SIZE = 6; @@ -432,21 +417,11 @@ static bool create_challenge_req_framed(IoFrame &f, const uint8_t *dst, const ui /// Build a challenge request (0x3C) using a caller-supplied challenge. See proto_commands.h. /// -/// Framed exactly like the device-role builder below (`0E 00 ...`), which is not an oversight. -/// This used to set START and LOW_POWER on the theory that a controller challenging a device -/// opens its own exchange and should flag its target's power class. Nothing on air supports that: -/// across three field captures (2026-08-14) every single 0x3C observed — from a Somfy RS100 and -/// three other actuators — is `0E 00`, and the one reference 2W hub on that network (a Velux -/// KIG300) never issues a 0x3C at all, only ever answering them. Our hub was the only node -/// emitting `4E 20`, and not one of those challenges was ever answered: five sent, zero replies, -/// which is why every status update addressed to us ends in `auth_failed` and its position is -/// discarded. -/// -/// So the controller-role framing was an assumption with no positive evidence behind it, and the -/// observed-on-air framing is the better default. Kept as a separate entry point from -/// create_challenge_req_device_role() because the call sites and rationale differ (inbound -/// authentication vs the key-extraction responder) and because this is a field experiment — if -/// devices still do not answer, the flags here are the one thing to put back. +/// Framed exactly like the device-role builder below (`0E 00`, no START, no LOW_POWER) — matches +/// every 0x3C observed on air across multiple actuators and vendors. Kept as a separate entry +/// point from create_challenge_req_device_role() because the call sites and rationale differ +/// (inbound authentication vs the key-extraction responder). If devices ever stop answering our +/// challenges, START/LOW_POWER framing is the first thing to try restoring here. bool create_challenge_req(IoFrame &f, const uint8_t *dst, const uint8_t *src, const uint8_t challenge[HMAC_SIZE]) { return create_challenge_req_framed(f, dst, src, challenge, /*start=*/false, /*low_power=*/false); } diff --git a/components/home_io_control/proto_timing.h b/components/home_io_control/proto_timing.h index a55836a..c3fe0c9 100644 --- a/components/home_io_control/proto_timing.h +++ b/components/home_io_control/proto_timing.h @@ -43,24 +43,16 @@ static constexpr int32_t RESPONSE_WAIT_MS = 500; ///< Wait for response /// Wait for a response to a start frame — the first frame of an exchange, and the one a sleeping /// device has just been woken by. /// -/// Sized from measurement, not from a worst case. On 2026-08-14 the hub logged its own -/// request→reply latency (`Auth challenge ... wait_ms=`) across a run against a Somfy RS100: 235, -/// 486, 486, 236, 486 ms — try 1 always ~235 ms, try 2 always ~486 ms, which is 235 plus the -/// EXCHANGE_RETRY_DELAY_MS gap. Zero variance. Subtract the 213 ms long preamble and the device is -/// answering within a few milliseconds of the carrier dropping. +/// This device class replies within a few milliseconds of the carrier dropping, or not at all — +/// it is fast-or-never, not slow. A failure therefore shows up as `saw_challenge=0` with no frame +/// received at all, rather than as a late arrival, so a longer window cannot fix a device that +/// genuinely fails to answer. /// -/// The device is therefore *not* slow: it replies almost immediately or not at all, and a failure -/// shows up as `saw_challenge=0` with no frame received at all rather than as a late arrival. An -/// earlier 1000 ms value here was set from a much larger apparent latency spread (29 ms–3052 ms), -/// which turned out to be an artifact of pairing frames by proximity in a third-party sniff with -/// three controllers sharing one channel — the pairings were wrong. Nothing was ever caught later -/// than ~235 ms once the hub measured it directly. -/// -/// 400 ms is comfortably above every observed reply while keeping a failed exchange inside -/// EXCHANGE_TOTAL_BUDGET_MS, so a dead device no longer blocks the ESPHome loop past its own -/// warning threshold (ADR 0013). Raise `exchange_start_response_wait_ms` from YAML if a device -/// ever genuinely answers late — but check `wait_ms` in the logs first, because a fast-or-never -/// device is a turnaround problem and a longer window cannot fix it. +/// 400 ms sits comfortably above every directly measured reply while keeping a failed exchange +/// inside EXCHANGE_TOTAL_BUDGET_MS, so a dead device no longer blocks the ESPHome loop past its own +/// warning threshold (ADR 0013). Raise `exchange_start_response_wait_ms` from YAML if a device ever +/// genuinely answers late — but check `wait_ms` in the logs first, since a fast-or-never device is a +/// turnaround problem that a longer window cannot fix. static constexpr int32_t RESPONSE_START_WAIT_MS = 400; static constexpr int32_t RESPONSE_AUTH_WAIT_MS = @@ -71,16 +63,14 @@ static constexpr uint8_t EXCHANGE_RETRY_COUNT = 3; ///< Attempts per comma /// Wall-clock ceiling on one whole exchange, retries included. /// /// EXCHANGE_RETRY_COUNT tries x (long preamble + response window + retry gap) is what actually -/// determines how long a failing command blocks the ESPHome loop (ADR 0013). Once -/// RESPONSE_START_WAIT_MS grew to cover slow devices, three full tries reached ~4.2 s and tripped -/// ESPHome's own "took a long time for an operation" warning (threshold 2550 ms) on every failure -/// -- observed in the field on 2026-08-14 with a hub driving ~18 shutters, where that blocking -/// also starves the receive path the rest of the exchange depends on. +/// determines how long a failing command blocks the ESPHome loop, and that blocking also starves +/// the receive path the rest of the exchange depends on. ESPHome itself warns when one operation +/// takes longer than 2550 ms (ADR 0013); this budget must stay under that threshold. /// /// So the retry count is a maximum, not a promise: a try only starts if the exchange has budget -/// left. At the current 400 ms window all three tries still fit (~2.3 s); it only starts trimming -/// them if the window is raised well past the default, which is exactly when three full tries stop -/// being affordable. One long listen is the better trade there anyway. +/// left. At the current 400 ms response window all three tries still fit (~2.3 s); raising the +/// window well past the default is what starts trimming retries, since three full tries stop being +/// affordable at that point — one long listen is the better trade there anyway. static constexpr uint16_t EXCHANGE_TOTAL_BUDGET_MS = 2500; /// Listen-before-talk (LBT) parameters for ETSI EN 300 220 compliance. diff --git a/components/home_io_control/radio_soft_phy_driver_base.cpp b/components/home_io_control/radio_soft_phy_driver_base.cpp index f73ac51..ce4c8e2 100644 --- a/components/home_io_control/radio_soft_phy_driver_base.cpp +++ b/components/home_io_control/radio_soft_phy_driver_base.cpp @@ -433,14 +433,11 @@ bool SoftPhyDriverBase::send_packet(const uint8_t *data, uint8_t len, const Radi // point, so a frame arriving during the delay is still captured in hardware. delayMicroseconds(this->post_tx_settle_us_); - // A peer can reply within a millisecond or two of our carrier dropping — field measurement of a - // Somfy RS100 puts its challenge at a fixed ~22 ms after exchange start, i.e. within ms of the - // transmission ending. That makes this number the margin the whole exchange lives on: if - // re-arming outlasts the peer's turnaround the reply is not late, it is never heard at all, and - // no response-window length can recover it. Behind the frame-log flag with the rest of the - // PHY-level instrumentation: it fires on *every* transmission, and the measurement it exists to - // settle (~390 us, against the ~20 ms available) is long since settled. The timing calls compile - // out with it, because this is the one code path where microseconds were ever in question. + // A peer can reply within a millisecond or two of our carrier dropping, so re-arm time is the + // margin the whole exchange lives on: if it outlasts the peer's turnaround, the reply is not + // late, it is never heard at all, and no response-window length can recover it. Behind the + // frame-log flag with the rest of the PHY-level instrumentation because it fires on *every* + // transmission; the timing calls compile out with it. #ifdef IOHOME_FRAME_LOG ESP_LOGD(TAG, "TX->RX re-arm: %" PRIu32 " us (+%u us settle)", micros() - tx_done_us, this->post_tx_settle_us_); #endif diff --git a/components/home_io_control/tuning_config.h b/components/home_io_control/tuning_config.h index 4498393..3d6f962 100644 --- a/components/home_io_control/tuning_config.h +++ b/components/home_io_control/tuning_config.h @@ -22,19 +22,12 @@ namespace home_io_control { /// @brief Valid SX1262 RX bandwidth options (kHz register values). /// /// The numeric values are the register-encoded (double-sideband) bandwidth selectors used by -/// `RadioSX1262::set_rx_bandwidth()`. +/// `RadioSX1262::set_rx_bandwidth()` — a regular (mantissa, exponent) grid, with the bandwidth +/// roughly doubling per group. /// -/// Two of these were wrong until 2026-08-15, and this enum is where the error originated: -/// `LR1121RxBandwidth` below was created as a type alias of *this* enum, later found to carry two -/// bad codes, and corrected against RadioLib — but the correction was never brought back here, so -/// the SX1262 kept them. `0x09` is 467.0 kHz, not 156.2, and `0x07` is not a valid GFSK bandwidth -/// code at all. Field evidence matched exactly: selecting "187.2" left the radio unable to -/// complete a single exchange (18 failures, zero challenges answered, in 7.5 minutes), and -/// "156.2" behaved like the ~4x-too-wide filter it actually was. -/// -/// The whole table is a regular (mantissa, exponent) grid — triples of 0x1X / 0x1X-8 / 0x0X with -/// the bandwidth roughly doubling per group — so the three codes that were always right (0x0C, -/// 0x1B, 0x0B) confirm the two that were not. +/// Byte-for-byte identical to `LR1121RxBandwidth` below, since both chips share the same Semtech +/// GFSK bandwidth grid; the `Sx1262AndLr1121BandwidthTablesAgree` test pins that. If these two +/// tables ever need to diverge for a real chip difference, say why here. enum class SX1262RxBandwidth : uint8_t { BW_39_0_KHZ = 0x1C, ///< 39.0 kHz — narrowest; closest to the SX1276's validated 41.7 kHz. BW_46_9_KHZ = 0x14, ///< 46.9 kHz — narrow. @@ -63,18 +56,8 @@ enum class SX1276RxBandwidth : uint8_t { /// /// Byte-for-byte identical to `SX1262RxBandwidth` — both chips use the same Semtech GFSK /// bandwidth grid — and kept as a distinct enum only so each driver's options can diverge if a -/// future chip's table does. -/// -/// This enum used to be a type alias of `SX1262RxBandwidth`. 2026-07-17 LR1121 bring-up -/// cross-checked the bytes against RadioLib's `LR11x0_commands.h` and found two of the five wrong: -/// the code for "156.2 kHz" actually selects 467.0 kHz, and the one for "187.2 kHz" is not a valid -/// GFSK bandwidth code at all. The conclusion drawn at the time — that the two chips must use -/// *different* encodings — was the wrong one. The encodings agree; the SX1262 values had simply -/// always been wrong, and this enum inherited them. -/// -/// Because the diagnosis landed on "different table" rather than "shared bug", the correction was -/// never carried back, and the SX1262 kept both bad codes for another month until a 2026-08-15 -/// field sweep hit them. If these two tables ever need to differ for real, say why here. +/// future chip's table does. See `Sx1262AndLr1121BandwidthTablesAgree`, which pins the two tables +/// together; if they ever need to differ for a real chip difference, say why here. enum class LR1121RxBandwidth : uint8_t { BW_39_0_KHZ = 0x1C, ///< 39.0 kHz — narrowest; close to SX1276's validated 41.7 kHz default. BW_46_9_KHZ = 0x14, ///< 46.9 kHz — narrow. @@ -180,13 +163,11 @@ struct TuningConfig { // --- Radio / physical layer --- /// SX1262 RX bandwidth selector. /// - /// 58.6 kHz, not the former 117.3. The wide default existed to tolerate local-oscillator offset - /// across the TX->RX turnaround, back when that turnaround was slow and unmeasured; it is now - /// 390 us plus a 500 us settle, and the reason has expired. Narrow also matches the SX1276, - /// whose long-validated default is 41.7 kHz on the identical waveform, and a 2026-08-15 sweep - /// through every option on real hardware found reception improved monotonically as the filter - /// narrowed — at 58.6 kHz even a shutter that had never once obeyed this hub responded, and all - /// devices reported status correctly. + /// 58.6 kHz: narrower rejects more noise, and reception improves as the filter narrows on this + /// waveform. Matches the SX1276's long-validated 41.7 kHz default on the identical waveform. A + /// wide default would exist only to tolerate local-oscillator offset across the TX->RX + /// turnaround, but that turnaround is now a measured ~390 us plus a 500 us settle, well within + /// what the narrow filter tolerates. SX1262RxBandwidth sx1262_rx_bandwidth{SX1262RxBandwidth::BW_58_6_KHZ}; uint16_t sx1262_response_preamble{SX1262_RESPONSE_PREAMBLE}; ///< SX1262 response preamble in bytes. uint16_t sx1262_post_tx_settle_us{SX1262_POST_TX_SETTLE_US}; ///< Delay after SX1262 TX before RX (µs). diff --git a/docs/radio_diagnostics.md b/docs/radio_diagnostics.md index 2435cac..5acb736 100644 --- a/docs/radio_diagnostics.md +++ b/docs/radio_diagnostics.md @@ -134,8 +134,9 @@ your device may differ. | `sx1276_response_preamble` | SX1276 | `12` | 8–256 B | Preamble length on reply frames, for the peer to lock on. | | `sx1276_discovery_hop_slice_ms` | SX1276 | `5` | 5–200 ms | Per-channel dwell while hopping during discovery. | | `sx1262_discovery_hop_slice_ms` | SX1262 | `200` | 50–500 ms | Per-channel dwell while hopping during discovery. | -| `exchange_start_response_wait_ms` | both | `1000` | 200–4000 ms | How long to listen for a reply to a *start* frame (the first frame of a command). | +| `exchange_start_response_wait_ms` | both | `400` | 200–4000 ms | How long to listen for a reply to a *start* frame (the first frame of a command). | | `exchange_response_wait_ms` | both | `500` | 200–4000 ms | How long to listen for a reply to a continuation frame, and for the post-auth final response. | +| `exchange_total_budget_ms` | both | `2500` | 500–12000 ms | Wall-clock ceiling on one whole exchange, including retries. | | `lr1121_rx_bandwidth` | LR1121 | `117.3` | `39.0` / `46.9` / `58.6` / `78.2` / `117.3` / `156.2` / `187.2` (kHz) | Receiver bandwidth. Still `117.3` by default — untested on LR1121, but the SX1262 result below suggests trying narrower. | | `lr1121_response_preamble` | LR1121 | `8` | 8–256 B | Preamble length on reply frames, for the peer to lock on. | | `lr1121_post_tx_settle_us` | LR1121 | `500` | 0–2000 µs | Settling delay after TX before switching back to RX. | @@ -160,20 +161,13 @@ not a tunable. GFSK receiver bandwidth on the SX1262. Change it when frames arrive but fail to decode — the `did not parse as a frame` warnings in the log are a direct count of that. -*Observations:* the default was `117.3` for a long time, chosen to tolerate local-oscillator offset -across the TX→RX turnaround when that turnaround was slow and unmeasured. It is now ~390 µs plus a -500 µs settle, and that rationale has expired. A 2026-08-15 sweep through every option on real -hardware found reception improved as the filter narrowed: at `58.6` a shutter that had never once -obeyed this hub responded, and every device reported status correctly. The default is now `58.6`, -which also brings the SX1262 into line with the SX1276's long-validated `41.7` on the identical -waveform. +*Observations:* `58.6` kHz is the default — narrower rejects more out-of-band noise, and reception +on this waveform improves as the filter narrows. It also brings the SX1262 into line with the +SX1276's long-validated `41.7` kHz default on the identical waveform. A wide default would exist +only to tolerate local-oscillator offset across the TX→RX turnaround, but that turnaround is now a +measured ~390 µs plus a 500 µs settle, well within what the narrow filter tolerates. -`39.0` and `46.9` are new, and bracket the SX1276's `41.7` — worth trying if `58.6` still shows -decode failures. - -> **Two of these options were wrong before 2026-08-15.** `156.2` selected 467.0 kHz and `187.2` was -> not a valid GFSK code at all, so it disabled reception entirely. If you tested those settings on -> an older build, the result told you nothing about bandwidth. See `SX1262RxBandwidth`. +`39.0` and `46.9` bracket the SX1276's `41.7` — worth trying if `58.6` still shows decode failures. #### `sx1262_response_preamble` @@ -205,27 +199,30 @@ How long the hub listens for a device's reply before giving up on a try. Raise `exchange_start_response_wait_ms` when a device ignores commands but is known to be in range and correctly paired. -*Observations:* reply latency is a property of the **device model**, not of the radio, and the -spread is enormous. A third-party capture of one network (2026-08-14, three controllers) measured a -Somfy RS100 replying at 29 ms, 781 ms, 1548 ms, 1945 ms, 2469 ms and 3052 ms within a few minutes, -while a Somfy Oximo 40 on the same network answered in ~23 ms every time. +*Observations:* for this device class, reply latency is fast-or-never rather than variably slow — +a device answers within a few milliseconds of the carrier dropping, or it does not answer at all. +A failure therefore shows up as no frame received rather than as a late arrival, so raising this +value cannot fix a device that genuinely fails to respond; check `wait_ms` in the logs first. The +`400` ms default sits comfortably above every directly measured reply from this device class while +keeping a failed exchange inside `exchange_total_budget_ms`. Raise it only for a device you have +confirmed genuinely answers late. + +Every millisecond here is loop-blocking time on a *failed* exchange only (a successful one returns +as soon as the reply lands, see `docs/adr/0013-blocking-exchange-on-the-esphome-loop.md`), and a +failure costs this window once per retry. Raise it for a stubborn device; lower it if slow failures +are worse for you than missed commands. -Both of those are solar actuators, so **don't try to predict this from the power source** — the -Oximo stays fast at night, the RS100 does not. Nor is it stable per device: the RS100's own spread -covers two orders of magnitude, and it drifts slower over the course of a day. Treat the setting as -something to measure per installation, not to infer. +#### `exchange_total_budget_ms` -The start-frame default used to be `300`, *shorter* than the continuation default of `500` even -though its own comment promised "longer" — backwards for the one case where the target may have -been asleep until the 213 ms wake-up preamble reached it. Against 300 ms the RS100 was reachable -only in its fastest state, so it answered intermittently and got worse as the day went on. Both -radios failed identically, which is the signature of a chip-neutral protocol constant rather than a -radio problem. The default is now `1000`. +Wall-clock ceiling on one whole exchange, including retries. `exchange_start_response_wait_ms` and +`exchange_response_wait_ms` set how long each try waits; this caps how long *all* of them together +may run, so a try only starts if there is still budget left for it. -Every millisecond here is loop-blocking time on a *failed* exchange only (a successful one returns -as soon as the reply lands, see [ADR 0013](adr/0013-blocking-exchange-on-the-esphome-loop.md)), and -a failure costs this window once per retry. Raise it for a stubborn device; lower it if slow -failures are worse for you than missed commands. +*Observations:* this exists to keep a failing command from blocking the ESPHome loop past its own +"took a long time for an operation" warning threshold (2550 ms — see +`docs/adr/0013-blocking-exchange-on-the-esphome-loop.md`). If you raise either response-wait +parameter, raise this too, or later retries within the same command will silently be skipped once +the budget runs out. #### `sx1276_rx_bandwidth` diff --git a/tests/exchange_test.cpp b/tests/exchange_test.cpp index 5af1b4d..046a238 100644 --- a/tests/exchange_test.cpp +++ b/tests/exchange_test.cpp @@ -82,6 +82,7 @@ class TestableComponent : public IOHomeControlComponent { using IOHomeControlComponent::radio_; using IOHomeControlComponent::node_id_; using IOHomeControlComponent::system_key_; + using IOHomeControlComponent::exchange_engine_; }; // --- Frame builders --------------------------------------------------------- @@ -398,6 +399,154 @@ TEST(Exchange, SendAndReceive_MissingFinalResponseIsUnconfirmedSuccessNotFailure EXPECT_EQ(radio.get_send_count(), 2) << "expected exactly the request plus the auth response, with no retries"; } +TEST(Exchange, SendAndReceive_ExecuteStopsAfterOneUnconfirmedAccept) { + TestableComponent comp; + comp.initialized_ = true; + MockRadio radio; + comp.radio_ = &radio; + memcpy(comp.node_id_, test::OWN_ID, NODE_ID_SIZE); + memcpy(comp.system_key_, test::TEST_SYSTEM_KEY, AES_KEY_SIZE); + + IoFrame request{}; + create_execute_position(request, comp.node_id_, test::DST_ID, false, 100); + + uint8_t chal_data[6] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06}; + IoFrame challenge = build_challenge(test::DST_ID, comp.node_id_, chal_data); + uint8_t raw_chal[64]; + uint8_t len_chal = serialize(challenge, raw_chal, sizeof(raw_chal)); + RadioRxPacket chal_pkt{}; + chal_pkt.len = len_chal; + memcpy(chal_pkt.data, raw_chal, len_chal); + radio.queue_rx(chal_pkt); + // No final response queued. + + IoFrame response{}; + const ExchangeOutcome outcome = comp.send_and_receive_(request, response, FREQ_CH2); + + EXPECT_EQ(outcome, ExchangeOutcome::SUCCESS_UNCONFIRMED) + << "CMD_EXECUTE authenticated without a final reply must still count as accepted"; + EXPECT_EQ(radio.get_send_count(), 2) + << "CMD_EXECUTE must not retry after an unconfirmed accept: the device is already acting on it"; +} + +TEST(Exchange, SendAndReceive_StatusPollRetriesAfterUnconfirmedAccept) { + // Same radio script as SendAndReceive_ExecuteStopsAfterOneUnconfirmedAccept: one challenge + // answered, then silence for the rest of the exchange. Unlike CMD_EXECUTE, CMD_PRIVATE has no + // side effect to repeat, so an unconfirmed accept on try 1 must not stop the retry loop. + TestableComponent comp; + comp.initialized_ = true; + MockRadio radio; + comp.radio_ = &radio; + memcpy(comp.node_id_, test::OWN_ID, NODE_ID_SIZE); + memcpy(comp.system_key_, test::TEST_SYSTEM_KEY, AES_KEY_SIZE); + + IoFrame request{}; + create_get_status(request, comp.node_id_, test::DST_ID); + + uint8_t chal_data[6] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06}; + IoFrame challenge = build_challenge(test::DST_ID, comp.node_id_, chal_data); + uint8_t raw_chal[64]; + uint8_t len_chal = serialize(challenge, raw_chal, sizeof(raw_chal)); + RadioRxPacket chal_pkt{}; + chal_pkt.len = len_chal; + memcpy(chal_pkt.data, raw_chal, len_chal); + radio.queue_rx(chal_pkt); + // No final response, and no more challenges: tries 2 and 3 see nothing at all. + + IoFrame response{}; + const ExchangeOutcome outcome = comp.send_and_receive_(request, response, FREQ_CH2); + + EXPECT_EQ(outcome, ExchangeOutcome::SUCCESS_UNCONFIRMED) + << "a status poll that never got a reply is still an unconfirmed accept, not a failure"; + EXPECT_EQ(comp.exchange_engine_.get_debug().tries, EXCHANGE_RETRY_COUNT) + << "every one of the EXCHANGE_RETRY_COUNT tries must actually run, not stop after the first accept"; + EXPECT_EQ(radio.get_send_count(), 4) + << "try 1 sends the request plus the auth response (2); tries 2 and 3 each send only the " + "request, since no challenge arrives to answer (1 + 1)"; +} + +namespace { + +/// @brief Reactive mock: answers every transmitted request with a fresh challenge, but only closes +/// the exchange with a real final response on the second auth response — modelling a device that +/// accepted the first try silently (challenge answered, no reply) and only replied on retry. +/// +/// A pre-queued script doesn't work here: MockRadio's RX queue is strict FIFO across the whole +/// exchange, so a final response queued ahead of time gets dequeued during try 1's final-response +/// wait instead of try 2's — classify_exchange_final_response() only checks endpoints, and a 0x3C +/// challenge matches them just as well as a 0x04 reply. Reacting to each outbound frame as it is +/// sent keeps every queued reply aligned with the wait it's meant for. +class UnconfirmedThenFinalOnRetryMockRadio : public MockRadio { + public: + bool send_packet(const uint8_t *data, uint8_t len, const RadioTxConfig &tx) override { + bool result = MockRadio::send_packet(data, len, tx); + IoFrame frame; + if (!parse(data, len, frame)) + return result; + if (frame.cmd == CMD_PRIVATE) { + this->auth_count_this_try_ = 0; + this->queue_rx(build_challenge_packet(frame)); + } else if (frame.cmd == CMD_CHALLENGE_RESP) { + if (++this->auth_count_this_try_ == 1 && ++this->try_count_ == 2) + this->queue_rx(build_final_packet(frame)); + } + return result; + } + + private: + static RadioRxPacket build_challenge_packet(const IoFrame &request) { + uint8_t chal_data[6] = {0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF}; + IoFrame challenge{}; + init_frame(challenge, true, false, false, false); + set_dst(challenge, request.src); + set_src(challenge, request.dst); + set_cmd(challenge, CMD_CHALLENGE_REQ, chal_data, sizeof(chal_data)); + RadioRxPacket pkt{}; + uint8_t raw[64]; + pkt.len = serialize(challenge, raw, sizeof(raw)); + memcpy(pkt.data, raw, pkt.len); + return pkt; + } + + static RadioRxPacket build_final_packet(const IoFrame &auth_response) { + IoFrame resp{}; + init_frame(resp, true, false, true, false); + set_dst(resp, auth_response.src); + set_src(resp, auth_response.dst); + uint8_t payload[6] = {0}; + set_cmd(resp, CMD_PRIVATE_RESP, payload, sizeof(payload)); + RadioRxPacket pkt{}; + uint8_t raw[64]; + pkt.len = serialize(resp, raw, sizeof(raw)); + memcpy(pkt.data, raw, pkt.len); + return pkt; + } + + uint8_t auth_count_this_try_{0}; + uint8_t try_count_{0}; +}; + +} // namespace + +TEST(Exchange, SendAndReceive_StatusPollUnconfirmedThenAnsweredReturnsResponse) { + TestableComponent comp; + comp.initialized_ = true; + UnconfirmedThenFinalOnRetryMockRadio radio; + comp.radio_ = &radio; + memcpy(comp.node_id_, test::OWN_ID, NODE_ID_SIZE); + memcpy(comp.system_key_, test::TEST_SYSTEM_KEY, AES_KEY_SIZE); + + IoFrame request{}; + create_get_status(request, comp.node_id_, test::DST_ID); + + IoFrame response{}; + const ExchangeOutcome outcome = comp.send_and_receive_(request, response, FREQ_CH2); + + EXPECT_EQ(outcome, ExchangeOutcome::SUCCESS_WITH_RESPONSE) + << "a retried status poll must still return the response once one arrives"; + EXPECT_EQ(response.cmd, CMD_PRIVATE_RESP); +} + // ============================================================================ // response_preamble() behavior tests // ============================================================================ diff --git a/tests/hub_operations_test.cpp b/tests/hub_operations_test.cpp index 69b38f1..8890fe7 100644 --- a/tests/hub_operations_test.cpp +++ b/tests/hub_operations_test.cpp @@ -570,6 +570,32 @@ TEST(HubOperations, RequestDeviceStatusAuthFailureBacksOffAggressively) { << "auth-shaped failures should increment their own streak"; } +TEST(HubOperations, RequestDeviceStatusUnconfirmedAcceptStillReturnsFalse) { + // ExchangeEngine now reports SUCCESS_UNCONFIRMED for a status poll that authenticated but never + // got a final reply (Step 1's retry fix). execute_request_and_update_()'s + // unconfirmed_counts_as_success rule must still treat that as a failure for anything but + // CMD_EXECUTE — a status poll exists to obtain a payload, and there is none to hand back. + TestableComponent comp; + MockRadio radio; + setup_cover_component(comp, radio); + + auto *dev = comp.get_device("ABC123"); + ASSERT_NE(dev, nullptr); + + IoFrame challenge = build_challenge_request(dev->node_id, comp.node_id_); + uint8_t raw[64]; + uint8_t raw_len = serialize(challenge, raw, sizeof(raw)); + RadioRxPacket pkt{}; + pkt.len = raw_len; + memcpy(pkt.data, raw, raw_len); + pkt.freq_hz = FREQ_CH2; + radio.queue_rx(pkt); + // No final response queued on any try. + + EXPECT_FALSE(comp.request_device_status("ABC123")) + << "a status poll ending SUCCESS_UNCONFIRMED must still be reported as a failure to the caller"; +} + TEST(HubOperations, SetDevicePositionArmsTrackedPollingWhenConfigured) { TestableComponent comp; MockRadio radio; @@ -620,12 +646,16 @@ TEST(HubOperations, SetDevicePositionWithoutConfiguredIntervalUsesTrackedSettleP } TEST(HubOperations, StopCommandArmsTrackedPollingToConfirmRestingPosition) { + // The execute ack is never trusted for position (see update_device_status_()'s trust_position + // parameter), but is_stopped IS applied from it — and an untrusted "stopped" claim used to clear + // tracking outright, leaving the one thing that actually writes position (a trusted CMD_PRIVATE + // poll) never scheduled. arm_execute_confirmation_poll_() re-arms the window after every execute, + // so a STOP must still end with a confirming poll due within STOP_SETTLE_POLL_CAP_MS. TestableComponent comp; MockRadio radio; setup_cover_component(comp, radio); - // STOP reply arrives with is_stopped=true — tracking should be cleared after confirming position. - IoFrame resp = build_status_response(comp.node_id_); + IoFrame resp = build_status_response(comp.node_id_); // is_stopped=true, as a real STOP ack reports uint8_t raw[64]; uint8_t raw_len = serialize(resp, raw, sizeof(raw)); RadioRxPacket pkt{}; @@ -634,12 +664,15 @@ TEST(HubOperations, StopCommandArmsTrackedPollingToConfirmRestingPosition) { pkt.freq_hz = FREQ_CH2; radio.queue_rx(pkt); + uint32_t const before_ms = esphome::millis(); EXPECT_TRUE(comp.execute_device_command_("ABC123", CoverCommand::STOP)); - EXPECT_EQ(comp.poll_policy_.get_poll_deadline("ABC123"), 0u) - << "STOP confirmed by stopped reply should clear tracking"; - EXPECT_EQ(comp.poll_policy_.get_next_update("ABC123"), 0u) - << "no further polls needed once the stopped position is confirmed"; + EXPECT_NE(comp.poll_policy_.get_poll_deadline("ABC123"), 0u) + << "STOP must leave an active tracking window so the confirming poll can actually fire"; + uint32_t const next_update = comp.poll_policy_.get_next_update("ABC123"); + EXPECT_NE(next_update, 0u) << "STOP must schedule a confirming poll despite the untrusted stopped ack"; + EXPECT_LE(next_update, before_ms + STOP_SETTLE_POLL_CAP_MS + 50u) + << "the confirming poll after STOP must be due within the STOP settle cap"; } TEST(HubOperations, StopCommandWithMovingReplySchedulesShortSettlePoll) { @@ -674,6 +707,69 @@ TEST(HubOperations, StopCommandWithMovingReplySchedulesShortSettlePoll) { << "STOP must settle faster than a normal move for this test to be meaningful"; } +TEST(HubOperations, MoveWithStoppedAckSchedulesConfirmingPollWithoutConfiguredInterval) { + // The bug class is broader than STOP: set_device_position() used to re-arm only when a poll + // interval was configured, so an interval-less device hit the same dead end on an ordinary move + // whenever its untrusted execute ack happened to claim is_stopped=true. + TestableComponent comp; + MockRadio radio; + setup_cover_component(comp, radio); // no poll interval configured for "ABC123" + + IoFrame resp = build_status_response(comp.node_id_); // is_stopped=true in the untrusted execute ack + uint8_t raw[64]; + uint8_t raw_len = serialize(resp, raw, sizeof(raw)); + RadioRxPacket pkt{}; + pkt.len = raw_len; + memcpy(pkt.data, raw, raw_len); + pkt.freq_hz = FREQ_CH2; + radio.queue_rx(pkt); + + uint32_t const before_ms = esphome::millis(); + EXPECT_TRUE(comp.set_device_position("ABC123", 50)); + + uint32_t const next_update = comp.poll_policy_.get_next_update("ABC123"); + EXPECT_NE(next_update, 0u) << "a move must schedule a confirming poll even without a configured interval"; + EXPECT_LE(next_update, before_ms + DEFAULT_SETTLE_POLL_DELAY_MS + 50u) + << "an interval-less device must fall back to DEFAULT_SETTLE_POLL_DELAY_MS"; +} + +TEST(HubOperations, ConfirmingPollStopsOnceATrustedReplyReportsStopped) { + // arm_execute_confirmation_poll_() re-arms tracking after every execute, so its termination + // argument matters: the confirming poll itself is a real CMD_PRIVATE request, whose reply IS + // trusted, so once it reports the device at rest polling must actually stop. + TestableComponent comp; + MockRadio radio; + setup_cover_component(comp, radio); + + IoFrame moving_resp = build_moving_status_response(comp.node_id_, 0xFF); + uint8_t raw1[64]; + uint8_t raw1_len = serialize(moving_resp, raw1, sizeof(raw1)); + RadioRxPacket pkt1{}; + pkt1.len = raw1_len; + memcpy(pkt1.data, raw1, raw1_len); + pkt1.freq_hz = FREQ_CH2; + radio.queue_rx(pkt1); + + ASSERT_TRUE(comp.execute_device_command_("ABC123", CoverCommand::STOP)); + ASSERT_NE(comp.poll_policy_.get_next_update("ABC123"), 0u) << "setup: confirming poll must be armed"; + + IoFrame stopped_resp = build_status_response(comp.node_id_); + uint8_t raw2[64]; + uint8_t raw2_len = serialize(stopped_resp, raw2, sizeof(raw2)); + RadioRxPacket pkt2{}; + pkt2.len = raw2_len; + memcpy(pkt2.data, raw2, raw2_len); + pkt2.freq_hz = FREQ_CH2; + radio.queue_rx(pkt2); + + EXPECT_TRUE(comp.request_device_status("ABC123")); + + EXPECT_EQ(comp.poll_policy_.get_next_update("ABC123"), 0u) + << "a trusted stopped reply must stop the confirming poll from repeating — one extra poll, not a loop"; + EXPECT_EQ(comp.poll_policy_.get_poll_deadline("ABC123"), 0u) + << "a trusted stopped reply must clear the tracking window entirely"; +} + TEST(HubOperations, ForceOpenSendsPositionZeroForNonInvertedDevice) { TestableComponent comp; MockRadio radio;