From d1a612ede0216a546d5feb211991818fba64f66b Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Sat, 1 Aug 2026 21:04:03 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20v2.24.0=20=E2=80=94=20PWM=5FOUT=20write?= =?UTF-8?q?=20semantics=20+=20GPIO=20input-pin=20masking=20(BREAKING)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PWM_OUT (issue #104): a prior fix (issue #70) had narrowed handle_write to Replace only, reasoning that PWM_OUT's own request-handling section describes no write combination. That missed that §13.5 Table 30 — the table governing evt[2:0] across every endpoint type — explicitly assigns PWM_OUT the same eight write semantics as GPIO, including Add/Subtract, whose own worked examples in that row name PWM_out's duty cycle directly. Restored Or/And/Xor/Add/Subtract via a dedicated 16-bit-width combinator (not GPIO's uint32_t apply_bitmask_write directly, which would saturate Add/Subtract at the wrong width and silently wrap instead of clamping). GPIO (issue #105): TC18 §13.7.4.3 states a write to an input-configured pin is ignored for that pin. apply_gpio_write never consulted GpioState::directions — every write semantics, including a bare Replace, overwrote every addressed bit regardless of direction, so a request could silently corrupt an input pin's externally-driven value. Now masked: the combinator's result is committed only for output-configured bits. CAN format (flagged, investigated): no live code path found. rcp/can.hpp is validation-only (identifier ranges, frame-format ceilings, XL filtering) with no ACF wire codec anywhere in this codebase for CAN — the existing requirements accurately describe that scope. Left as a known, documented gap (a feature build-out, not a bug fix), not silently closed. Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- .fusa-reqs.json | 28 ++++++++---- CMakeLists.txt | 2 +- ROADMAP.md | 52 ++++++++++++++++++++++ include/rcp/gpio.hpp | 14 +++++- include/rcp/pwm.hpp | 95 +++++++++++++++++++++++++++++------------ include/rcp/version.hpp | 2 +- tests/test_capi.cpp | 17 +++++++- tests/test_gpio.cpp | 53 ++++++++++++++++++++++- tests/test_mock.cpp | 10 +++++ tests/test_pwm.cpp | 54 ++++++++++++++++------- 10 files changed, 270 insertions(+), 57 deletions(-) diff --git a/.fusa-reqs.json b/.fusa-reqs.json index 0a1be78..820913b 100644 --- a/.fusa-reqs.json +++ b/.fusa-reqs.json @@ -2368,6 +2368,15 @@ "level": "ASIL-B", "asil": "ASIL-B" }, + { + "id": "REQ-GPIO-009", + "title": "A write to a pin configured as input leaves that pin's value unchanged", + "text": "rcp::gpio::apply_gpio_write shall, for every non-Reconfigure write semantics, commit the write combinator's result to GpioState::values only for bit positions where GpioState::directions selects output, and shall leave GpioState::values unchanged at every bit position where directions selects input, regardless of what the request's operand or combinator result specify for that position.", + "standard": "iso26262", + "level": "ASIL-B", + "asil": "ASIL-B", + "tc18": "§13.7.4.3 (\"A write request to an input pin is ignored for this input pin\")" + }, { "id": "REQ-SPI-001", "title": "SPI channel selection via evt[2:0]", @@ -2563,18 +2572,20 @@ { "id": "REQ-PWM-002", "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.", + "text": "rcp::pwm::PwmOutEndpoint::handle_write shall apply WriteSemantics::Replace by directly assigning `period` and `active_duration` from the request, matching one of the eight write semantics the OPEN Alliance TC18 Remote Control Protocol Specification's §13.5 Table 30 GPIO/PWM_OUT row defines.", "standard": "iso26262", "level": "ASIL-B", - "asil": "ASIL-B" + "asil": "ASIL-B", + "tc18": "§13.5 Table 30 (GPIO/PWM_OUT row, evt[2:0]=000b)" }, { "id": "REQ-PWM-003", - "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.", + "title": "PWM_OUT write semantics: Or/And/Xor/Add/Subtract combine against the previous state, per field", + "text": "rcp::pwm::PwmOutEndpoint::handle_write shall apply WriteSemantics::Or/::And/::Xor/::Add/::Subtract independently against `period` and `active_duration`, each within its own uint16_t range (Add/Subtract saturating at 0x0000/0xFFFF rather than wrapping, per §13.5 Table 30's saturation note and independently of GPIO's own 32-bit saturation bound for the same write semantics).", "standard": "iso26262", "level": "ASIL-B", - "asil": "ASIL-B" + "asil": "ASIL-B", + "tc18": "§13.5 Table 30 (GPIO/PWM_OUT row, evt[2:0]=001b-011b,101b-110b), TC18.txt L3694-3714" }, { "id": "REQ-PWM-004", @@ -2594,11 +2605,12 @@ }, { "id": "REQ-PWM-006", - "title": "PWM_IN mid-pulse trigger signal", - "text": "rcp::pwm::PwmInEndpoint::record_measurement shall notify the MidPulse trigger signal on every call, observable via drain() only when MidPulse was armed via triggers().enable().", + "title": "PWM_IN rising/falling edge trigger signals (Table 44)", + "text": "rcp::pwm::PwmInEndpoint::record_measurement shall notify both the RisingEdge and FallingEdge trigger signals on every call (one full input-capture cycle spans both edges); record_edge shall notify exactly the one signal it is given, without the other, matching TC18 Table 44's two independent trigger outputs rather than an invented single combined signal.", "standard": "iso26262", "level": "ASIL-B", - "asil": "ASIL-B" + "asil": "ASIL-B", + "tc18": "§13.7.5.1 Table 44 (pwmi trigger outputs)" }, { "id": "REQ-PWM-007", diff --git a/CMakeLists.txt b/CMakeLists.txt index 12a8c7f..ba43dc2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,7 +1,7 @@ cmake_minimum_required(VERSION 3.21) project(cpp-rcp - VERSION 2.23.0 + VERSION 2.24.0 DESCRIPTION "Remote Control Protocol for automotive zonal architecture (C++)" HOMEPAGE_URL "https://github.com/SoundMatt/cpp-RCP" LANGUAGES CXX diff --git a/ROADMAP.md b/ROADMAP.md index a295f1c..97afdf2 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2334,6 +2334,58 @@ 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. +### Post-hoc write-semantics fix pass (v2.24.0, issues #104, #105; BREAKING) + +A requirements-extraction pass flagged three items for follow-up: PWM_OUT's +write semantics, GPIO's input-pin write handling, and CAN's evt/format +usage. Investigating each against the OPEN Alliance TC18 Remote Control +Protocol Specification directly: + +- `rcp/pwm.hpp` (issue #104): the "post-hoc wire-format fix pass" above + (issue #70) had narrowed `PwmOutEndpoint::handle_write` to + `WriteSemantics::Replace` only, reasoning that PWM_OUT's own + request-handling section (§13.7.5.3) describes no combination against + the endpoint's previous state. That reasoning missed that §13.7.5.3 + doesn't need to redescribe write semantics: §13.5 Table 30 ("EP specific + usage of evt-field") is the table that governs evt[2:0] across every + endpoint type, and its GPIO/PWM_OUT row explicitly assigns PWM_OUT the + *same* eight write semantics as GPIO — including Add/Subtract, whose own + worked examples in that row name PWM_out's duty cycle directly. This is + a real regression, not a design choice: `rcp/endpoint.hpp`'s own + `saturating_add`/`saturating_subtract` were built templated on the + caller's unsigned width specifically so GPIO's 32-bit pin mask and + PWM_OUT's narrower period/duration fields could share one + implementation, per the v2.4.0 roadmap note issue #70's fix + contradicted without updating. `handle_write` now applies every + semantics via a dedicated 16-bit-width combinator (not + `apply_bitmask_write` directly — that function is hardcoded to + `uint32_t`, and calling it against a `uint16_t` field then narrowing the + result would saturate at the wrong width, silently wrapping instead of + clamping at 0xFFFF), independently per field. +- `rcp/gpio.hpp` (issue #105): TC18 §13.7.4.3 states that a write request + to a pin currently configured as an input is ignored for that pin — + inputs are driven externally. `apply_gpio_write` never consulted + `GpioState::directions` before this fix: every write semantics, + including a bare Replace, overwrote every addressed bit regardless of + its configured direction, so a request could silently corrupt an input + pin's externally-driven value. The write combinator's result is now + committed only for bit positions where `directions` selects output, + preserving `values` unchanged wherever `directions` selects input. +- CAN format: investigated and found to have no live code path. This + module (`rcp/can.hpp`) is validation-only — identifier range checks, + frame-format payload ceilings, XL acceptance filtering — with no + ACF-level wire encode/decode for CAN requests/responses anywhere in this + codebase. The `.fusa-reqs.json` entries for CAN accurately describe this + validation-only scope; there is no evt[2:0]-carries-format defect to fix + because there is no wire codec to have it. A full CAN ACF wire codec is + a separate, much larger feature gap than a bug fix and is out of scope + here — left as a known gap, not silently resolved. + +Both real fixes are behavior changes for any caller relying on the +previous (incorrect) behavior — BREAKING. `rcp/pwm.hpp`/`rcp/gpio.hpp` +are the only files touched; every other endpoint header, `rcp/endpoint.hpp` +itself, and the shared AVTPDU/ACF framing are unmodified. + --- ### Appendix A — Legacy Roadmap (v0.1.0–v1.11, superseded) --- diff --git a/include/rcp/gpio.hpp b/include/rcp/gpio.hpp index 65015f4..b73d0cf 100644 --- a/include/rcp/gpio.hpp +++ b/include/rcp/gpio.hpp @@ -6,6 +6,7 @@ // fusa:req REQ-GPIO-006 // fusa:req REQ-GPIO-007 // fusa:req REQ-GPIO-008 +// fusa:req REQ-GPIO-009 // GPIO endpoint (ep_type 0x02) — the OPEN Alliance TC18 Remote Control // Protocol Specification v0.5.1_RC's simplest endpoint type: a 32-pin @@ -94,6 +95,17 @@ inline std::error_code make_error_code(GpioErrc e) noexcept { // Reconfigure by treating `operand` as the *new pin-direction mask* // (replacing, not combining, `state.directions`) rather than a value // combined with the pin-level state (extraction §5.3, §4.5 Group C). +// +// Input-pin write masking (issue #105, cpp-RCP-15): TC18 §13.7.4.3 states +// that a write request to a pin currently configured as an input is ignored +// for that pin — inputs are driven externally, and a write combinator's +// result for those bit positions must not be committed to state.values. +// This applied uniformly and without exception before this fix: every write +// semantics (including a bare Replace) overwrote every addressed bit +// regardless of state.directions, so a request could silently corrupt an +// input pin's externally-driven value. The masking below commits the +// combinator's result only for bits where directions == output (1), +// preserving state.values unchanged wherever directions == input (0). inline std::error_code apply_gpio_write(endpoint::WriteSemantics op, GpioState& state, PinMask operand) noexcept { if (op == endpoint::WriteSemantics::Reconfigure) { @@ -103,7 +115,7 @@ inline std::error_code apply_gpio_write(endpoint::WriteSemantics op, GpioState& uint32_t out = 0; auto ec = endpoint::apply_bitmask_write(op, state.values, operand, out); if (ec) return ec; - state.values = out; + state.values = (out & state.directions) | (state.values & ~state.directions); return {}; } diff --git a/include/rcp/pwm.hpp b/include/rcp/pwm.hpp index a60c816..3dc0dd7 100644 --- a/include/rcp/pwm.hpp +++ b/include/rcp/pwm.hpp @@ -34,23 +34,29 @@ // 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. +// PWM_OUT write-semantics correction (issue #104, cpp-RCP-14): issue #70 +// had narrowed PwmOutEndpoint::handle_write to WriteSemantics::Replace only, +// on the reasoning that the PWM_OUT-specific request-handling section +// (§13.7.5.3) describes only one write behavior. That reasoning missed that +// §13.7.5.3 doesn't need to redescribe write semantics at all: §13.5 Table +// 30 ("EP specific usage of evt-field") is the governing table for +// evt[2:0]'s meaning across every endpoint type, and its GPIO/PWM_OUT row +// explicitly assigns PWM_OUT the *same* eight write semantics as GPIO — +// including Add/Subtract, whose own worked examples in that row name +// PWM_out's duty cycle directly ("this can be used to increase/decrease the +// duty cycle of PWM_out"). rcp::endpoint::saturating_add/saturating_subtract +// (below, in endpoint.hpp) were built templated on the caller's unsigned +// width specifically so GPIO's 32-bit pin mask and PWM_OUT's narrower +// period/duration fields could share one implementation — v2.4.0's own +// roadmap note this file's header used to cite — which issue #70 then +// contradicted without updating. PwmOutEndpoint::handle_write below now +// applies every non-Reconfigure write semantics via +// rcp::endpoint::apply_bitmask_write, per field (period and +// active_duration independently, each saturating within its own uint16_t +// range per Table 30's saturation note). Reconfigure remains rejected: this +// endpoint type has no EP_func addressed-write path implemented yet (a +// separate, larger gap common to every endpoint but one in this codebase, +// not specific to PWM_OUT's write-semantics bug this fixes). // // Field names and behavior below implement TC18's *behavior* as described in // an internal structured extraction of the specification named above; no @@ -137,22 +143,55 @@ inline std::error_code make_error_code(PwmErrc e) noexcept { return {static_cast(e), pwm_category()}; } +// apply_write_field is PWM_OUT's own field-width combinator: it mirrors +// rcp::endpoint::apply_bitmask_write's case set exactly, but instantiates +// saturating_add/saturating_subtract at uint16_t rather than uint32_t — +// apply_bitmask_write itself is hardcoded to uint32_t (see its own +// declaration in endpoint.hpp), so calling it directly against a uint16_t +// field and narrowing the uint32_t result back down would compute the +// Add/Subtract saturation bound at the wrong width (saturating at +// 0xFFFFFFFF, then truncating — silently wrapping instead of the 0xFFFF +// saturation Table 30's own note requires for a 16-bit field). +inline std::error_code apply_write_field(endpoint::WriteSemantics op, uint16_t current, + uint16_t operand, uint16_t& out) noexcept { + switch (op) { + case endpoint::WriteSemantics::Replace: out = operand; return {}; + case endpoint::WriteSemantics::Or: out = static_cast(current | operand); return {}; + case endpoint::WriteSemantics::And: out = static_cast(current & operand); return {}; + case endpoint::WriteSemantics::Xor: out = static_cast(current ^ operand); return {}; + case endpoint::WriteSemantics::Add: out = endpoint::saturating_add(current, operand); return {}; + case endpoint::WriteSemantics::Subtract: out = endpoint::saturating_subtract(current, operand); return {}; + case endpoint::WriteSemantics::Reserved: + return endpoint::make_error_code(endpoint::EndpointErrc::reserved_write_semantics); + case endpoint::WriteSemantics::Reconfigure: + default: + return endpoint::make_error_code(endpoint::EndpointErrc::non_combinable_write_semantics); + } +} + // ── PwmOutEndpoint (ep_type 0x07) ──────────────────────────────────────────── -// 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. +// handle_write applies every write semantics Table 30's GPIO/PWM_OUT row +// defines — Replace/Or/And/Xor/Add/Subtract — via apply_write_field above, +// independently against `period` and `active_duration` (each its own +// uint16_t, so each combines and saturates within its own 16-bit range, per +// Table 30's saturation note). Reconfigure is rejected: PWM_OUT has no +// EP_func addressed-write path in this codebase yet (see the header comment +// above), so there is nothing for it to target. class PwmOutEndpoint { public: std::error_code handle_write(endpoint::WriteSemantics op, PwmValue operand, PwmValue& out_value) noexcept { - if (op != endpoint::WriteSemantics::Replace) { - return endpoint::make_error_code(endpoint::EndpointErrc::non_combinable_write_semantics); - } - state_ = operand; - out_value = state_; + uint16_t new_period = 0; + auto ec = apply_write_field(op, state_.period, operand.period, new_period); + if (ec) return ec; + + uint16_t new_active = 0; + ec = apply_write_field(op, state_.active_duration, operand.active_duration, new_active); + if (ec) return ec; + + state_.period = new_period; + state_.active_duration = new_active; + out_value = state_; return {}; } diff --git a/include/rcp/version.hpp b/include/rcp/version.hpp index cb0fb8d..270c8cd 100644 --- a/include/rcp/version.hpp +++ b/include/rcp/version.hpp @@ -6,6 +6,6 @@ namespace rcp { // Semantic version of the cpp-RCP implementation (matches the latest git tag). -constexpr std::string_view kVersion = "2.23.0"; +constexpr std::string_view kVersion = "2.24.0"; } // namespace rcp diff --git a/tests/test_capi.cpp b/tests/test_capi.cpp index 57dd02a..50df3e9 100644 --- a/tests/test_capi.cpp +++ b/tests/test_capi.cpp @@ -257,9 +257,24 @@ TEST_CASE("capi: rcp_send against a real rcp::mock::Server GPIO endpoint", "[cap REQUIRE(rcp_ctrl_init(0, rcp::mock::kGpioByteBusId, mock_server_request_fn, &srv, buf, sizeof(buf), &ctrl) == RCP_OK); + uint8_t out[8] = {}; + + // Pin 0 must be configured as output before a write to it can take + // effect (REQ-GPIO-009) — every pin defaults to input. + uint8_t reconfig_payload[4] = {0x00, 0x00, 0x00, 0x01}; + rcp_request_t reconfig_req{}; + reconfig_req.info.byte_bus_id = rcp::mock::kGpioByteBusId; + reconfig_req.info.op = 1; // write + reconfig_req.info.evt_op = 7; // WriteSemantics::Reconfigure + reconfig_req.payload = reconfig_payload; + reconfig_req.payload_len = sizeof(reconfig_payload); + rcp_response_t reconfig_resp{}; + reconfig_resp.payload = out; + reconfig_resp.payload_cap = sizeof(out); + REQUIRE(rcp_send(ctrl, &reconfig_req, &reconfig_resp, 0) == RCP_OK); + // Write pins 0x00000001 (big-endian 4-byte GPIO payload), then read back. uint8_t write_payload[4] = {0x00, 0x00, 0x00, 0x01}; - uint8_t out[8] = {}; rcp_request_t req{}; req.info.byte_bus_id = rcp::mock::kGpioByteBusId; diff --git a/tests/test_gpio.cpp b/tests/test_gpio.cpp index 28f792f..30990a9 100644 --- a/tests/test_gpio.cpp +++ b/tests/test_gpio.cpp @@ -6,6 +6,7 @@ // fusa:test REQ-GPIO-006 // fusa:test REQ-GPIO-007 // fusa:test REQ-GPIO-008 +// fusa:test REQ-GPIO-009 // Tests for rcp/gpio.hpp — the GPIO endpoint type (ROADMAP.md milestone 47, // "Basic Endpoint Types I — GPIO & SPI", v2.3.0). @@ -54,7 +55,8 @@ TEST_CASE("decode_gpio_payload rejects an over-long buffer (spec requires exactl TEST_CASE("apply_gpio_write applies Replace/Or/And/Xor to state.values", "[gpio][REQ-GPIO-002]") { GpioState state; - state.values = 0x0000FFFF; + state.directions = 0xFFFFFFFF; // every pin output, so masking is a no-op here + state.values = 0x0000FFFF; REQUIRE_FALSE(apply_gpio_write(WriteSemantics::Or, state, 0xFFFF0000)); REQUIRE(state.values == 0xFFFFFFFF); @@ -73,7 +75,8 @@ TEST_CASE("apply_gpio_write applies Replace/Or/And/Xor to state.values", "[gpio] TEST_CASE("apply_gpio_write applies saturating Add/Subtract to state.values", "[gpio][REQ-GPIO-003]") { GpioState state; - state.values = 0xFFFFFFF0; + state.directions = 0xFFFFFFFF; // every pin output, so masking is a no-op here + state.values = 0xFFFFFFF0; REQUIRE_FALSE(apply_gpio_write(WriteSemantics::Add, state, 0x20)); REQUIRE(state.values == 0xFFFFFFFF); // saturates, does not wrap past max @@ -104,6 +107,48 @@ TEST_CASE("apply_gpio_write's Reconfigure replaces state.directions, not state.v REQUIRE(state.values == 0xAAAAAAAA); // unaffected by a Reconfigure write } +// ── Write masking against input-configured pins (TC18 §13.7.4.3) ──────────── + +TEST_CASE("apply_gpio_write's Replace does not modify input-configured pins", + "[gpio][REQ-GPIO-009]") { + GpioState state; + state.directions = 0x0000000F; // pins 0-3 output, pins 4-31 input + state.values = 0xABCD0005; // pin 4 (input) currently reads 1 + + REQUIRE_FALSE(apply_gpio_write(WriteSemantics::Replace, state, 0xFFFFFFF0)); + // Output pins 0-3 take the request's bits (0x0); every input pin (4-31) + // keeps its prior value (0xABCD0000) untouched, including bit 4. + REQUIRE(state.values == 0xABCD0000); +} + +TEST_CASE("apply_gpio_write's Or/And/Xor only affect output-configured pins", + "[gpio][REQ-GPIO-009]") { + GpioState state; + state.directions = 0x000000FF; // pins 0-7 output, rest input + state.values = 0x00000F0F; // output byte = 0x0F, input bits = 0x000000_0 + + REQUIRE_FALSE(apply_gpio_write(WriteSemantics::Or, state, 0xFFFFFFF0)); + // Combinator computes 0x0FFF (0x0F0F | 0xFFF0) but only the low 8 bits + // (output) may actually change; bits 8-31 (input) stay exactly as before. + REQUIRE(state.values == 0x00000FFF); + + state.values = 0x0000FFFF; // output byte = 0xFF, input bits = 0x0000FF00 + REQUIRE_FALSE(apply_gpio_write(WriteSemantics::And, state, 0x00000000)); + REQUIRE(state.values == 0x0000FF00); // output byte actually cleared, input untouched +} + +TEST_CASE("apply_gpio_write's Add/Subtract saturation still respects input masking", + "[gpio][REQ-GPIO-009]") { + GpioState state; + state.directions = 0x0000FFFF; // low 16 bits output, high 16 bits input + state.values = 0xBEEF0000; + + REQUIRE_FALSE(apply_gpio_write(WriteSemantics::Add, state, 0xFFFFFFFF)); + // The 32-bit saturating add itself saturates to 0xFFFFFFFF, but masking + // still confines the committed change to the output half. + REQUIRE(state.values == 0xBEEFFFFF); +} + // ── Per-pin change/rising/falling trigger signals ──────────────────────────── TEST_CASE("evaluate_gpio_triggers fires Change+Rising for a 0->1 transition when both are armed", @@ -185,7 +230,11 @@ TEST_CASE("GpioEndpoint::handle_write updates state and fires triggers in one ca ep.triggers().enable(gpio_signal_id(0, GpioEdge::Change)); ep.triggers().enable(gpio_signal_id(0, GpioEdge::Rising)); + // Pin 0 must be configured as output before a write to it can take + // effect (REQ-GPIO-009) — GpioState defaults every pin to input. PinMask out_value = 0; + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Reconfigure, /*operand=*/0x1, out_value)); + auto ec = ep.handle_write(WriteSemantics::Or, /*operand=*/0x1, out_value); REQUIRE_FALSE(ec); REQUIRE(out_value == 0x1); diff --git a/tests/test_mock.cpp b/tests/test_mock.cpp index 5c8e71f..c6d4699 100644 --- a/tests/test_mock.cpp +++ b/tests/test_mock.cpp @@ -126,6 +126,16 @@ TEST_CASE("GPIO write applies evt[2:0] semantics and read reflects the new state mock::Server server; REQUIRE_FALSE(server.advance_to_rcp_configured()); + // Pins 0-3 must be configured as output before a write to them can take + // effect (REQ-GPIO-009) — every pin defaults to input. + auto reconfig_req = standard_request(mock::kGpioByteBusId, /*write=*/true, + static_cast(WriteSemantics::Reconfigure)); + auto reconfig_payload = gpio::encode_gpio_payload(0x0000'000F); + acf::AcfMessageInfo reconfig_resp; + std::vector reconfig_resp_payload; + REQUIRE_FALSE( + server.dispatch(0, reconfig_req, reconfig_payload, reconfig_resp, reconfig_resp_payload)); + auto write_req = standard_request(mock::kGpioByteBusId, /*write=*/true, static_cast(WriteSemantics::Or)); auto payload = gpio::encode_gpio_payload(0x0000'000F); diff --git a/tests/test_pwm.cpp b/tests/test_pwm.cpp index 12c4e69..91b19b6 100644 --- a/tests/test_pwm.cpp +++ b/tests/test_pwm.cpp @@ -77,10 +77,12 @@ TEST_CASE("decode_pwm_payload rejects an over-long buffer (spec requires exactly } // ── 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. +// TC18 §13.5 Table 30's GPIO/PWM_OUT row assigns PWM_OUT the same eight +// write semantics as GPIO — Replace/Or/And/Xor/Add/Subtract combine against +// the endpoint's current period/active_duration, each independently and +// each saturating within its own uint16_t range (Table 30's own saturation +// note); Reserved is rejected; Reconfigure has no target in this codebase +// yet (no EP_func addressed-write path exists for any endpoint but one). TEST_CASE("PwmOutEndpoint::handle_write applies Replace directly", "[pwm][REQ-PWM-002]") { PwmOutEndpoint ep; @@ -107,21 +109,43 @@ TEST_CASE("PwmOutEndpoint::handle_write applying a period of 0 is a normal Repla REQUIRE(ep.read().period == 0); } -TEST_CASE("PwmOutEndpoint::handle_write rejects Or/And/Xor/Add/Subtract as non-combinable", +TEST_CASE("PwmOutEndpoint::handle_write applies Or/And/Xor against the previous state, per field", "[pwm][REQ-PWM-003]") { PwmOutEndpoint ep; PwmValue out; - REQUIRE_FALSE(ep.handle_write(WriteSemantics::Replace, {10, 5}, out)); + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Replace, {0x0F00, 0x00F0}, out)); + + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Or, {0x00F0, 0x000F}, out)); + REQUIRE(out.period == 0x0FF0); + REQUIRE(out.active_duration == 0x00FF); + + REQUIRE_FALSE(ep.handle_write(WriteSemantics::And, {0x0FF0, 0x00FF}, out)); + REQUIRE(out.period == 0x0FF0); + REQUIRE(out.active_duration == 0x00FF); - 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); - } + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Xor, {0x00F0, 0x00FF}, out)); + REQUIRE(out.period == 0x0F00); + REQUIRE(out.active_duration == 0x0000); +} + +TEST_CASE("PwmOutEndpoint::handle_write applies saturating Add/Subtract per field, " + "independently of GPIO's 32-bit saturation bound", + "[pwm][REQ-PWM-003]") { + PwmOutEndpoint ep; + PwmValue out; + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Replace, {0xFFF0, 10}, out)); + + // Saturates at 0xFFFF (16-bit), not 0xFFFFFFFF (32-bit, GPIO's own + // bound) — the exact bug class this test guards against: applying + // GPIO's 32-bit combinator directly to a 16-bit field and truncating + // the result would wrap instead of saturate. + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Add, {0x0020, 5}, out)); + REQUIRE(out.period == 0xFFFF); + REQUIRE(out.active_duration == 15); + + REQUIRE_FALSE(ep.handle_write(WriteSemantics::Subtract, {0xFFFF, 20}, out)); + REQUIRE(out.period == 0); + REQUIRE(out.active_duration == 0); // saturates at 0, does not wrap } TEST_CASE("PwmOutEndpoint::handle_write rejects Reserved without changing state",