Repository navigation
rewrite(phase4): port discovery from c-RCP, fix claim-release and validation gaps - #144
Merged
Merged
Conversation
…eneric/functional split Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Phase 4 batch A of a two-batch regmap port (batch B — HW pin map, named-signal index, request-stream/response-queue config, EP-ID map, optional-subsystem sections — is a separate later PR). c-RCP's regmap.h/.c (3549+2662 = 6211 lines) is regmap's own content source of truth; cpp-RCP's prior regmap.hpp (534 lines) was a stub with real content gaps, matching ROADMAP.md's own flagged '~7x more complete in c-RCP' finding that originally scoped this phase. ## Ported (c-RCP file:line -> cpp-RCP) - EP0/root-client model (regmap.h:358-371, regmap.c:12-15) -> regmap::is_ep0(), kEp0. - svr_implemented_options (REQ-RMAP-030, regmap.h:384-413): the 5 real bits (kOptCompoundWait/Trigger/Chained/TimeSync/EnhCancel); kept legacy kOptConditionalRequests/kOptSafetyRequests/kOptFragmentation (still referenced by request.hpp/e2e.hpp) with a comment explaining they have no TC18 basis per REQ-RMAP-004..008's retirement. - General register map (regmap.h:416-1141, regmap.c:41-288): new GeneralMap struct (~40 fields, REQ-RMAP-023/026-039), render(), encode_read_response()/decode_read_response()/decode_write_request() (Table 20 wire codec), GeneralMapErrc. - Root-client/writer_ctx (regmap.h:1152-1211, regmap.c:1684-1741): EpClient, writer_ctx() deriving lifecycle::WriterCtx. - SvrEpCfg (TC18 Sec13.7.1.2, regmap.h:1211-1303): svr_discovery_timeout, svr_ep_status. - EndpointGenericConfig (regmap.h:1310-1608): real fields (ep_type/ep_used/ep_delay_time/ep_req_storage_size/ep_description/ ep_tx_buffer_size/ep_rx_buffer_size), ep_generic_cfg:: namespace with boundary conversions (issue #311 batches 2-4), render(), apply_reconfig() (per-field, matching c-RCP's documented rationale), EpGenericCfgReconfigErrc. ## Deliberate deferrals (marked TODO(phase4-batch-b) in-file) - writer_ctx()'s via_valid_stream_association always returns false (fail-closed) until batch B's EP-ID/byte_bus_id association table lands (needs a request_stream_index field EpIdMappingEntry doesn't have yet). - HW pin map, request-stream-cfg, response-queue-cfg, EP-ID map, optional-subsystem config, ep_functional_cfg_t: untouched, batch B. ## Cross-cutting note c-RCP's ep_generic_cfg_t has no per-role CRC fields (that's ep_functional_cfg_t's, batch B); cpp-RCP's pre-existing EndpointGenericConfig already had three (ep_req/ack/response_crc_enable) depended on by e2e.hpp/test_e2e.cpp -- kept, marked in-file as pre-existing/non-c-RCP content rather than silently dropped or relocated (relocation needs ep_functional_cfg_t, batch B's scope). ## Call sites mock.hpp: RegisterMap::endpoint_count -> general.svr_ep_count, magic -> general.magic (2 lines). test_mock.cpp: matching 4-line fix. No other files touched. ## Verification test_regmap.cpp: 558 -> 1105 lines, 28 -> 57 TEST_CASEs (327 assertions). Full tree rebuilds clean (0 errors, 0 warnings), 56/56 ctest suites pass (100%) -- independently re-verified from a fresh build, not just the porting agent's own report. Heavy regmap dependents (test_e2e, test_watchdog, test_request, test_discovery, test_mock, test_gpio, test_adc) spot-checked with unchanged assertion counts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
…P-ID map, optional subsystems Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Phase 4 batch B of the two-batch regmap port; batch A (general map, EP0, generic/functional config split, merged as #141) is the prerequisite this builds on. ## Ported (c-RCP regmap.h/.c -> cpp-RCP regmap.hpp) - EpFunctionalCfg (regmap.h:1614-1627) -- content-modeling only, see reconciliation below. - HW pin mapping: HwPinMapEntry (regmap.h:1685-1697, replaces the old pin_id/function placeholder -- zero consumers existed for it anywhere in the tree), hw_pin::k* bit constants (Table 22), hw_pin_map::render/apply_reconfig, HwPinMapReconfigErrc (REQ-RMAP-040/041). - NamedSignal enum + named_signal_string()/named_signal_ep_signal_nr() (regmap.h:1798-1906). - RequestStreamConfig extended (not replaced -- rx_wd_timeout_interval etc. kept verbatim since e2e.hpp/watchdog.hpp/sim.hpp and their tests depend on those exact names) with rx_secure_channel_index/ rx_ack_stream_index/rx_resp_stream_index(default 1)/ rx_stream_max_request_size; request_stream_cfg:: wd_timeout_ms_to_ticks/_ticks_to_ms, render/apply_reconfig (TC18 0.5.1_RC5 issue #458 4-bit reconciliation, AND-not-OR coupling), resolve_index. - ResponseQueueConfig replaced (old shape had zero real consumers; comments in respqueue.hpp/deadline.hpp already referenced fields that never existed) with c-RCP's real per-queue row + response_queue_stream_id(), render/apply_reconfig. - EpIdMappingEntry extended with request_stream_index/crc_required appended last (preserves existing 2-element brace-inits in mock.hpp/tests) + ep_id_map::{is_ascending, effective_count, row_init_default, render, apply_reconfig, has_single_client_per_ep, shared_bus_homogeneous, ep_type_has_fixed_ep_id, byte_bus_ids_for_stream, is_valid_association}. - OptionalSubsystemCfg + optional_subsystem_cfg::apply_reconfig (REQ-RMAP-039), wired into RegisterMap as 4 new fields. ## Batch-A TODOs closed - writer_ctx()'s via_valid_stream_association now calls the real ep_id_map::is_valid_association() instead of the fail-closed stub. - EndpointGenericConfig's 3 pre-existing CRC fields: confirmed as a deliberate, distinct mechanism (not c-RCP's ep_req_crc_enable) -- every concrete endpoint type from this project's own Phase 3 (gpio.hpp, adc.hpp, etc.) already independently ported c-RCP's real functional-cfg fields inline into its own struct, so EpFunctionalCfg is content-modeling only, matching SvrEpCfg's existing disposition. ## Deliberately deferred (documented in-file) c-RCP's EP0 address-routed dispatcher (regmap.h:3072-3549, ~500 lines combining every sub-table with lifecycle authorization + sequencer ownership) and the sequencer-table wire codec -- neither is named in batch B's scope; only their content/mechanism pieces were ported. No bugs found versus c-RCP during the port (render/apply_reconfig logic, including the AND-not-OR bit coupling and saturation rules, reproduced faithfully); one self-contradictory assertion in this batch's own new test was caught and fixed before it reached the build. ## Verification regmap.hpp: 1328 -> 2654 lines. test_regmap.cpp: 1105 -> 1904 lines, 59 -> 120 TEST_CASEs (332 -> 2433 assertions). Full tree rebuilds clean (0 errors, 0 warnings), 56/56 ctest suites pass (100%) -- independently re-verified from a fresh build, not just the porting agent's own report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
…idation gaps Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Closes the gap on discovery.hpp, which the Phase 4 scoping pass found already about 1/3 of the way there (core request/response and the DiscoveryClaim state machine already worked). ## Kept as-is (already correct) kDiscoveryByteBusId/kDiscoveryRegisterAddress/kDiscoveryDefaultReadSize, make_discovery_request/encode_discovery_request, should_answer_discovery, and the core DiscoveryClaim claim/timeout state machine (ClaimOutcome, on_discovery_request, may_configure, has_active_claim/current_holder) -- all already matched c-RCP's semantics. ## Ported (c-RCP src/discovery.c/include/rcp/discovery.h) - should_drop_discovery() (c-RCP:56-59, REQ-DISC-001): generalized the NTSCF-only check to any non-NTSCF subtype, not just TSCF. - Shared detail::decode_common_frame() (c-RCP:68-102) for request/response/fragment decode -- see bug fix below. - DiscoveryResult, make_discovery_response, encode_discovery_response, decode_discovery_response (c-RCP:174-245, REQ-DISC-009..014), 14-octet general slice with the documented svr_version-is-32-bit-not-16 layout, pinned by an octet-address test. - Fragmented response path (c-RCP:250-378, REQ-DISC-025..028): discovery_response_fragment_count, encode/decode fragment, decode_discovery_reassembled_response, built on rcp::fragment following uart.hpp's existing wiring idiom (std::vector-returning, not c-RCP's fixed 255-entry stack array -- this codebase already heap-allocates on every other wire-encode path, so that particular ASIL-D optimization doesn't apply here). - DiscoveryCache (c-RCP:440-503, REQ-DISC-023/030): thin client-side convenience, std::vector-backed by design (not a protocol state machine, no fixed-capacity requirement). - release() on DiscoveryClaim (REQ-DISC-022, was missing entirely). - Expanded DiscoveryErrc from 1 to 5 values (short_frame, tscf_headed_request_dropped, bad_msg_type, wrong_bus, wrong_op), numbered to mirror c-RCP's rcp_discovery_errc_t 1:1. ## Bugs found and fixed 1. decode_discovery_request never validated byte_bus_id/op at all (REQ-DISC-006/007 untested and unenforced) -- fixed via the shared decode_common_frame. 2. DiscoveryClaim::on_configuration_request() previously *released* the claim on the first config write (holder_.reset()), contradicting c-RCP's rcp_discovery_claim_note_config_write() (c-RCP src/discovery.c:423-430), which *extends* the deadline and keeps the claim held so a claimant can issue multiple writes over time. Fixed to refresh claimed_at_, matching c-RCP's own test_claim_config_write_refreshes_deadline_for_claimant, now ported. ## regmap.hpp interplay Confirmed c-RCP's discovery.c never calls rcp_regmap_ep_id_map_is_valid_association() -- that's lifecycle.hpp's concern (via_valid_stream_association), not discovery's. Documented in-file rather than wiring it in. ## Deferred Wiring any of this into mock.hpp's dispatch -- out of scope, part of the later mock.hpp batch that closes out Phase 4. ## Verification discovery.hpp: 270 -> 818 lines. test_discovery.cpp: 230 -> 812 lines, ~20 -> 49 TEST_CASEs (686 assertions). Full tree rebuilds clean (0 errors, 0 warnings), 57/57 ctest suites pass (100%) -- independently re-verified from a fresh build against the fully-merged regmap base (both batches), not just the porting agent's own report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the v3.0.0 ground-up rewrite (#129, ROADMAP.md Phase 17), branch
rewrite/v3-from-c-rcp. Closes the gap ondiscovery.hpp, which the Phase 4 scoping pass found already about 1/3 of the way there.Kept as-is
Core request/response,
should_answer_discovery, and theDiscoveryClaimclaim/timeout state machine — already matched c-RCP's semantics.Ported
should_drop_discovery()(REQ-DISC-001): generalized to any non-NTSCF subtype.decode_common_frame()for request/response/fragment decode.DiscoveryResult, response encode/decode (REQ-DISC-009..014), 14-octet general slice.rcp::fragment.DiscoveryCache(REQ-DISC-023/030).release()onDiscoveryClaim(REQ-DISC-022, was missing entirely).DiscoveryErrcexpanded 1→5 values, mirroring c-RCP's enum 1:1.Bugs found and fixed
decode_discovery_requestnever validatedbyte_bus_id/opat all (REQ-DISC-006/007 untested and unenforced) — fixed via the shared decode path.DiscoveryClaim::on_configuration_request()previously released the claim on the first config write, contradicting c-RCP's semantics (which extends the deadline, keeping the claim held across multiple writes). Fixed to refresh the deadline instead, matching c-RCP's own ported test.regmap.hpp interplay
Confirmed c-RCP's
discovery.cnever callsis_valid_association()— that'slifecycle.hpp's concern, not discovery's. Documented rather than wired in.Deferred
Wiring into
mock.hpp's dispatch — part of the latermock.hppbatch closing out Phase 4.Verification
discovery.hpp: 270→818 lines.test_discovery.cpp: 230→812 lines, ~20→49 TEST_CASEs (686 assertions). Full tree rebuilds clean (0 errors/warnings), 57/57 ctest suites pass (100%) — independently re-verified against the fully-merged regmap base.