diff --git a/include/rcp/discovery.hpp b/include/rcp/discovery.hpp index 53b497c..7a4a04e 100644 --- a/include/rcp/discovery.hpp +++ b/include/rcp/discovery.hpp @@ -7,41 +7,154 @@ // fusa:req REQ-DISC-007 // fusa:req REQ-DISC-008 // fusa:req REQ-DISC-009 +// fusa:req REQ-DISC-010 +// fusa:req REQ-DISC-011 +// fusa:req REQ-DISC-012 +// fusa:req REQ-DISC-013 +// fusa:req REQ-DISC-014 +// fusa:req REQ-DISC-015 +// fusa:req REQ-DISC-016 +// fusa:req REQ-DISC-017 +// fusa:req REQ-DISC-018 +// fusa:req REQ-DISC-019 +// fusa:req REQ-DISC-020 +// fusa:req REQ-DISC-021 +// fusa:req REQ-DISC-022 +// fusa:req REQ-DISC-023 +// fusa:req REQ-DISC-024 -// RC Server discovery — the broadcastable, byte_bus_id-0 read request every +// TC18 requirements-corpus completeness pass: REQ-DISC-029 is catalogued in +// this module's own requirements catalog with a "tc18" citation and a +// "status" of "not-implemented" -- it describes normative TC18 behaviour +// (DISCOVERY_STREAM_OCCUPIED, an error label that appears only in TC18 +// Figure 17's diagram, not among Table 30's 17 numbered wire error codes) +// this module does NOT itself resolve; see on_discovery_request()'s own +// doc comment. Its test pins the deviation rather than force-mapping it. +// fusa:req REQ-DISC-029 + +// RC Server discovery -- the broadcastable, byte_bus_id-0 read request every // OPEN Alliance TC18 Remote Control Protocol Specification v0.5.1_RC server // must answer regardless of lifecycle state, plus discovery-stream claiming -// for configuration (extraction §3.1, §3.5). +// for configuration (extraction §3.1, §3.5), the discovery response itself +// (a register-map slice at address 0 populated from the server's own +// generic-recognition fields, extraction §12.7.5 Table 18), its Phase 20 +// (rcp/fragment.hpp) fragmented-response counterpart, and a purely +// client-side discovery-result cache. // -// ROADMAP.md milestone 46, "Discovery (v2.2.0)": this header rides directly -// on rcp/avtp.hpp's NTSCF framing and rcp/acf.hpp's ACF_ABB message format -// and byte_bus_id addressing (v2.0.0, split from the original rcp/wire.hpp -// per RELAY spec §13.7.2) -// — it needs no changes to that codec — queries rcp/lifecycle.hpp's -// ServerState to decide whether an incoming discovery request is eligible to -// claim the discovery stream (v2.1.0), and targets rcp/regmap.hpp's EP0 -// (byte_bus_id 0, register-map address 0 — the general bootstrap/magic-number -// field region) as the thing being read (also v2.1.0). This module does not -// itself implement a byte-level serialization of the whole register map to -// and from the wire; per regmap.hpp's own header comment that remains a -// later-milestone concern. It only fixes the address a discovery request -// must carry so that later serialization work has a stable, already-decided -// target to hook into. +// ROADMAP.md Phase 17 (cpp-RCP issue #129), "Phase 4" (server/dispatch +// layer): re-derived in full from c-RCP's include/rcp/discovery.h + +// src/discovery.c (this project's RC5-spec-conformant reference +// implementation for this module) -- this header now rides on +// rcp/avtp.hpp's NTSCF framing, rcp/acf.hpp's ACF_ABB message format and +// byte_bus_id addressing, rcp/lifecycle.hpp's ServerState (to decide +// whether an incoming discovery request is eligible to claim the discovery +// stream), rcp/regmap.hpp's GeneralMap (the register-map slice a discovery +// response actually carries) and kEp0 (byte_bus_id 0 / register-map address +// 0, the general bootstrap/magic-number field region), and rcp/fragment.hpp +// (the generic ms/segment_num primitive a discovery response's own +// Phase-20 fragmented counterpart wires in). This module does not itself +// implement a byte-level serialization of the whole register map to and +// from the wire -- only the leading, generic-recognition slice +// (kDiscoveryGeneralSliceLen below) that a discovery response actually +// carries; rcp/regmap.hpp's own render()/encode_read_response() own the +// full Table 20 extent as a separate, later-milestone concern. // -// Field names and behavior below implement TC18's *behavior* as described in -// an internal structured extraction of the specification named above; no -// text from that document is reproduced here. The concrete claim-state +// Field names and behavior below implement TC18's *behavior* as described +// in an internal structured extraction of the specification named above; +// no text from that document is reproduced here. The concrete claim-state // machine and default timeout chosen in this file are this implementation's // own encoding of that behavior, same as the equivalent disclaimers in -// rcp/avtp.hpp, rcp/acf.hpp, rcp/lifecycle.hpp, and rcp/regmap.hpp. +// rcp/avtp.hpp, rcp/acf.hpp, rcp/lifecycle.hpp, rcp/regmap.hpp, and +// rcp/fragment.hpp. +// +// ── The discovery request/response exchange ───────────────────────────────── +// +// Discovery is a single ACF_ABB read request addressed to kDiscoveryByteBusId +// -- the same id regmap::kEp0 uses -- and is the one request an RC Server +// answers regardless of its current lifecycle::ServerState. +// encode_discovery_request()/decode_discovery_request() build and parse +// that request; encode_discovery_response()/decode_discovery_response() +// build and parse the reply, a register-map slice starting at address 0 of +// the requested read_size, populated from regmap::GeneralMap's own +// generic-recognition fields (magic, svr_version, vendor_id, device_id, +// svr_ep_count) -- see kDiscoveryGeneralSliceLen below for exactly how many +// of those octets this milestone actually defines. Anything vendor/device- +// specific beyond that generic slice is explicitly out of this exchange's +// scope (e.g. carried in a datasheet, not on the wire here). +// +// ── NTSCF-only ─────────────────────────────────────────────────────────────── +// +// A discovery request or response never rides on a TSCF-headed frame, in +// any lifecycle state, independent of an RC Node's own time-sync +// capability. This is a dedicated, directly-testable rule +// (should_drop_discovery(), mirroring rcp/avtp.hpp's own +// is_tscf_should_drop()-shaped convention) layered on top of -- not a +// replacement for -- rcp/avtp.hpp's general time-sync-gated TSCF drop rule +// and rcp/lifecycle.hpp's own per-state acceptance filtering. +// +// ── Discovery-stream claiming ─────────────────────────────────────────────── +// +// DiscoveryClaim models the reservation described in extraction §3.5: the +// first discovery request a server receives while it is in HwUnconfigured +// or HwConfigured reserves the discovery stream for that client's +// subsequent configuration writes, until Discovery_TimeOut elapses with no +// follow-up configuration write (on_configuration_request() extends the +// reservation on each one, matching c-RCP's rcp_discovery_claim_note_config_write() +// — see that method's own doc comment; this port's own prior revision +// instead released the claim on its very first configuration write, a +// genuine behavioral bug versus c-RCP fixed in this pass — see that +// method's own doc comment for the file:line citation). The timeout is a configurable +// constructor parameter (mirroring rcp/regmap.hpp's own request-stream +// rx_wd_timeout_ms convention), defaulted to kDefaultTimeout, not hardcoded +// elsewhere in this module. A lapsed or never-held claim is open and may be +// granted to a new requester; ordinary read-only discovery from any client +// is unaffected by claim state (it is answered per the paragraph above +// regardless), only a configuration *write*'s authorization consults the +// claim. This module deliberately does not itself decide whether a given +// register field is writable in the server's current lifecycle state -- +// that remains rcp/lifecycle.hpp's own field-writability logic's job; a +// caller combines may_configure()'s answer with that as one more input to +// an overall write-authorization decision. +// +// Every method below that reasons about elapsed time takes an explicit +// TimePoint parameter (std::chrono::steady_clock::now(), read by the +// caller) rather than reaching for the clock itself -- matching this +// codebase's standing convention (see rcp/avtp.hpp/rcp/acf.hpp/ +// rcp/lifecycle.hpp) of functions here consuming already-classified/ +// already-read inputs. This also makes the claim/timeout state machine +// fully deterministic to test. +// +// ── Client-side discovery result persistence ──────────────────────────────── +// +// DiscoveryCache is an explicitly thin convenience API, not a protocol +// requirement: nothing in this module consults it to decide whether to +// (re-)issue a discovery request, and a client that never touches it +// remains fully conformant. It exists purely so a client on a known, +// stable topology is not forced to rediscover every power cycle, should it +// choose to persist (or simply cache in-process) a prior DiscoveryResult. +// +// ── regmap::ep_id_map::is_valid_association() interplay: none ────────────── +// +// c-RCP's own discovery.c never calls rcp_regmap_ep_id_map_is_valid_association() +// (or any equivalent stream_id/byte_bus_id association check) -- discovery's +// own responsibility begins and ends with the claim/timeout state machine +// above; whether a *non-discovery* stream_id/byte_bus_id pairing is a valid +// association is entirely regmap.hpp's/lifecycle.hpp's own concern (see +// lifecycle.hpp's via_valid_stream_association). This port follows that +// same division of labor: nothing below calls +// regmap::ep_id_map::is_valid_association(). #pragma once #include #include +#include #include #include +#include +#include #include +#include #include #include #include @@ -64,13 +177,39 @@ constexpr uint32_t kDiscoveryRegisterAddress = 0; // bootstrap block may pass a larger read_size to make_discovery_request. constexpr uint16_t kDiscoveryDefaultReadSize = sizeof(uint32_t); -// ── Errors ──────────────────────────────────────────────────────────────────── +// The number of leading octets of the general register slice this +// milestone actually populates: magic (4) + svr_version (4) + vendor_id +// (2) + device_id (2) + svr_ep_count (2), each big-endian, in that order -- +// the field widths and their order are the specification's own for the +// leading, device-recognition part of the RC Server general register map +// (TC18 v0.5.1_RC §12.7.5 Table 18 "RC Server configuration static part", +// absolute addresses 0x0000..0x000D); only the field set regmap::GeneralMap +// already models is drawn from it, by reference. svr_version is FOUR octets +// wide, not two: a two-octet svr_version shifts vendor_id, device_id and +// svr_ep_count each two octets earlier than a conforming peer reads them, +// so every one of those three fields is misparsed -- pinned exactly by +// this file's own tests (mirroring c-RCP's own regression test for this, +// tests/test_discovery.c's test_response_general_slice_octet_layout()). A +// response's payload length always equals the request's read_size exactly: +// the leading min(read_size, kDiscoveryGeneralSliceLen) octets carry this +// slice, and any remaining octets (a read_size greater than this constant) +// are zero-filled reserved space for fields a future milestone may define. +constexpr size_t kDiscoveryGeneralSliceLen = 14; +// ── Errors ──────────────────────────────────────────────────────────────────── +// Numbered to match c-RCP's own rcp_discovery_errc_t (include/rcp/discovery.h) +// 1:1 for cross-reference, though the numeric value itself carries no +// meaning to std::error_code beyond equality within this category. enum class DiscoveryErrc : int { - // A TSCF-headed discovery request was received. Discovery is NTSCF-only - // (extraction §3.5); such a request is dropped rather than answered or - // partially processed. - tscf_headed_request_dropped = 1, + short_frame = 1, // b is shorter than either the AVTP or ACF fixed header, or than their declared payload lengths + // A TSCF-headed (or otherwise unrecognized-subtype) discovery frame was + // received. Discovery is NTSCF-only (extraction §3.5); such a frame is + // dropped rather than answered or partially processed. Checked before + // any ACF-level parsing is attempted (see should_drop_discovery()). + tscf_headed_request_dropped = 2, + bad_msg_type = 3, // the NTSCF payload is not an ACF_ABB message + wrong_bus = 4, // the ACF header's byte_bus_id is not kDiscoveryByteBusId + wrong_op = 5, // the ACF header's op is not "read" }; inline const std::error_category& discovery_category() noexcept { @@ -78,8 +217,16 @@ inline const std::error_category& discovery_category() noexcept { const char* name() const noexcept override { return "rcp.discovery"; } std::string message(int ev) const override { switch (static_cast(ev)) { + case DiscoveryErrc::short_frame: + return "rcp/discovery: frame too short"; case DiscoveryErrc::tscf_headed_request_dropped: return "rcp/discovery: TSCF-headed discovery request dropped (discovery is NTSCF-only)"; + case DiscoveryErrc::bad_msg_type: + return "rcp/discovery: unexpected ACF message type"; + case DiscoveryErrc::wrong_bus: + return "rcp/discovery: wrong byte_bus_id"; + case DiscoveryErrc::wrong_op: + return "rcp/discovery: wrong ACF op"; default: return "rcp/discovery: unknown error"; } @@ -93,6 +240,60 @@ inline std::error_code make_error_code(DiscoveryErrc e) noexcept { return {static_cast(e), discovery_category()}; } +// ── NTSCF-only rule ────────────────────────────────────────────────────────── (REQ-DISC-001) + +// True iff avtp_subtype is anything other than avtp::kSubtypeNtscf — in +// particular, true for avtp::kSubtypeTscf regardless of lifecycle state or +// time-sync capability, and true for any other, unrecognized subtype byte. +// Ported from c-RCP's rcp_discovery_should_drop() (src/discovery.c). +constexpr bool should_drop_discovery(uint8_t avtp_subtype) noexcept { + return avtp_subtype != avtp::kSubtypeNtscf; +} + +// ── Shared request/response ACF-level validation ──────────────────────────── (REQ-DISC-002..008/012..014) + +namespace detail { + +// decode_common_frame — common to both decode_discovery_request() and +// decode_discovery_response()/decode_discovery_response_fragment(): peel +// the NTSCF layer (applying the NTSCF-only rule first, REQ-DISC-001), then +// the ACF_ABB layer, then check byte_bus_id and op. Neither caller is +// expected to see a non-ABB, wrong-bus, or wrong-op frame in practice, but +// both validate the same way rather than assuming it. Ported from c-RCP's +// decode_common() (src/discovery.c:68-102). +inline std::error_code decode_common_frame(const uint8_t* buf, size_t len, + avtp::NtscfHeader& out_hdr, + acf::AcfMessageInfo& out_info, + std::vector& out_payload) { + if (len < 1) return make_error_code(DiscoveryErrc::short_frame); + + // Checked before any further parsing is attempted, per this file's own + // header comment: a TSCF-headed (or otherwise non-NTSCF) frame is + // dropped outright, independent of lifecycle state or time-sync + // capability. + if (should_drop_discovery(buf[0])) return make_error_code(DiscoveryErrc::tscf_headed_request_dropped); + + const auto ntscf_ec = avtp::decode_ntscf_header(buf, len, out_hdr); + if (ntscf_ec == avtp::make_error_code(avtp::AvtpErrc::short_buffer)) + return make_error_code(DiscoveryErrc::short_frame); + if (ntscf_ec) return make_error_code(DiscoveryErrc::tscf_headed_request_dropped); + + const size_t acf_off = avtp::kNtscfHeaderLen; + if (len < acf_off) return make_error_code(DiscoveryErrc::short_frame); + + const auto acf_ec = acf::decode_acf_abb(buf + acf_off, len - acf_off, out_info, out_payload); + if (acf_ec == avtp::make_error_code(avtp::AvtpErrc::short_buffer)) + return make_error_code(DiscoveryErrc::short_frame); + if (acf_ec) return make_error_code(DiscoveryErrc::bad_msg_type); + + if (out_info.byte_bus_id != kDiscoveryByteBusId) return make_error_code(DiscoveryErrc::wrong_bus); + if (out_info.op) return make_error_code(DiscoveryErrc::wrong_op); // discovery is always a read + + return {}; +} + +} // namespace detail + // ── Discovery request framing ──────────────────────────────────────────────── // make_discovery_request builds the ACF_ABB-level header for a discovery // read: unconditional read (op=false), targeting byte_bus_id 0 @@ -128,39 +329,302 @@ inline std::vector encode_discovery_request(const avtp::StreamId& strea } // decode_discovery_request decodes a raw AVTPDU frame as a discovery -// request. Per extraction §3.5, discovery is NTSCF-only: if the frame's -// AVTPDU subtype byte is TSCF, this function returns -// DiscoveryErrc::tscf_headed_request_dropped without decoding further — -// modeling "drop" as a decode failure the caller cannot accidentally ignore -// the way it could ignore a boolean flag on an otherwise-successful decode. +// request via the shared detail::decode_common_frame() validation above: +// NTSCF-only (DiscoveryErrc::tscf_headed_request_dropped otherwise), +// ACF_ABB (DiscoveryErrc::bad_msg_type otherwise), addressed to +// kDiscoveryByteBusId (DiscoveryErrc::wrong_bus otherwise), and a read, not +// a write (DiscoveryErrc::wrong_op otherwise) — DiscoveryErrc::short_frame +// for any AVTP/ACF-level short buffer along the way. On success, out_hdr's +// own stream_id is the requester's identity (NTSCF's stream_id is always +// sender-assigned) and out_info.read_size_or_segment_num/transaction_num +// are the request's own read_size and transaction_num. inline std::error_code decode_discovery_request(const uint8_t* buf, size_t len, avtp::NtscfHeader& out_hdr, acf::AcfMessageInfo& out_info, std::vector& out_payload) { - if (len < 1) return avtp::make_error_code(avtp::AvtpErrc::short_buffer); - if (buf[0] == avtp::kSubtypeTscf) { - return make_error_code(DiscoveryErrc::tscf_headed_request_dropped); - } - - auto ec = avtp::decode_ntscf_header(buf, len, out_hdr); - if (ec) return ec; - - const size_t acf_off = avtp::kNtscfHeaderLen; - if (len < acf_off) return avtp::make_error_code(avtp::AvtpErrc::short_buffer); - return acf::decode_acf_abb(buf + acf_off, len - acf_off, out_info, out_payload); + return detail::decode_common_frame(buf, len, out_hdr, out_info, out_payload); } // should_answer_discovery documents, as a single always-true call site // rather than an implicit assumption scattered across a lifecycle switch // statement, that a server answers a (NTSCF-headed) discovery request in -// *every* lifecycle state — including RCP_CONFIGURED, where the claiming +// *every* lifecycle state — including RcpConfigured, where the claiming // mechanism below no longer applies but discovery reads still must // (extraction §3.1, §3.5's mandatory-baseline requirement). constexpr bool should_answer_discovery(lifecycle::ServerState /*state*/) noexcept { return true; } -// ── Discovery-stream claiming ──────────────────────────────────────────────── +// ── Discovery response ─────────────────────────────────────────────────────── (REQ-DISC-009..014) + +namespace detail { + +// build_general_slice_payload — builds a read_size-octet payload: the +// leading min(read_size, kDiscoveryGeneralSliceLen) octets carry map's own +// magic/svr_version/vendor_id/device_id/svr_ep_count, each big-endian, in +// that order (see kDiscoveryGeneralSliceLen's own doc comment); any +// remaining octets are left zero-filled reserved space. Shared by +// encode_discovery_response() and encode_discovery_response_fragmented() +// below — c-RCP's own rcp_discovery_encode_response()/ +// _encode_response_fragmented() (src/discovery.c) duplicate this same +// slice-building logic inline in both functions; folded into one helper +// here instead. +inline std::vector build_general_slice_payload(const regmap::GeneralMap& map, + uint8_t read_size) { + std::array slice{}; + avtp::detail::put_u32(&slice[0], map.magic); + avtp::detail::put_u32(&slice[4], map.svr_version); // 32 bit, not 16 -- see kDiscoveryGeneralSliceLen + avtp::detail::put_u16(&slice[8], map.vendor_id); + avtp::detail::put_u16(&slice[10], map.device_id); + avtp::detail::put_u16(&slice[12], map.svr_ep_count); + + // A response's payload always spans exactly read_size octets -- see + // kDiscoveryGeneralSliceLen's own comment. Any octets beyond the + // populated slice are left as this zero-fill. + std::vector payload(read_size, 0); + const size_t copy_len = std::min(read_size, kDiscoveryGeneralSliceLen); + std::copy(slice.begin(), slice.begin() + static_cast(copy_len), payload.begin()); + return payload; +} + +} // namespace detail + +// make_discovery_response builds the ACF_ABB-level header for a discovery +// response: a read (op=false) response (rsp=true — TC18.txt:1885, rsp=1b +// identifies a response), echoing read_size and transaction_num. +inline acf::AcfMessageInfo make_discovery_response(uint8_t read_size, uint8_t transaction_num) noexcept { + acf::AcfMessageInfo info; + info.byte_bus_id = kDiscoveryByteBusId; + info.op = false; // read + info.rsp = true; + info.read_size_or_segment_num = read_size; + info.transaction_num = transaction_num; + return info; +} + +// Encodes the discovery response: a full NTSCF-framed ACF_ABB message whose +// payload is exactly read_size octets long (see kDiscoveryGeneralSliceLen +// above), echoing transaction_num from the originating request. +// server_stream_id becomes the NTSCF header's own stream_id — the +// responding RC Server's identity. Addressing the underlying carrier frame +// back to the *requester's* MAC (e.g. as an Ethernet destination address) +// is a transport-level concern this module does not model: a caller wiring +// this to a real carrier uses the requester field decode_discovery_request() +// already surfaces (its NtscfHeader::stream_id.mac) as that destination. +inline std::vector encode_discovery_response(const regmap::GeneralMap& map, + const avtp::StreamId& server_stream_id, + uint8_t sequence_num, + uint8_t transaction_num, + uint8_t read_size = static_cast(kDiscoveryGeneralSliceLen)) { + const auto info = make_discovery_response(read_size, transaction_num); + const auto payload = detail::build_general_slice_payload(map, read_size); + const auto acf_msg = acf::encode_acf_abb(info, payload); + + avtp::NtscfHeader hdr; + hdr.stream_id = server_stream_id; + hdr.sequence_num = sequence_num; + hdr.control_data_length = static_cast(acf_msg.size()); + + auto out = avtp::encode_ntscf_header(hdr); + out.insert(out.end(), acf_msg.begin(), acf_msg.end()); + return out; +} + +// A decoded, client-side discovery result: the responding server's own +// identity (from the response frame's NTSCF stream_id) plus the generic +// compatible-device-recognition fields carried in the response's leading +// kDiscoveryGeneralSliceLen octets. valid is always true when this struct +// is populated by decode_discovery_response()/decode_discovery_reassembled_response() +// on success; it exists so DiscoveryCache::find() can hand back a pointer +// to a "no result" cache slot without that ever being confused with a +// genuine all-zero discovery response. +struct DiscoveryResult { + bool valid = false; + avtp::StreamId server_stream_id{}; + uint32_t magic = 0; + uint32_t svr_version = 0; // 32 bit on the wire -- see kDiscoveryGeneralSliceLen + uint16_t vendor_id = 0; + uint16_t device_id = 0; + uint16_t svr_ep_count = 0; +}; + +// Decodes a discovery response frame. On success (empty std::error_code), +// out_result is populated with valid = true. Failure modes mirror +// decode_discovery_request()'s AVTP/ACF-level ones (short_frame, +// tscf_headed_request_dropped, bad_msg_type, wrong_bus, wrong_op — a +// response is expected to echo the same byte_bus_id and a read-classified +// op, checked the same way as for a request); a response payload shorter +// than kDiscoveryGeneralSliceLen also yields DiscoveryErrc::short_frame, +// since a genuinely useful result cannot be extracted from it. +inline std::error_code decode_discovery_response(const uint8_t* buf, size_t len, + DiscoveryResult& out_result) { + avtp::NtscfHeader ntscf_hdr; + acf::AcfMessageInfo info; + std::vector payload; + + const auto ec = detail::decode_common_frame(buf, len, ntscf_hdr, info, payload); + if (ec) return ec; + + if (payload.size() < kDiscoveryGeneralSliceLen) return make_error_code(DiscoveryErrc::short_frame); + + out_result.valid = true; + out_result.server_stream_id = ntscf_hdr.stream_id; + out_result.magic = avtp::detail::get_u32(&payload[0]); + out_result.svr_version = avtp::detail::get_u32(&payload[4]); + out_result.vendor_id = avtp::detail::get_u16(&payload[8]); + out_result.device_id = avtp::detail::get_u16(&payload[10]); + out_result.svr_ep_count = avtp::detail::get_u16(&payload[12]); + return {}; +} + +// ── Fragmented response (Phase 20, rcp/fragment.hpp) ────────────────────────── (REQ-DISC-025..028) +// +// read_size is one octet wide, so a discovery response's payload (always +// exactly read_size octets, per kDiscoveryGeneralSliceLen's own comment) is +// always well under any plausible max_fragment_payload — this endpoint's +// traffic never actually needs fragment.hpp's ms/segment_num mechanism in +// real-world use, the same reasoning rcp/uart.hpp's own fragmented-read- +// response comment gives for its own endpoint. The functions below are +// nonetheless provided for API consistency across every Phase 20 target +// endpoint and are exercised end-to-end in this module's own test suite +// against a deliberately small max_fragment_payload. +// +// Unlike c-RCP's own rcp_discovery_encode_response_fragmented() (which, +// per an ASIL-D-oriented no-dynamic-allocation push, plans into a fixed +// RCP_DISCOVERY_MAX_FRAGMENT_SEGMENTS-capacity (255) stack array rather +// than a heap-allocated one), this port follows this codebase's own +// already-established Phase 20 wiring convention instead (see +// rcp/uart.hpp's encode_read_response_fragmented()): a std::vector +// sized to the exact plan count, and a std::vector> of +// encoded frames returned by value. This codebase already allocates freely +// on every other encode path in this file (encode_acf_abb/ +// encode_ntscf_header both return std::vector) — c-RCP's 255-entry +// fixed-array optimization is specific to its own from-scratch +// no-allocation-anywhere goal for this one function and does not carry +// over to a codebase that has not adopted that goal for one-shot wire +// encoders in general (only for persistent state structs, e.g. +// DiscoveryClaim/rcp::fragment::Reassembler's own fixed-capacity buffer). + +// The number of ACF frames encode_discovery_response_fragmented() would +// produce for read_size octets of discovery-response payload split into +// fragments of at most max_fragment_payload octets each. +inline size_t discovery_response_fragment_count(uint8_t read_size, size_t max_fragment_payload) noexcept { + return fragment::plan_count(read_size, max_fragment_payload); +} + +// Encodes the discovery response as one or more full NTSCF-framed ACF_ABB +// messages, fragmenting via rcp/fragment.hpp's ms/segment_num mechanism +// whenever read_size exceeds max_fragment_payload octets. Same conventions +// as encode_discovery_response() otherwise (every fragment echoes +// transaction_num and is addressed via server_stream_id); only the ms flag, +// read_size_or_segment_num (meaningful only on an ms=true fragment — the +// final fragment carries read_size itself, exactly as +// encode_discovery_response() always does), and each fragment's own +// payload slice differ. When read_size already fits in one fragment, this +// produces exactly one frame identical to what encode_discovery_response() +// itself would have. Returns an empty vector under the same conditions +// discovery_response_fragment_count() returns 0 for (max_fragment_payload +// == 0 with an oversized read_size, or a split needing more intermediate +// segments than fragment::kMaxIntermediateSegments addresses). +inline std::vector> +encode_discovery_response_fragmented(const regmap::GeneralMap& map, + const avtp::StreamId& server_stream_id, + uint8_t sequence_num, + uint8_t transaction_num, + uint8_t read_size, + size_t max_fragment_payload) { + const size_t count = discovery_response_fragment_count(read_size, max_fragment_payload); + if (count == 0) return {}; + + std::vector segs(count); + if (fragment::plan(read_size, max_fragment_payload, segs.data(), count)) return {}; + + const auto payload = detail::build_general_slice_payload(map, read_size); + + std::vector> out_frames; + out_frames.reserve(count); + for (size_t i = 0; i < count; ++i) { + std::vector slice(payload.begin() + static_cast(segs[i].offset), + payload.begin() + static_cast(segs[i].offset + segs[i].len)); + + acf::AcfMessageInfo hdr; + hdr.byte_bus_id = kDiscoveryByteBusId; + hdr.op = false; // read + hdr.rsp = true; // TC18.txt:1885 -- rsp=1b identifies a response + hdr.transaction_num = transaction_num; + hdr.ms = segs[i].ms; + // The final (ms=false) fragment carries read_size itself -- exactly + // what encode_discovery_response() above always sends -- not 0; + // only an ms=true fragment's read_size_or_segment_num means + // "segment_num" (mirrors c-RCP's src/discovery.c:312). + hdr.read_size_or_segment_num = segs[i].ms ? segs[i].segment_num : static_cast(read_size); + + const auto acf_msg = acf::encode_acf_abb(hdr, slice); + + avtp::NtscfHeader ntscf_hdr; + ntscf_hdr.stream_id = server_stream_id; + ntscf_hdr.sequence_num = sequence_num; + ntscf_hdr.control_data_length = static_cast(acf_msg.size()); + + auto frame = avtp::encode_ntscf_header(ntscf_hdr); + frame.insert(frame.end(), acf_msg.begin(), acf_msg.end()); + out_frames.push_back(std::move(frame)); + } + return out_frames; +} + +// Decodes one fragment of a (possibly multi-fragment) discovery response +// frame — the same AVTP/ACF-level validation decode_discovery_response() +// applies (see that function's own failure-mode list), but surfaces the +// fragment's own ms bit and read_size_or_segment_num (as *out_segment_num, +// meaningful only when *out_ms) alongside the raw ACF payload +// (out_payload), for a caller to feed straight into a +// rcp::fragment::Reassembler. *out_server_stream_id is populated the same +// way decode_discovery_response()'s own result.server_stream_id is. +inline std::error_code decode_discovery_response_fragment(const uint8_t* buf, size_t len, + avtp::StreamId& out_server_stream_id, + bool& out_ms, uint8_t& out_segment_num, + std::vector& out_payload) { + avtp::NtscfHeader ntscf_hdr; + acf::AcfMessageInfo info; + + const auto ec = detail::decode_common_frame(buf, len, ntscf_hdr, info, out_payload); + if (ec) return ec; + + out_server_stream_id = ntscf_hdr.stream_id; + out_ms = info.ms; + out_segment_num = static_cast(info.read_size_or_segment_num); + return {}; +} + +// Once a rcp::fragment::Reassembler reports ReasmResult::kComplete, applies +// this module's own general-register-slice parsing (see +// kDiscoveryGeneralSliceLen's own comment) to its data()/size() output — +// the second half of what decode_discovery_response() does in one step for +// a single, unfragmented frame. server_stream_id is whatever +// decode_discovery_response_fragment() reported for (any of) that +// sequence's own fragments (round-tripped identically on every fragment of +// one logical response, the same NTSCF sender-assigns-stream_id convention +// every fragment shares). Returns DiscoveryErrc::short_frame if +// reassembled_len is shorter than kDiscoveryGeneralSliceLen, same as +// decode_discovery_response() does for an unfragmented response. On +// success, out_result is populated with valid = true. +inline std::error_code decode_discovery_reassembled_response(const uint8_t* reassembled, size_t reassembled_len, + const avtp::StreamId& server_stream_id, + DiscoveryResult& out_result) { + if (reassembled_len < kDiscoveryGeneralSliceLen) return make_error_code(DiscoveryErrc::short_frame); + + out_result.valid = true; + out_result.server_stream_id = server_stream_id; + out_result.magic = avtp::detail::get_u32(&reassembled[0]); + out_result.svr_version = avtp::detail::get_u32(&reassembled[4]); + out_result.vendor_id = avtp::detail::get_u16(&reassembled[8]); + out_result.device_id = avtp::detail::get_u16(&reassembled[10]); + out_result.svr_ep_count = avtp::detail::get_u16(&reassembled[12]); + return {}; +} + +// ── Discovery-stream claiming ──────────────────────────────────────────────── (REQ-DISC-015..022/029) // DiscoveryClaim models the reservation described in extraction §3.5: the // first discovery request a server receives while it is in HW_UNCONFIGURED // or HW_CONFIGURED reserves the discovery stream for that client's @@ -178,9 +642,10 @@ class DiscoveryClaim { using Clock = std::chrono::steady_clock; using TimePoint = Clock::time_point; - // Discovery_TimeOut default, ~20 ms per extraction §3.5. The - // specification calls this out as configurable; this is this - // implementation's chosen default, not a mandated value. + // Discovery_TimeOut default, ~20 ms per extraction §3.5 (matches + // c-RCP's own RCP_DISCOVERY_DEFAULT_TIMEOUT_MS). The specification + // calls this out as configurable; this is this implementation's chosen + // default, not a mandated value. static constexpr std::chrono::milliseconds kDefaultTimeout{20}; // Outcome of a discovery request arriving at on_discovery_request. @@ -203,6 +668,27 @@ class DiscoveryClaim { // existing claim, since by that point configuration is already done. // This call never affects whether the request's *read* gets answered — // callers must answer it regardless of the outcome returned here. + // + // REQ-DISC-029: when the claim is already held by a not-yet-lapsed + // claimant, this returns HeldByOther (a different client) or + // AlreadyHeld (the same one re-requesting) — TC18 Figure 17's own two + // "Discovery request received" transitions read literally ("& no + // discovery stream assigned -> assign discovery stream -> send + // discovery response" versus "& discovery stream assigned -> send + // error response DISCOVERY_STREAM_OCCUPIED") with no carve-out for + // requester identity, so this refusal-to-(re-)claim applies uniformly + // either way (re-requesting does not itself refresh the deadline + // either way — only an actual configuration write does, via + // on_configuration_request()). DISCOVERY_STREAM_OCCUPIED is a + // Figure-16-diagram-only label: TC18 §12.9.6 Table 30's own 17 + // numbered wire error codes do not include it, and unlike + // LOCKED_CONFIG_ACCESS (which cleanly maps onto a numbered code with a + // semantically matching name), no numbered code here has an obviously + // corresponding meaning — flagged as a genuine, unresolved ambiguity, + // not force-mapped. This method's own ClaimOutcome return is therefore + // as far as this codebase goes: which numbered wire error code (if + // any) a caller should send for HeldByOther/AlreadyHeld is not decided + // here. ClaimOutcome on_discovery_request(size_t client, lifecycle::ServerState server_state, TimePoint now) noexcept { if (server_state != lifecycle::ServerState::HwUnconfigured && @@ -227,16 +713,30 @@ class DiscoveryClaim { // on_configuration_request should be called when a configuration // request arrives on the discovery stream. It returns true iff `client` - // held an active claim at `now`, in which case that claim is consumed - // (released) — a spent reservation is not carried forward, so the very - // next discovery request, from any client, is free to claim the stream - // again. A request from a client that does not hold the active claim - // returns false and leaves the existing claim (if any, held by someone - // else) untouched, so the real holder's window is not disturbed by an - // unrelated client's rejected attempt. + // held an active claim at `now`, in which case the claim's deadline is + // extended by another Discovery_TimeOut window from `now` (matching + // c-RCP's rcp_discovery_claim_note_config_write(), src/discovery.c:423-430) + // — a claimant may issue any number of configuration writes over time, + // each one resetting the clock, so long as no gap between them (or + // since the initial claim) ever reaches Discovery_TimeOut. A request + // from a client that does not hold the active claim returns false and + // leaves the existing claim (if any, held by someone else, or already + // lapsed) untouched — this never resurrects a lapsed claim, and the + // real holder's window is never disturbed by an unrelated client's + // rejected attempt. + // + // Bug fix (this port, Phase 4): a prior revision of this method called + // holder_.reset() here instead — consuming/releasing the claim after + // its very first configuration write, forcing a claimant to + // re-request (and re-win) discovery before every subsequent write. That + // contradicted c-RCP's own ground-truth behavior and its own test + // (tests/test_discovery.c's test_claim_config_write_refreshes_deadline_for_claimant(), + // which asserts the claimant is STILL the claimant at a time past the + // *original* deadline, precisely because the write refreshed it rather + // than ending the claim) — fixed here to extend instead of release. bool on_configuration_request(size_t client, TimePoint now) noexcept { if (!may_configure(client, now)) return false; - holder_.reset(); + claimed_at_ = now; // extends the deadline to now + timeout_, does not release the claim return true; } @@ -250,6 +750,11 @@ class DiscoveryClaim { return has_active_claim(now) ? holder_ : std::nullopt; } + // release unconditionally drops the claim (matching c-RCP's + // rcp_discovery_claim_release()), e.g. on the server's demotion back to + // HW_UNCONFIGURED (lifecycle.hpp's own reset path). + void release() noexcept { holder_.reset(); } + private: bool lapsed(TimePoint now) const noexcept { return !holder_.has_value() || (now - claimed_at_) >= timeout_; @@ -260,6 +765,49 @@ class DiscoveryClaim { TimePoint claimed_at_{}; }; +// ── Client-side discovery result persistence (thin convenience API) ───────── (REQ-DISC-023/030) +// DiscoveryCache is a growable list of previously discovered results, keyed +// by each result's server_stream_id. See this file's own header comment: +// purely a client-side convenience, never itself consulted by this module +// to decide whether to (re-)issue a discovery request. Backed by an +// ordinary std::vector rather than a fixed-capacity buffer — unlike +// DiscoveryClaim (real server-side protocol state on the dispatch hot +// path), this cache is an optional, unbounded, client-owned convenience +// with no ASIL-D no-allocation requirement of its own, matching c-RCP's +// own rcp_discovery_cache_t (a realloc()-grown array). +class DiscoveryCache { +public: + // Records result, keyed by result.server_stream_id. An existing entry + // for the same server_stream_id is overwritten in place + // (last-discovered-wins for that server; no history is retained). + // Otherwise a new entry is appended. + void put(const DiscoveryResult& result) { + for (auto& entry : entries_) { + if (entry.server_stream_id == result.server_stream_id) { + entry = result; + return; + } + } + entries_.push_back(result); + } + + // Looks up a previously cached result for stream_id. Returns nullptr if + // none is on record. The returned pointer is borrowed: it is + // invalidated by any subsequent put() call on this cache. + const DiscoveryResult* find(const avtp::StreamId& stream_id) const noexcept { + for (const auto& entry : entries_) { + if (entry.server_stream_id == stream_id) return &entry; + } + return nullptr; + } + + // Number of entries currently held in this cache. + size_t size() const noexcept { return entries_.size(); } + +private: + std::vector entries_; +}; + } // namespace discovery } // namespace rcp diff --git a/tests/test_discovery.cpp b/tests/test_discovery.cpp index c5888d6..29d10f4 100644 --- a/tests/test_discovery.cpp +++ b/tests/test_discovery.cpp @@ -7,13 +7,40 @@ // fusa:test REQ-DISC-007 // fusa:test REQ-DISC-008 // fusa:test REQ-DISC-009 - -// Tests for rcp/discovery.hpp — the RC Server discovery request and -// discovery-stream claiming mechanism (ROADMAP.md milestone 46, "Discovery", -// v2.2.0). +// fusa:test REQ-DISC-010 +// fusa:test REQ-DISC-011 +// fusa:test REQ-DISC-012 +// fusa:test REQ-DISC-013 +// fusa:test REQ-DISC-014 +// fusa:test REQ-DISC-015 +// fusa:test REQ-DISC-016 +// fusa:test REQ-DISC-017 +// fusa:test REQ-DISC-018 +// fusa:test REQ-DISC-019 +// fusa:test REQ-DISC-020 +// fusa:test REQ-DISC-021 +// fusa:test REQ-DISC-022 +// fusa:test REQ-DISC-023 +// fusa:test REQ-DISC-024 +// fusa:test REQ-DISC-025 +// fusa:test REQ-DISC-026 +// fusa:test REQ-DISC-027 +// fusa:test REQ-DISC-028 +// fusa:test REQ-DISC-029 +// fusa:test REQ-DISC-030 + +// Tests for rcp/discovery.hpp — the RC Server discovery request/response +// exchange, its Phase 20 fragmented-response counterpart, discovery-stream +// claiming, and the client-side discovery-result cache (ROADMAP.md Phase +// 17, cpp-RCP issue #129, "Phase 4"). Ported from c-RCP's +// tests/test_discovery.c (this project's RC5-spec-conformant reference +// test suite for this module) — there is no separate +// test_tc18_gaps_discovery.c file; this module's gap coverage lives +// entirely here, same as in c-RCP. #include #include +#include using namespace rcp::discovery; using rcp::lifecycle::ServerState; @@ -27,11 +54,105 @@ TimePoint at(int64_t ms) { return TimePoint(std::chrono::milliseconds(ms)); } +rcp::avtp::StreamId make_stream_id(std::array mac, uint16_t suffix) { + rcp::avtp::StreamId sid; + sid.mac = mac; + sid.suffix = suffix; + return sid; +} + +const rcp::avtp::StreamId kClientSid = make_stream_id({0x02, 0x00, 0x00, 0x00, 0x00, 0x01}, 7); +const rcp::avtp::StreamId kServerSid = make_stream_id({0x02, 0x00, 0x00, 0x00, 0x00, 0x02}, 3); +const rcp::avtp::StreamId kOtherSid = make_stream_id({0x02, 0x00, 0x00, 0x00, 0x00, 0x03}, 2); + +rcp::regmap::GeneralMap sample_map() { + rcp::regmap::GeneralMap map; + map.magic = 0xC0FFEE01u; + // A 32-bit value whose upper half is non-zero, so a 16-bit svr_version + // field cannot round-trip it -- see the octet-layout test below. + map.svr_version = 0x00010501u; + map.vendor_id = 0x1234u; + map.device_id = 0x5678u; + map.svr_ep_count = 9u; + return map; +} + +DiscoveryResult sample_result(std::array mac, uint16_t suffix, uint16_t device_id) { + DiscoveryResult r; + r.valid = true; + r.server_stream_id = make_stream_id(mac, suffix); + r.magic = 0xAAAAAAAAu; + r.svr_version = 1; + r.vendor_id = 2; + r.device_id = device_id; + r.svr_ep_count = 3; + return r; +} + +// Builds a raw NTSCF-headed ACF_ABB frame directly from an arbitrary +// (possibly deliberately invalid) AcfMessageInfo — used by the negative +// decode tests below to construct frames decode_discovery_request()/ +// decode_discovery_response() must reject, the same way c-RCP's own test +// file builds these by hand rather than through encode_discovery_request()/ +// _response() (which can only ever produce valid frames). +std::vector raw_ntscf_abb_frame(const rcp::acf::AcfMessageInfo& hdr, + const rcp::avtp::StreamId& stream_id) { + auto acf_msg = rcp::acf::encode_acf_abb(hdr, {}); + + rcp::avtp::NtscfHeader ntscf_hdr; + ntscf_hdr.stream_id = stream_id; + ntscf_hdr.control_data_length = static_cast(acf_msg.size()); + + auto out = rcp::avtp::encode_ntscf_header(ntscf_hdr); + out.insert(out.end(), acf_msg.begin(), acf_msg.end()); + return out; +} + +std::vector raw_ntscf_gbb_frame(const rcp::acf::AcfMessageInfo& hdr, + const rcp::avtp::StreamId& stream_id) { + auto acf_msg = rcp::acf::encode_acf_gbb(hdr, /*message_timestamp=*/0, {}); + + rcp::avtp::NtscfHeader ntscf_hdr; + ntscf_hdr.stream_id = stream_id; + ntscf_hdr.control_data_length = static_cast(acf_msg.size()); + + auto out = rcp::avtp::encode_ntscf_header(ntscf_hdr); + out.insert(out.end(), acf_msg.begin(), acf_msg.end()); + return out; +} + +std::vector raw_tscf_abb_frame(const rcp::acf::AcfMessageInfo& hdr, + const rcp::avtp::StreamId& stream_id) { + auto acf_msg = rcp::acf::encode_acf_abb(hdr, {}); + + rcp::avtp::TscfHeader tscf_hdr; + tscf_hdr.stream_id = stream_id; + tscf_hdr.control_data_length = static_cast(acf_msg.size()); + + auto out = rcp::avtp::encode_tscf_header(tscf_hdr); + out.insert(out.end(), acf_msg.begin(), acf_msg.end()); + return out; +} + } // namespace +// ── NTSCF-only rule ────────────────────────────────────────────────────────── (REQ-DISC-001) + +TEST_CASE("should_drop_discovery is true for a TSCF-headed frame", "[discovery][REQ-DISC-001]") { + REQUIRE(should_drop_discovery(rcp::avtp::kSubtypeTscf)); +} + +TEST_CASE("should_drop_discovery is false for an NTSCF-headed frame", "[discovery][REQ-DISC-001]") { + REQUIRE_FALSE(should_drop_discovery(rcp::avtp::kSubtypeNtscf)); +} + +TEST_CASE("should_drop_discovery is true for an unrecognized subtype", "[discovery][REQ-DISC-001]") { + REQUIRE(should_drop_discovery(0x00)); +} + // ── Discovery request framing ──────────────────────────────────────────────── -TEST_CASE("make_discovery_request targets byte_bus_id 0 as an unconditional read", "[discovery][REQ-DISC-001]") { +TEST_CASE("make_discovery_request targets byte_bus_id 0 as an unconditional read", "[discovery][REQ-DISC-002]") { auto info = make_discovery_request(/*transaction_num=*/7); REQUIRE(info.byte_bus_id == kDiscoveryByteBusId); REQUIRE(info.byte_bus_id == 0); @@ -42,55 +163,42 @@ TEST_CASE("make_discovery_request targets byte_bus_id 0 as an unconditional read } TEST_CASE("kDiscoveryRegisterAddress fixes the discovery read at register-map address 0", - "[discovery][REQ-DISC-001]") { + "[discovery][REQ-DISC-002]") { REQUIRE(kDiscoveryRegisterAddress == 0); } -// ── NTSCF-only framing; TSCF-headed discovery is dropped ──────────────────────── - -TEST_CASE("encode_discovery_request always produces an NTSCF-headed frame", "[discovery][REQ-DISC-002]") { - rcp::avtp::StreamId sid; - sid.mac = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06}; - sid.suffix = 1; - - auto frame = encode_discovery_request(sid, /*sequence_num=*/0, /*transaction_num=*/1); - REQUIRE_FALSE(frame.empty()); - REQUIRE(frame[0] == rcp::avtp::kSubtypeNtscf); -} - -TEST_CASE("decode_discovery_request round-trips an encoded discovery request", "[discovery][REQ-DISC-002]") { - rcp::avtp::StreamId sid; - sid.mac = {0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF}; - sid.suffix = 42; +// ── Discovery request round-trip ────────────────────────────────────────────── (REQ-DISC-002/003) - auto frame = encode_discovery_request(sid, /*sequence_num=*/3, /*transaction_num=*/9, /*read_size=*/4); +TEST_CASE("decode_discovery_request round-trips an encoded discovery request", "[discovery][REQ-DISC-002][REQ-DISC-003]") { + auto frame = encode_discovery_request(kClientSid, /*sequence_num=*/3, /*transaction_num=*/42, /*read_size=*/12); - rcp::avtp::NtscfHeader hdr; + rcp::avtp::NtscfHeader hdr; rcp::acf::AcfMessageInfo info; std::vector payload; auto ec = decode_discovery_request(frame.data(), frame.size(), hdr, info, payload); REQUIRE_FALSE(ec); - REQUIRE(hdr.stream_id == sid); + REQUIRE(hdr.stream_id == kClientSid); REQUIRE(hdr.sequence_num == 3); REQUIRE(info.byte_bus_id == kDiscoveryByteBusId); - REQUIRE(info.transaction_num == 9); + REQUIRE(info.read_size_or_segment_num == 12); + REQUIRE(info.transaction_num == 42); REQUIRE_FALSE(info.op); } -TEST_CASE("A TSCF-headed discovery request is dropped, not decoded", "[discovery][REQ-DISC-002]") { - // Build a TSCF frame around the same discovery-shaped ACF_ABB payload — - // this must never be treated as a valid discovery request. - rcp::avtp::TscfHeader hdr; - hdr.stream_id.mac = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06}; - hdr.stream_id.suffix = 1; +TEST_CASE("encode_discovery_request always produces an NTSCF-headed frame", "[discovery][REQ-DISC-002]") { + auto frame = encode_discovery_request(kClientSid, /*sequence_num=*/0, /*transaction_num=*/1); + REQUIRE_FALSE(frame.empty()); + REQUIRE(frame[0] == rcp::avtp::kSubtypeNtscf); +} - auto info = make_discovery_request(/*transaction_num=*/1); - auto acf = rcp::acf::encode_acf_abb(info, {}); - hdr.control_data_length = static_cast(acf.size()); +// ── Discovery request validation ───────────────────────────────────────────── (REQ-DISC-004..008) - auto frame = rcp::avtp::encode_tscf_header(hdr); - frame.insert(frame.end(), acf.begin(), acf.end()); +TEST_CASE("A TSCF-headed discovery request is dropped, not decoded", "[discovery][REQ-DISC-004]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = kDiscoveryByteBusId; + hdr.op = false; + auto frame = raw_tscf_abb_frame(hdr, kClientSid); rcp::avtp::NtscfHeader out_hdr; rcp::acf::AcfMessageInfo out_info; @@ -101,7 +209,68 @@ TEST_CASE("A TSCF-headed discovery request is dropped, not decoded", "[discovery REQUIRE(ec == make_error_code(DiscoveryErrc::tscf_headed_request_dropped)); } -// ── Any-state answering ────────────────────────────────────────────────────────── +TEST_CASE("decode_discovery_request rejects a non-ACF_ABB message type", "[discovery][REQ-DISC-005]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = kDiscoveryByteBusId; + hdr.op = false; + auto frame = raw_ntscf_gbb_frame(hdr, kClientSid); + + rcp::avtp::NtscfHeader out_hdr; + rcp::acf::AcfMessageInfo out_info; + std::vector out_payload; + auto ec = decode_discovery_request(frame.data(), frame.size(), out_hdr, out_info, out_payload); + + REQUIRE(ec == make_error_code(DiscoveryErrc::bad_msg_type)); +} + +TEST_CASE("decode_discovery_request rejects the wrong byte_bus_id", "[discovery][REQ-DISC-006]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = 7u; // not the discovery bus + hdr.op = false; + auto frame = raw_ntscf_abb_frame(hdr, kClientSid); + + rcp::avtp::NtscfHeader out_hdr; + rcp::acf::AcfMessageInfo out_info; + std::vector out_payload; + auto ec = decode_discovery_request(frame.data(), frame.size(), out_hdr, out_info, out_payload); + + REQUIRE(ec == make_error_code(DiscoveryErrc::wrong_bus)); +} + +TEST_CASE("decode_discovery_request rejects a write op", "[discovery][REQ-DISC-007]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = kDiscoveryByteBusId; + hdr.op = true; // discovery is always a read + auto frame = raw_ntscf_abb_frame(hdr, kClientSid); + + rcp::avtp::NtscfHeader out_hdr; + rcp::acf::AcfMessageInfo out_info; + std::vector out_payload; + auto ec = decode_discovery_request(frame.data(), frame.size(), out_hdr, out_info, out_payload); + + REQUIRE(ec == make_error_code(DiscoveryErrc::wrong_op)); +} + +TEST_CASE("decode_discovery_request rejects a short frame", "[discovery][REQ-DISC-008]") { + uint8_t tiny[2] = {rcp::avtp::kSubtypeNtscf, 0}; + rcp::avtp::NtscfHeader out_hdr; + rcp::acf::AcfMessageInfo out_info; + std::vector out_payload; + + auto ec = decode_discovery_request(tiny, sizeof(tiny), out_hdr, out_info, out_payload); + REQUIRE(ec == make_error_code(DiscoveryErrc::short_frame)); +} + +TEST_CASE("decode_discovery_request rejects an empty buffer", "[discovery][REQ-DISC-008]") { + rcp::avtp::NtscfHeader out_hdr; + rcp::acf::AcfMessageInfo out_info; + std::vector out_payload; + + auto ec = decode_discovery_request(nullptr, 0, out_hdr, out_info, out_payload); + REQUIRE(ec == make_error_code(DiscoveryErrc::short_frame)); +} + +// ── Any-state answering ────────────────────────────────────────────────────── TEST_CASE("A server answers discovery in every lifecycle state", "[discovery][REQ-DISC-009]") { REQUIRE(should_answer_discovery(ServerState::HwUnconfigured)); @@ -109,10 +278,344 @@ TEST_CASE("A server answers discovery in every lifecycle state", "[discovery][RE REQUIRE(should_answer_discovery(ServerState::RcpConfigured)); } -// ── Discovery-stream claiming: first request claims it ────────────────────────── +// ── Discovery response ──────────────────────────────────────────────────────── (REQ-DISC-009..014) + +TEST_CASE("encode/decode_discovery_response round-trip at exactly kDiscoveryGeneralSliceLen", + "[discovery][REQ-DISC-009][REQ-DISC-010][REQ-DISC-012]") { + auto map = sample_map(); + auto frame = encode_discovery_response(map, kServerSid, /*sequence_num=*/0, /*transaction_num=*/9, + static_cast(kDiscoveryGeneralSliceLen)); + + DiscoveryResult result; + auto ec = decode_discovery_response(frame.data(), frame.size(), result); + + REQUIRE_FALSE(ec); + REQUIRE(result.valid); + REQUIRE(result.server_stream_id == kServerSid); + REQUIRE(result.magic == map.magic); + REQUIRE(result.svr_version == map.svr_version); + REQUIRE(result.vendor_id == map.vendor_id); + REQUIRE(result.device_id == map.device_id); + REQUIRE(result.svr_ep_count == map.svr_ep_count); +} + +// TC18 v0.5.1_RC §12.7.5 "RC Server Register map - General part", Table 18 +// "RC Server configuration static part". The absolute addresses and widths +// of the leading, device-recognition part of the block are: +// +// 0x0000 svr_oa_tc18_magic_nr 32 bit R +// 0x0004 svr_version 32 bit R +// 0x0008 svr_vendor_id 16 bit R +// 0x000A svr_device_id 16 bit R +// 0x000C svr_ep_count 16 bit R +// +// svr_version is 32 bit, so vendor_id starts at 0x0008 -- not 0x0006, as a +// 16-bit svr_version would put it. This test pins each field to its cited +// absolute address in the encoded payload, so the two-octet regression +// (which would shift vendor_id, device_id and svr_ep_count each two octets +// early, misparsing all three for any conforming peer) cannot come back +// unnoticed. Mirrors c-RCP's own test_response_general_slice_octet_layout() +// (tests/test_discovery.c). +TEST_CASE("The discovery response's general slice is laid out at its documented absolute addresses", + "[discovery][REQ-DISC-010]") { + auto map = sample_map(); + auto frame = encode_discovery_response(map, kServerSid, /*sequence_num=*/0, /*transaction_num=*/9, + static_cast(kDiscoveryGeneralSliceLen)); + + rcp::avtp::NtscfHeader ntscf_hdr; + rcp::acf::AcfMessageInfo acf_hdr; + std::vector payload; + REQUIRE_FALSE(rcp::avtp::decode_ntscf_header(frame.data(), frame.size(), ntscf_hdr)); + REQUIRE_FALSE(rcp::acf::decode_acf_abb(frame.data() + rcp::avtp::kNtscfHeaderLen, + frame.size() - rcp::avtp::kNtscfHeaderLen, acf_hdr, payload)); + + REQUIRE(kDiscoveryGeneralSliceLen == 14u); + REQUIRE(payload.size() == kDiscoveryGeneralSliceLen); + + // 0x0000 svr_oa_tc18_magic_nr, 32 bit big-endian: 0xC0FFEE01 + REQUIRE(payload[0x00] == 0xC0u); + REQUIRE(payload[0x01] == 0xFFu); + REQUIRE(payload[0x02] == 0xEEu); + REQUIRE(payload[0x03] == 0x01u); + // 0x0004 svr_version, 32 bit big-endian: 0x00010501 + REQUIRE(payload[0x04] == 0x00u); + REQUIRE(payload[0x05] == 0x01u); + REQUIRE(payload[0x06] == 0x05u); + REQUIRE(payload[0x07] == 0x01u); + // 0x0008 svr_vendor_id, 16 bit big-endian: 0x1234 + REQUIRE(payload[0x08] == 0x12u); + REQUIRE(payload[0x09] == 0x34u); + // 0x000A svr_device_id, 16 bit big-endian: 0x5678 + REQUIRE(payload[0x0A] == 0x56u); + REQUIRE(payload[0x0B] == 0x78u); + // 0x000C svr_ep_count, 16 bit big-endian: 9 + REQUIRE(payload[0x0C] == 0x00u); + REQUIRE(payload[0x0D] == 0x09u); +} + +TEST_CASE("A discovery response's payload always spans exactly read_size octets", "[discovery][REQ-DISC-009]") { + auto map = sample_map(); + auto frame_small = encode_discovery_response(map, kServerSid, 0, 1, /*read_size=*/4); + auto frame_large = encode_discovery_response(map, kServerSid, 0, 1, /*read_size=*/40); + + rcp::avtp::NtscfHeader ntscf_hdr; + rcp::acf::AcfMessageInfo acf_hdr; + std::vector payload; + + REQUIRE_FALSE(rcp::avtp::decode_ntscf_header(frame_small.data(), frame_small.size(), ntscf_hdr)); + REQUIRE_FALSE(rcp::acf::decode_acf_abb(frame_small.data() + rcp::avtp::kNtscfHeaderLen, + frame_small.size() - rcp::avtp::kNtscfHeaderLen, acf_hdr, payload)); + REQUIRE(payload.size() == 4); + + REQUIRE_FALSE(rcp::avtp::decode_ntscf_header(frame_large.data(), frame_large.size(), ntscf_hdr)); + REQUIRE_FALSE(rcp::acf::decode_acf_abb(frame_large.data() + rcp::avtp::kNtscfHeaderLen, + frame_large.size() - rcp::avtp::kNtscfHeaderLen, acf_hdr, payload)); + REQUIRE(payload.size() == 40); +} + +TEST_CASE("decode_discovery_response treats a slice truncated below kDiscoveryGeneralSliceLen as short", + "[discovery][REQ-DISC-013]") { + auto map = sample_map(); + // read_size of 4 -- only room for the magic field. + auto frame = encode_discovery_response(map, kServerSid, 0, 1, /*read_size=*/4); + + DiscoveryResult result; + auto ec = decode_discovery_response(frame.data(), frame.size(), result); + + // Too short to extract a full generic slice -- must be treated as + // short, not silently fabricate zeros for the missing fields. + REQUIRE(ec == make_error_code(DiscoveryErrc::short_frame)); +} + +TEST_CASE("A discovery response zero-fills any octets beyond the general slice", "[discovery][REQ-DISC-011]") { + auto map = sample_map(); + auto frame = encode_discovery_response(map, kServerSid, 0, 1, + static_cast(kDiscoveryGeneralSliceLen + 4)); + + rcp::avtp::NtscfHeader ntscf_hdr; + rcp::acf::AcfMessageInfo acf_hdr; + std::vector payload; + REQUIRE_FALSE(rcp::avtp::decode_ntscf_header(frame.data(), frame.size(), ntscf_hdr)); + REQUIRE_FALSE(rcp::acf::decode_acf_abb(frame.data() + rcp::avtp::kNtscfHeaderLen, + frame.size() - rcp::avtp::kNtscfHeaderLen, acf_hdr, payload)); + + REQUIRE(payload.size() == kDiscoveryGeneralSliceLen + 4); + for (size_t i = kDiscoveryGeneralSliceLen; i < payload.size(); ++i) { + REQUIRE(payload[i] == 0); + } +} + +// REQ-DISC-014: decode_discovery_response() applies the same AVTP/ACF-level +// checks decode_discovery_request() does (both go through the shared +// detail::decode_common_frame()) -- one test per condition, mirroring the +// request-side tests above but through the response entry point. +TEST_CASE("decode_discovery_response is dropped when TSCF-headed", "[discovery][REQ-DISC-014]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = kDiscoveryByteBusId; + hdr.op = false; + hdr.rsp = true; + auto frame = raw_tscf_abb_frame(hdr, kServerSid); + + DiscoveryResult result; + auto ec = decode_discovery_response(frame.data(), frame.size(), result); + REQUIRE(ec == make_error_code(DiscoveryErrc::tscf_headed_request_dropped)); +} + +TEST_CASE("decode_discovery_response rejects a non-ACF_ABB message type", "[discovery][REQ-DISC-014]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = kDiscoveryByteBusId; + hdr.op = false; + hdr.rsp = true; + auto frame = raw_ntscf_gbb_frame(hdr, kServerSid); + + DiscoveryResult result; + auto ec = decode_discovery_response(frame.data(), frame.size(), result); + REQUIRE(ec == make_error_code(DiscoveryErrc::bad_msg_type)); +} + +TEST_CASE("decode_discovery_response rejects the wrong byte_bus_id", "[discovery][REQ-DISC-014]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = 7u; + hdr.op = false; + hdr.rsp = true; + auto frame = raw_ntscf_abb_frame(hdr, kServerSid); + + DiscoveryResult result; + auto ec = decode_discovery_response(frame.data(), frame.size(), result); + REQUIRE(ec == make_error_code(DiscoveryErrc::wrong_bus)); +} + +TEST_CASE("decode_discovery_response rejects a write op", "[discovery][REQ-DISC-014]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = kDiscoveryByteBusId; + hdr.op = true; + hdr.rsp = true; + auto frame = raw_ntscf_abb_frame(hdr, kServerSid); + + DiscoveryResult result; + auto ec = decode_discovery_response(frame.data(), frame.size(), result); + REQUIRE(ec == make_error_code(DiscoveryErrc::wrong_op)); +} + +// ── Fragmented response (Phase 20, rcp/fragment.hpp) ────────────────────────── (REQ-DISC-025..028) + +TEST_CASE("discovery_response_fragment_count is 1 when unfragmented", "[discovery][REQ-DISC-025]") { + REQUIRE(discovery_response_fragment_count(12, 100) == 1); + REQUIRE(discovery_response_fragment_count(0, 0) == 1); +} + +TEST_CASE("An unfragmented encode_discovery_response_fragmented matches the single-frame path", + "[discovery][REQ-DISC-025][REQ-DISC-026]") { + auto map = sample_map(); + auto plain = encode_discovery_response(map, kServerSid, 0, 9, static_cast(kDiscoveryGeneralSliceLen)); + + auto fragmented = encode_discovery_response_fragmented( + map, kServerSid, 0, 9, static_cast(kDiscoveryGeneralSliceLen), /*max_fragment_payload=*/255); + + REQUIRE(fragmented.size() == 1); + REQUIRE(fragmented[0] == plain); +} + +// Closes the deferred single-AVTPDU-worst-case scenario: exercises +// rcp/fragment.hpp's ms/segment_num mechanism against this module's own +// NTSCF+ACF wire codec end-to-end, using a deliberately small +// max_fragment_payload -- read_size's one-octet width means genuine +// discovery traffic never actually needs more than one fragment in +// practice; this test proves the mechanism composes correctly regardless. +TEST_CASE("A deliberately small fragment cap round-trips through the reassembler", + "[discovery][REQ-DISC-026][REQ-DISC-027][REQ-DISC-028]") { + auto map = sample_map(); + const uint8_t read_size = 20; + const size_t max_fragment_payload = 6; + + REQUIRE(discovery_response_fragment_count(read_size, max_fragment_payload) == 4); // ceil(20/6) + + auto frames = encode_discovery_response_fragmented(map, kServerSid, 0, 42, read_size, max_fragment_payload); + REQUIRE(frames.size() == 4); + + rcp::fragment::Reassembler reasm(read_size); + for (size_t i = 0; i < frames.size(); ++i) { + rcp::avtp::StreamId from_stream; + bool ms = false; + uint8_t segnum = 0; + std::vector payload; + + auto ec = decode_discovery_response_fragment(frames[i].data(), frames[i].size(), from_stream, ms, segnum, + payload); + REQUIRE_FALSE(ec); + REQUIRE(from_stream == kServerSid); + + auto rc = reasm.feed(ms, segnum, payload.data(), payload.size()); + if (i + 1 < frames.size()) { + REQUIRE(rc == rcp::fragment::ReasmResult::kContinue); + } else { + REQUIRE(rc == rcp::fragment::ReasmResult::kComplete); + } + } + + REQUIRE(reasm.size() == read_size); + + DiscoveryResult result; + auto ec = decode_discovery_reassembled_response(reasm.data(), reasm.size(), kServerSid, result); + REQUIRE_FALSE(ec); + REQUIRE(result.valid); + REQUIRE(result.server_stream_id == kServerSid); + REQUIRE(result.magic == map.magic); + REQUIRE(result.svr_version == map.svr_version); + REQUIRE(result.vendor_id == map.vendor_id); + REQUIRE(result.device_id == map.device_id); + REQUIRE(result.svr_ep_count == map.svr_ep_count); +} + +// The actual worst case fragment.hpp's segment_num width is sized against: +// read_size = 255 (its own max value) with max_fragment_payload = 1, so the +// plan needs exactly 254 intermediate (ms=true) segments plus one final — +// well inside fragment::kMaxIntermediateSegments (4096). +TEST_CASE("Maximum read_size with minimum fragment payload round-trips", "[discovery][REQ-DISC-026]") { + auto map = sample_map(); + const uint8_t read_size = 255; + const size_t max_fragment_payload = 1; + + REQUIRE(discovery_response_fragment_count(read_size, max_fragment_payload) == 255); + + auto frames = encode_discovery_response_fragmented(map, kServerSid, 0, 7, read_size, max_fragment_payload); + REQUIRE(frames.size() == 255); + + rcp::fragment::Reassembler reasm(read_size); + for (size_t i = 0; i < frames.size(); ++i) { + rcp::avtp::StreamId from_stream; + bool ms = false; + uint8_t segnum = 0; + std::vector payload; + + auto ec = decode_discovery_response_fragment(frames[i].data(), frames[i].size(), from_stream, ms, segnum, + payload); + REQUIRE_FALSE(ec); + + auto rc = reasm.feed(ms, segnum, payload.data(), payload.size()); + if (i + 1 < frames.size()) { + REQUIRE(rc == rcp::fragment::ReasmResult::kContinue); + } else { + REQUIRE(rc == rcp::fragment::ReasmResult::kComplete); + } + } + + DiscoveryResult result; + auto ec = decode_discovery_reassembled_response(reasm.data(), reasm.size(), kServerSid, result); + REQUIRE_FALSE(ec); + REQUIRE(result.valid); +} + +TEST_CASE("encode_discovery_response_fragmented is disabled when max_fragment_payload is 0 and oversized", + "[discovery][REQ-DISC-026]") { + auto map = sample_map(); + auto frames = encode_discovery_response_fragmented(map, kServerSid, 0, 1, /*read_size=*/20, + /*max_fragment_payload=*/0); + REQUIRE(frames.empty()); +} + +// REQ-DISC-027: decode_discovery_response_fragment() applies the same +// AVTP/ACF-level validation decode_discovery_response() does (both go +// through detail::decode_common_frame()) -- one representative condition +// proves the fragment entry point actually reaches that shared check, +// rather than bypassing it. +TEST_CASE("decode_discovery_response_fragment rejects the wrong byte_bus_id", "[discovery][REQ-DISC-027]") { + rcp::acf::AcfMessageInfo hdr; + hdr.byte_bus_id = 7u; + hdr.op = false; + hdr.rsp = true; + hdr.ms = true; + auto frame = raw_ntscf_abb_frame(hdr, kServerSid); + + rcp::avtp::StreamId from_stream; + bool ms = false; + uint8_t segnum = 0; + std::vector payload; + auto ec = decode_discovery_response_fragment(frame.data(), frame.size(), from_stream, ms, segnum, payload); + REQUIRE(ec == make_error_code(DiscoveryErrc::wrong_bus)); +} + +// REQ-DISC-028: a reassembled buffer shorter than kDiscoveryGeneralSliceLen +// is rejected, same short-frame-not-fabricated-zeros discipline as the +// truncated-slice response test above. +TEST_CASE("decode_discovery_reassembled_response rejects a short buffer", "[discovery][REQ-DISC-028]") { + std::array short_buf{}; + DiscoveryResult result; + + auto ec = decode_discovery_reassembled_response(short_buf.data(), short_buf.size(), kServerSid, result); + REQUIRE(ec == make_error_code(DiscoveryErrc::short_frame)); +} + +// ── Discovery-stream claiming ────────────────────────────────────────────────── (REQ-DISC-015..022/029) + +TEST_CASE("A freshly-constructed DiscoveryClaim is open and unheld", "[discovery][REQ-DISC-015][REQ-DISC-016]") { + DiscoveryClaim claim; + REQUIRE_FALSE(claim.has_active_claim(at(0))); + REQUIRE(claim.current_holder(at(0)) == std::nullopt); +} TEST_CASE("The first discovery request in HW_UNCONFIGURED claims the discovery stream", - "[discovery][REQ-DISC-003]") { + "[discovery][REQ-DISC-017]") { DiscoveryClaim claim; auto outcome = claim.on_discovery_request(/*client=*/1, ServerState::HwUnconfigured, at(0)); REQUIRE(outcome == DiscoveryClaim::ClaimOutcome::Claimed); @@ -121,16 +624,14 @@ TEST_CASE("The first discovery request in HW_UNCONFIGURED claims the discovery s } TEST_CASE("The first discovery request in HW_CONFIGURED also claims the discovery stream", - "[discovery][REQ-DISC-003]") { + "[discovery][REQ-DISC-017]") { DiscoveryClaim claim; auto outcome = claim.on_discovery_request(/*client=*/5, ServerState::HwConfigured, at(0)); REQUIRE(outcome == DiscoveryClaim::ClaimOutcome::Claimed); REQUIRE(claim.may_configure(5, at(1))); } -// ── Claim scoped to HW_UNCONFIGURED / HW_CONFIGURED only ──────────────────────── - -TEST_CASE("A discovery request in RCP_CONFIGURED never claims the stream", "[discovery][REQ-DISC-007]") { +TEST_CASE("A discovery request in RCP_CONFIGURED never claims the stream", "[discovery][REQ-DISC-017]") { DiscoveryClaim claim; auto outcome = claim.on_discovery_request(/*client=*/1, ServerState::RcpConfigured, at(0)); REQUIRE(outcome == DiscoveryClaim::ClaimOutcome::NotEligible); @@ -138,12 +639,36 @@ TEST_CASE("A discovery request in RCP_CONFIGURED never claims the stream", "[dis REQUIRE_FALSE(claim.may_configure(1, at(0))); } -// ── Claim lapse after Discovery_TimeOut ────────────────────────────────────────── +// REQ-DISC-029: TC18 Figure 17's two "Discovery request received" +// transitions apply uniformly regardless of requester identity -- neither +// a different client's request nor the current claimant's own +// re-request re-claims or refreshes an already-active claim. +TEST_CASE("A second client's request during an active claim does not preempt or dislodge it", + "[discovery][REQ-DISC-018][REQ-DISC-029]") { + DiscoveryClaim claim; + REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(0)) == DiscoveryClaim::ClaimOutcome::Claimed); + + auto outcome = claim.on_discovery_request(2, ServerState::HwUnconfigured, at(5)); + REQUIRE(outcome == DiscoveryClaim::ClaimOutcome::HeldByOther); + // Read-only discovery is unaffected regardless of claim state. + REQUIRE(should_answer_discovery(ServerState::HwUnconfigured)); + // The original holder's claim survives the other client's request. + REQUIRE(claim.may_configure(1, at(6))); +} + +TEST_CASE("The claim holder re-requesting discovery before it lapses is reported as AlreadyHeld, " + "not re-claimed", + "[discovery][REQ-DISC-018][REQ-DISC-029]") { + DiscoveryClaim claim; + REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(0)) == DiscoveryClaim::ClaimOutcome::Claimed); + auto outcome = claim.on_discovery_request(1, ServerState::HwUnconfigured, at(2)); + REQUIRE(outcome == DiscoveryClaim::ClaimOutcome::AlreadyHeld); +} -TEST_CASE("An unclaimed-follow-up claim lapses after Discovery_TimeOut elapses", "[discovery][REQ-DISC-004]") { +TEST_CASE("A claim lapses after Discovery_TimeOut elapses and reopens to a new claimant", + "[discovery][REQ-DISC-016][REQ-DISC-017]") { DiscoveryClaim claim(std::chrono::milliseconds(20)); - REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(0)) - == DiscoveryClaim::ClaimOutcome::Claimed); + REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(0)) == DiscoveryClaim::ClaimOutcome::Claimed); REQUIRE(claim.may_configure(1, at(19))); // Past the timeout, the claim has lapsed: the original holder may no @@ -156,75 +681,132 @@ TEST_CASE("An unclaimed-follow-up claim lapses after Discovery_TimeOut elapses", auto outcome = claim.on_discovery_request(2, ServerState::HwUnconfigured, at(25)); REQUIRE(outcome == DiscoveryClaim::ClaimOutcome::Claimed); REQUIRE(claim.may_configure(2, at(25))); + REQUIRE_FALSE(claim.may_configure(1, at(25))); } -TEST_CASE("Discovery_TimeOut defaults to approximately 20ms", "[discovery][REQ-DISC-004]") { +TEST_CASE("Discovery_TimeOut defaults to 20ms", "[discovery][REQ-DISC-015]") { REQUIRE(DiscoveryClaim::kDefaultTimeout == std::chrono::milliseconds(20)); } -// ── Configuration request consumes the active claim ───────────────────────────── - -TEST_CASE("A configuration request from the claim holder succeeds and consumes the claim", - "[discovery][REQ-DISC-006]") { +TEST_CASE("may_configure is false for a client that never held the claim", "[discovery][REQ-DISC-019]") { DiscoveryClaim claim; - REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(0)) - == DiscoveryClaim::ClaimOutcome::Claimed); + REQUIRE_FALSE(claim.may_configure(1, at(0))); +} + +// Bug fix pinned (this port, Phase 4): a configuration request from the +// claim holder must EXTEND the deadline, not consume/release the claim -- +// see DiscoveryClaim::on_configuration_request()'s own doc comment. Mirrors +// c-RCP's test_claim_config_write_refreshes_deadline_for_claimant() +// (tests/test_discovery.c) exactly, including its own numbers. +TEST_CASE("A configuration request from the claim holder refreshes the deadline instead of consuming the claim", + "[discovery][REQ-DISC-020]") { + DiscoveryClaim claim(std::chrono::milliseconds(20)); + REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(1000)) == DiscoveryClaim::ClaimOutcome::Claimed); - REQUIRE(claim.on_configuration_request(1, at(5))); - // The claim is spent: neither the same client's follow-up configuration - // request, nor a query, sees it as still active. - REQUIRE_FALSE(claim.has_active_claim(at(5))); - REQUIRE_FALSE(claim.on_configuration_request(1, at(6))); + REQUIRE(claim.on_configuration_request(1, at(1015))); + // Without the refresh the claim would have lapsed at 1020; the write + // at t=1015 should have pushed the deadline out to 1035. + REQUIRE(claim.may_configure(1, at(1025))); + REQUIRE(claim.has_active_claim(at(1025))); } TEST_CASE("A configuration request from a client that does not hold the claim is refused, " "leaving the real holder's claim untouched", - "[discovery][REQ-DISC-006]") { - DiscoveryClaim claim; - REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(0)) - == DiscoveryClaim::ClaimOutcome::Claimed); + "[discovery][REQ-DISC-021]") { + DiscoveryClaim claim(std::chrono::milliseconds(20)); + REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(1000)) == DiscoveryClaim::ClaimOutcome::Claimed); - REQUIRE_FALSE(claim.on_configuration_request(/*client=*/2, at(1))); - // Client 1's claim survives client 2's rejected attempt. - REQUIRE(claim.may_configure(1, at(2))); + REQUIRE_FALSE(claim.on_configuration_request(2, at(1005))); + // Client 1's claim survives client 2's rejected attempt, and still + // lapses on its own original schedule (unrefreshed). + REQUIRE(claim.may_configure(1, at(1019))); + REQUIRE_FALSE(claim.may_configure(1, at(1020))); } -// ── Concurrent reads: other clients keep being answered during an active claim ── +TEST_CASE("A configuration request never resurrects an already-lapsed claim", "[discovery][REQ-DISC-021]") { + DiscoveryClaim claim(std::chrono::milliseconds(20)); + REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(1000)) == DiscoveryClaim::ClaimOutcome::Claimed); -TEST_CASE("Another client's discovery request during an active claim does not dislodge or " - "duplicate it, but that client's read is unaffected", - "[discovery][REQ-DISC-005]") { - DiscoveryClaim claim; - REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(0)) - == DiscoveryClaim::ClaimOutcome::Claimed); + REQUIRE_FALSE(claim.on_configuration_request(1, at(1025))); // already lapsed + REQUIRE_FALSE(claim.has_active_claim(at(1025))); +} - // A second client's discovery request during the still-active window: it - // does not claim the stream ... - auto outcome = claim.on_discovery_request(2, ServerState::HwUnconfigured, at(5)); - REQUIRE(outcome == DiscoveryClaim::ClaimOutcome::HeldByOther); - // ... yet answering that client's underlying discovery *read* is a - // separate, unconditional concern the claim never gates — modeled here - // by should_answer_discovery being state-only and never consulting claim - // state at all. - REQUIRE(should_answer_discovery(ServerState::HwUnconfigured)); +TEST_CASE("release unconditionally drops the claim", "[discovery][REQ-DISC-022]") { + DiscoveryClaim claim(std::chrono::milliseconds(20)); + REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(1000)) == DiscoveryClaim::ClaimOutcome::Claimed); + REQUIRE(claim.has_active_claim(at(1000))); - // The original holder's claim is unaffected by the other client's request. - REQUIRE(claim.may_configure(1, at(6))); + claim.release(); + + REQUIRE_FALSE(claim.has_active_claim(at(1000))); + REQUIRE_FALSE(claim.may_configure(1, at(1000))); } -TEST_CASE("The claim holder re-requesting discovery before it lapses is reported as AlreadyHeld", - "[discovery][REQ-DISC-005]") { - DiscoveryClaim claim; - REQUIRE(claim.on_discovery_request(1, ServerState::HwUnconfigured, at(0)) - == DiscoveryClaim::ClaimOutcome::Claimed); - auto outcome = claim.on_discovery_request(1, ServerState::HwUnconfigured, at(2)); - REQUIRE(outcome == DiscoveryClaim::ClaimOutcome::AlreadyHeld); +// ── Client-side discovery result persistence ─────────────────────────────────── (REQ-DISC-023/030) + +TEST_CASE("A freshly-constructed DiscoveryCache starts empty", "[discovery][REQ-DISC-023]") { + DiscoveryCache cache; + REQUIRE(cache.size() == 0); +} + +TEST_CASE("DiscoveryCache::put then find round-trips a result", "[discovery][REQ-DISC-023][REQ-DISC-030]") { + DiscoveryCache cache; + auto r = sample_result({0x02, 0x00, 0x00, 0x00, 0x00, 0x02}, 1, 100); + + cache.put(r); + REQUIRE(cache.size() == 1); + + auto* found = cache.find(r.server_stream_id); + REQUIRE(found != nullptr); + REQUIRE(found->device_id == 100); } -// ── DiscoveryErrc category sanity ──────────────────────────────────────────────── +TEST_CASE("DiscoveryCache::put updates an existing entry in place", "[discovery][REQ-DISC-023]") { + DiscoveryCache cache; + auto r1 = sample_result({0x02, 0x00, 0x00, 0x00, 0x00, 0x02}, 1, 100); + auto r2 = sample_result({0x02, 0x00, 0x00, 0x00, 0x00, 0x02}, 1, 200); // same stream_id + + cache.put(r1); + cache.put(r2); + + REQUIRE(cache.size() == 1); // updated, not appended + + auto* found = cache.find(r1.server_stream_id); + REQUIRE(found != nullptr); + REQUIRE(found->device_id == 200); +} + +TEST_CASE("DiscoveryCache::find returns nullptr on a miss", "[discovery][REQ-DISC-030]") { + DiscoveryCache cache; + REQUIRE(cache.find(kOtherSid) == nullptr); +} + +TEST_CASE("DiscoveryCache grows past any small initial capacity", "[discovery][REQ-DISC-023]") { + DiscoveryCache cache; + for (uint16_t i = 0; i < 40; ++i) { + cache.put(sample_result({0x02, 0x00, 0x00, 0x00, 0x00, 0x02}, i, i)); + } + REQUIRE(cache.size() == 40); +} -TEST_CASE("DiscoveryErrc reports a non-empty message in its own category", "[discovery][REQ-DISC-008]") { - auto ec = make_error_code(DiscoveryErrc::tscf_headed_request_dropped); - REQUIRE(ec.category() == discovery_category()); - REQUIRE_FALSE(ec.message().empty()); +// ── DiscoveryErrc category sanity ────────────────────────────────────────────── (REQ-DISC-024) + +TEST_CASE("Every DiscoveryErrc reports a unique, non-empty message in its own category", + "[discovery][REQ-DISC-024]") { + const DiscoveryErrc codes[] = { + DiscoveryErrc::short_frame, + DiscoveryErrc::tscf_headed_request_dropped, + DiscoveryErrc::bad_msg_type, + DiscoveryErrc::wrong_bus, + DiscoveryErrc::wrong_op, + }; + + for (size_t i = 0; i < std::size(codes); ++i) { + auto ec = make_error_code(codes[i]); + REQUIRE(ec.category() == discovery_category()); + REQUIRE_FALSE(ec.message().empty()); + for (size_t j = 0; j < i; ++j) { + REQUIRE(ec.message() != make_error_code(codes[j]).message()); + } + } }