Repository navigation
fix: v2.24.0 — PWM_OUT write semantics + GPIO input-pin masking (BREAKING) - #104
Merged
Merged
Conversation
…KING) 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PwmOutEndpoint::handle_writetoWriteSemantics::Replaceonly, reasoning that PWM_OUT's own request-handling section (§13.7.5.3) describes no write combination. That missed that §13.5 Table 30 — the table that governsevt[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 ("this can be used to increase/decrease the duty cycle of PWM_out"). This was a real regression:rcp/endpoint.hpp's ownsaturating_add/saturating_subtractwere 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 a surviving v2.4.0 roadmap comment issue [cpp-RCP-01] PWM_OUT payload fields are 32-bit instead of the required 16-bit (field order is correct) #70's fix then contradicted without updating. Restored Or/And/Xor/Add/Subtract via a dedicated 16-bit-width combinator — not GPIO'suint32_t apply_bitmask_writedirectly, which would saturate at the wrong width and silently wrap instead of clamping at 0xFFFF.apply_gpio_writenever consultedGpioState::directionsbefore this fix — every write semantics, including a bare Replace, overwrote every addressed bit regardless of configured direction, so a request could silently corrupt an input pin's externally-driven value. The combinator's result is now committed only for output-configured bits; input-configured bits are left unchanged.rcp/can.hppis validation-only (identifier range checks, frame-format payload ceilings, XL acceptance filtering) — there is no ACF-level wire encode/decode for CAN anywhere in this codebase, so there's noevt[2:0]-carries-format defect to fix. The existing.fusa-reqs.jsonentries accurately describe this validation-only scope. Left as a known, documented gap (a feature build-out, not a bug), not silently closed..fusa-reqs.jsonentries found along the way:REQ-PWM-002/003described the (buggy) Replace-only behavior, andREQ-PWM-006still described an invented "MidPulse" trigger signal superseded by a real Table-44 fix (RisingEdge/FallingEdge) whose code+tests were already correct.Test plan
test_gpio.cppx2,test_mock.cpp,test_capi.cpp) by explicitly configuring pin direction via Reconfigure before writing, rather than weakening the fixctest: 53/53 passedctest: 53/53 passed cleancpfusa check(fresh v0.18.0 build matching CI's exact pin): 0 errorscpfusa trace --req-coverage 100 --sec-tested 100: 376/376 requirements traced and tested