Repository navigation
rewrite(phase3): port adc/gpio from c-RCP, fix a critical ADC averaging regression - #137
Merged
Merged
Conversation
…ng regression Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Third of Phase 3's Wave 1 batches, alongside iseled/i2c and can/lin (separate PRs). adc.hpp (55 requirements) was substantially wrong, not just incomplete: - Critical bug: the pre-rewrite pilot-era AdcEndpoint::request_reading averaged adc_combine_avg_values interval-means down to *one* output value. c-RCP's own file header records this exact mistake was already made and fixed once in c-RCP's history -- adc_combine_avg_values is a packing count, not a second reduction; a response carries N values, never one (Sec13.7.9.2/.3). Fixed: average_interval/ collect_response_values now port rcp_ep_adc_average_interval()/ rcp_ep_adc_collect_response_values() 1:1; AdcEndpoint redesigned around execute_measurement_cycle()/response_ready()/ collect_response(). - Field-naming bug: AdcAveragingConfig::adc_avg_intervals_per_request was actually being used for a layer-1 samples-per-interval count -- c-RCP's adc_samples_per_avg_interval's role, not layer 2's meaning. - Missing entirely, now ported: wall-clock inter-sample spacing validation (REQ-ADC-033), cadence classification (REQ-ADC-037/053), Table 53 threshold/measurement-finished triggers (REQ-ADC-031/ 048-052), the full EP_func register block + apply_reconfig (REQ-ADC-035/036/038-040, closing the evt[2:0]=111b gap the old header stubbed as config_write_not_supported), lifecycle-authorized functional-config setters, real ACF-level read-request/response/ reconfig wire codec. gpio.hpp (46 requirements) core was already correct; extended: - Bug found and fixed: apply_gpio_write's Subtract semantics computed current-operand; c-RCP's rcp_ep_gpio_apply_write() (REQ-GPIO-011) defines the row normatively as request minus current. Fixed locally in gpio.hpp, not in the shared endpoint.hpp helper, to avoid silently changing pwm.hpp's PWM_OUT semantics (out of this phase's scope, not yet re-verified). - Missing entirely, now ported: Table 43 wire trigger-signal numbering (REQ-GPIO-034), per-pin debounce filtering (REQ-GPIO-035/044), response-timing classification (REQ-GPIO-036), reserved evt[2:0]= 100b wire rejection (REQ-GPIO-012/045/046), the full EP_func register block + apply_reconfig (REQ-GPIO-013/038-042, with c-RCP's own resolved gpio_debounce_IO31 editorial-defect offset at 0x0028), functional-config setters, real ACF-level wire codec. - rcp_ep_gpio_toggle_pin_direction() (no longer a real TC18 mechanism per c-RCP's own comment) deliberately not ported -- low-value, non-wire caller convenience. Fixed-capacity: AdcEndpoint's pending-averaged-value accumulator is a std::array<AdcAvgValue, 255>-backed BoundedVector (255 = Table 54's own 8-bit adc_combine_avg_values register width). GPIO's existing std::array<uint8_t, kMaxPins> pattern extended to GpioFunctionalConfig::pins/debounce. test_adc.cpp: 18 -> 62 TEST_CASEs. test_gpio.cpp: 20 -> 62 TEST_CASEs. Call sites touched: mock.hpp's dispatch_adc() rewritten against the new AdcEndpoint API and to route evt[2:0]=111b through the now-real adc::apply_reconfig instead of rejecting it; wire_error_code_for() updated (removed config_write_not_supported, added reconfig_short/ reconfig_out_of_range mappings). tests/test_mock.cpp: two ADC config-write tests updated for the now-real reconfig behavior. Left as // TODO(phase3-followup): rcp_ep_gpio_toggle_pin_direction() not ported (noted in gpio.hpp's header, no stub added). Full dispatch-level wiring (response-shape composition, RxSequenceGuard, fragmentation) for both endpoints' new EP_func/reconfig paths remains mock.hpp's own Phase 4 scope -- this pass's minimal dispatch_adc wiring is enough to exercise apply_reconfig and keep the tree green, not a full Phase 4 implementation. Verified: full tree rebuilds clean (223/223 targets), 56/56 ctest suites pass (100%) -- independently re-verified from a fresh build, not just the porting agent's own report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
10 tasks
SoundMatt
added a commit
that referenced
this pull request
Aug 21, 2026
… operand order Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Third of Phase 3 Wave 2's three batches, alongside mdio and spi/uart (separate PRs). ## pwm.hpp - Fixed the same operand-order bug class found and fixed in gpio.hpp during PR #137: PWM_OUT's Subtract conditional-request handling used current - operand instead of c-RCP's normative request - current. This is deliberately fixed per-endpoint (not centralized into endpoint.hpp), matching the same caution documented in #137 that this bug needed independent re-verification at every endpoint using Subtract semantics, not an assumption that one fix covers all of them -- confirmed here to be a real, independent instance of the same class, not a duplicate report. - Ported against c-RCP's current RC5-conformant ep_pwm.h/.c. ## wakeup.hpp - Prior cpp-RCP wakeup.hpp content was a wholesale invention with no real relationship to c-RCP's actual ep_wakeup.h/.c -- re-derived from scratch against c-RCP's current implementation. - REQ-WAKEUP-020/026's regmap.c-scoped behavior intentionally NOT ported here (out of scope for the endpoint module itself; deferred to Phase 4's regmap port, noted in-file). ## Call sites None touched -- mock.hpp and powerstate.hpp (and their tests) confirmed compatible with the new pwm.hpp/wakeup.hpp APIs without modification. ## Verification Full tree rebuilds clean (223/223 targets), 56/56 ctest suites pass (100%) -- independently re-verified from a fresh build in the worktree, not just the porting agent's own report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
SoundMatt
added a commit
that referenced
this pull request
Aug 21, 2026
… operand order (#140) Part of the v3.0.0 ground-up rewrite (cpp-RCP #129, ROADMAP.md Phase 17), branch rewrite/v3-from-c-rcp. Third of Phase 3 Wave 2's three batches, alongside mdio and spi/uart (separate PRs). ## pwm.hpp - Fixed the same operand-order bug class found and fixed in gpio.hpp during PR #137: PWM_OUT's Subtract conditional-request handling used current - operand instead of c-RCP's normative request - current. This is deliberately fixed per-endpoint (not centralized into endpoint.hpp), matching the same caution documented in #137 that this bug needed independent re-verification at every endpoint using Subtract semantics, not an assumption that one fix covers all of them -- confirmed here to be a real, independent instance of the same class, not a duplicate report. - Ported against c-RCP's current RC5-conformant ep_pwm.h/.c. ## wakeup.hpp - Prior cpp-RCP wakeup.hpp content was a wholesale invention with no real relationship to c-RCP's actual ep_wakeup.h/.c -- re-derived from scratch against c-RCP's current implementation. - REQ-WAKEUP-020/026's regmap.c-scoped behavior intentionally NOT ported here (out of scope for the endpoint module itself; deferred to Phase 4's regmap port, noted in-file). ## Call sites None touched -- mock.hpp and powerstate.hpp (and their tests) confirmed compatible with the new pwm.hpp/wakeup.hpp APIs without modification. ## Verification Full tree rebuilds clean (223/223 targets), 56/56 ctest suites pass (100%) -- independently re-verified from a fresh build in the worktree, not just the porting agent's own report. Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the v3.0.0 ground-up rewrite (#129, ROADMAP.md Phase 17). Third of Phase 3's Wave 1 batches, alongside
iseled/i2candcan/lin(separate PRs).adc.hpp(55 requirements) — was substantially wrong, not just incompleteCritical bug: the pre-rewrite pilot-era code averaged
adc_combine_avg_valuesinterval-means down to one output value. c-RCP's own file header records this exact mistake was already made and fixed once in c-RCP's own history —adc_combine_avg_valuesis a packing count, not a second reduction; a response carries N values, never one. Fixed against c-RCP's currentep_adc.c1:1. Also a field-naming bug (a config field was doing double duty for two distinct layer-1/layer-2 meanings). Missing entirely, now ported: wall-clock sample-spacing validation, cadence classification, Table 53 threshold triggers, the full EP_func register block +apply_reconfig(closing an evt[2:0]=111b gap the old header just stubbed out), real ACF-level wire codec.gpio.hpp(46 requirements) — core was correct, extendedBug found and fixed:
apply_gpio_write'sSubtractsemantics computed the operands in the wrong order — c-RCP defines the row normatively as request-minus-current, not current-minus-request. Fixed locally ingpio.hpp(not in the sharedendpoint.hpphelper, to avoid silently changingpwm.hpp's semantics before that module gets its own Phase 3 pass). Missing entirely, now ported: Table 43 trigger-signal numbering, per-pin debounce filtering, response-timing classification, the full EP_func register block +apply_reconfig.test_adc.cpp: 18→62 cases.test_gpio.cpp: 20→62 cases.Call sites touched
mock.hpp'sdispatch_adc()rewritten against the new API, now routes reconfig through the realapply_reconfiginstead of rejecting it; 2test_mock.cppcases updated to match.Verified
Full tree rebuilds clean (223/223 targets), 56/56 ctest suites pass (100%) — independently re-verified from a fresh build.
Tracked at #129 — completes Phase 3 Wave 1.