From 2e059abb29360ff227f387fd121d9eb193fb016f Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:47:05 -0700 Subject: [PATCH 1/2] fix: correct PWM_OUT/ISELED/MDIO/ADC wire formats (issues #70, #71, #72, #77) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four endpoint-type conformance fixes, each verified against the OPEN Alliance TC18 Remote Control Protocol Specification's own figures/tables rather than against prior extraction summaries: - rcp/pwm.hpp (#70): PWM_OUT/PWM_IN's period/active_duration were uint32_t (8-byte payload); the spec's pwmo request format figure shows a fixed 4-byte payload (two big-endian 16-bit words, period then active — the existing field order was already correct). Narrowed both fields to uint16_t and added encode_pwm_payload/decode_pwm_payload for the exact 4-byte wire codec. Also removed PWM_OUT's reuse of GPIO's 8-way bitmask/saturating-write combinator: the spec's PWM_OUT request handling only ever applies a request's values directly, so handle_write now only honors WriteSemantics::Replace and rejects everything else as non-combinable. - rcp/iseled.hpp (#71): IseledResponse::address was a full uint16_t, ::data was an open vector, and the header carried an invented 8-bit CRC (poly 0x07) its own comment admitted wasn't from the ISELED standard. The response format figure specifies a 12-bit address, a 12-bit data value, and an optional CRC whose real algorithm/width the ISELED standard (not TC18) defines. Repacked address/data to 12-bit range-validated fields and dropped the invented CRC outright rather than inventing a differently-sized one. - rcp/mdio.hpp (#72): replaced the invented IEEE 802.3 Clause 22/45 PHY-addressing model with the spec's actual mdio_mode/mdio_address/ mdio_payload model (Figure 42 + Table 57): a 2-bit mode selector, an opaque address field, and a payload that's 16 bits except for MMS multi-word access to MMS0/MMS1, which is 32 bits. - rcp/adc.hpp (#77): narrowed sample/result values from uint32_t to uint16_t throughout AdcEndpoint/compute_average, matching the spec's 16-bit ADC resolution ceiling and response-frame figures. Also removed the invented "ADC_NO_SIGNAL" identifier from the no-signal diagnostic message — Table 27 defines no ADC-specific no-signal error code (the only *_NO_SIGNAL entry, PWM_IN_NO_SIGNAL, belongs to PWM_IN). Updated .fusa-reqs.json requirement text for the affected REQ-PWM-002/003, REQ-ISELED-002/003, and REQ-MDIO-002/003/004 entries, and added a post-hoc fix-pass note to ROADMAP.md. All 53 ctest cases pass. Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- .fusa-reqs.json | 28 +++---- ROADMAP.md | 52 +++++++++++++ include/rcp/adc.hpp | 90 +++++++++++---------- include/rcp/iseled.hpp | 173 +++++++++++++++++++---------------------- include/rcp/mdio.hpp | 148 ++++++++++++++++++++++------------- include/rcp/pwm.hpp | 114 +++++++++++++++++++-------- tests/test_adc.cpp | 59 +++++++++----- tests/test_iseled.cpp | 93 ++++++++++++---------- tests/test_mdio.cpp | 146 +++++++++++++++++++--------------- tests/test_pwm.cpp | 89 +++++++++++++++++---- 10 files changed, 621 insertions(+), 371 deletions(-) diff --git a/.fusa-reqs.json b/.fusa-reqs.json index 44c56f4..8c0ff6d 100644 --- a/.fusa-reqs.json +++ b/.fusa-reqs.json @@ -2986,16 +2986,16 @@ }, { "id": "REQ-PWM-002", - "title": "PWM_OUT write semantics: generic combinators applied independently", - "text": "rcp::pwm::PwmOutEndpoint::handle_write shall apply Replace/Or/And/Xor to `period` and `active_duration` independently via rcp::endpoint::apply_bitmask_write.", + "title": "PWM_OUT write semantics: Replace applies directly", + "text": "rcp::pwm::PwmOutEndpoint::handle_write shall apply WriteSemantics::Replace by directly assigning `period` and `active_duration` from the request, matching the single write behavior the OPEN Alliance TC18 Remote Control Protocol Specification's PWM_OUT request-handling section describes.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" }, { "id": "REQ-PWM-003", - "title": "PWM_OUT write semantics: saturating Add/Subtract shared with GPIO", - "text": "rcp::pwm::PwmOutEndpoint::handle_write shall apply the saturating Add/Subtract rule to `period` and `active_duration` independently, clamping instead of wrapping, via the same rcp::endpoint saturation helpers rcp::gpio::apply_gpio_write uses.", + "title": "PWM_OUT write semantics: Or/And/Xor/Add/Subtract rejected as non-combinable", + "text": "rcp::pwm::PwmOutEndpoint::handle_write shall return rcp::endpoint::EndpointErrc::non_combinable_write_semantics without modifying its state for WriteSemantics::Or, ::And, ::Xor, ::Add, and ::Subtract, since the specification's PWM_OUT request-handling section defines no bitmask/saturating-write combination against the endpoint's previous period/active values.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" @@ -3226,16 +3226,16 @@ }, { "id": "REQ-ISELED-002", - "title": "ISELED Instruction/Address/Data request and Address/Data(/CRC) response shape", - "text": "rcp::iseled::IseledEndpoint::transact shall record the exact IseledRequest (instruction/address/data) and IseledResponse (address/data/optional native_crc) fields passed to it, retrievable via last_request()/last_response().", + "title": "ISELED Instruction/Address/Data request and Address/Data response shape", + "text": "rcp::iseled::IseledEndpoint::transact shall record the exact IseledRequest (instruction/address/data) and IseledResponse (address/data) fields passed to it, retrievable via last_request()/last_response(), for a request and response whose fields fit their documented wire widths.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" }, { "id": "REQ-ISELED-003", - "title": "ISELED native CRC is additional to, not a replacement for, the general E2E CRC", - "text": "rcp::iseled::verify_native_crc shall succeed without error when IseledResponse::native_crc is std::nullopt, shall succeed when native_crc equals compute_native_crc8(crc_coverage_bytes(response)), and shall return IseledErrc::native_crc_mismatch when it does not; this header shall have no dependency on and shall not call into rcp/e2e.hpp.", + "title": "ISELED 12-bit address / 12-bit data field-width validation", + "text": "rcp::iseled::validate_request shall return IseledErrc::field_out_of_range when instruction exceeds kIseledInstructionMask (4 bits) or address exceeds kIseledFieldMask (12 bits); rcp::iseled::validate_response shall return IseledErrc::field_out_of_range when address or data exceeds kIseledFieldMask (12 bits); rcp::iseled::IseledEndpoint::transact shall propagate either failure without recording the request/response or firing TransferComplete.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" @@ -3266,24 +3266,24 @@ }, { "id": "REQ-MDIO-002", - "title": "MDIO address-range validation", - "text": "rcp::mdio::validate_request shall return MdioErrc::phy_address_out_of_range for phy_address > kMaxPhyAddress, MdioErrc::device_or_reg_out_of_range for device_or_reg > kMaxDeviceOrRegField, and shall accept every field at or below its own bound.", + "title": "MDIO mdio_payload width validation per mdio_mode", + "text": "rcp::mdio::payload_width_bits shall return 16 for MdioMode::MmdSingleWord, MdioMode::MmdMultiWord, and MdioMode::MmsSingleWord regardless of mms_is_0_or_1, and shall return 32 for MdioMode::MmsMultiWord only when mms_is_0_or_1 is true (16 otherwise); rcp::mdio::validate_request shall return MdioErrc::payload_exceeds_mode_width when mdio_payload exceeds that width and shall accept every payload at or below it.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" }, { "id": "REQ-MDIO-003", - "title": "MDIO Clause22 flat register write-then-read round-trip", - "text": "rcp::mdio::MdioEndpoint::handle_request shall, for MdioClause::Clause22, return the value most recently written to a given (phy_address, device_or_reg) register on a subsequent read, and shall return 0 for a register that has never been written.", + "title": "MDIO register write-then-read round-trip", + "text": "rcp::mdio::MdioEndpoint::handle_request shall, for a given (mdio_mode, mdio_address) key, return the value most recently written to that register on a subsequent read, and shall return 0 for a register that has never been written.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" }, { "id": "REQ-MDIO-004", - "title": "MDIO Clause45 device+register addressing is distinct from Clause22's address space", - "text": "rcp::mdio::MdioEndpoint::handle_request shall key a Clause45 access on (phy_address, device_or_reg, register_address) such that it never observes a value written via MdioClause::Clause22 at the same phy_address/device_or_reg bit pattern, or vice versa, and shall notify TransferComplete on every request regardless of clause.", + "title": "MDIO mdio_mode keeps each mode's register space distinct", + "text": "rcp::mdio::MdioEndpoint::handle_request shall key a register access on (mdio_mode, mdio_address) such that it never observes a value written under a different mdio_mode at the same mdio_address, and shall notify TransferComplete on every request regardless of mdio_mode.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" diff --git a/ROADMAP.md b/ROADMAP.md index ab6882c..5b9403c 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1665,6 +1665,58 @@ corrected to match, without any behavior change: real cross-repo architectural difference, not a naming issue, and is intentionally left unresolved here — see issue #45. +### Post-hoc wire-format fix pass (issues #70, #71, #72, #77) + +A conformance audit of the milestone 48 (v2.4.0) and milestone 51 (v2.7.0) +endpoint types found four wire-format/error-identity defects, each verified +directly against the OPEN Alliance TC18 Remote Control Protocol +Specification's own figures/tables rather than against the extraction +summaries milestone 48/51 above were written from: + +- `rcp/pwm.hpp` (issue #70): PWM_OUT/PWM_IN's `PwmValue::period`/ + `active_duration` were `uint32_t` (an 8-byte payload); the spec's "pwmo + request format" figure (§13.7.5.3) shows a fixed 4-byte payload — two + big-endian 16-bit words, period then active (the milestone 48 field + *order* was already correct). Both fields are now `uint16_t`, and + `encode_pwm_payload`/`decode_pwm_payload` implement the exact 4-byte wire + codec. Separately, `PwmOutEndpoint::handle_write` no longer reuses + `rcp::endpoint::apply_bitmask_write`'s GPIO-style 8-way bitmask/ + saturating-write combinator — the spec's PWM_OUT request-handling section + describes only direct value application, not combination against the + endpoint's previous state, so only `WriteSemantics::Replace` is honored + now; every other write semantics is rejected as non-combinable. +- `rcp/iseled.hpp` (issue #71): `IseledResponse::address` was a full + `uint16_t`, `IseledResponse::data` was an open `std::vector`, and + the header carried an invented 8-bit CRC (polynomial `0x07`) its own prior + comment admitted was not from the ISELED standard. The spec's "iseled + response format" figure (Figure 41, §13.7.12.3) is explicit: a 12-bit + address, a 12-bit `Data[11:0]` value, and an *optional* CRC whose own + algorithm/width the ISELED standard (not TC18) defines. `address`/`data` + are now range-validated to 12 bits (`kIseledFieldMask`); the invented CRC + is removed outright rather than replaced with a differently-sized + invented one, since no verified ISELED-standard CRC algorithm is + available in this codebase — see issue #71 discussion. +- `rcp/mdio.hpp` (issue #72): the header modeled an invented IEEE 802.3 + Clause 22/Clause 45 PHY-addressing scheme with no basis in the spec's own + MDIO section. Rebuilt around the spec's actual `mdio_mode`/ + `mdio_address`/`mdio_payload` model (Figure 42 + Table 57, §13.7.13.3): + a 2-bit mode selector, an opaque address field ("as per IEEE & OA SPI + spec"), and a payload whose width is 16 bits except for MMS + multiple-(double-)word access to MMS0/MMS1, which is 32 bits. +- `rcp/adc.hpp` (issue #77): ADC sample/result values were `uint32_t` + throughout; the spec caps ADC resolution at 16 bits and its response + figures show "16 bit ADC value" fields (§13.7.9.1, Figure 32/34) — narrowed + to `uint16_t` throughout `AdcEndpoint`/`compute_average`. Separately, the + no-signal condition's diagnostic message previously rendered as + "ADC_NO_SIGNAL", implying a TC18-defined error code; Table 27 ("Error + codes in responses", §12.9.6) has no ADC-specific no-signal entry (the + only `*_NO_SIGNAL` code, `PWM_IN_NO_SIGNAL` = 9, belongs to PWM_IN) — the + message no longer claims that identity. + +None of these four are wire-format changes to any *other* endpoint type or +to the shared AVTPDU/ACF framing; `rcp/endpoint.hpp`, +`rcp/regmap.hpp`, and every other endpoint header are unmodified. + --- ### Appendix A — Legacy Roadmap (v0.1.0–v1.11, superseded) --- diff --git a/include/rcp/adc.hpp b/include/rcp/adc.hpp index 0db46d3..a36ed2a 100644 --- a/include/rcp/adc.hpp +++ b/include/rcp/adc.hpp @@ -8,9 +8,9 @@ // ADC endpoint (ep_type 0x09) — the three-level averaging model // (adc_sample_interval -> adc_avg_intervals_per_request -> // adc_combine_avg_values), request-driven sampling only (no free-running -// push), two self-triggering cadence patterns, and a no-signal timeout -// handling path analogous to PWM_IN's PWM_IN_NO_SIGNAL error (extraction -// §5.9). +// push), two self-triggering cadence patterns, and an internal no-signal +// condition analogous in *purpose*, but not in TC18 error-code identity, to +// PWM_IN's PWM_IN_NO_SIGNAL (extraction §5.9). // // ROADMAP.md milestone 48, "Basic Endpoint Types II — I2C, UART, ADC, // PWM_OUT, PWM_IN (v2.4.0)": ADC has no combinable write-request payload @@ -19,28 +19,32 @@ // rcp/endpoint.hpp's WriteSemantics/apply_bitmask_write at all — its shared // surface with the rest of this milestone is only the general // request-dispatch shape (one entry point per request) and the no-signal -// error-path pattern rcp/pwm.hpp's PwmInEndpoint also implements -// (PwmErrc::no_signal / AdcErrc::no_signal are deliberately parallel). +// error-path pattern rcp/pwm.hpp's PwmInEndpoint also implements. // -// Three-level averaging model (extraction §5.9), as implemented below: -// 1. adc_sample_interval: spacing between individual raw -// samples (a caller/clock concern — -// this header does not itself model -// wall-clock time, see AdcCadence) -// 2. adc_avg_intervals_per_request: how many adc_sample_interval-spaced -// raw samples are averaged into one -// "averaged-interval" value -// 3. adc_combine_avg_values: how many averaged-interval values -// are further combined into the -// single value returned to the -// request +// Sample/result width fix (issue #77, cpp-RCP-09): this header previously +// modeled ADC sample and result values as uint32_t throughout. Verified +// against the OPEN Alliance TC18 Remote Control Protocol Specification's +// ADC section (§13.7.9.1, "This endpoint type is for ADC's with a +// resolution of up to 16bits") and its response-frame figures (Figure 32, +// Figure 34), which are explicit about "16 bit ADC value" fields: ADC +// samples and results are 16-bit quantities, not 32-bit. compute_average +// and AdcEndpoint::request_reading/request_reading_from_trigger_queue below +// operate on uint16_t samples/results (widening only internally, to a +// 64-bit accumulator, to sum without overflow — the averaged *result* +// itself is always narrowed back to uint16_t, matching the wire width). // -// Two self-triggering cadence patterns (extraction §5.9), as implemented -// below: SelfTimed, where the caller supplies raw samples already paced by -// adc_sample_interval (e.g. a wall-clock-driven sampler), and -// ExternalTrigger, where raw samples are instead paced by an external -// trigger-signal occurrence — the roadmap's own example being rcp/pwm.hpp's -// PwmInEndpoint mid-pulse signal keying ADC sampling cadence. +// No-signal error-identity fix (issue #77, cpp-RCP-09): this header +// previously rendered AdcErrc::no_signal's message as literally containing +// "ADC_NO_SIGNAL", implying a TC18-defined numeric error code by that name. +// Checked against the spec's Table 27 ("Error codes in responses", +// §12.9.6): there is no ADC-specific no-signal entry there at all — the +// only "*_NO_SIGNAL" entry in the whole table is PWM_IN_NO_SIGNAL (value 9), +// which belongs to the PWM_IN endpoint type, not ADC. AdcErrc::no_signal +// below is now documented and rendered as a plain internal library +// condition with no claimed spec error-code identity; it remains +// conceptually parallel to PWM_IN's no-signal handling (a request that +// cannot produce a valid sample) without pretending to be, or to map to, +// any numbered TC18 error code. // // Field names and behavior below implement TC18's *behavior* as described in // an internal structured extraction of the specification named above; no @@ -78,7 +82,7 @@ enum class AdcCadence : uint8_t { // ── Errors ──────────────────────────────────────────────────────────────────── enum class AdcErrc : int { - no_signal = 1, // analogous to PWM_IN_NO_SIGNAL: no valid sample before timeout/underrun + no_signal = 1, // internal condition: no valid sample before timeout/underrun — not a TC18 error code (see header comment; Table 27 has no ADC-specific no-signal entry) invalid_averaging_config = 2, // adc_avg_intervals_per_request or adc_combine_avg_values is 0 }; @@ -88,7 +92,8 @@ inline const std::error_category& adc_category() noexcept { std::string message(int ev) const override { switch (static_cast(ev)) { case AdcErrc::no_signal: - return "rcp/adc: no signal — no valid sample captured (ADC_NO_SIGNAL)"; + return "rcp/adc: no signal — no valid sample captured (an internal condition; " + "TC18's Table 27 defines no ADC-specific no-signal error code)"; case AdcErrc::invalid_averaging_config: return "rcp/adc: adc_avg_intervals_per_request/adc_combine_avg_values must be >= 1"; default: @@ -108,13 +113,16 @@ inline std::error_code make_error_code(AdcErrc e) noexcept { // Both levels of the averaging model use the same arithmetic-mean // combinator in this implementation — the extraction does not mandate a // different combination function per level, so reusing one keeps the two -// levels' behavior obviously consistent with each other. +// levels' behavior obviously consistent with each other. Samples and the +// averaged result are 16-bit (the spec's ADC resolution ceiling, §13.7.9.1); +// summation widens internally to avoid overflow but the result is narrowed +// back to uint16_t. -inline std::error_code compute_average(const std::vector& samples, uint32_t& out_avg) noexcept { +inline std::error_code compute_average(const std::vector& samples, uint16_t& out_avg) noexcept { if (samples.empty()) return make_error_code(AdcErrc::no_signal); uint64_t sum = 0; - for (const uint32_t s : samples) sum += s; - out_avg = static_cast(sum / samples.size()); + for (const uint16_t s : samples) sum += s; + out_avg = static_cast(sum / samples.size()); return {}; } @@ -130,25 +138,25 @@ class AdcEndpoint { // by adc_sample_interval on the caller's side, e.g. by a wall-clock // driven sampler) and must return std::nullopt to report a failed/ // missing capture, which this function surfaces as AdcErrc::no_signal — - // this endpoint's analog of PWM_IN's PWM_IN_NO_SIGNAL on the request - // path. + // this endpoint's analog of PWM_IN's no-signal handling on the request + // path (see header comment: not itself a TC18 error code). template std::error_code request_reading(const AdcAveragingConfig& cfg, SampleFn take_sample, - uint32_t& out_value) { + uint16_t& out_value) { if (cfg.adc_avg_intervals_per_request == 0 || cfg.adc_combine_avg_values == 0) return make_error_code(AdcErrc::invalid_averaging_config); - std::vector averaged_intervals; + std::vector averaged_intervals; averaged_intervals.reserve(cfg.adc_combine_avg_values); for (uint16_t i = 0; i < cfg.adc_combine_avg_values; ++i) { - std::vector raw; + std::vector raw; raw.reserve(cfg.adc_avg_intervals_per_request); for (uint16_t j = 0; j < cfg.adc_avg_intervals_per_request; ++j) { - std::optional sample = take_sample(); + std::optional sample = take_sample(); if (!sample.has_value()) return make_error_code(AdcErrc::no_signal); raw.push_back(*sample); } - uint32_t interval_avg = 0; + uint16_t interval_avg = 0; auto ec = compute_average(raw, interval_avg); if (ec) return ec; averaged_intervals.push_back(interval_avg); @@ -166,8 +174,8 @@ class AdcEndpoint { // with no valid capture) or a queue underrun (fewer trigger occurrences // than required). std::error_code request_reading_from_trigger_queue( - const AdcAveragingConfig& cfg, std::vector>& triggered_samples, - uint32_t& out_value) { + const AdcAveragingConfig& cfg, std::vector>& triggered_samples, + uint16_t& out_value) { if (cfg.adc_avg_intervals_per_request == 0 || cfg.adc_combine_avg_values == 0) return make_error_code(AdcErrc::invalid_averaging_config); @@ -175,17 +183,17 @@ class AdcEndpoint { static_cast(cfg.adc_avg_intervals_per_request) * cfg.adc_combine_avg_values; if (triggered_samples.size() < needed) return make_error_code(AdcErrc::no_signal); - std::vector averaged_intervals; + std::vector averaged_intervals; averaged_intervals.reserve(cfg.adc_combine_avg_values); size_t cursor = 0; for (uint16_t i = 0; i < cfg.adc_combine_avg_values; ++i) { - std::vector raw; + std::vector raw; raw.reserve(cfg.adc_avg_intervals_per_request); for (uint16_t j = 0; j < cfg.adc_avg_intervals_per_request; ++j, ++cursor) { if (!triggered_samples[cursor].has_value()) return make_error_code(AdcErrc::no_signal); raw.push_back(*triggered_samples[cursor]); } - uint32_t interval_avg = 0; + uint16_t interval_avg = 0; auto ec = compute_average(raw, interval_avg); if (ec) return ec; averaged_intervals.push_back(interval_avg); diff --git a/include/rcp/iseled.hpp b/include/rcp/iseled.hpp index f1b537a..4e47019 100644 --- a/include/rcp/iseled.hpp +++ b/include/rcp/iseled.hpp @@ -5,48 +5,58 @@ // fusa:req REQ-ISELED-005 // ISELED endpoint (ep_type 0x0C) — the OPEN Alliance TC18 Remote Control -// Protocol Specification v0.5.1_RC's native ISELED daisy-chain framing: an -// Instruction/Address/Data request shape and an Address/Data/optional- -// native-CRC response shape (extraction §5.12). +// Protocol Specification v0.5.1_RC's native ISELED daisy-chain framing: a +// 4-bit Instruction / 12-bit Address / variable-length Data request shape +// and a 12-bit Address / 12-bit Data response shape (extraction §5.12). // // ROADMAP.md milestone 51, "Remaining Endpoint Types — LIN, CAN (incl. CAN -// XL), ISELED, MDIO, Wakeup Control (v2.7.0)": the point the roadmap calls -// out explicitly for ISELED is that its own native CRC (the optional -// trailing field in the response shape below) is ADDITIONAL to, not a -// replacement for, the general RCP-level end-to-end CRC rcp/e2e.hpp already -// implements (v2.6.0). This header has no dependency on rcp/e2e.hpp and does -// not call into it: rcp/e2e.hpp's CRC covers the whole ACF message -// (stream_id + avtp_timestamp + ACF header + payload) regardless of -// endpoint type, and continues to do so unmodified around an ISELED -// request/response exactly as it does around every other endpoint type's -// payload — ISELED's native CRC below is a second, independent, narrower -// check scoped only to the Address/Data content of one daisy-chain -// response, layered on top of that outer coverage rather than folded into -// or substituted for it. +// XL), ISELED, MDIO, Wakeup Control (v2.7.0)". // -// The ISELED daisy-chain protocol itself (its addressing scheme, its actual -// native CRC polynomial/width, its per-device propagation timing) is -// defined by the ISELED standard, not by TC18 RCP, and no such external -// specification is reproduced or matched exactly here — `compute_native_crc8` -// below computes a CRC-8 of this implementation's own choosing, present so -// this header's encode/decode/verify round-trip has a concrete algorithm to -// exercise, not a claim of bit-exact conformance with any particular ISELED -// silicon's real CRC. +// Wire-format fix (issue #71, cpp-RCP-02): this header previously modeled +// IseledResponse::address as a full uint16_t, IseledResponse::data as an +// open-ended std::vector, and carried an invented 8-bit CRC +// (polynomial 0x07) the header's own prior comment admitted was not from +// the ISELED standard. Verified against the OPEN Alliance TC18 Remote +// Control Protocol Specification's "iseled request format" (Figure 40) and +// "iseled response format" (Figure 41) figures, §13.7.12.3: the address +// field is 12 bits wide (both in the request and, per Figure 41, again in +// the response), and a response's data is a single 12-bit value +// ("Data[11:0]"), not an open byte vector. kIseledFieldMask below is that +// shared 12-bit width, applied to IseledRequest::address, +// IseledResponse::address, and IseledResponse::data via validate_request/ +// validate_response. IseledRequest::data is left as std::vector: +// unlike the response's single Data[11:0] value, the request's data is +// realistic-payload-length-dependent (Figure 40 shows Data1/Data2/Data3/... +// plus padding), so a byte vector remains the right shape there — only the +// response's fixed-width fields were wrong. +// +// This header's invented CRC-8 (polynomial 0x07) is removed outright rather +// than replaced with a differently-sized invented polynomial: the real +// ISELED-standard CRC algorithm and its optional-4-bit-trailer width (per +// Figure 41's "CRC (optional)" field) are not sourced anywhere in this +// codebase or in the TC18 spec extract this header is built from — the +// spec explicitly defers the ISELED CRC's own definition to the separate +// ISELED standard, which this repository does not have a verified copy of +// — so inventing a differently-sized polynomial here would repeat the same +// mistake this fix is for. IseledErrc::field_out_of_range takes over the +// error category in its place: it is a direct consequence of the same +// width fix (rejecting an instruction/address/data value that does not fit +// its documented field width), not a new invented protocol behavior. +// IseledSignal::NativeCrcError is removed along with it, leaving +// IseledSignal::TransferComplete as the endpoint's only trigger signal. // // 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 request/response -// field widths, the native-CRC algorithm, and the trigger-signal id -// encoding chosen in this file are this implementation's own, same as the -// equivalent disclaimers in rcp/avtp.hpp, rcp/regmap.hpp, rcp/endpoint.hpp, -// and rcp/e2e.hpp. +// text from that document is reproduced here. The concrete request `data` +// vector representation and trigger-signal id encoding chosen in this file +// are this implementation's own, same as the equivalent disclaimers in +// rcp/avtp.hpp, rcp/regmap.hpp, rcp/endpoint.hpp, and rcp/e2e.hpp. #pragma once #include #include #include -#include #include #include #include @@ -54,53 +64,31 @@ namespace rcp { namespace iseled { +// ── Field widths ────────────────────────────────────────────────────────────── +// 12-bit address/data field width shared by the request's Address field and +// the response's Address/Data[11:0] fields (Figures 40/41, §13.7.12.3); 4-bit +// instruction field width (Figure 40). + +constexpr uint16_t kIseledFieldMask = 0x0FFFu; // 12 bits: address, and response data +constexpr uint8_t kIseledInstructionMask = 0x0Fu; // 4 bits: request instruction + // ── Request / response shapes ──────────────────────────────────────────────── struct IseledRequest { - uint8_t instruction = 0; - uint16_t address = 0; // this implementation's own width for a chain-position + register address - std::vector data; + uint8_t instruction = 0; // 4-bit instruction (kIseledInstructionMask), per Figure 40 + uint16_t address = 0; // 12-bit address (kIseledFieldMask), per Figure 40 + std::vector data; // variable-length data bytes (Data1/Data2/Data3/... + padding), per Figure 40 }; struct IseledResponse { - uint16_t address = 0; - std::vector data; - std::optional native_crc; // optional, per the response shape's own "optional-native-CRC" naming + uint16_t address = 0; // 12-bit address (kIseledFieldMask), per Figure 41 + uint16_t data = 0; // 12-bit data value ("Data[11:0]"), per Figure 41 — a single field, not a byte vector }; -// ── Native CRC (this implementation's own algorithm — see header comment) ─── - -constexpr uint8_t kNativeCrcPoly = 0x07; // CRC-8/CCITT-shaped choice; not a claim of matching real ISELED silicon - -inline uint8_t compute_native_crc8(const std::vector& bytes) noexcept { - uint8_t crc = 0; - for (uint8_t b : bytes) { - crc = static_cast(crc ^ b); - for (int bit = 0; bit < 8; ++bit) { - crc = (crc & 0x80u) ? static_cast((crc << 1) ^ kNativeCrcPoly) - : static_cast(crc << 1); - } - } - return crc; -} - -// crc_coverage_bytes assembles exactly the bytes compute_native_crc8 covers -// for a response: the 2-byte big-endian address followed by the data field, -// matching how verify_native_crc below reconstructs the same buffer to -// check a response's optional native_crc. -inline std::vector crc_coverage_bytes(const IseledResponse& resp) { - std::vector buf; - buf.reserve(2 + resp.data.size()); - buf.push_back(static_cast(resp.address >> 8)); - buf.push_back(static_cast(resp.address & 0xFFu)); - buf.insert(buf.end(), resp.data.begin(), resp.data.end()); - return buf; -} - // ── Errors ──────────────────────────────────────────────────────────────────── enum class IseledErrc : int { - native_crc_mismatch = 1, // response carried a native_crc that does not match its own address+data + field_out_of_range = 1, // instruction/address/data exceeds its documented wire field width }; inline const std::error_category& iseled_category() noexcept { @@ -108,8 +96,8 @@ inline const std::error_category& iseled_category() noexcept { const char* name() const noexcept override { return "rcp.iseled"; } std::string message(int ev) const override { switch (static_cast(ev)) { - case IseledErrc::native_crc_mismatch: - return "rcp/iseled: response native CRC does not match its address+data"; + case IseledErrc::field_out_of_range: + return "rcp/iseled: instruction/address/data exceeds its wire field width"; default: return "rcp/iseled: unknown error"; } @@ -123,25 +111,28 @@ inline std::error_code make_error_code(IseledErrc e) noexcept { return {static_cast(e), iseled_category()}; } -// verify_native_crc is a no-op success when response.native_crc is not -// present (the field is optional per the response shape) and otherwise -// recomputes compute_native_crc8 over crc_coverage_bytes(response) and -// compares. This check is independent of, and has no effect on, the -// separate general RCP-level E2E CRC that rcp/e2e.hpp applies around the -// whole ACF message — see header comment. -inline std::error_code verify_native_crc(const IseledResponse& response) noexcept { - if (!response.native_crc.has_value()) return {}; - const uint8_t expected = compute_native_crc8(crc_coverage_bytes(response)); - if (expected != *response.native_crc) return make_error_code(IseledErrc::native_crc_mismatch); +// validate_request rejects an instruction wider than 4 bits or an address +// wider than 12 bits (Figure 40). +inline std::error_code validate_request(const IseledRequest& req) noexcept { + if (req.instruction > kIseledInstructionMask) return make_error_code(IseledErrc::field_out_of_range); + if (req.address > kIseledFieldMask) return make_error_code(IseledErrc::field_out_of_range); + return {}; +} + +// validate_response rejects an address or data value wider than 12 bits +// (Figure 41). +inline std::error_code validate_response(const IseledResponse& resp) noexcept { + if (resp.address > kIseledFieldMask) return make_error_code(IseledErrc::field_out_of_range); + if (resp.data > kIseledFieldMask) return make_error_code(IseledErrc::field_out_of_range); return {}; } // ── Trigger signals ─────────────────────────────────────────────────────────── -// One TransferComplete/NativeCrcError pair per IseledEndpoint instance, built -// on rcp/endpoint.hpp's generic TriggerRegistry, same primitive +// One TransferComplete signal per IseledEndpoint instance, built on +// rcp/endpoint.hpp's generic TriggerRegistry, same primitive // rcp/i2c.hpp's and rcp/spi.hpp's transfer-complete signals use. -enum class IseledSignal : uint8_t { TransferComplete = 0, NativeCrcError = 1 }; +enum class IseledSignal : uint8_t { TransferComplete = 0 }; constexpr endpoint::TriggerRegistry::SignalId iseled_signal_id(IseledSignal sig) noexcept { return static_cast(sig); @@ -150,20 +141,20 @@ constexpr endpoint::TriggerRegistry::SignalId iseled_signal_id(IseledSignal sig) // ── IseledEndpoint ──────────────────────────────────────────────────────────── // Mirrors rcp::spi::SpiEndpoint's/rcp::i2c::I2cEndpoint's shape: one // request-dispatch entry point per daisy-chain transaction, recording the -// exact Instruction/Address/Data request and Address/Data/optional-CRC -// response (supplied together by the caller — this header models the -// request/response and trigger-signal shape of one ISELED transaction, not -// an actual daisy-chain driver or its per-device propagation timing). +// exact Instruction/Address/Data request and Address/Data response +// (supplied together by the caller — this header models the request/ +// response and trigger-signal shape of one ISELED transaction, not an +// actual daisy-chain driver or its per-device propagation timing). A +// request or response whose fields do not fit their documented wire widths +// is rejected without being recorded. class IseledEndpoint { public: std::error_code transact(IseledRequest request, IseledResponse response) { - last_request_ = std::move(request); - auto ec = verify_native_crc(response); + if (auto ec = validate_request(request)) return ec; + if (auto ec = validate_response(response)) return ec; + + last_request_ = std::move(request); last_response_ = std::move(response); - if (ec) { - triggers_.notify(iseled_signal_id(IseledSignal::NativeCrcError)); - return ec; - } triggers_.notify(iseled_signal_id(IseledSignal::TransferComplete)); return {}; } diff --git a/include/rcp/mdio.hpp b/include/rcp/mdio.hpp index c0a54cf..a22523f 100644 --- a/include/rcp/mdio.hpp +++ b/include/rcp/mdio.hpp @@ -5,30 +5,65 @@ // fusa:req REQ-MDIO-005 // MDIO endpoint (ep_type 0x0D) — the OPEN Alliance TC18 Remote Control -// Protocol Specification v0.5.1_RC's Clause-22/Clause-45-style mode-selected -// PHY register access (extraction §5.13). +// Protocol Specification v0.5.1_RC's mdio_mode-selected register access +// (extraction §5.13). // // ROADMAP.md milestone 51, "Remaining Endpoint Types — LIN, CAN (incl. CAN -// XL), ISELED, MDIO, Wakeup Control (v2.7.0)": two points the roadmap calls -// out explicitly for MDIO. First, this endpoint type is fully specified in -// the source document but is not named in that document's own informative -// "ten interfaces" scope-summary list (extraction §1.2) — this +// XL), ISELED, MDIO, Wakeup Control (v2.7.0)": this endpoint type is fully +// specified in the source document but is not named in that document's own +// informative "ten interfaces" scope-summary list (extraction §1.2) — this // implementation treats that as an editorial omission in the summary list, // not as evidence MDIO is actually out of scope, and builds it anyway. -// Second, MDIO carries essentially no type-specific functional -// configuration beyond rcp/regmap.hpp's already-existing generic/functional -// endpoint config split (v2.1.0) — unlike rcp/gpio.hpp (which layers a -// pin-direction-plus-edge-mask block on top of that split), this header -// defines no MDIO-specific functional-config encode/decode pair at all, -// because the extraction does not call for one; MdioRequest/MdioResponse -// below are the entire type-specific surface. +// +// Addressing-model fix (issue #72, cpp-RCP-03): this header previously +// modeled MDIO addressing as an invented IEEE 802.3 Clause 22/Clause +// 45-style PHY-address scheme (a 5-bit PHY address, a Clause22 register / +// Clause45 device-address field, and an MdioClause selector) that has no +// basis in the TC18 spec's own MDIO section. Verified against the OPEN +// Alliance TC18 Remote Control Protocol Specification's "mdio request +// format" figure and Table 57 (§13.7.13.3): the real addressing model is a +// 2-bit mdio_mode selector, an mdio_address field ("as per IEEE & OA SPI +// spec" — i.e. opaque to this header, not decomposed into a PHY/device/ +// register split of this header's own invention), and an mdio_payload whose +// width mdio_mode (and, for one mode, which MMS device is addressed) +// determines. The Clause 22/Clause 45 abstraction (MdioClause, +// phy_address, device_or_reg, register_address, kMaxPhyAddress, +// kMaxDeviceOrRegField) is removed entirely and rebuilt around +// mdio_mode/mdio_address/mdio_payload below. +// +// mdio_mode's four values, per Table 57: +// 00b: MMD, single word access (16-bit payload) +// 01b: MMD, multiple byte access (16-bit payload) +// 10b: MMS, single word access (16-bit payload) +// 11b: MMS, multiple (double) word access (32-bit payload, but only when +// the addressed MMS device is MMS0 or MMS1 — any other MMS device +// number still uses a 16-bit payload even in this mode) +// Table 57 itself prints "01b" for both the first two rows (MMD single word +// and MMD multiple byte access), which cannot both be correct in a 2-bit +// field with four otherwise-unambiguous rows (10b and 11b are printed once +// each, for the two MMS rows) — this is treated as a transcription defect +// in the v0.5.1_RC table, not as evidence of a 3-value field. By +// elimination (00b is the only 2-bit value the table's other three rows +// leave unclaimed), MMD single word access is 00b and MMD multiple byte +// access is 01b, below. +// +// mdio_payload's width, per Table 57 ("for MMD, data fields are 16 bits. +// for MMS0 & 1: data fields are 32 bits. For other MMS, data fields are 16 +// bits"): always 16 bits for MMD (either sub-mode) and for MMS single word +// access; 32 bits for MMS multiple (double) word access specifically when +// the addressed MMS device is MMS0 or MMS1, 16 bits otherwise. Which MMS +// device number mdio_address addresses is part of the external IEEE 802.3 / +// OPEN Alliance SPI addressing scheme Table 57 itself defers to (mdio +// address: "As per IEEE & OA SPI spec") and is not decoded by this header; +// MdioRequest::mms_is_0_or_1 below is the caller-supplied fact +// payload_width_bits needs for the MmsMultiWord case. // // 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 request/response -// field widths and register-key composition chosen in this file are this -// implementation's own, same as the equivalent disclaimers in rcp/avtp.hpp, -// rcp/regmap.hpp, and rcp/endpoint.hpp. +// text from that document is reproduced here. The concrete register-key +// composition chosen in this file is this implementation's own, same as the +// equivalent disclaimers in rcp/avtp.hpp, rcp/regmap.hpp, and +// rcp/endpoint.hpp. #pragma once #include @@ -41,34 +76,42 @@ namespace rcp { namespace mdio { -// ── Clause selection ───────────────────────────────────────────────────────── -// Clause 22's addressing is a flat 5-bit register address within a PHY; -// Clause 45 adds a device (MMD) address plus a wider 16-bit register address -// within that device (extraction §5.13). +// ── mdio_mode ───────────────────────────────────────────────────────────────── +// See header comment for the 00b/01b elimination reasoning. -enum class MdioClause : uint8_t { Clause22 = 0, Clause45 = 1 }; +enum class MdioMode : uint8_t { + MmdSingleWord = 0b00, + MmdMultiWord = 0b01, + MmsSingleWord = 0b10, + MmsMultiWord = 0b11, +}; + +// payload_width_bits returns the mdio_payload width Table 57 assigns for +// `mode`, given whether the addressed MMS device (relevant only for +// MmsMultiWord) is MMS0 or MMS1. +constexpr uint8_t payload_width_bits(MdioMode mode, bool mms_is_0_or_1) noexcept { + if (mode == MdioMode::MmsMultiWord && mms_is_0_or_1) return 32; + return 16; +} -constexpr uint8_t kMaxPhyAddress = 0x1Fu; // 5-bit field -constexpr uint8_t kMaxDeviceOrRegField = 0x1Fu; // 5-bit field: Clause22 register addr, or Clause45 MMD device addr +// ── Request / response shapes ──────────────────────────────────────────────── struct MdioRequest { - MdioClause clause = MdioClause::Clause22; - uint8_t phy_address = 0; // 5-bit PHY address - uint8_t device_or_reg = 0; // Clause22: register address; Clause45: MMD device address - uint16_t register_address = 0; // Clause45 only: 16-bit register within the MMD; unused for Clause22 - bool is_write = false; - uint16_t write_value = 0; // meaningful only when is_write is true + MdioMode mode = MdioMode::MmdSingleWord; + uint16_t mdio_address = 0; // opaque per IEEE 802.3 / OA SPI addressing (Table 57); not decoded here + bool mms_is_0_or_1 = false; // only meaningful when mode == MmsMultiWord; see payload_width_bits + bool is_write = false; + uint32_t mdio_payload = 0; // width per payload_width_bits(mode, mms_is_0_or_1) }; struct MdioResponse { - uint16_t value = 0; + uint32_t mdio_payload = 0; }; // ── Errors ──────────────────────────────────────────────────────────────────── enum class MdioErrc : int { - phy_address_out_of_range = 1, - device_or_reg_out_of_range = 2, + payload_exceeds_mode_width = 1, // mdio_payload does not fit payload_width_bits(mode, mms_is_0_or_1) }; inline const std::error_category& mdio_category() noexcept { @@ -76,10 +119,8 @@ inline const std::error_category& mdio_category() noexcept { const char* name() const noexcept override { return "rcp.mdio"; } std::string message(int ev) const override { switch (static_cast(ev)) { - case MdioErrc::phy_address_out_of_range: - return "rcp/mdio: PHY address exceeds its 5-bit range"; - case MdioErrc::device_or_reg_out_of_range: - return "rcp/mdio: device/register address exceeds its 5-bit range"; + case MdioErrc::payload_exceeds_mode_width: + return "rcp/mdio: mdio_payload exceeds the width mdio_mode assigns it"; default: return "rcp/mdio: unknown error"; } @@ -94,9 +135,9 @@ inline std::error_code make_error_code(MdioErrc e) noexcept { } inline std::error_code validate_request(const MdioRequest& req) noexcept { - if (req.phy_address > kMaxPhyAddress) return make_error_code(MdioErrc::phy_address_out_of_range); - if (req.device_or_reg > kMaxDeviceOrRegField) - return make_error_code(MdioErrc::device_or_reg_out_of_range); + const uint8_t width = payload_width_bits(req.mode, req.mms_is_0_or_1); + const uint64_t max_value = (width == 32) ? 0xFFFFFFFFull : 0xFFFFull; + if (req.mdio_payload > max_value) return make_error_code(MdioErrc::payload_exceeds_mode_width); return {}; } @@ -112,10 +153,11 @@ constexpr endpoint::TriggerRegistry::SignalId mdio_signal_id(MdioSignal sig) noe } // ── MdioEndpoint ────────────────────────────────────────────────────────────── -// register_key folds (clause, phy_address, device_or_reg[, register_address -// for Clause45 only]) into one lookup key so Clause22's flat address space -// and Clause45's device+register address space never collide with each -// other, even for identical phy_address/device_or_reg bit patterns. +// register_key folds (mode, mdio_address) into one lookup key, so the four +// mdio_mode values never collide with each other even for an identical +// mdio_address bit pattern (mirroring how the register spaces of, e.g., +// MMD vs. MMS access are logically distinct on real MDIO-manageable +// devices, per the spec's own MMD/MMS split in Table 57). class MdioEndpoint { public: std::error_code handle_request(MdioRequest req, MdioResponse& out) { @@ -125,11 +167,11 @@ class MdioEndpoint { last_request_ = req; const uint64_t key = register_key(req); if (req.is_write) { - registers_[key] = req.write_value; - out.value = req.write_value; + registers_[key] = req.mdio_payload; + out.mdio_payload = req.mdio_payload; } else { const auto it = registers_.find(key); - out.value = (it != registers_.end()) ? it->second : uint16_t{0}; + out.mdio_payload = (it != registers_.end()) ? it->second : uint32_t{0}; } triggers_.notify(mdio_signal_id(MdioSignal::TransferComplete)); return {}; @@ -140,16 +182,12 @@ class MdioEndpoint { private: static uint64_t register_key(const MdioRequest& req) noexcept { - uint64_t key = (static_cast(req.clause) << 40) | - (static_cast(req.phy_address) << 32) | - (static_cast(req.device_or_reg) << 16); - if (req.clause == MdioClause::Clause45) key |= req.register_address; - return key; + return (static_cast(req.mode) << 16) | req.mdio_address; } - endpoint::TriggerRegistry triggers_; - MdioRequest last_request_; - std::unordered_map registers_; + endpoint::TriggerRegistry triggers_; + MdioRequest last_request_; + std::unordered_map registers_; }; } // namespace mdio diff --git a/include/rcp/pwm.hpp b/include/rcp/pwm.hpp index c2127c1..f1a0c2f 100644 --- a/include/rcp/pwm.hpp +++ b/include/rcp/pwm.hpp @@ -7,26 +7,50 @@ // fusa:req REQ-PWM-007 // PWM_OUT (ep_type 0x07) and PWM_IN (ep_type 0x08) endpoints — the shared -// period/active-duration two-field payload shape, PWM_OUT's 8-way -// write-semantics reuse from GPIO, PWM_IN's response-only read model and -// PWM_IN_NO_SIGNAL error path, and the mid-pulse trigger signal used to key -// ADC sampling cadence (extraction §5.5, §5.6). +// period/active-duration two-field payload shape, PWM_OUT's fixed 4-byte +// wire payload, PWM_IN's response-only read model and PWM_IN_NO_SIGNAL +// error path, and the mid-pulse trigger signal used to key ADC sampling +// cadence (extraction §5.5, §5.6). // // ROADMAP.md milestone 48, "Basic Endpoint Types II — I2C, UART, ADC, // PWM_OUT, PWM_IN (v2.4.0)": kept in one header, mirroring how // rcp/endpoint.hpp is shared scaffolding for rcp/gpio.hpp and rcp/spi.hpp // rather than each having its own copy — PWM_OUT and PWM_IN share one // payload struct (PwmValue) and differ only in read/write direction and -// trigger-signal behavior. PWM_OUT reuses rcp/endpoint.hpp's -// WriteSemantics/apply_bitmask_write directly (including the -// saturating-add/subtract rule it already implements generically), the -// same primitive rcp/gpio.hpp's GpioEndpoint builds on, rather than -// re-deriving its own combinator. PWM_OUT defines no Reconfigure target of -// its own (unlike GPIO's pin-direction retarget) — the extraction does not -// name one for PWM_OUT, so a Reconfigure write is left to -// apply_bitmask_write's own built-in rejection -// (EndpointErrc::non_combinable_write_semantics) rather than this header -// inventing a meaning for it. +// trigger-signal behavior. +// +// Wire-format fix (issue #70, cpp-RCP-01): PWM_OUT/PWM_IN's payload is a +// fixed 4 bytes on the wire — two big-endian 16-bit words, PWM_Period +// followed by PWM_active, verified against the OPEN Alliance TC18 Remote +// Control Protocol Specification's "pwmo request format" figure (§13.7.5.3) +// — not the open-ended 8-byte uint32_t pair this header modeled before. +// `period`/`active_duration` are narrowed to uint16_t and encode_pwm_payload/ +// decode_pwm_payload below produce/consume exactly that 4-byte, period-then- +// active big-endian layout, reusing rcp/avtp.hpp's own put_u16/get_u16 +// helpers the same way rcp/gpio.hpp's encode_gpio_payload/ +// decode_gpio_payload already reuse its put_u32/get_u32 for GPIO's 4-byte +// payload, rather than re-deriving byte order here. The field *order* in +// PwmValue (period first, then active_duration) was already correct and is +// unchanged — only the two fields' width and the addition of an explicit +// wire codec are new. +// +// PWM_OUT write-semantics fix (issue #70, cpp-RCP-01, second half): this +// header previously layered rcp/gpio.hpp's/rcp/endpoint.hpp's generic +// 8-way evt[2:0] bitmask/saturating-write combinator +// (rcp::endpoint::apply_bitmask_write) onto PWM_OUT's period/active pair. +// The specification's own PWM_OUT request-handling section describes only +// one write behavior: a request supplies both values and they take effect +// directly (a request with PWM_Period of 0 stops generation; the whole +// point of Add/Or/Xor-style combination — bitwise OR/AND/XOR or saturating +// add/subtract against the *previous* period/active values — is never +// described for this endpoint type, and does not obviously make sense for +// a period/duty-cycle pair the way it does for GPIO's independent pin +// bits). PwmOutEndpoint::handle_write below therefore no longer calls +// apply_bitmask_write at all: WriteSemantics::Replace applies the request +// directly, and every other semantics (Or/And/Xor/Add/Subtract/Reserved/ +// Reconfigure) is rejected via the same +// EndpointErrc::non_combinable_write_semantics sentinel Reconfigure already +// used, without reusing apply_bitmask_write's combinator logic itself. // // Field names and behavior below implement TC18's *behavior* as described in // an internal structured extraction of the specification named above; no @@ -38,8 +62,10 @@ // and rcp/gpio.hpp. #pragma once +#include #include +#include #include #include #include @@ -50,13 +76,40 @@ namespace pwm { // ── Shared period/active-duration payload shape ─────────────────────────────── // `period` and `active_duration` share one implementation-defined tick unit // (extraction §5.5, §5.6); this header does not itself fix that unit to a -// physical time value. +// physical time value. Both fields are 16 bits wide on the wire — the +// PWM_OUT/PWM_IN payload is a fixed 4 bytes total (verified against the +// spec's "pwmo request format" figure, §13.7.5.3), not an open-ended +// 32-bit-per-field pair. struct PwmValue { - uint32_t period = 0; - uint32_t active_duration = 0; + uint16_t period = 0; + uint16_t active_duration = 0; }; +// ── Wire codec ──────────────────────────────────────────────────────────────── +// Fixed 4-byte payload: PWM_Period (big-endian 16 bit) followed by PWM_active +// (big-endian 16 bit), per the spec's "pwmo request format" figure +// (§13.7.5.3) — a request not having exactly four bytes is rejected there +// with INVALID_PARAMETER, which decode_pwm_payload below surfaces as +// avtp::AvtpErrc::short_buffer for a too-short buffer. + +constexpr size_t kPwmPayloadLen = 4; +using PwmWireBytes = std::array; + +inline PwmWireBytes encode_pwm_payload(const PwmValue& value) noexcept { + PwmWireBytes buf{}; + avtp::detail::put_u16(&buf[0], value.period); + avtp::detail::put_u16(&buf[2], value.active_duration); + return buf; +} + +inline std::error_code decode_pwm_payload(const uint8_t* buf, size_t len, PwmValue& out) noexcept { + if (len < kPwmPayloadLen) return avtp::make_error_code(avtp::AvtpErrc::short_buffer); + out.period = avtp::detail::get_u16(&buf[0]); + out.active_duration = avtp::detail::get_u16(&buf[2]); + return {}; +} + // ── Errors ──────────────────────────────────────────────────────────────────── enum class PwmErrc : int { @@ -82,27 +135,20 @@ inline std::error_code make_error_code(PwmErrc e) noexcept { } // ── PwmOutEndpoint (ep_type 0x07) ──────────────────────────────────────────── -// Applies one of GPIO's 8 write semantics independently to `period` and to -// `active_duration` — each field is combined against its own current value -// (this implementation's own choice: the extraction does not itself say -// whether the two fields share one combinator application or each combines -// independently, and independent combination is what this header -// implements). Reuses rcp::endpoint::apply_bitmask_write directly, so the -// saturating add/subtract rule endpoint.hpp already implements generically -// applies to both fields without PWM_OUT re-deriving it. +// handle_write applies WriteSemantics::Replace directly (the only write +// behavior the spec's PWM_OUT request-handling section describes) and +// rejects every other write semantics as non-combinable — see the header +// comment above for why this no longer reuses +// rcp::endpoint::apply_bitmask_write's generic bitmask/saturating-write +// combinator the way rcp/gpio.hpp does. class PwmOutEndpoint { public: std::error_code handle_write(endpoint::WriteSemantics op, PwmValue operand, PwmValue& out_value) noexcept { - uint32_t period_out = 0; - auto ec = endpoint::apply_bitmask_write(op, state_.period, operand.period, period_out); - if (ec) return ec; - uint32_t duration_out = 0; - ec = endpoint::apply_bitmask_write(op, state_.active_duration, operand.active_duration, - duration_out); - if (ec) return ec; - state_.period = period_out; - state_.active_duration = duration_out; + if (op != endpoint::WriteSemantics::Replace) { + return endpoint::make_error_code(endpoint::EndpointErrc::non_combinable_write_semantics); + } + state_ = operand; out_value = state_; return {}; } diff --git a/tests/test_adc.cpp b/tests/test_adc.cpp index 390cb13..8ca5f86 100644 --- a/tests/test_adc.cpp +++ b/tests/test_adc.cpp @@ -18,16 +18,27 @@ using namespace rcp::adc; // ── Three-level averaging model ────────────────────────────────────────────── TEST_CASE("compute_average computes an arithmetic mean", "[adc][REQ-ADC-001]") { - uint32_t out = 0; + uint16_t out = 0; REQUIRE_FALSE(compute_average({10, 20, 30}, out)); REQUIRE(out == 20); } TEST_CASE("compute_average reports no_signal on an empty sample set", "[adc][REQ-ADC-001]") { - uint32_t out = 0; + uint16_t out = 0; REQUIRE(compute_average({}, out) == make_error_code(AdcErrc::no_signal)); } +// ADC's resolution ceiling is 16 bits (§13.7.9.1) — compute_average must +// average values right up to that ceiling without overflowing internally +// (it widens to a 64-bit accumulator before narrowing the result back to +// uint16_t), unlike the previous uint32_t-everywhere model this replaces. +TEST_CASE("compute_average handles samples at the 16-bit ceiling without overflow", + "[adc][REQ-ADC-001]") { + uint16_t out = 0; + REQUIRE_FALSE(compute_average({0xFFFF, 0xFFFF, 0xFFFF}, out)); + REQUIRE(out == 0xFFFF); +} + TEST_CASE("request_reading combines level-1 and level-2 averaging correctly", "[adc][REQ-ADC-002]") { AdcEndpoint ep; AdcAveragingConfig cfg; @@ -36,15 +47,15 @@ TEST_CASE("request_reading combines level-1 and level-2 averaging correctly", "[ // Raw samples, consumed in order: interval 1 = {10,20} -> avg 15; // interval 2 = {30,40} -> avg 35; combined = avg(15,35) = 25. - std::deque raw{10, 20, 30, 40}; - auto take_sample = [&raw]() -> std::optional { + std::deque raw{10, 20, 30, 40}; + auto take_sample = [&raw]() -> std::optional { if (raw.empty()) return std::nullopt; - uint32_t v = raw.front(); + uint16_t v = raw.front(); raw.pop_front(); return v; }; - uint32_t out_value = 0; + uint16_t out_value = 0; auto ec = ep.request_reading(cfg, take_sample, out_value); REQUIRE_FALSE(ec); REQUIRE(out_value == 25); @@ -54,9 +65,9 @@ TEST_CASE("request_reading rejects a zero-valued averaging config field", "[adc] AdcEndpoint ep; AdcAveragingConfig cfg; cfg.adc_avg_intervals_per_request = 0; - auto take_sample = []() -> std::optional { return 1; }; + auto take_sample = []() -> std::optional { return 1; }; - uint32_t out_value = 0; + uint16_t out_value = 0; auto ec = ep.request_reading(cfg, take_sample, out_value); REQUIRE(ec == make_error_code(AdcErrc::invalid_averaging_config)); } @@ -69,9 +80,9 @@ TEST_CASE("request_reading reports no_signal when take_sample underruns", "[adc] cfg.adc_avg_intervals_per_request = 3; cfg.adc_combine_avg_values = 1; - auto take_sample = []() -> std::optional { return std::nullopt; }; // no signal captured + auto take_sample = []() -> std::optional { return std::nullopt; }; // no signal captured - uint32_t out_value = 0; + uint16_t out_value = 0; auto ec = ep.request_reading(cfg, take_sample, out_value); REQUIRE(ec == make_error_code(AdcErrc::no_signal)); } @@ -84,12 +95,12 @@ TEST_CASE("request_reading only invokes take_sample exactly the required number cfg.adc_combine_avg_values = 3; int calls = 0; - auto take_sample = [&calls]() -> std::optional { + auto take_sample = [&calls]() -> std::optional { ++calls; - return 42u; + return uint16_t{42}; }; - uint32_t out_value = 0; + uint16_t out_value = 0; REQUIRE_FALSE(ep.request_reading(cfg, take_sample, out_value)); REQUIRE(calls == 6); // 2 * 3, no free-running/extra sampling REQUIRE(out_value == 42); @@ -104,9 +115,9 @@ TEST_CASE("request_reading_from_trigger_queue implements the ExternalTrigger cad cfg.adc_avg_intervals_per_request = 2; cfg.adc_combine_avg_values = 2; - std::vector> queue{10u, 20u, 30u, 40u, 999u}; // one extra entry left over + std::vector> queue{10u, 20u, 30u, 40u, 999u}; // one extra entry left over - uint32_t out_value = 0; + uint16_t out_value = 0; auto ec = ep.request_reading_from_trigger_queue(cfg, queue, out_value); REQUIRE_FALSE(ec); REQUIRE(out_value == 25); // same combination as the SelfTimed test above @@ -121,8 +132,8 @@ TEST_CASE("request_reading_from_trigger_queue reports no_signal on queue underru cfg.adc_avg_intervals_per_request = 2; cfg.adc_combine_avg_values = 2; - std::vector> queue{1u, 2u}; // fewer than the 4 needed - uint32_t out_value = 0; + std::vector> queue{1u, 2u}; // fewer than the 4 needed + uint16_t out_value = 0; auto ec = ep.request_reading_from_trigger_queue(cfg, queue, out_value); REQUIRE(ec == make_error_code(AdcErrc::no_signal)); } @@ -134,8 +145,8 @@ TEST_CASE("request_reading_from_trigger_queue reports no_signal on a missing-cap cfg.adc_avg_intervals_per_request = 2; cfg.adc_combine_avg_values = 1; - std::vector> queue{10u, std::nullopt}; // trigger occurred, no valid capture - uint32_t out_value = 0; + std::vector> queue{10u, std::nullopt}; // trigger occurred, no valid capture + uint16_t out_value = 0; auto ec = ep.request_reading_from_trigger_queue(cfg, queue, out_value); REQUIRE(ec == make_error_code(AdcErrc::no_signal)); } @@ -147,3 +158,13 @@ TEST_CASE("AdcErrc reports a non-empty message in its own category", "[adc][REQ- REQUIRE(ec.category() == adc_category()); REQUIRE_FALSE(ec.message().empty()); } + +// AdcErrc::no_signal must not claim a TC18-defined error-code identity it +// does not have (issue #77): Table 27 defines no ADC-specific no-signal +// code, so the message must not contain an invented "ADC_NO_SIGNAL" +// identifier. +TEST_CASE("AdcErrc::no_signal's message does not claim an invented ADC_NO_SIGNAL spec identifier", + "[adc][REQ-ADC-006]") { + auto ec = make_error_code(AdcErrc::no_signal); + REQUIRE(ec.message().find("ADC_NO_SIGNAL") == std::string::npos); +} diff --git a/tests/test_iseled.cpp b/tests/test_iseled.cpp index 645b569..aee6776 100644 --- a/tests/test_iseled.cpp +++ b/tests/test_iseled.cpp @@ -20,7 +20,7 @@ TEST_CASE("ISELED's ep_type id is 0x0C", "[iseled][REQ-ISELED-001]") { REQUIRE(rcp::endpoint::kEndpointTypeIseled == 0x0C); } -// ── Instruction/Address/Data request, Address/Data(/CRC) response shape ───── +// ── Instruction/Address/Data request, Address/Data response shape ─────────── TEST_CASE("IseledEndpoint::transact records the exact request and response fields", "[iseled][REQ-ISELED-002]") { @@ -32,81 +32,94 @@ TEST_CASE("IseledEndpoint::transact records the exact request and response field IseledResponse resp; resp.address = 0x0102; - resp.data = {0xBE, 0xEF}; - // no native_crc — optional field left absent + resp.data = 0x0BEF; REQUIRE_FALSE(ep.transact(req, resp)); REQUIRE(ep.last_request().instruction == 0x03); REQUIRE(ep.last_request().address == 0x0102); REQUIRE(ep.last_request().data == std::vector{0xDE, 0xAD}); REQUIRE(ep.last_response().address == 0x0102); - REQUIRE(ep.last_response().data == std::vector{0xBE, 0xEF}); - REQUIRE_FALSE(ep.last_response().native_crc.has_value()); + REQUIRE(ep.last_response().data == 0x0BEF); } -// ── Native CRC: additional to, not a replacement for, the general E2E CRC ─── +// ── 12-bit address / 12-bit data field-width fix ───────────────────────────── +// Verified against the spec's "iseled request format" (Figure 40) and +// "iseled response format" (Figure 41): the address field is 12 bits wide +// in both request and response, and a response's data is a single 12-bit +// value, not an open byte vector. -TEST_CASE("compute_native_crc8/verify_native_crc round-trip a correct native CRC", +TEST_CASE("kIseledFieldMask is 12 bits and kIseledInstructionMask is 4 bits", "[iseled][REQ-ISELED-003]") { - IseledResponse resp; - resp.address = 0x00AB; - resp.data = {0x01, 0x02, 0x03}; - resp.native_crc = compute_native_crc8(crc_coverage_bytes(resp)); - - REQUIRE_FALSE(verify_native_crc(resp)); + REQUIRE(kIseledFieldMask == 0x0FFF); + REQUIRE(kIseledInstructionMask == 0x0F); } -TEST_CASE("verify_native_crc reports native_crc_mismatch for a tampered native CRC", +TEST_CASE("validate_request accepts an in-range instruction and address", "[iseled][REQ-ISELED-003]") { + IseledRequest req; + req.instruction = kIseledInstructionMask; + req.address = kIseledFieldMask; + REQUIRE_FALSE(validate_request(req)); +} + +TEST_CASE("validate_request rejects an instruction wider than 4 bits", "[iseled][REQ-ISELED-003]") { + IseledRequest req; + req.instruction = static_cast(kIseledInstructionMask + 1); + REQUIRE(validate_request(req) == make_error_code(IseledErrc::field_out_of_range)); +} + +TEST_CASE("validate_request rejects an address wider than 12 bits", "[iseled][REQ-ISELED-003]") { + IseledRequest req; + req.address = static_cast(kIseledFieldMask + 1); + REQUIRE(validate_request(req) == make_error_code(IseledErrc::field_out_of_range)); +} + +TEST_CASE("validate_response accepts an in-range address and data", "[iseled][REQ-ISELED-003]") { IseledResponse resp; - resp.address = 0x00AB; - resp.data = {0x01, 0x02, 0x03}; - resp.native_crc = static_cast(compute_native_crc8(crc_coverage_bytes(resp)) ^ 0xFFu); + resp.address = kIseledFieldMask; + resp.data = kIseledFieldMask; + REQUIRE_FALSE(validate_response(resp)); +} - REQUIRE(verify_native_crc(resp) == make_error_code(IseledErrc::native_crc_mismatch)); +TEST_CASE("validate_response rejects an address wider than 12 bits", "[iseled][REQ-ISELED-003]") { + IseledResponse resp; + resp.address = static_cast(kIseledFieldMask + 1); + REQUIRE(validate_response(resp) == make_error_code(IseledErrc::field_out_of_range)); } -TEST_CASE("verify_native_crc succeeds trivially when native_crc is absent (it is optional)", - "[iseled][REQ-ISELED-003]") { +TEST_CASE("validate_response rejects a data value wider than 12 bits", "[iseled][REQ-ISELED-003]") { IseledResponse resp; - resp.address = 0x0001; - resp.data = {0xFF}; - REQUIRE_FALSE(verify_native_crc(resp)); + resp.data = static_cast(kIseledFieldMask + 1); + REQUIRE(validate_response(resp) == make_error_code(IseledErrc::field_out_of_range)); } -TEST_CASE("IseledEndpoint::transact propagates a native CRC mismatch and fires NativeCrcError", +TEST_CASE("IseledEndpoint::transact rejects an out-of-range field without recording it", "[iseled][REQ-ISELED-003]") { IseledEndpoint ep; - ep.triggers().enable(iseled_signal_id(IseledSignal::TransferComplete)); - ep.triggers().enable(iseled_signal_id(IseledSignal::NativeCrcError)); - IseledRequest req; + req.address = static_cast(kIseledFieldMask + 1); + IseledResponse resp; - resp.address = 0x0010; - resp.data = {0x22}; - resp.native_crc = static_cast(compute_native_crc8(crc_coverage_bytes(resp)) ^ 0x01u); + resp.address = 0x0001; + resp.data = 0x0001; auto ec = ep.transact(req, resp); - REQUIRE(ec == make_error_code(IseledErrc::native_crc_mismatch)); - - auto drained = ep.triggers().drain(); - REQUIRE(drained.size() == 1); - REQUIRE(drained[0] == iseled_signal_id(IseledSignal::NativeCrcError)); + REQUIRE(ec == make_error_code(IseledErrc::field_out_of_range)); + // Nothing recorded, and no trigger fired for a rejected transaction. + REQUIRE_FALSE(ep.triggers().has_pending()); } // ── Trigger signals ─────────────────────────────────────────────────────────── -TEST_CASE("IseledEndpoint::transact fires only TransferComplete on a clean transaction", +TEST_CASE("IseledEndpoint::transact fires TransferComplete on a valid transaction", "[iseled][REQ-ISELED-004]") { IseledEndpoint ep; ep.triggers().enable(iseled_signal_id(IseledSignal::TransferComplete)); - ep.triggers().enable(iseled_signal_id(IseledSignal::NativeCrcError)); IseledRequest req; IseledResponse resp; resp.address = 0x0010; - resp.data = {0x22}; - resp.native_crc = compute_native_crc8(crc_coverage_bytes(resp)); + resp.data = 0x0022; REQUIRE_FALSE(ep.transact(req, resp)); @@ -118,7 +131,7 @@ TEST_CASE("IseledEndpoint::transact fires only TransferComplete on a clean trans // ── IseledErrc category sanity ──────────────────────────────────────────────── TEST_CASE("IseledErrc reports a non-empty message in its own category", "[iseled][REQ-ISELED-005]") { - auto ec = make_error_code(IseledErrc::native_crc_mismatch); + auto ec = make_error_code(IseledErrc::field_out_of_range); REQUIRE(ec.category() == iseled_category()); REQUIRE_FALSE(ec.message().empty()); } diff --git a/tests/test_mdio.cpp b/tests/test_mdio.cpp index e3289e9..84453b7 100644 --- a/tests/test_mdio.cpp +++ b/tests/test_mdio.cpp @@ -20,101 +20,121 @@ TEST_CASE("MDIO's ep_type id is 0x0D", "[mdio][REQ-MDIO-001]") { REQUIRE(rcp::endpoint::kEndpointTypeMdio == 0x0D); } -// ── Address-range validation ────────────────────────────────────────────────── +// ── mdio_payload width rule (Table 57) ─────────────────────────────────────── + +TEST_CASE("payload_width_bits is 16 for both MMD sub-modes regardless of mms_is_0_or_1", + "[mdio][REQ-MDIO-002]") { + REQUIRE(payload_width_bits(MdioMode::MmdSingleWord, false) == 16); + REQUIRE(payload_width_bits(MdioMode::MmdSingleWord, true) == 16); + REQUIRE(payload_width_bits(MdioMode::MmdMultiWord, false) == 16); + REQUIRE(payload_width_bits(MdioMode::MmdMultiWord, true) == 16); +} -TEST_CASE("validate_request rejects an out-of-range PHY address", "[mdio][REQ-MDIO-002]") { - MdioRequest req; - req.phy_address = kMaxPhyAddress + 1; - REQUIRE(validate_request(req) == make_error_code(MdioErrc::phy_address_out_of_range)); +TEST_CASE("payload_width_bits is 16 for MMS single word access", "[mdio][REQ-MDIO-002]") { + REQUIRE(payload_width_bits(MdioMode::MmsSingleWord, false) == 16); + REQUIRE(payload_width_bits(MdioMode::MmsSingleWord, true) == 16); } -TEST_CASE("validate_request rejects an out-of-range device/register address", "[mdio][REQ-MDIO-002]") { - MdioRequest req; - req.device_or_reg = kMaxDeviceOrRegField + 1; - REQUIRE(validate_request(req) == make_error_code(MdioErrc::device_or_reg_out_of_range)); +TEST_CASE("payload_width_bits is 32 for MMS multi-word access only on MMS0/MMS1", + "[mdio][REQ-MDIO-002]") { + REQUIRE(payload_width_bits(MdioMode::MmsMultiWord, true) == 32); + REQUIRE(payload_width_bits(MdioMode::MmsMultiWord, false) == 16); } -TEST_CASE("validate_request accepts every in-range field", "[mdio][REQ-MDIO-002]") { +TEST_CASE("validate_request accepts a payload within its mode's width", "[mdio][REQ-MDIO-002]") { MdioRequest req; - req.phy_address = kMaxPhyAddress; - req.device_or_reg = kMaxDeviceOrRegField; + req.mode = MdioMode::MmdSingleWord; + req.mdio_payload = 0xFFFF; + REQUIRE_FALSE(validate_request(req)); + + req.mode = MdioMode::MmsMultiWord; + req.mms_is_0_or_1 = true; + req.mdio_payload = 0xFFFFFFFFu; REQUIRE_FALSE(validate_request(req)); } -// ── Clause 22: flat register write-then-read round-trip ───────────────────── +TEST_CASE("validate_request rejects a payload exceeding its mode's width", "[mdio][REQ-MDIO-002]") { + MdioRequest req; + req.mode = MdioMode::MmdSingleWord; + req.mdio_payload = 0x10000; // exceeds 16 bits + REQUIRE(validate_request(req) == make_error_code(MdioErrc::payload_exceeds_mode_width)); + + MdioRequest req2; + req2.mode = MdioMode::MmsMultiWord; + req2.mms_is_0_or_1 = false; // 16-bit width for non-MMS0/1 + req2.mdio_payload = 0x10000; + REQUIRE(validate_request(req2) == make_error_code(MdioErrc::payload_exceeds_mode_width)); +} + +// ── Register write-then-read round-trip, keyed on (mode, mdio_address) ────── -TEST_CASE("MdioEndpoint::handle_request Clause22 write then read round-trips the value", +TEST_CASE("MdioEndpoint::handle_request write then read round-trips the value", "[mdio][REQ-MDIO-003]") { MdioEndpoint ep; MdioRequest write_req; - write_req.clause = MdioClause::Clause22; - write_req.phy_address = 3; - write_req.device_or_reg = 5; // Clause22 register address - write_req.is_write = true; - write_req.write_value = 0xBEEF; + write_req.mode = MdioMode::MmdSingleWord; + write_req.mdio_address = 5; + write_req.is_write = true; + write_req.mdio_payload = 0xBEEF; MdioResponse out; REQUIRE_FALSE(ep.handle_request(write_req, out)); - REQUIRE(out.value == 0xBEEF); + REQUIRE(out.mdio_payload == 0xBEEF); - MdioRequest read_req = write_req; - read_req.is_write = false; - read_req.write_value = 0; + MdioRequest read_req = write_req; + read_req.is_write = false; + read_req.mdio_payload = 0; REQUIRE_FALSE(ep.handle_request(read_req, out)); - REQUIRE(out.value == 0xBEEF); + REQUIRE(out.mdio_payload == 0xBEEF); } TEST_CASE("MdioEndpoint::handle_request reads an unwritten register as zero", "[mdio][REQ-MDIO-003]") { MdioEndpoint ep; MdioRequest req; - req.clause = MdioClause::Clause22; - req.phy_address = 1; - req.device_or_reg = 1; - req.is_write = false; + req.mode = MdioMode::MmsSingleWord; + req.mdio_address = 1; + req.is_write = false; MdioResponse out; REQUIRE_FALSE(ep.handle_request(req, out)); - REQUIRE(out.value == 0); + REQUIRE(out.mdio_payload == 0); } -// ── Clause 45: device+register addressing distinct from Clause 22 ─────────── +// ── mdio_mode keeps each mode's register space distinct ───────────────────── -TEST_CASE("MdioEndpoint::handle_request Clause45 keys on register_address in addition to " - "device_or_reg, distinct from Clause22's flat address space", +TEST_CASE("MdioEndpoint::handle_request keys on mdio_mode in addition to mdio_address, " + "so different modes at the same address never collide", "[mdio][REQ-MDIO-004]") { MdioEndpoint ep; - MdioRequest c22_write; - c22_write.clause = MdioClause::Clause22; - c22_write.phy_address = 2; - c22_write.device_or_reg = 4; - c22_write.is_write = true; - c22_write.write_value = 0x1111; + MdioRequest mmd_write; + mmd_write.mode = MdioMode::MmdSingleWord; + mmd_write.mdio_address = 4; + mmd_write.is_write = true; + mmd_write.mdio_payload = 0x1111; MdioResponse out; - REQUIRE_FALSE(ep.handle_request(c22_write, out)); - - // Same phy_address/device_or_reg bit pattern, but Clause45 with a - // distinct register_address — must not read back Clause22's value. - MdioRequest c45_read; - c45_read.clause = MdioClause::Clause45; - c45_read.phy_address = 2; - c45_read.device_or_reg = 4; - c45_read.register_address = 0x0010; - c45_read.is_write = false; - REQUIRE_FALSE(ep.handle_request(c45_read, out)); - REQUIRE(out.value == 0); - - MdioRequest c45_write = c45_read; - c45_write.is_write = true; - c45_write.write_value = 0x2222; - REQUIRE_FALSE(ep.handle_request(c45_write, out)); - REQUIRE(out.value == 0x2222); - - // Clause22's earlier write is unaffected. - MdioRequest c22_read = c22_write; - c22_read.is_write = false; - REQUIRE_FALSE(ep.handle_request(c22_read, out)); - REQUIRE(out.value == 0x1111); + REQUIRE_FALSE(ep.handle_request(mmd_write, out)); + + // Same mdio_address, but MmsSingleWord mode — must not read back the + // MMD write above. + MdioRequest mms_read; + mms_read.mode = MdioMode::MmsSingleWord; + mms_read.mdio_address = 4; + mms_read.is_write = false; + REQUIRE_FALSE(ep.handle_request(mms_read, out)); + REQUIRE(out.mdio_payload == 0); + + MdioRequest mms_write = mms_read; + mms_write.is_write = true; + mms_write.mdio_payload = 0x2222; + REQUIRE_FALSE(ep.handle_request(mms_write, out)); + REQUIRE(out.mdio_payload == 0x2222); + + // The earlier MMD write is unaffected. + MdioRequest mmd_read = mmd_write; + mmd_read.is_write = false; + REQUIRE_FALSE(ep.handle_request(mmd_read, out)); + REQUIRE(out.mdio_payload == 0x1111); } TEST_CASE("MdioEndpoint::handle_request fires TransferComplete on every request", "[mdio][REQ-MDIO-004]") { @@ -133,7 +153,7 @@ TEST_CASE("MdioEndpoint::handle_request fires TransferComplete on every request" // ── MdioErrc category sanity ─────────────────────────────────────────────────── TEST_CASE("MdioErrc reports a non-empty message in its own category", "[mdio][REQ-MDIO-005]") { - auto ec = make_error_code(MdioErrc::phy_address_out_of_range); + auto ec = make_error_code(MdioErrc::payload_exceeds_mode_width); REQUIRE(ec.category() == mdio_category()); REQUIRE_FALSE(ec.message().empty()); } diff --git a/tests/test_pwm.cpp b/tests/test_pwm.cpp index 119eb73..d8a23a9 100644 --- a/tests/test_pwm.cpp +++ b/tests/test_pwm.cpp @@ -11,6 +11,7 @@ // PWM_OUT, PWM_IN", v2.4.0). #include +#include #include using namespace rcp::pwm; @@ -24,10 +25,54 @@ TEST_CASE("PwmValue defaults both fields to zero", "[pwm][REQ-PWM-001]") { REQUIRE(v.active_duration == 0); } -// ── PWM_OUT: 8-way write-semantics reuse from GPIO ─────────────────────────── +// ── Wire codec: fixed 4-byte payload, period then active, big-endian ──────── +// Verified against the spec's "pwmo request format" figure (§13.7.5.3), +// which shows PWM_Period occupying the first two bytes of the payload and +// PWM_active the last two. -TEST_CASE("PwmOutEndpoint::handle_write applies Replace/Or/And/Xor independently to both fields", - "[pwm][REQ-PWM-002]") { +TEST_CASE("kPwmPayloadLen is exactly 4 bytes", "[pwm][REQ-PWM-001]") { + REQUIRE(kPwmPayloadLen == 4); + REQUIRE(sizeof(PwmWireBytes) == 4); +} + +TEST_CASE("encode_pwm_payload places period first, then active_duration, big-endian", + "[pwm][REQ-PWM-001]") { + PwmValue v; + v.period = 0x1234; + v.active_duration = 0x5678; + + auto wire = encode_pwm_payload(v); + REQUIRE(wire == PwmWireBytes{0x12, 0x34, 0x56, 0x78}); +} + +TEST_CASE("decode_pwm_payload round-trips encode_pwm_payload", "[pwm][REQ-PWM-001]") { + PwmValue in; + in.period = 60000; + in.active_duration = 12345; + + auto wire = encode_pwm_payload(in); + + PwmValue out; + auto ec = decode_pwm_payload(wire.data(), wire.size(), out); + REQUIRE_FALSE(ec); + REQUIRE(out.period == in.period); + REQUIRE(out.active_duration == in.active_duration); +} + +TEST_CASE("decode_pwm_payload reports short_buffer for fewer than 4 bytes", "[pwm][REQ-PWM-001]") { + uint8_t short_buf[3] = {0, 0, 0}; + PwmValue out; + auto ec = decode_pwm_payload(short_buf, sizeof(short_buf), out); + REQUIRE(ec == rcp::avtp::make_error_code(rcp::avtp::AvtpErrc::short_buffer)); +} + +// ── PWM_OUT: Replace applies directly, every other semantics is rejected ──── +// The spec's PWM_OUT request-handling section describes only direct +// application of the request's period/active values — no GPIO-style +// bitmask/saturating-write combination against the previous state — so +// handle_write no longer reuses rcp::endpoint::apply_bitmask_write. + +TEST_CASE("PwmOutEndpoint::handle_write applies Replace directly", "[pwm][REQ-PWM-002]") { PwmOutEndpoint ep; PwmValue out; @@ -35,22 +80,38 @@ TEST_CASE("PwmOutEndpoint::handle_write applies Replace/Or/And/Xor independently REQUIRE(out.period == 100); REQUIRE(out.active_duration == 40); - REQUIRE_FALSE(ep.handle_write(WriteSemantics::Or, {0x0F, 0x01}, out)); - REQUIRE(out.period == (100 | 0x0F)); - REQUIRE(out.active_duration == (40 | 0x01)); + // A second Replace fully overwrites the previous state (no combination + // against it — e.g. this is not an OR of 0x0F/0x01 against {100, 40}). + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Replace, {0x0F, 0x01}, out)); + REQUIRE(out.period == 0x0F); + REQUIRE(out.active_duration == 0x01); } -TEST_CASE("PwmOutEndpoint::handle_write applies saturating Add/Subtract to both fields", - "[pwm][REQ-PWM-003]") { +TEST_CASE("PwmOutEndpoint::handle_write applying a period of 0 is a normal Replace (stop request)", + "[pwm][REQ-PWM-002]") { PwmOutEndpoint ep; PwmValue out; - REQUIRE_FALSE(ep.handle_write(WriteSemantics::Replace, {0xFFFFFFF0, 5}, out)); + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Replace, {500, 250}, out)); + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Replace, {0, 0}, out)); + REQUIRE(out.period == 0); + REQUIRE(ep.read().period == 0); +} - REQUIRE_FALSE(ep.handle_write(WriteSemantics::Add, {0x20, 0}, out)); - REQUIRE(out.period == 0xFFFFFFFF); // saturates, does not wrap +TEST_CASE("PwmOutEndpoint::handle_write rejects Or/And/Xor/Add/Subtract as non-combinable", + "[pwm][REQ-PWM-003]") { + PwmOutEndpoint ep; + PwmValue out; + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Replace, {10, 5}, out)); - REQUIRE_FALSE(ep.handle_write(WriteSemantics::Subtract, {0, 10}, out)); - REQUIRE(out.active_duration == 0); // saturates at 0, does not wrap + for (auto op : {WriteSemantics::Or, WriteSemantics::And, WriteSemantics::Xor, + WriteSemantics::Add, WriteSemantics::Subtract}) { + auto ec = ep.handle_write(op, {999, 999}, out); + REQUIRE(ec == rcp::endpoint::make_error_code( + rcp::endpoint::EndpointErrc::non_combinable_write_semantics)); + // State is unchanged by a rejected write. + REQUIRE(ep.read().period == 10); + REQUIRE(ep.read().active_duration == 5); + } } TEST_CASE("PwmOutEndpoint::handle_write rejects Reserved without changing state", @@ -65,7 +126,7 @@ TEST_CASE("PwmOutEndpoint::handle_write rejects Reserved without changing state" REQUIRE(ep.read().active_duration == 5); } -TEST_CASE("PwmOutEndpoint::handle_write leaves Reconfigure to apply_bitmask_write's own rejection", +TEST_CASE("PwmOutEndpoint::handle_write rejects Reconfigure (PWM_OUT defines no Reconfigure target)", "[pwm][REQ-PWM-004]") { PwmOutEndpoint ep; PwmValue out; From 7a4fb6896586e69515e49eae433b56cd08f525d0 Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:21:02 -0700 Subject: [PATCH 2/2] fix: cast ADC test fixture literals to uint16_t to fix MSVC /WX narrowing error test_adc.cpp constructed std::optional from bare int/unsigned literals in several fixtures (added by the uint32_t->uint16_t ADC width fix). GCC/Clang accept the implicit narrowing conversion silently, but MSVC's warnings-as-errors build (C4244 -> C2220) fails on it. Wrap the literals in explicit uint16_t{...} so the conversion is exact everywhere. Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- tests/test_adc.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_adc.cpp b/tests/test_adc.cpp index 8ca5f86..0da317c 100644 --- a/tests/test_adc.cpp +++ b/tests/test_adc.cpp @@ -65,7 +65,7 @@ TEST_CASE("request_reading rejects a zero-valued averaging config field", "[adc] AdcEndpoint ep; AdcAveragingConfig cfg; cfg.adc_avg_intervals_per_request = 0; - auto take_sample = []() -> std::optional { return 1; }; + auto take_sample = []() -> std::optional { return uint16_t{1}; }; uint16_t out_value = 0; auto ec = ep.request_reading(cfg, take_sample, out_value); @@ -115,7 +115,7 @@ TEST_CASE("request_reading_from_trigger_queue implements the ExternalTrigger cad cfg.adc_avg_intervals_per_request = 2; cfg.adc_combine_avg_values = 2; - std::vector> queue{10u, 20u, 30u, 40u, 999u}; // one extra entry left over + std::vector> queue{uint16_t{10}, uint16_t{20}, uint16_t{30}, uint16_t{40}, uint16_t{999}}; // one extra entry left over uint16_t out_value = 0; auto ec = ep.request_reading_from_trigger_queue(cfg, queue, out_value); @@ -132,7 +132,7 @@ TEST_CASE("request_reading_from_trigger_queue reports no_signal on queue underru cfg.adc_avg_intervals_per_request = 2; cfg.adc_combine_avg_values = 2; - std::vector> queue{1u, 2u}; // fewer than the 4 needed + std::vector> queue{uint16_t{1}, uint16_t{2}}; // fewer than the 4 needed uint16_t out_value = 0; auto ec = ep.request_reading_from_trigger_queue(cfg, queue, out_value); REQUIRE(ec == make_error_code(AdcErrc::no_signal)); @@ -145,7 +145,7 @@ TEST_CASE("request_reading_from_trigger_queue reports no_signal on a missing-cap cfg.adc_avg_intervals_per_request = 2; cfg.adc_combine_avg_values = 1; - std::vector> queue{10u, std::nullopt}; // trigger occurred, no valid capture + std::vector> queue{uint16_t{10}, std::nullopt}; // trigger occurred, no valid capture uint16_t out_value = 0; auto ec = ep.request_reading_from_trigger_queue(cfg, queue, out_value); REQUIRE(ec == make_error_code(AdcErrc::no_signal));