fix(autoresearch): correct port auto-detect for teensy* + data-driven USB vendor names - #3610
Closed
zackees wants to merge 1 commit into
Closed
fix(autoresearch): correct port auto-detect for teensy* + data-driven USB vendor names#3610zackees wants to merge 1 commit into
zackees wants to merge 1 commit into
Conversation
…USB vendor names
`bash autoresearch teensy41` auto-detected the WRONG port (an ESP32's
CP2102 on COM11) instead of the Teensy: `ENVIRONMENT_TO_VCOM_VID_PIDS` had
no teensy entry, so port selection fell through to the generic USB
heuristic that prefers CP2102/CH340/FTDI adapters.
- Add PJRC Teensy fingerprints (16C0:0483 / 16C0:0489) for teensy40/41/lc/
31/35/36/mm so teensy* selects the Teensy CDC port precisely.
- Match VID:PIDs in DECLARED PREFERENCE ORDER (not a set), so an entry can
list its unambiguous fingerprint first — LPC8xx now keys only on the
LPC-Link2 1FC9:0132 (16C0:0483 is dropped there: on Windows the legacy
LPCXpresso-VCOM firmware is byte-identical to a Teensy — same VID:PID,
generic "USB Serial Device" — so keeping it made lpc* grab a co-attached
Teensy).
- New `ci/util/usb_vendors.py`: lazy-decompress the `usb-vendors.tar.zst`
VID→vendor table into memory on first use (mirrors fbuild's embedded
resolver), used to annotate port-detection diagnostics
("COM11=10C4:EA60 (Silicon Labs)").
Tests: ci/tests/test_usb_vendors.py (3) pass; teensy41→COM20, lpc845→
1FC9:0132 verified against live attached ports.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a lazy-loading USB vendor ID-to-name resolver ( ChangesUSB vendor lookup and port matching
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant auto_detect_upload_port
participant Ports
participant _describe_port
participant vendor_name
participant _vendor_map
auto_detect_upload_port->>Ports: enumerate connected ports
auto_detect_upload_port->>auto_detect_upload_port: iterate declared VID:PID pairs in order
alt matching port found
auto_detect_upload_port-->>auto_detect_upload_port: return first matching port
else no match
auto_detect_upload_port->>_describe_port: describe each detected port
_describe_port->>vendor_name: resolve VID to vendor name
vendor_name->>_vendor_map: load/get cached vendor table
_vendor_map-->>vendor_name: vendor dict
vendor_name-->>_describe_port: vendor name or None
_describe_port-->>auto_detect_upload_port: "COMx=VID:PID (Vendor)"
auto_detect_upload_port-->>auto_detect_upload_port: raise error with formatted port list
end
Related PRs: None specified. Suggested labels: ci, testing Suggested reviewers: None specified. 🐰 A vendor's name, once locked in a tar, 🚥 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 |
![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
🧹 Nitpick comments (1)
ci/tests/test_usb_vendors.py (1)
26-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock classes may fail static type checking against
_describe_port.
_describe_portis typed to acceptListPortInfo(a concrete pyserial class perci/util/port_utils.py)._Pand_NoIdsduck-type the needed attributes but don't inherit from or structurally satisfy that type, which mypy/pyright would flag as an incompatible argument. Runtime behavior is fine; only static analysis is at risk.🔧 Suggested fix using cast or a minimal subclass
+from typing import cast +from serial.tools.list_ports_common import ListPortInfo + def test_describe_port_appends_vendor() -> None: class _P: device = "COM7" vid = 0x303A pid = 0x1001 class _NoIds: device = "COM8" vid = None pid = None - assert _describe_port(_P()) == "COM7=303A:1001 (Espressif Systems)" - assert _describe_port(_NoIds()) == "COM8=----:----" + assert _describe_port(cast(ListPortInfo, _P())) == "COM7=303A:1001 (Espressif Systems)" + assert _describe_port(cast(ListPortInfo, _NoIds())) == "COM8=----:----"🤖 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 `@ci/tests/test_usb_vendors.py` around lines 26 - 37, The test uses mock helper classes that don’t match the concrete ListPortInfo type expected by _describe_port, which can trigger static type-checking errors. Update the assertions in test_usb_vendors.py to either cast the mock instances to the expected type or make the mocks inherit from a minimal ListPortInfo-compatible base so mypy/pyright accepts the call while preserving the same runtime behavior. Keep the fix localized around _describe_port and the _P/_NoIds test doubles.
🤖 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 `@ci/util/port_utils.py`:
- Around line 85-89: The fingerprint mapping for lpc845brk no longer matches the
expectations in the tests. Either keep 16C0:0483 in the lpc845brk entry in
port_utils, or update the related tests to the new fingerprint set. Make the
change consistent with test_lpc845brk_lpcxpresso_vcom_fingerprint_matches and
test_lpc845brk_neither_fingerprint_matches_reports_both so the expected VID:PID
values and error text align.
In `@ci/util/usb_vendors.py`:
- Around line 60-75: The lazy import in usb_vendors.py can hide a missing
dependency, causing fresh installs to silently fall back to an empty vendor map.
Make zstandard an explicit dependency in pyproject.toml or change the decompress
path in the usb_vendors helper to clearly treat zstandard as optional and
log/handle the fallback intentionally, keeping the behavior around
_MAX_DECOMPRESSED_BYTES and the usb-vendors.txt extraction unchanged.
---
Nitpick comments:
In `@ci/tests/test_usb_vendors.py`:
- Around line 26-37: The test uses mock helper classes that don’t match the
concrete ListPortInfo type expected by _describe_port, which can trigger static
type-checking errors. Update the assertions in test_usb_vendors.py to either
cast the mock instances to the expected type or make the mocks inherit from a
minimal ListPortInfo-compatible base so mypy/pyright accepts the call while
preserving the same runtime behavior. Keep the fix localized around
_describe_port and the _P/_NoIds test doubles.
🪄 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: e24e5117-e796-40cc-96d1-221e896a3398
⛔ Files ignored due to path filters (1)
ci/data/usb-vendors.tar.zstis excluded by!**/*.zst
📒 Files selected for processing (3)
ci/tests/test_usb_vendors.pyci/util/port_utils.pyci/util/usb_vendors.py
Comment on lines
+85
to
+89
| "lpc845brk": ((0x1FC9, 0x0132),), | ||
| "lpc845": ((0x1FC9, 0x0132),), | ||
| "lpc804": ((0x1FC9, 0x0132),), | ||
| "lpcxpresso845max": ((0x1FC9, 0x0132),), | ||
| "lpcxpresso804": ((0x1FC9, 0x0132),), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '16C0:0483|lpc845brk_lpcxpresso_vcom_fingerprint_matches|neither_fingerprint_matches_reports_both' ci/tests/test_autoresearch_phases.pyRepository: FastLED/FastLED
Length of output: 1039
Keep 16C0:0483 for lpc845brk, or update the matching tests in this PR.
test_lpc845brk_lpcxpresso_vcom_fingerprint_matches still expects 16C0:0483, and test_lpc845brk_neither_fingerprint_matches_reports_both still asserts the error message includes both 16C0:0483 and 1FC9:0132. Removing the old VID:PID from lpc845brk will break those checks.
🤖 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 `@ci/util/port_utils.py` around lines 85 - 89, The fingerprint mapping for
lpc845brk no longer matches the expectations in the tests. Either keep 16C0:0483
in the lpc845brk entry in port_utils, or update the related tests to the new
fingerprint set. Make the change consistent with
test_lpc845brk_lpcxpresso_vcom_fingerprint_matches and
test_lpc845brk_neither_fingerprint_matches_reports_both so the expected VID:PID
values and error text align.
Comment on lines
+60
to
+75
| try: | ||
| import zstandard | ||
|
|
||
| decompressed = zstandard.ZstdDecompressor().decompress( | ||
| raw, max_output_size=_MAX_DECOMPRESSED_BYTES | ||
| ) | ||
| with tarfile.open(fileobj=io.BytesIO(decompressed)) as tf: | ||
| member = tf.extractfile("usb-vendors.txt") | ||
| if member is None: | ||
| return {} | ||
| text = member.read().decode("utf-8", "replace") | ||
| except KeyboardInterrupt as ki: | ||
| handle_keyboard_interrupt(ki) | ||
| raise | ||
| except Exception: # noqa: BLE001 — any decode failure → empty map fallback | ||
| return {} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether zstandard is a declared project dependency
fd -t f 'pyproject.toml|requirements.*\.txt|setup.cfg|setup.py' --exec rg -n -i 'zstandard' {} +Repository: FastLED/FastLED
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Dependency/manifests in repo:\n'
git ls-files | rg '(^|/)(pyproject\.toml|requirements[^/]*\.txt|setup\.cfg|setup\.py|poetry\.lock|Pipfile|Pipfile\.lock|uv\.lock|requirements\.in)$' || true
printf '\nSearch for zstandard references:\n'
git ls-files | xargs rg -n -i --no-heading '\bzstandard\b' || true
printf '\nTarget file existence:\n'
git ls-files 'ci/util/usb_vendors.py' || trueRepository: FastLED/FastLED
Length of output: 420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== manifests containing zstandard =='
for f in $(git ls-files | rg '(^|/)(pyproject\.toml|requirements[^/]*\.txt|setup\.cfg|setup\.py|poetry\.lock|Pipfile|Pipfile\.lock|uv\.lock|requirements\.in)$'); do
if rg -n -i '\bzstandard\b' "$f"; then
echo
fi
done
echo '== ci/util/usb_vendors.py (relevant lines) =='
nl -ba ci/util/usb_vendors.py | sed -n '1,130p'Repository: FastLED/FastLED
Length of output: 277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== all tracked files mentioning zstandard =='
git ls-files | xargs rg -n -i --no-heading '\bzstandard\b' || true
echo
echo '== dependency-related tracked files =='
git ls-files | rg '(^|/)(pyproject\.toml|requirements[^/]*\.txt|setup\.cfg|setup\.py|tox\.ini|noxfile\.py|Pipfile|Pipfile\.lock|uv\.lock)$' || true
echo
echo '== target source excerpt =='
nl -ba ci/util/usb_vendors.py | sed -n '50,90p'Repository: FastLED/FastLED
Length of output: 464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== pyproject.toml references to zstandard =='
rg -n -i '\bzstandard\b' pyproject.toml || true
echo
echo '== ci/util/usb_vendors.py excerpt =='
sed -n '1,120p' ci/util/usb_vendors.py | cat -nRepository: FastLED/FastLED
Length of output: 4647
Declare zstandard or make the fallback explicit. ci/util/usb_vendors.py imports it lazily, but it isn’t listed in pyproject.toml, so fresh installs will silently drop to {} and lose USB vendor resolution.
🤖 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 `@ci/util/usb_vendors.py` around lines 60 - 75, The lazy import in
usb_vendors.py can hide a missing dependency, causing fresh installs to silently
fall back to an empty vendor map. Make zstandard an explicit dependency in
pyproject.toml or change the decompress path in the usb_vendors helper to
clearly treat zstandard as optional and log/handle the fallback intentionally,
keeping the behavior around _MAX_DECOMPRESSED_BYTES and the usb-vendors.txt
extraction unchanged.
Source: Coding guidelines
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.
Problem
bash autoresearch teensy41auto-detected the wrong port — an ESP32's CP2102 on COM11 instead of the Teensy on COM20.ENVIRONMENT_TO_VCOM_VID_PIDShad noteensy*entry, so selection fell through to the generic USB heuristic that prefers CP2102/CH340/FTDI adapters.Fix
16C0:0483/16C0:0489) forteensy40/41/lc/31/35/36/mm.1FC9:0132—16C0:0483is dropped there because on Windows the legacy LPCXpresso-VCOM firmware is byte-identical to a Teensy (same VID:PID, generic "USB Serial Device"), so keeping it madelpc*grab a co-attached Teensy.ci/util/usb_vendors.py: lazily decompresses theusb-vendors.tar.zstVID→vendor table into memory on first use (mirrors fbuild's embedded resolver) to annotate port diagnostics — e.g.COM11=10C4:EA60 (Silicon Labs).Verified
teensy41 → COM20,lpc845 → 1FC9:0132against live attached ports;ci/tests/test_usb_vendors.py(3) pass; ruff + KBI clean.Companion to FastLED/fbuild#982 (fbuild embeds the same VID:PID resolution from the FastLED/boards pipeline) and FastLED/fbuild#962 (Teensy shown in
fbuild port scan).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes