Skip to content

rewrite(phase4): fix adapt.hpp's missing read_size_or_segment_num field, add test_adapt.cpp - #145

Merged
SoundMatt merged 6 commits into
rewrite/v3-from-c-rcpfrom
phase4/adapt
Aug 22, 2026
Merged

rewrite(phase4): fix adapt.hpp's missing read_size_or_segment_num field, add test_adapt.cpp#145
SoundMatt merged 6 commits into
rewrite/v3-from-c-rcpfrom
phase4/adapt

Conversation

@SoundMatt

@SoundMatt SoundMatt commented Aug 22, 2026

Copy link
Copy Markdown
Owner

Part of the v3.0.0 ground-up rewrite (#129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Closes out adapt.hpp, the last piece before mock.hpp finishes Phase 4.

The scoping claim did NOT fully hold up

A prior scoping pass characterized adapt.hpp's simpler generic RequestFn/op+evt_op design as behaviorally equivalent to c-RCP's richer per-op field-table adapt.c/.h, with the only real gap being missing test coverage. That was a sizing estimate, not a line-by-line verification — this batch actually did that comparison and found a genuine, load-bearing bug underneath the otherwise-correct shape.

Bug found and fixed (real, not cosmetic)

message_to_request()/response_to_message() never threaded the ACF header's own 12-bit read_size_or_segment_num field through at all, silently leaving it 0 for every relay::Message. This field is what uart.hpp/spi.hpp/adc.hpp/i2c.hpp (and mock.hpp's dispatch_uart()) directly depend on. For I2C it is the ONLY thing selecting read vs. write direction (0 = write, nonzero = read): before this fix, an I2C read could never be requested through Adapt() at all, and a UART read would always drain 0 bytes regardless of what was actually buffered.

c-RCP's own src/adapt.c threads the identical wire field through this same layer via 5 separate per-op table rows. Fixed here with one generic read_size_from_meta() ("rcp.read_size", decimal, 0-4095 matching the ACF header's own field width) wired into both directions — one key suffices since it's the same wire field regardless of endpoint type, consistent with how rcp.evt_op already covers GPIO/PWM_OUT/SPI generically.

Purely additive — no call site needed updating (checked redundancy.hpp, faultinject.hpp, cli.hpp and their tests).

Flagged, explicitly out of scope

CAN's frame_format/arbitration_id and MDIO's clause/prtad/devad/regad have no path through Adapt() today either — a pre-existing, already-documented gap (no CAN wire codec exists yet in this layer; MDIO's address-packing helpers are namespace detail, not public), not an adapt.hpp regression.

Verification

New tests/test_adapt.cpp: 508 lines, 29 TEST_CASEs, 67 assertions — meta parsing, full field mapping, opaque-payload passthrough, error propagation, and end-to-end regression tests against mock::Server proving the fix (a UART read via Adapt() now correctly drains exactly rcp.read_size bytes). adapt.hpp: 228→304 lines.

Full tree rebuilds clean (0 errors/warnings), 58/58 ctest suites pass (100%) — independently re-verified from a fresh build against the merged discovery+regmap+server base.

SoundMatt and others added 6 commits August 21, 2026 15:01
@SoundMatt @claude
rewrite(phase4): port regmap batch A from c-RCP — general map, EP0, g…
83d95f1 
…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>
@SoundMatt @claude
rewrite(phase4): port regmap batch B from c-RCP — HW pins, streams, E…
119d54e 
…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>
@SoundMatt
merge: pick up #143 (regmap batch B) before finalizing discovery
450dd26 
Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
@SoundMatt @claude
rewrite(phase4): port discovery from c-RCP, fix claim-release and val…
b94b676 
…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>
@SoundMatt
merge: pick up #144 (discovery) before finalizing adapt
9d1d2dd 
Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
@SoundMatt @claude
rewrite(phase4): fix adapt.hpp's missing read_size_or_segment_num fie…
d79b351 
…ld, add test_adapt.cpp

Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17),
branch rewrite/v3-from-c-rcp. Closes out adapt.hpp, the last piece
before mock.hpp finishes Phase 4.

## The scoping claim did NOT fully hold up

A prior scoping pass characterized adapt.hpp's simpler generic
RequestFn/op+evt_op design as behaviorally equivalent to c-RCP's richer
per-op field-table adapt.c/.h, with the only real gap being missing
test coverage. That was a sizing estimate, not a line-by-line
verification -- this batch actually did that comparison and found a
genuine, load-bearing bug underneath the otherwise-correct shape.

## Bug found and fixed (real, not cosmetic)

message_to_request()/response_to_message() never threaded the ACF
header's own 12-bit read_size_or_segment_num field through at all,
silently leaving it 0 for every relay::Message. This field is what
uart.hpp/spi.hpp/adc.hpp/i2c.hpp (and mock.hpp's dispatch_uart())
directly depend on -- confirmed against their own doc comments and
dispatch code. For I2C it is the ONLY thing selecting read vs. write
direction (0 = write, nonzero = read): before this fix, an I2C read
could never be requested through Adapt() at all, and a UART read would
always drain 0 bytes regardless of what was actually buffered.

c-RCP's own src/adapt.c threads the identical wire field through this
same layer via 5 separate per-op table rows (rcp.uart.read_size /
rcp.spi.read_size / rcp.adc.read_size / rcp.i2c.read_size /
rcp.iseled.read_size, src/adapt.c ~246-359). Fixed here with one
generic read_size_from_meta() (rcp.read_size, decimal, 0-4095
matching the ACF header's own field width) wired into both directions
-- one key suffices since it's the same wire field regardless of
endpoint type, consistent with how rcp.evt_op already covers
GPIO/PWM_OUT/SPI generically. REQ-RELAY-005's .fusa-reqs.json text
updated to mention read_size_or_segment_num.

Purely additive (new optional meta key, old default-0 behavior
preserved when absent) -- no call site needed updating. Checked
redundancy.hpp, faultinject.hpp, cli.hpp (both its direct-flag
send_one() path, unaffected, and its NDJSON send_stream() path, which
already forwards meta generically and gains read_size support for
free) plus their existing tests.

## Flagged, explicitly out of scope

CAN's frame_format/arbitration_id and MDIO's clause/prtad/devad/regad
have no path through Adapt() today -- a pre-existing, already-
documented gap (no CAN wire codec exists yet in this layer; MDIO's
address-packing helpers are namespace detail, not public), not an
adapt.hpp regression. Inventing a convention here without an
established downstream codec would be unfounded.

## Verification

New tests/test_adapt.cpp: 508 lines, 29 TEST_CASEs, 67 assertions --
meta parsing (including read_size's new boundary/garbage cases), full
message_to_request/response_to_message field mapping, opaque-payload
passthrough, RequestFn error propagation, and end-to-end regression
tests against mock::Server proving the fix (a UART read via Adapt()
now correctly drains exactly rcp.read_size bytes, GPIO-write/SPI-
channel evt_op equivalence). adapt.hpp: 228 -> 304 lines.

Full tree rebuilds clean (0 errors, 0 warnings), 58/58 ctest suites
pass (100%) -- independently re-verified from a fresh build against
the merged discovery+regmap+server base, 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>
@SoundMatt
SoundMatt merged commit ac93355 into rewrite/v3-from-c-rcp Aug 22, 2026
22 checks passed
@SoundMatt
SoundMatt deleted the phase4/adapt branch August 22, 2026 05:58
@SoundMatt SoundMatt mentioned this pull request Aug 22, 2026
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@SoundMatt