Skip to content

feat(serial): BoardFamily reset_method + dispatch_reset (#687) - #703

Merged
zackees merged 2 commits into
mainfrom
feat/687-board-family-reset-method
Jun 20, 2026
Merged

feat(serial): BoardFamily reset_method + dispatch_reset (#687)#703
zackees merged 2 commits into
mainfrom
feat/687-board-family-reset-method

Conversation

@zackees

@zackeeszackees commented Jun 20, 2026

Copy link
Copy Markdown
Member

Summary

#687 — polymorphic reset dispatch. Until now reset was implicit: deploy paths called esp_hard_reset_blocking directly, LPC callers had to do their own SWD-via-CMSIS-DAP, and Teensy/SAMD/RP2040 had no path at all. This PR builds the dispatch surface.

What this PR ships

boards.rs extensions

  • New ResetMethod enum: DtrRtsPulse | DtrPulse | TouchBaud1200 | SwdViaCmsisDap. Only DtrRtsPulse has an in-fbuild-serial implementation today (esp_hard_reset_blocking); the other three delegate to the caller.
  • BoardFamily::reset_method() — the family → primitive mapping.
  • BoardFamily::reset_is_serial_native()true iff dispatch_reset can run the reset inline.
  • Expanded variants: added Teensy, NativeUsbCdcReset1200Bps, ArduinoAutoReset. Renamed ArduinoArduinoAutoReset. Existing CdcAcmBridge stays without payload (future enhancement noted in doc-comment).
  • family_for_vid_pid updated: RP2040 now classifies as NativeUsbCdcReset1200Bps. Arduino returns ArduinoAutoReset. Teensy shares VID:PID with LPC11U35 — kept as CdcAcmBridge (more common case); Teensy variant must be constructed explicitly.

esp_reset.rs extensions

manager::open_port signature change

Added family: Option<BoardFamily> as a 4th positional arg. Consults idle_dtr_rts() for the post-open DTR/RTS state; None defaults to (true, true) (the prior hardcoded behavior). Updated three external callers (websockets, deploy, monitor) and the internal test to pass None.

Tests — 80 passed (was 72)

8 new tests, key one being dispatch_reset_cdc_acm_delegates_without_touching_dtr_rts which pins the FastLED/FastLED#3300 invariant at the dispatcher level — CDC-ACM dispatch MUST NOT pulse DTR/RTS.

test result: ok. 80 passed; 0 failed; 1 ignored; ...

cargo clippy -p fbuild-serial -p fbuild-daemon --all-targets -- -D warnings clean. cargo fmt --all --check clean.

Out of scope (follow-ups)

  • BoardFamily plumbing through the daemon's request layer (websockets, deploy, monitor all pass None for now). Needs an HTTP-API contract decision: client-supplied vs server-inferred via family_for_vid_pid.
  • Implementations of DtrPulse / TouchBaud1200 — each gets its own follow-up. The dispatcher returns DelegateToCaller so callers can wire them externally without blocking this landing.
  • CdcAcmBridge { vid, pid } payload — non-breaking variant evolution later.
  • Renaming esp_hard_reset_blockingesp_dtr_rts_reset_blocking (criterion 3): kept current name (already renamed in fbuild-serial: hard_reset_blocking leaves DTR=low — fatal for CDC-ACM bridges (LPC11U35, FTDI CDC). Mirror the FastLED #3300/#3339 lessons. #684); a second rename burns caller breakage for marginal clarity. The debug-assert intent is satisfied by dispatch_reset's family check.

Test plan

  • cargo test -p fbuild-serial --lib (80/80)
  • cargo clippy -p fbuild-serial -p fbuild-daemon --all-targets -- -D warnings clean
  • cargo fmt --all --check clean
  • Workspace CI green

Closes#687.

Summary by CodeRabbit

  • Refactor
    • Improved serial port initialization and board type detection for enhanced compatibility with various development boards (Arduino, RP2040, Teensy, ESP32).
    • Enhanced reset handling to provide more reliable device connections across different board types.

…iring (#687)
#687 — polymorphic reset dispatch. Until now reset was
implicit: the deploy paths called `esp_hard_reset_blocking` directly,
LPC callers had to do their own SWD-via-CMSIS-DAP, and Teensy/SAMD/
RP2040 had no path at all. This PR builds the dispatch surface.
## What this PR ships
### `boards.rs` extensions
- New `ResetMethod` enum: `DtrRtsPulse | DtrPulse | TouchBaud1200 |
SwdViaCmsisDap`. Only `DtrRtsPulse` has an in-`fbuild-serial`
implementation today (`esp_hard_reset_blocking`); the other three
delegate to the caller via `dispatch_reset`'s `DelegateToCaller`
return.
- `BoardFamily::reset_method()` — the family → primitive mapping.
- `BoardFamily::reset_is_serial_native()` — `true` iff `dispatch_reset`
can run the reset inline. Convenience for callers picking whether
to wire their own SWD path.
- Expanded `BoardFamily` variants (per #687's enum spec): added
`Teensy`, `NativeUsbCdcReset1200Bps`, `ArduinoAutoReset`. Renamed
`Arduino` → `ArduinoAutoReset` to match the issue spec. Existing
`CdcAcmBridge` stays without VID/PID payload (future enhancement
noted in doc-comment).
- `family_for_vid_pid` updated: RP2040 (0x2E8A:*) now classifies as
`NativeUsbCdcReset1200Bps` instead of `CdcAcmBridge`. Arduino
(0x2341:*) returns the renamed `ArduinoAutoReset`. Teensy shares
VID:PID 0x16C0:0483 with LPC11U35 — kept as `CdcAcmBridge` (the
more common case in this codebase); the Teensy variant must be
constructed explicitly when the caller knows.
### `esp_reset.rs` extensions
- `ResetDispatchOutcome { Done | DelegateToCaller(ResetMethod) }`.
- `dispatch_reset(family, port) -> Result<ResetDispatchOutcome>` —
routes ESP families to `esp_hard_reset_blocking`, returns
`DelegateToCaller(other)` for the three primitives `fbuild-serial`
doesn't own. The match check on `reset_method()` IS the debug-assert
#687 acceptance criterion 3 asks for: a future contributor
accidentally passing a non-ESP family ends at `DelegateToCaller`
instead of pulsing DTR=low on an LPC VCOM bridge.
### `manager::open_port` signature change
Added `family: Option<crate::boards::BoardFamily>` as a 4th
positional arg. Consults `idle_dtr_rts()` for the post-open DTR/RTS
state; `None` defaults to `(true, true)` (the prior hardcoded
behavior — the universal safe state per #684's analysis). Updated
the three external callers (websockets, deploy, monitor) and the
internal test to pass `None`. Plumbing the actual `BoardFamily`
through the daemon's request → board-family lookup is its own
follow-up.
## Tests
8 new fbuild-serial tests:
* `reset_method_maps_each_family_to_its_primitive` — every family
→ primitive (all 6 rows).
* `reset_is_serial_native_true_only_for_esp_families` — the
convenience predicate.
* `non_esp_families_all_idle_at_host_ready` —
FastLED/FastLED#3300 regression guard: every non-ESP family idles
at `(true, true)`. If a future refactor reintroduces a CDC family
that idles at `(false, false)`, this test fails immediately.
* `new_variants_appear_in_family_for_vid_pid_classification` —
RP2040 / Arduino / LPC-VCOM mappings.
* `dispatch_reset_esp_families_run_inline_and_report_done` — ESP
variants emit the canonical reset sequence + return `Done`.
* `dispatch_reset_cdc_acm_delegates_without_touching_dtr_rts` —
CDC-ACM family returns `DelegateToCaller(SwdViaCmsisDap)` and the
port's DTR/RTS history is empty. **This is the FastLED/FastLED#3300
guard at the dispatcher level.**
* `dispatch_reset_1200bps_families_delegate` — Teensy + RP2040 /
SAMD return `DelegateToCaller(TouchBaud1200)` with no DTR/RTS
side-effects.
* `dispatch_reset_arduino_delegates_to_dtr_pulse` — Arduino auto-
reset returns `DelegateToCaller(DtrPulse)`.
`cargo test -p fbuild-serial` — 80 passed (was 72). `cargo clippy
-p fbuild-serial -p fbuild-daemon --all-targets -- -D warnings`
clean. `cargo fmt --all --check` clean.
## Out of scope (follow-ups)
* `BoardFamily` plumbing through the daemon's request layer
(websockets, deploy, monitor all pass `None` for now) — needs an
HTTP-API contract decision: client-supplied vs server-inferred via
`family_for_vid_pid`.
* Implementations of `DtrPulse` (Arduino auto-reset), `TouchBaud1200`
(SAMD / RP2040 / Teensy) — each gets its own follow-up issue. The
dispatcher returns `DelegateToCaller` so callers can wire them
externally without `fbuild-serial` blocking landing.
* `CdcAcmBridge { vid, pid }` payload — useful for SWD dispatch to
the right CMSIS-DAP probe; non-breaking enum-variant evolution
later.
* Renaming `esp_hard_reset_blocking` to `esp_dtr_rts_reset_blocking`
(issue criterion 3): kept the current name (already renamed in
#684) because a second rename burns caller breakage for marginal
clarity. The debug-assert intent is satisfied by `dispatch_reset`'s
family check.
Closes#687.
@coderabbitai

coderabbitaiBot commented Jun 20, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The PR restructures BoardFamily by renaming Arduino to ArduinoAutoReset, splitting out CdcAcmBridge, Teensy, and NativeUsbCdcReset1200Bps variants, and updating idle_dtr_rts, reset_method, and reset_is_serial_native accordingly. open_port gains an Option<BoardFamily> parameter to drive per-family DTR/RTS idle-state selection. A new dispatch_reset function and ResetDispatchOutcome enum route reset handling by family. Daemon call sites pass None to match the new signature.

Changes

BoardFamily enum refactor, reset dispatch, and open_port signature

Layer / File(s)Summary
BoardFamily variants and methods
crates/fbuild-serial/src/boards.rs
BoardFamily enum restructured with CdcAcmBridge, Teensy, NativeUsbCdcReset1200Bps, ArduinoAutoReset variants replacing Arduino; idle_dtr_rts, reset_method, reset_is_serial_native, and family_for_vid_pid updated for the new set; ResetMethod docs updated; existing tests revised and new invariant tests added.
open_port signature extended with Option<BoardFamily>
crates/fbuild-serial/src/manager.rs
open_port accepts a new family: Option<BoardFamily> parameter; post-open DTR/RTS values derived from family.idle_dtr_rts() or default (true, true); logging reports chosen idle levels and family; regression test updated to pass None.
dispatch_reset and ResetDispatchOutcome
crates/fbuild-serial/src/esp_reset.rs
New ResetDispatchOutcome enum (Done, DelegateToCaller(ResetMethod)) and dispatch_reset function: calls esp_hard_reset_blocking for DtrRtsPulse families and returns DelegateToCaller for all others; dispatcher tests verify ESP families complete and non-ESP families delegate without DTR/RTS events.
Daemon call-site updates
crates/fbuild-daemon/src/handlers/operations/deploy.rs, ...monitor.rs, crates/fbuild-daemon/src/handlers/websockets.rs
ctx.serial_manager.open_port calls in the deploy monitor path, /api/monitor handler, and WebSocket attach handshake each pass an additional None argument to match the new signature.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

  • FastLED/fbuild#577: Introduced hard_reset_blocking in esp_reset.rs; this PR builds on that by adding dispatch_reset and ResetDispatchOutcome on top of esp_hard_reset_blocking.
  • FastLED/fbuild#685: Renamed/introduced ESP reset primitives including esp_hard_reset_blocking, which dispatch_reset in this PR directly calls for DTR/RTS-pulse families.
  • FastLED/fbuild#700: Adds the boards/BoardFamily infrastructure that drives the per-family DTR/RTS idle-state decisions consumed by the open_port changes here.

Poem

🐇 Hoppity-hop, the board variants grow,
ArduinoAutoReset now steals the show!
dispatch_reset routes each family's call —
ESP gets a pulse, the bridge gets SWD hall.
open_port takes None with a knowing wink,
All the daemon sites updated in a blink! 🎉

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe PR title accurately describes the main change: introducing BoardFamily enum with reset_method dispatch and the accompanying infrastructure for polymorphic reset handling.
Linked Issues check✅ PassedAll core acceptance criteria from #687 are met: BoardFamily enum created with idle_dtr_rts() and reset_method(), manager::open_port updated to accept optional BoardFamily, dispatch_reset() implemented with family routing, and documentation linking FastLED#3300 incident provided.
Out of Scope Changes check✅ PassedAll changes align with the PR scope; unimplemented TouchBaud1200 and DtrPulse primitives are intentionally deferred per the linked issue, and no extraneous modifications beyond the required structural additions are present.
Docstring Coverage✅ PassedDocstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/687-board-family-reset-method

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

The replace_all from BoardFamily::Arduino → BoardFamily::ArduinoAutoReset
matched as a prefix inside my own newly-written doctest references
that already said ArduinoAutoReset, producing
ArduinoAutoResetAutoReset. Fix the doc strings — code paths were never
affected because nothing called the malformed identifier outside the
doctest.
Caught by CI Check on Linux/macOS/Windows.
@zackees
zackees merged commit ca61f91 into mainJun 20, 2026
83 of 90 checks passed
@zackees
zackees deleted the feat/687-board-family-reset-method branch June 20, 2026 20:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

fbuild-serial: BoardFamily enum + polymorphic ResetMethod dispatch registry

1 participant

@zackees