fix(serial): list Windows Status=Unknown ports so Teensy (16C0) is visible (#962)#1011
Conversation
…sible (#962) `fbuild port scan` (and the Teensy deploy port-discovery snapshot) missed every PJRC/Teensy serial port on Windows. Upstream `serialport::available_ports` enumerates the Ports class with `DIGCF_PRESENT` and additionally skips any devnode whose `CM_Get_DevNode_Status` problem code isn't 0. A Teensy's serial functions enumerate as composite MI_00 interfaces whose function driver never starts (PnP `Status = Unknown`), so they are non-present phantoms and get dropped twice over — a physically-attached Teensy was invisible. Add a blessed `fbuild_serial::ports::available_ports()`: a Windows fork of serialport's SetupAPI walk that (1) enumerates with `flags = 0` so phantom devnodes are returned, (2) recovers VID/PID from the composite child's own hardware id when the phantom parent is unreachable, and (3) includes a non-present port only when it is a USB serial device (never resurrecting stale non-USB junk). Route `port scan` and Teensy `port_discovery` through it. Also enable serialport's `usbportinfo-interface` feature and surface the composite `MI_xx` index in `port scan`, so a Teensy's Serial vs Serial+MIDI functions are distinguishable. Teensy VID:PID product names come from the embedded FastLED/boards archive (no hardcoded table). Verified on real hardware: COM19 16C0:0483 -> Teensyduino Serial (MI_00), COM21/COM7 16C0:0489 -> Teensyduino MIDI+Serial; previously-hidden Status=Unknown ESP32 ports now appear too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a cross-platform "blessed" serial port enumerator ( ChangesComposite serial port enumeration and interface surfacing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PortScanCLI
participant available_ports
participant WindowsEnumerator
participant SetupDiRegistry
PortScanCLI->>available_ports: request port list
available_ports->>WindowsEnumerator: enumerate device-class GUIDs (Windows)
WindowsEnumerator->>SetupDiRegistry: query hardware IDs, status, PortName
SetupDiRegistry-->>WindowsEnumerator: composite MI_xx info, COM names
WindowsEnumerator->>SetupDiRegistry: merge SERIALCOMM fallback entries
WindowsEnumerator-->>available_ports: deduplicated port list with interface index
available_ports-->>PortScanCLI: SerialPortInfo list
PortScanCLI->>PortScanCLI: render_usb_port appends if=MI_XX
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-serial/src/ports.rs`:
- Around line 375-396: The property lookup in `property` is passing the buffer
length in `u16` elements instead of bytes to
`SetupDiGetDeviceRegistryPropertyW`, which can truncate valid `REG_SZ` values.
Update the `property_buf` size argument to use the byte size of the buffer in
the `property` method so the API receives the full capacity. Keep the existing
`value_type` check and string parsing logic in place after the buffer sizing
fix.
🪄 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: aa4c3c47-c282-4a34-93c2-213d8fceeb78
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/fbuild-cli/src/cli/port_scan.rscrates/fbuild-core/src/usb/resolver.rscrates/fbuild-deploy/src/esp32_native/transport.rscrates/fbuild-deploy/src/teensy/port_discovery.rscrates/fbuild-serial/Cargo.tomlcrates/fbuild-serial/src/lib.rscrates/fbuild-serial/src/ports.rs
| fn property(&mut self, property_id: u32) -> Option<String> { | ||
| let mut value_type = 0; | ||
| let mut property_buf = [0u16; MAX_PATH as usize]; | ||
| let res = unsafe { | ||
| SetupDiGetDeviceRegistryPropertyW( | ||
| self.hdi, | ||
| &self.devinfo_data, | ||
| property_id, | ||
| &mut value_type, | ||
| property_buf.as_mut_ptr() as *mut u8, | ||
| property_buf.len() as u32, | ||
| ptr::null_mut(), | ||
| ) | ||
| }; | ||
| if res == FALSE || value_type != REG_SZ { | ||
| return None; | ||
| } | ||
| from_utf16_lossy_trimmed(&property_buf) | ||
| .split(';') | ||
| .next_back() | ||
| .map(str::to_string) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n "SetupDiGetDeviceRegistryPropertyW|PropertyBufferSize|property_buf.len\(\) as u32|MAX_PATH as usize" crates/fbuild-serial src -SRepository: FastLED/fbuild
Length of output: 1107
🌐 Web query:
SetupDiGetDeviceRegistryPropertyW PropertyBufferSize bytes documentation
💡 Result:
In the Windows API function SetupDiGetDeviceRegistryPropertyW, the PropertyBufferSize parameter specifies the size, in bytes, of the buffer provided via the PropertyBuffer parameter [1][2][3]. This parameter serves two primary purposes in managing memory for device property retrieval: 1. Standard Retrieval: When PropertyBuffer points to a valid memory location, PropertyBufferSize tells the function how many bytes are available to store the retrieved property data [1][3]. 2. Buffer Size Inquiry: You can determine the required size for a property without pre-allocating a large buffer by performing a two-step process [4][2]: - Set the PropertyBuffer parameter to NULL and the PropertyBufferSize parameter to zero [1][3]. - When called this way, the function returns FALSE (as the buffer is too small), and the RequiredSize parameter is populated with the number of bytes necessary to hold the requested property data [4][1][3]. You can then allocate a buffer of the size specified in RequiredSize and call the function again to retrieve the actual data [4]. It is important to note that the RequiredSize value reflects the total bytes needed for the property, and the function will return an error if the provided PropertyBufferSize is smaller than this value [2][3].
Citations:
- 1: https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/setupapi/nf-setupapi-setupdigetdeviceregistrypropertyw.md
- 2: https://learn.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertya
- 3: https://learn.microsoft.com/hu-hu/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertyw
- 4: https://learn.microsoft.com/en-us/windows/win32/api/setupapi/nf-setupapi-setupdigetdeviceregistrypropertyw
🏁 Script executed:
sed -n '360,395p' crates/fbuild-serial/src/ports.rsRepository: FastLED/fbuild
Length of output: 1529
Pass the property buffer size in bytes, not u16 elements. SetupDiGetDeviceRegistryPropertyW expects PropertyBufferSize in bytes, so property_buf.len() as u32 halves the usable capacity and causes valid manufacturer/friendly-name values over 260 bytes to fail and return None.
Proposed fix
SetupDiGetDeviceRegistryPropertyW(
self.hdi,
&self.devinfo_data,
property_id,
&mut value_type,
property_buf.as_mut_ptr() as *mut u8,
- property_buf.len() as u32,
+ (property_buf.len() * 2) as u32,
ptr::null_mut(),
)📝 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.
| fn property(&mut self, property_id: u32) -> Option<String> { | |
| let mut value_type = 0; | |
| let mut property_buf = [0u16; MAX_PATH as usize]; | |
| let res = unsafe { | |
| SetupDiGetDeviceRegistryPropertyW( | |
| self.hdi, | |
| &self.devinfo_data, | |
| property_id, | |
| &mut value_type, | |
| property_buf.as_mut_ptr() as *mut u8, | |
| property_buf.len() as u32, | |
| ptr::null_mut(), | |
| ) | |
| }; | |
| if res == FALSE || value_type != REG_SZ { | |
| return None; | |
| } | |
| from_utf16_lossy_trimmed(&property_buf) | |
| .split(';') | |
| .next_back() | |
| .map(str::to_string) | |
| } | |
| fn property(&mut self, property_id: u32) -> Option<String> { | |
| let mut value_type = 0; | |
| let mut property_buf = [0u16; MAX_PATH as usize]; | |
| let res = unsafe { | |
| SetupDiGetDeviceRegistryPropertyW( | |
| self.hdi, | |
| &self.devinfo_data, | |
| property_id, | |
| &mut value_type, | |
| property_buf.as_mut_ptr() as *mut u8, | |
| (property_buf.len() * 2) as u32, | |
| ptr::null_mut(), | |
| ) | |
| }; | |
| if res == FALSE || value_type != REG_SZ { | |
| return None; | |
| } | |
| from_utf16_lossy_trimmed(&property_buf) | |
| .split(';') | |
| .next_back() | |
| .map(str::to_string) | |
| } |
🤖 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-serial/src/ports.rs` around lines 375 - 396, The property
lookup in `property` is passing the buffer length in `u16` elements instead of
bytes to `SetupDiGetDeviceRegistryPropertyW`, which can truncate valid `REG_SZ`
values. Update the `property_buf` size argument to use the byte size of the
buffer in the `property` method so the API receives the full capacity. Keep the
existing `value_type` check and string parsing logic in place after the buffer
sizing fix.
What
Fixes #962 —
fbuild port scannow lists PJRC/Teensy (VID16C0) serial ports on Windows, resolves them, and surfaces the compositeMI_xxinterface index.Root cause
serialport::available_ports()on Windows:DIGCF_PRESENT, andCM_Get_DevNode_Statusproblem code isn't0.A Teensy's serial functions enumerate as composite
MI_00interfaces whoseusbserfunction driver never starts — PnPStatus = Unknown. To SetupAPI they are non-present phantoms (CR_NO_SUCH_DEVINST), so they're dropped byDIGCF_PRESENTbefore the problem-code filter even runs. Result: a physically-attached Teensy is invisible toport scan(and to the Teensy deploy port-discovery snapshot, which used the same call).Confirmed on real hardware — 4 Teensy COM ports (
16C0:0483×2,16C0:0489×2) present in Windows PnP but absent fromport scan; also affected someStatus=UnknownESP32 ports.Fix
New blessed enumerator
fbuild_serial::ports::available_ports()(theban_direct_serialportwrapper crate). A Windows fork of serialport's SetupAPI walk that:flags = 0(notDIGCF_PRESENT) so phantom devnodes are returned;Nonehere → no VID/PID);DEVICEMAP\SERIALCOMMfallback for parity.port scanand Teensyport_discoverynow route through it. Non-Windows delegates straight toserialport.Also: enable serialport's
usbportinfo-interfacefeature and render the compositeMI_xxindex (if=MI_00) so a Teensy's Serial vs Serial+MIDI functions are distinguishable. Teensy product names come from the embedded FastLED/boards VID:PID archive — no hardcoded table.Verification (real hardware)
Tests:
fbuild-serial(148),fbuild-core(237),port_scan(16) all green; new unit tests cover phantom-composite parsing (no parent), the MI index render, and archive-driven PJRC resolution.Follow-up (tracked under #959)
The remaining hardcoded
FRIENDLY_PRODUCTSentries (Espressif303A, NXP MCU-Link1FC9:0143) still aren't in the embedded archive. Migrating them into the FastLED/boardsotherbranch and deleting the in-code table is the#959"build-time embedding" cleanup, done there to avoid regressing offline ESP32 names.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes