feat(serial): board fingerprints + fbuild serial probe CLI (#686)#700
Merged
Conversation
#686 mirrors two artifacts that landed on the FastLED side in FastLED/FastLED#3339: a vendored `(vid, pid) → human hint` table and a port-probe CLI that opens with the correct DTR/RTS for the inferred board family. Both close gaps the closed #684 surfaced: with the rename to `esp_hard_reset_blocking` and the new `cdc_vcom_safe_assert` on the library side, the next missing piece was data + tooling. ## What this PR ships ### `crates/fbuild-serial/src/boards.rs` - `BOARD_FINGERPRINTS: &[(u16, u16, &str)]` — vendored from FastLED's `ci/util/serial_probe.py`. Covers NXP (LPC8xx CMSIS-DAP + LPC11U35), Espressif native USB + USB JTAG/serial, Silicon Labs CP210x, WCH CH340/CH9102, FTDI FT232R/FT231X, Arduino official, RP2040/Pico. - `ENVIRONMENT_TO_VCOM: &[(&str, u16, u16)]` — env-name → VCOM (vid, pid) for boards where the data port is NOT the primary USB endpoint (LPC845-BRK, LPC804, LPCXpresso845-MAX, LPCXpresso804 → LPC11U35). - `pub fn board_hint(vid, pid) -> Option<&'static str>` — exact-match lookup. - `pub fn vcom_for_env(env) -> Option<(u16, u16)>` — env-keyed lookup. - `pub fn family_for_vid_pid(vid, pid) -> Option<BoardFamily>` — classifier that returns the right `BoardFamily` so probe / monitor open paths can pick the right DTR/RTS via `idle_dtr_rts()` instead of the ESP-default that drops bytes on CDC-ACM bridges. - `pub enum BoardFamily { Esp32NativeUsbCdc, Esp32ExternalUart, CdcAcmBridge, Arduino }` with `idle_dtr_rts(&self) -> (bool, bool)`. Minimal scope per the #687 follow-up that owns the polymorphic `ResetMethod` registry. ### `crates/fbuild-cli/src/cli/serial_probe.rs` New `fbuild serial probe …` subcommand tree: * `fbuild serial probe list` — enumerate every visible serial port with VID:PID + board hint. Verified locally — output matches FastLED's `serial_probe.py list` shape exactly: ``` COM25 303A:1001 ser=… USB Serial Device (COM25) [Espressif native USB CDC …] COM20 16C0:0483 ser=… USB Serial Device (COM20) [LPC11U35 VCOM bridge …] COM10 1FC9:0132 ser=… USB Serial Device (COM10) [NXP CMSIS-DAP debug …] ``` * `fbuild serial probe find --vid-pid V:P | --env <name>` — return one port path on stdout (exit 0) or exit 1 when no match. Verified locally: `--env lpc845brk` → `COM20` (the VCOM bridge), NOT `COM10` (the CMSIS-DAP debug probe) — the disambiguation step that FastLED's `port_utils.py` had to add post-#3300. * `fbuild serial probe read PORT [--baud N] [--seconds N] [--send STR]` — open with `family_for_vid_pid(...).idle_dtr_rts()` so CDC-ACM bridges see DTR=true / RTS=true instead of the ESP-default that silently drops every byte the target MCU transmits. Supports a small set of `\n`/`\r`/`\t`/`\0`/`\xHH` escapes in the `--send` payload so scripted probes don't need shell-quoting gymnastics. ### Tests * 9 boards-module tests — every public API + a regression test pinning that `family_for_vid_pid(0x16C0, 0x0483).idle_dtr_rts()` == `(true, true)` (the LPC11U35 VCOM bridge MUST end at host-ready, or we reintroduce the FastLED/FastLED#3300 silent-byte-drop bug). * 6 CLI tests covering `parse_vid_pid` (colon + comma forms, malformed rejection) and `unescape_send` (common escapes, hex bytes, unknown escape pass-through, trailing backslash edge case). * No new tests for the hardware-touching paths (`list`, `find`, `read`) — those need a board attached. Manually smoke-verified locally against a multi-board host (LPC845-BRK on COM20, three ESP32-S3 on COM9 / COM22 / COM25, LPC CMSIS-DAP on COM10) and the disambiguation works. `cargo clippy -p fbuild-serial -p fbuild-cli --all-targets -- -D warnings` clean. `cargo fmt --all --check` clean. ## Out of scope (deferred to siblings of #686) - Cross-repo sync (acceptance criterion 3) — the data table is now in fbuild and the FastLED side could load it directly in a follow-up PR (option a from #686), OR a CI diff check could compare the two tables (option b). Neither is in this PR; both options remain open. - Polymorphic `ResetMethod` / `BootModeClassifier` dispatch registry — scope of #687, #688 respectively. `BoardFamily` here is the minimum needed by #686 criterion 4; the polymorphic dispatch comes next. Closes #686.
📝 WalkthroughWalkthroughAdds a board fingerprint registry crate module ( ChangesSerial Probe CLI and Board Registry
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent / Human
participant fbuild as fbuild CLI
participant SerialProbe as serial_probe.rs
participant Boards as fbuild-serial::boards
participant Port as serialport
Agent->>fbuild: fbuild serial probe read /dev/ttyUSB0 --baud 115200 --send '{"id":1}'
fbuild->>SerialProbe: run_serial(SerialAction::Probe { action: Read { ... } })
SerialProbe->>Port: available_ports()
Port-->>SerialProbe: port list with USB info
SerialProbe->>Boards: family_for_vid_pid(vid, pid)
Boards-->>SerialProbe: BoardFamily::CdcAcmBridge
SerialProbe->>Boards: idle_dtr_rts() → (true, true)
SerialProbe->>Port: open(port, baud)
SerialProbe->>Port: write_data_terminal_ready(true), write_request_to_send(true)
SerialProbe->>SerialProbe: unescape_send(payload)
SerialProbe->>Port: write(escaped_bytes)
loop until deadline
SerialProbe->>Port: read(buf)
Port-->>SerialProbe: bytes (or timeout)
SerialProbe->>Agent: stdout.write(bytes)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-cli/src/cli/serial_probe.rs`:
- Around line 112-125: The `list_ports()` function currently violates the
thin-client architecture by directly calling `serialport::available_ports()` to
enumerate serial ports in the CLI process. Replace this direct serial port
enumeration with an HTTP request to the daemon that returns the list of
available ports. The daemon should be responsible for all serial port operations
and device access. Apply this same refactoring pattern to the other serial probe
functions mentioned (in the 156-190 and 216-285 ranges) that also perform direct
device access operations like opening, reading, and writing to serial ports.
- Around line 246-247: The `seconds` parameter passed to
`Duration::from_secs_f64(seconds)` on line 246 is user-controlled CLI input that
lacks validation, which can cause panics if the value is negative, non-finite
(NaN/infinity), or exceeds Duration's maximum capacity. Before calling
`Duration::from_secs_f64(seconds)`, add validation to check that `seconds` is a
valid non-negative finite number within acceptable bounds, and return an
appropriate error to the user if validation fails instead of allowing the method
to panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fbf72b1-9b9f-40e7-a5a1-0ccbef872d51
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/fbuild-cli/Cargo.tomlcrates/fbuild-cli/src/cli/args.rscrates/fbuild-cli/src/cli/dispatch.rscrates/fbuild-cli/src/cli/mod.rscrates/fbuild-cli/src/cli/serial_probe.rscrates/fbuild-serial/src/boards.rscrates/fbuild-serial/src/lib.rs
Comment on lines
+112
to
+125
| fn list_ports() -> Result<()> { | ||
| let ports = serialport::available_ports() | ||
| .map_err(|e| FbuildError::SerialError(format!("serial port enumeration failed: {e}")))?; | ||
|
|
||
| if ports.is_empty() { | ||
| eprintln!("no serial ports visible"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| for port in &ports { | ||
| print_port_summary(port); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Route serial probe I/O through daemon instead of direct device access in CLI.
fbuild-cli now directly enumerates and opens serial ports (available_ports, open, read, write) in-process. That breaks the thin-client boundary and creates an additional hardware-access path outside daemon coordination.
As per coding guidelines, "fbuild-cli is a thin HTTP client to daemon ... that send JSON requests to the daemon."
Also applies to: 156-190, 216-285
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-cli/src/cli/serial_probe.rs` around lines 112 - 125, The
`list_ports()` function currently violates the thin-client architecture by
directly calling `serialport::available_ports()` to enumerate serial ports in
the CLI process. Replace this direct serial port enumeration with an HTTP
request to the daemon that returns the list of available ports. The daemon
should be responsible for all serial port operations and device access. Apply
this same refactoring pattern to the other serial probe functions mentioned (in
the 156-190 and 216-285 ranges) that also perform direct device access
operations like opening, reading, and writing to serial ports.
Source: Coding guidelines
Comment on lines
+246
to
+247
| let deadline = Instant::now() + Duration::from_secs_f64(seconds); | ||
| let mut buf = [0u8; 1024]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the panic-prone callsite and absence/presence of input guards.
rg -n -C4 'fn read_port|from_secs_f64\(seconds\)|is_finite|checked_add' crates/fbuild-cli/src/cli/serial_probe.rsRepository: FastLED/fbuild
Length of output: 1094
🏁 Script executed:
# Get the full function to see if there's any validation we missed
sed -n '216,350p' crates/fbuild-cli/src/cli/serial_probe.rsRepository: FastLED/fbuild
Length of output: 5176
🏁 Script executed:
# Find all callers of read_port to understand where seconds comes from
rg -n 'read_port\(' crates/fbuild-cli/src/cli/serial_probe.rsRepository: FastLED/fbuild
Length of output: 220
🏁 Script executed:
# Check what CLI argument parsing looks like for --seconds
rg -n -B5 -A5 '--seconds|seconds:' crates/fbuild-cli/src/cli/serial_probe.rsRepository: FastLED/fbuild
Length of output: 479
🏁 Script executed:
# Check the context around line 107 where read_port is called
sed -n '90,125p' crates/fbuild-cli/src/cli/serial_probe.rsRepository: FastLED/fbuild
Length of output: 1056
🏁 Script executed:
# Search for where seconds argument is defined in the CLI structure
rg -n 'seconds' crates/fbuild-cli/src/cli/serial_probe.rs | head -30Repository: FastLED/fbuild
Length of output: 503
🌐 Web query:
Rust Duration::from_secs_f64 panic behavior with negative non-finite values
💡 Result:
In Rust, the Duration::from_secs_f64 method will panic if the provided f64 value is negative, is non-finite (e.g., NaN or infinity), or exceeds the maximum capacity of a Duration [1][2][3]. The method is implemented to explicitly check these conditions by calling Duration::try_from_secs_f64 internally; if the checked conversion returns an error, the method panics with the error message [2][3]. To perform this conversion without panicking, you should use the stable method Duration::try_from_secs_f64, which returns a Result<Duration, TryFromFloatSecsError> instead [4][3]. This allows you to handle negative, non-finite, or overflowing input values gracefully as part of your program's error-handling logic [4].
Citations:
- 1: https://doc.rust-lang.org/std/time/struct.Duration.html
- 2: https://github.com/rust-lang/rust/blob/0f145634/library/core/src/time.rs
- 3: https://doc.rust-lang.org/src/core/time.rs.html
- 4: Tracking Issue for
Duration::try_from_secs_{f32, f64}rust-lang/rust#83400
Validate --seconds before Duration::from_secs_f64 to prevent process panic.
seconds is user-controlled CLI input currently passed directly to Duration::from_secs_f64(seconds) without validation. The method panics if the value is negative, non-finite (NaN/infinity), or exceeds the maximum capacity of a Duration. This will crash the CLI.
Proposed fix
fn read_port(port_name: &str, baud: u32, seconds: f64, send: Option<&str>) -> Result<()> {
+ if !seconds.is_finite() || seconds < 0.0 || seconds > (u64::MAX as f64) {
+ return Err(FbuildError::SerialError(format!(
+ "invalid `--seconds` value `{seconds}`; expected a finite non-negative number"
+ )));
+ }
+
// Resolve the family by looking up the connected port's VID:PID,
@@
- let deadline = Instant::now() + Duration::from_secs_f64(seconds);
+ let read_window = Duration::from_secs_f64(seconds);
+ let deadline = Instant::now()
+ .checked_add(read_window)
+ .ok_or_else(|| FbuildError::SerialError("`--seconds` is too large".to_string()))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let deadline = Instant::now() + Duration::from_secs_f64(seconds); | |
| let mut buf = [0u8; 1024]; | |
| fn read_port(port_name: &str, baud: u32, seconds: f64, send: Option<&str>) -> Result<()> { | |
| if !seconds.is_finite() || seconds < 0.0 || seconds > (u64::MAX as f64) { | |
| return Err(FbuildError::SerialError(format!( | |
| "invalid `--seconds` value `{seconds}`; expected a finite non-negative number" | |
| ))); | |
| } | |
| // Resolve the family by looking up the connected port's VID:PID, | |
| let read_window = Duration::from_secs_f64(seconds); | |
| let deadline = Instant::now() | |
| .checked_add(read_window) | |
| .ok_or_else(|| FbuildError::SerialError("`--seconds` is too large".to_string()))?; | |
| let mut buf = [0u8; 1024]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-cli/src/cli/serial_probe.rs` around lines 246 - 247, The
`seconds` parameter passed to `Duration::from_secs_f64(seconds)` on line 246 is
user-controlled CLI input that lacks validation, which can cause panics if the
value is negative, non-finite (NaN/infinity), or exceeds Duration's maximum
capacity. Before calling `Duration::from_secs_f64(seconds)`, add validation to
check that `seconds` is a valid non-negative finite number within acceptable
bounds, and return an appropriate error to the user if validation fails instead
of allowing the method to panic.
4 tasks
13 tasks
This was referenced Jun 30, 2026
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.
Summary
#686 mirrors two artifacts that landed on the FastLED side in FastLED/FastLED#3339: a vendored
(vid, pid) → human hinttable and a port-probe CLI that opens with the correct DTR/RTS for the inferred board family. Both close gaps the closed #684 surfaced — with the rename toesp_hard_reset_blockingand the newcdc_vcom_safe_asserton the library side, the next missing piece was the data + tooling.What this PR ships
crates/fbuild-serial/src/boards.rsBOARD_FINGERPRINTS— vendored VID:PID → human hint table covering NXP (LPC8xx CMSIS-DAP + LPC11U35), Espressif native USB + USB JTAG/serial, Silicon Labs CP210x, WCH CH340/CH9102, FTDI FT232R/FT231X, Arduino official, RP2040/Pico.ENVIRONMENT_TO_VCOM— env-name → VCOM (vid, pid) for boards where the data port is NOT the primary USB endpoint (LPC845-BRK, LPC804, LPCXpresso845-MAX/804 → LPC11U35).board_hint(vid, pid),vcom_for_env(env),family_for_vid_pid(vid, pid)lookup APIs.BoardFamily { Esp32NativeUsbCdc | Esp32ExternalUart | CdcAcmBridge | Arduino }withidle_dtr_rts(&self) -> (bool, bool). Minimal scope — polymorphicResetMethoddispatch is the scope of fbuild-serial: BoardFamily enum + polymorphic ResetMethod dispatch registry #687.crates/fbuild-cli/src/cli/serial_probe.rs+ dispatch wiringThe
readpath picks DTR/RTS viafamily_for_vid_pid(...).idle_dtr_rts()— so CDC-ACM bridges see "host ready" instead of the ESP-default "host not ready" that silently drops every byte the target transmits (the FastLED/FastLED#3300 failure).Tests
family_for_vid_pid(0x16C0, 0x0483).idle_dtr_rts() == (true, true). The LPC11U35 VCOM bridge MUST end at host-ready, or the [BLOCKER] LPC845-BRK AutoResearch bring-up echo returns empty RX (suspected HardFault) — blocks #3102 FastLED#3300 silent-byte-drop bug reappears.parse_vid_pid(colon + comma forms, malformed rejection) andunescape_send(common escapes, hex bytes, unknown escape pass-through, trailing backslash edge case).list,find,read) — those need a board attached. Manually smoke-verified locally against a multi-board host:find --env lpc845brk→COM20(the VCOM bridge), NOTCOM10(the CMSIS-DAP debug probe) — the disambiguation step that FastLED'sport_utils.pyhad to add post-#3300.cargo clippy -p fbuild-serial -p fbuild-cli --all-targets -- -D warningsclean.cargo fmt --all --checkclean.Out of scope (deferred to #686's siblings)
ResetMethod/BootModeClassifierdispatch registry — scope of fbuild-serial: BoardFamily enum + polymorphic ResetMethod dispatch registry #687 / fbuild-serial::boot_mode: generalize to per-family BootModeClassifier registry (ESP-only today) #688 respectively.BoardFamilyhere is the minimum needed by fbuild-serial: vendor a serial-probe CLI + shared BOARD_FINGERPRINTS table (companion to closed #684, parallel to FastLED #3339) #686 criterion 4.Test plan
cargo test -p fbuild-serial --lib boards::(9/9)cargo test -p fbuild-cli --bin fbuild cli::serial_probe::(6/6)cargo clippy -p fbuild-serial -p fbuild-cli --all-targets -- -D warningscleancargo fmt --all --checkcleanserial probe liston a multi-board hostserial probe find --env lpc845brkreturns COM20Closes #686.
Summary by CodeRabbit
Release Notes
fbuild serialdebugging command with port enumeration, lookup, and reading capabilitiesserial listdisplays available USB serial ports with board identification hintsserial findlocates ports by VID/PID or environment mappingserial readopens and reads from serial ports with configurable baud rate and signal control