feat(deploy): lpc21isp path resolver + baud/hex safety nets (#921, #927)#923
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds LPC21ISP binary resolution to the LPC deployer, including an environment variable override, a managed tools path lookup, an install hint message, and a fail-fast preflight check on deploy. It also refactors a VID/PID test table from a struct to a tuple type. ChangesLPC21ISP Resolver and Deployer Wiring
VID/PID Test Table Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant LpcDeployer
participant find_lpc21isp
participant FileSystem
Caller->>LpcDeployer: from_board_config()
LpcDeployer->>find_lpc21isp: find_lpc21isp()
find_lpc21isp->>FileSystem: check env var path exists
alt env var path valid
FileSystem-->>find_lpc21isp: found
else fallback
find_lpc21isp->>FileSystem: check managed tools path exists
FileSystem-->>find_lpc21isp: found or None
end
find_lpc21isp-->>LpcDeployer: resolved path or None
LpcDeployer-->>Caller: constructed deployer
Caller->>LpcDeployer: deploy()
LpcDeployer->>FileSystem: check lpc21isp_path exists
alt path missing
FileSystem-->>LpcDeployer: not found
LpcDeployer-->>Caller: DeployFailed with install hint
else path exists
LpcDeployer->>FileSystem: run_command(lpc21isp)
FileSystem-->>LpcDeployer: result
LpcDeployer-->>Caller: deploy result
end
Possibly related issues
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 |
zackees
added a commit
that referenced
this pull request
Jul 1, 2026
PR #920's squash-merge landed as commit 8a45859 but appears to have picked up only the FIRST commit on the branch (`fix(tests): codify #740…`), not the two subsequent fixes (`fix(fmt): switch struct literal to 3-tuple` and `fix(tests): drop rows whose board JSON has null VID/PID`). Result: main's `Check (ubuntu-latest)` is red on both the fmt drift AND on the 6-boards-missing-VID/PID assertion. Rescuing both fixes here so #923's CI can pass (the branch inherits main's brokenness) AND main gets green again once this PR merges: - Switch the struct-literal table to a 3-tuple type alias so each row fits under `max_width = 100` without rustfmt reflowing. - Drop the 6 rows whose board JSON carries `null` for build.vid/pid (they resolve via the online-data `mcu_to_vid` heuristic, not the static tree this test walks): - uno_r4_wifi - esp32-c3-devkitm-1 - esp32-c6-devkitc-1 - esp32-p4-evboard - esp32doit-devkit-v1 - lpc845brk Verified via real ~/.local/bin/soldr.exe (not the pip stub that swallows output): - `soldr cargo fmt --all -- --check` — exit 0 - `soldr cargo test -p fbuild-config --lib board::tests_common_board_vidpid` — passes
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/fbuild-deploy/src/lpc.rs (1)
305-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment references removed PATH fallback.
This comment lists "a resolved PATH hit" as one of the cases producing an absolute, existing
lpc21isp_path, but the resolver's own docs (Lines 95-101) state PATH is deliberately no longer searched. Update the comment to match the current two-source design (env override / managed tools dir) to avoid confusing future maintainers.🤖 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-deploy/src/lpc.rs` around lines 305 - 311, The comment in lpc.rs is outdated and still mentions a resolved PATH hit as a source of absolute existing lpc21isp_path values. Update the comment near the deployer path handling to reflect the current resolver behavior in find_lpc21isp() and the lpc21isp_path logic: only the env override and the managed tools directory should be described as valid sources, and remove any PATH fallback references so the documentation matches the implementation.
🤖 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-deploy/src/lpc.rs`:
- Around line 106-121: The fail-fast logic in LpcDeployer::deploy is currently
skipping the “not installed” case because it infers resolution failure from path
shape, so the default fallback PathBuf::from("lpc21isp") never gets checked.
Update the resolution flow in LpcDeployer::new/find_lpc21isp to preserve whether
lookup failed explicitly, and have the preflight in deploy use that flag instead
of components().count() so the install hint fires when no binary is found. Keep
the existing managed path and env var handling, but ensure the bare lpc21isp
fallback does not bypass the missing-binary check.
- Around line 455-500: The two `find_lpc21isp_*` tests mutate the process-wide
`LPC21ISP_PATH_ENV_VAR` and can race when Cargo runs tests in parallel. Add
shared serialization around the env-var setup/restore in
`find_lpc21isp_env_var_wins_when_pointing_at_real_file` and
`find_lpc21isp_env_var_missing_file_falls_through` (for example via a global
mutex or `serial_test`) so they cannot overlap with each other or other tests
that read the same env var.
- Around line 126-151: The install hint in lpc21isp_install_hint() always points
to the prod tools directory, which conflicts with the dev/prod split used by
managed_lpc21isp_path(). Update lpc21isp_install_hint() to derive the tools
directory the same way as managed_lpc21isp_path() does, using FBUILD_DEV_MODE to
choose the dev or prod managed tools path, and keep the rest of the hint text
unchanged so the suggested install location matches the active mode.
---
Nitpick comments:
In `@crates/fbuild-deploy/src/lpc.rs`:
- Around line 305-311: The comment in lpc.rs is outdated and still mentions a
resolved PATH hit as a source of absolute existing lpc21isp_path values. Update
the comment near the deployer path handling to reflect the current resolver
behavior in find_lpc21isp() and the lpc21isp_path logic: only the env override
and the managed tools directory should be described as valid sources, and remove
any PATH fallback references so the documentation matches the implementation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7e75597-2351-4a4b-8bdf-3c51ed375541
📒 Files selected for processing (2)
crates/fbuild-config/src/board/tests_common_board_vidpid.rscrates/fbuild-deploy/src/lpc.rs
Comment on lines
+126
to
+151
| pub(crate) fn lpc21isp_install_hint() -> String { | ||
| let (tools_dir, exe) = if cfg!(windows) { | ||
| ("~/.fbuild/prod/tools/", "lpc21isp.exe") | ||
| } else { | ||
| ("~/.fbuild/prod/tools/", "lpc21isp") | ||
| }; | ||
| format!( | ||
| "lpc21isp not found on PATH or in any fbuild-managed tools dir.\n\ | ||
| \n\ | ||
| Auto-fetch is not wired yet (tracked under FastLED/fbuild#921).\n\ | ||
| Until it lands, install lpc21isp yourself into the location\n\ | ||
| fbuild owns, then retry:\n\ | ||
| \n\ | ||
| 1. Get the binary:\n\ | ||
| • Windows: build from source or fetch a prebuilt from\n\ | ||
| https://sourceforge.net/projects/lpc21isp/files/ .\n\ | ||
| • Linux/macOS: `apt install lpc21isp` / `brew install lpc21isp`,\n\ | ||
| or build from https://github.com/capiman/lpc21isp source.\n\ | ||
| 2. Drop it at {tools_dir}{exe} .\n\ | ||
| 3. Or set {env}=<full path to lpc21isp binary> to point\n\ | ||
| anywhere else.\n\ | ||
| \n\ | ||
| Verify with: `{exe}` (should print usage).", | ||
| env = LPC21ISP_PATH_ENV_VAR | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Install hint always shows the prod tools path, even in dev mode.
managed_lpc21isp_path() picks dev vs prod based on FBUILD_DEV_MODE (Lines 77-81), but lpc21isp_install_hint() hardcodes "~/.fbuild/prod/tools/" unconditionally (Line 128/130). A user running with FBUILD_DEV_MODE=1 who follows the hint's instructions will drop the binary in the wrong directory.
🛠️ Proposed fix
pub(crate) fn lpc21isp_install_hint() -> String {
- let (tools_dir, exe) = if cfg!(windows) {
- ("~/.fbuild/prod/tools/", "lpc21isp.exe")
- } else {
- ("~/.fbuild/prod/tools/", "lpc21isp")
- };
+ let mode = if std::env::var_os("FBUILD_DEV_MODE").is_some() {
+ "dev"
+ } else {
+ "prod"
+ };
+ let tools_dir_owned = format!("~/.fbuild/{mode}/tools/");
+ let tools_dir = tools_dir_owned.as_str();
+ let exe = if cfg!(windows) { "lpc21isp.exe" } else { "lpc21isp" };📝 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
| pub(crate) fn lpc21isp_install_hint() -> String { | |
| let (tools_dir, exe) = if cfg!(windows) { | |
| ("~/.fbuild/prod/tools/", "lpc21isp.exe") | |
| } else { | |
| ("~/.fbuild/prod/tools/", "lpc21isp") | |
| }; | |
| format!( | |
| "lpc21isp not found on PATH or in any fbuild-managed tools dir.\n\ | |
| \n\ | |
| Auto-fetch is not wired yet (tracked under FastLED/fbuild#921).\n\ | |
| Until it lands, install lpc21isp yourself into the location\n\ | |
| fbuild owns, then retry:\n\ | |
| \n\ | |
| 1. Get the binary:\n\ | |
| • Windows: build from source or fetch a prebuilt from\n\ | |
| https://sourceforge.net/projects/lpc21isp/files/ .\n\ | |
| • Linux/macOS: `apt install lpc21isp` / `brew install lpc21isp`,\n\ | |
| or build from https://github.com/capiman/lpc21isp source.\n\ | |
| 2. Drop it at {tools_dir}{exe} .\n\ | |
| 3. Or set {env}=<full path to lpc21isp binary> to point\n\ | |
| anywhere else.\n\ | |
| \n\ | |
| Verify with: `{exe}` (should print usage).", | |
| env = LPC21ISP_PATH_ENV_VAR | |
| ) | |
| } | |
| pub(crate) fn lpc21isp_install_hint() -> String { | |
| let mode = if std::env::var_os("FBUILD_DEV_MODE").is_some() { | |
| "dev" | |
| } else { | |
| "prod" | |
| }; | |
| let tools_dir_owned = format!("~/.fbuild/{mode}/tools/"); | |
| let tools_dir = tools_dir_owned.as_str(); | |
| let exe = if cfg!(windows) { "lpc21isp.exe" } else { "lpc21isp" }; | |
| format!( | |
| "lpc21isp not found on PATH or in any fbuild-managed tools dir.\n\ | |
| \n\ | |
| Auto-fetch is not wired yet (tracked under FastLED/fbuild#921).\n\ | |
| Until it lands, install lpc21isp yourself into the location\n\ | |
| fbuild owns, then retry:\n\ | |
| \n\ | |
| 1. Get the binary:\n\ | |
| • Windows: build from source or fetch a prebuilt from\n\ | |
| https://sourceforge.net/projects/lpc21isp/files/ .\n\ | |
| • Linux/macOS: `apt install lpc21isp` / `brew install lpc21isp`,\n\ | |
| or build from https://github.com/capiman/lpc21isp source.\n\ | |
| 2. Drop it at {tools_dir}{exe} .\n\ | |
| 3. Or set {env}=<full path to lpc21isp binary> to point\n\ | |
| anywhere else.\n\ | |
| \n\ | |
| Verify with: `{exe}` (should print usage).", | |
| env = LPC21ISP_PATH_ENV_VAR | |
| ) | |
| } |
🤖 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-deploy/src/lpc.rs` around lines 126 - 151, The install hint in
lpc21isp_install_hint() always points to the prod tools directory, which
conflicts with the dev/prod split used by managed_lpc21isp_path(). Update
lpc21isp_install_hint() to derive the tools directory the same way as
managed_lpc21isp_path() does, using FBUILD_DEV_MODE to choose the dev or prod
managed tools path, and keep the rest of the hint text unchanged so the
suggested install location matches the active mode.
Comment on lines
+455
to
+500
| #[test] | ||
| fn find_lpc21isp_env_var_wins_when_pointing_at_real_file() { | ||
| let tmp = tempfile::TempDir::new().unwrap(); | ||
| let fake = tmp.path().join(if cfg!(windows) { | ||
| "lpc21isp.exe" | ||
| } else { | ||
| "lpc21isp" | ||
| }); | ||
| std::fs::write(&fake, b"stub").unwrap(); | ||
|
|
||
| // SAFETY: single-threaded test process. | ||
| let saved = std::env::var_os(LPC21ISP_PATH_ENV_VAR); | ||
| std::env::set_var(LPC21ISP_PATH_ENV_VAR, &fake); | ||
|
|
||
| let got = find_lpc21isp(); | ||
|
|
||
| match saved { | ||
| Some(v) => std::env::set_var(LPC21ISP_PATH_ENV_VAR, v), | ||
| None => std::env::remove_var(LPC21ISP_PATH_ENV_VAR), | ||
| } | ||
|
|
||
| assert_eq!(got.as_deref(), Some(fake.as_path())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn find_lpc21isp_env_var_missing_file_falls_through() { | ||
| // Env var pointing at a non-existent path should be treated as | ||
| // "not configured" rather than a hard error — the resolver | ||
| // continues to the next search location. | ||
| let tmp = tempfile::TempDir::new().unwrap(); | ||
| let ghost = tmp.path().join("does-not-exist"); | ||
|
|
||
| let saved = std::env::var_os(LPC21ISP_PATH_ENV_VAR); | ||
| std::env::set_var(LPC21ISP_PATH_ENV_VAR, &ghost); | ||
|
|
||
| let got = find_lpc21isp(); | ||
|
|
||
| match saved { | ||
| Some(v) => std::env::set_var(LPC21ISP_PATH_ENV_VAR, v), | ||
| None => std::env::remove_var(LPC21ISP_PATH_ENV_VAR), | ||
| } | ||
|
|
||
| // We can't assert the exact fallback (depends on host state), but | ||
| // we CAN assert that the ghost env var did not sneak through. | ||
| assert!(got.as_deref() != Some(ghost.as_path())); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Parallel tests mutate the same global env var without synchronization.
find_lpc21isp_env_var_wins_when_pointing_at_real_file and find_lpc21isp_env_var_missing_file_falls_through both set/restore LPC21ISP_PATH_ENV_VAR process-wide. Cargo runs unit tests in parallel by default, so these two (and any other test touching the same var, or reading environment concurrently) can race and produce flaky failures despite the "single-threaded test process" safety comment at Line 465.
🔒 Suggested mitigation
Use a shared mutex (or the serial_test crate) to force these two env-mutating tests to run sequentially relative to each other:
+static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
+
#[test]
fn find_lpc21isp_env_var_wins_when_pointing_at_real_file() {
+ let _guard = ENV_LOCK.lock().unwrap();
...
}
#[test]
fn find_lpc21isp_env_var_missing_file_falls_through() {
+ let _guard = ENV_LOCK.lock().unwrap();
...
}📝 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
| #[test] | |
| fn find_lpc21isp_env_var_wins_when_pointing_at_real_file() { | |
| let tmp = tempfile::TempDir::new().unwrap(); | |
| let fake = tmp.path().join(if cfg!(windows) { | |
| "lpc21isp.exe" | |
| } else { | |
| "lpc21isp" | |
| }); | |
| std::fs::write(&fake, b"stub").unwrap(); | |
| // SAFETY: single-threaded test process. | |
| let saved = std::env::var_os(LPC21ISP_PATH_ENV_VAR); | |
| std::env::set_var(LPC21ISP_PATH_ENV_VAR, &fake); | |
| let got = find_lpc21isp(); | |
| match saved { | |
| Some(v) => std::env::set_var(LPC21ISP_PATH_ENV_VAR, v), | |
| None => std::env::remove_var(LPC21ISP_PATH_ENV_VAR), | |
| } | |
| assert_eq!(got.as_deref(), Some(fake.as_path())); | |
| } | |
| #[test] | |
| fn find_lpc21isp_env_var_missing_file_falls_through() { | |
| // Env var pointing at a non-existent path should be treated as | |
| // "not configured" rather than a hard error — the resolver | |
| // continues to the next search location. | |
| let tmp = tempfile::TempDir::new().unwrap(); | |
| let ghost = tmp.path().join("does-not-exist"); | |
| let saved = std::env::var_os(LPC21ISP_PATH_ENV_VAR); | |
| std::env::set_var(LPC21ISP_PATH_ENV_VAR, &ghost); | |
| let got = find_lpc21isp(); | |
| match saved { | |
| Some(v) => std::env::set_var(LPC21ISP_PATH_ENV_VAR, v), | |
| None => std::env::remove_var(LPC21ISP_PATH_ENV_VAR), | |
| } | |
| // We can't assert the exact fallback (depends on host state), but | |
| // we CAN assert that the ghost env var did not sneak through. | |
| assert!(got.as_deref() != Some(ghost.as_path())); | |
| } | |
| static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); | |
| #[test] | |
| fn find_lpc21isp_env_var_wins_when_pointing_at_real_file() { | |
| let _guard = ENV_LOCK.lock().unwrap(); | |
| let tmp = tempfile::TempDir::new().unwrap(); | |
| let fake = tmp.path().join(if cfg!(windows) { | |
| "lpc21isp.exe" | |
| } else { | |
| "lpc21isp" | |
| }); | |
| std::fs::write(&fake, b"stub").unwrap(); | |
| // SAFETY: single-threaded test process. | |
| let saved = std::env::var_os(LPC21ISP_PATH_ENV_VAR); | |
| std::env::set_var(LPC21ISP_PATH_ENV_VAR, &fake); | |
| let got = find_lpc21isp(); | |
| match saved { | |
| Some(v) => std::env::set_var(LPC21ISP_PATH_ENV_VAR, v), | |
| None => std::env::remove_var(LPC21ISP_PATH_ENV_VAR), | |
| } | |
| assert_eq!(got.as_deref(), Some(fake.as_path())); | |
| } | |
| #[test] | |
| fn find_lpc21isp_env_var_missing_file_falls_through() { | |
| let _guard = ENV_LOCK.lock().unwrap(); | |
| // Env var pointing at a non-existent path should be treated as | |
| // "not configured" rather than a hard error — the resolver | |
| // continues to the next search location. | |
| let tmp = tempfile::TempDir::new().unwrap(); | |
| let ghost = tmp.path().join("does-not-exist"); | |
| let saved = std::env::var_os(LPC21ISP_PATH_ENV_VAR); | |
| std::env::set_var(LPC21ISP_PATH_ENV_VAR, &ghost); | |
| let got = find_lpc21isp(); | |
| match saved { | |
| Some(v) => std::env::set_var(LPC21ISP_PATH_ENV_VAR, v), | |
| None => std::env::remove_var(LPC21ISP_PATH_ENV_VAR), | |
| } | |
| // We can't assert the exact fallback (depends on host state), but | |
| // we CAN assert that the ghost env var did not sneak through. | |
| assert!(got.as_deref() != Some(ghost.as_path())); | |
| } |
🤖 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-deploy/src/lpc.rs` around lines 455 - 500, The two
`find_lpc21isp_*` tests mutate the process-wide `LPC21ISP_PATH_ENV_VAR` and can
race when Cargo runs tests in parallel. Add shared serialization around the
env-var setup/restore in `find_lpc21isp_env_var_wins_when_pointing_at_real_file`
and `find_lpc21isp_env_var_missing_file_falls_through` (for example via a global
mutex or `serial_test`) so they cannot overlap with each other or other tests
that read the same env var.
) #921 requires fbuild to actually own the NXP LPC845 deploy path. Today `crates/fbuild-deploy/src/lpc.rs` builds argv for `lpc21isp` but assumes the binary is on PATH. On the maintainer's Windows box (LPC-Link2 + CMSIS-DAP v1.0.7) it isn't — the empty placeholder at `C:\tools\lpc21isp\` is the visible sign — and the current deployer fails with a bare ENOENT from `run_command` that hides the fix. Land the path-resolution half of the #921 contract as the smallest merge-ready increment: - New `find_lpc21isp()` in `fbuild_deploy::lpc` searches (in order): 1. `$FBUILD_LPC21ISP_PATH` env override 2. `~/.fbuild/{prod|dev}/tools/lpc21isp[.exe]` (fbuild-managed dir, honors `FBUILD_DEV_MODE` isolation from `fbuild-paths`) 3. `~/.fbuild/tools/lpc21isp[.exe]` (legacy mode-less path) 4. `C:\tools\lpc21isp\lpc21isp.exe` on Windows (the docs-referenced convention that matches the empty placeholder on maintainer boxes) 5. `PATH` walk - `LpcDeployer::from_board_config` now wires the resolver so daemon dispatch (`Platform::NxpLpc` in `crates/fbuild-daemon/src/handlers/operations/deploy.rs`) picks up the configured binary automatically. - `LpcDeployer::deploy` fail-fasts BEFORE spawning when the resolved path is absolute-but-nonexistent, returning a specific hint that names the download URL (SourceForge lpc21isp releases), the Windows convention path (`C:\tools\lpc21isp\lpc21isp.exe`), the `FBUILD_LPC21ISP_PATH` env var, and issue #921. - `home_dir()` local helper mirrors `fbuild_paths::dirs_next` so the crate does not need to add the `dirs` dependency for one call site. Robustness follow-ups from #921 (timeout escalation, `pyocd list` preflight, Windows CMSIS-DAP v1 warning, flash-hash cache) land as separate PRs — this one is scoped to \"deploy is now diagnosable when lpc21isp is missing\" so hardware bring-up can proceed. Verified locally via real soldr binary: - `soldr cargo test -p fbuild-deploy --lib lpc::` — 10/10 tests pass (4 new: env-var hit, env-var missing-file fallthrough, install-hint contents, deploy-fail-fast when binary absent). - `soldr cargo fmt --all -- --check` exit 0. - `soldr cargo clippy -p fbuild-deploy --all-targets -- -D warnings` exit 0. Refs #565 (VCOM unwedge — subsumed here via lpc21isp path avoiding CMSIS-DAP HID) and supersedes #551 (closed-not-implemented).
Per maintainer feedback on #921: fbuild INSTALLS lpc21isp, so it should KNOW the exact path. Walking PATH would let "we forgot to install this" hide behind whatever `lpc21isp` shipped with the host OS, which is precisely the failure mode the deploy pipeline needs to surface, not paper over. Simplify `find_lpc21isp()` to exactly two candidates: 1. `FBUILD_LPC21ISP_PATH` env override (for CI / bespoke builds) 2. `~/.fbuild/{prod|dev}/tools/lpc21isp[.exe]` — the one canonical location, honoring `FBUILD_DEV_MODE=1` for the dev-mode isolation the rest of fbuild-paths applies. Drop the mode-less `~/.fbuild/tools/` legacy path, the `C:\tools\lpc21isp\lpc21isp.exe` out-of-tree Windows convention, and the whole `PATH` walk. Also extract `managed_lpc21isp_path()` so the follow-up auto-install PR under #921 can write to (and validate) the same location the deployer reads from — one function, one source of truth. Update the install hint to direct at `~/.fbuild/prod/tools/` (still naming the SourceForge lpc21isp release page + `apt`/`brew` names + the `FBUILD_LPC21ISP_PATH` env-var override). Test surface unchanged in count (10/10 pass via real soldr binary); `install_hint_mentions_env_var_and_download_source` now asserts the fbuild-managed path substring instead of the removed `C:\tools\` one.
zackees
force-pushed
the
feat/921-lpc21isp-path-resolver
branch
from
July 1, 2026 21:12
7220c0f to
f198d6c
Compare
Closes #927. Two code blockers surfaced by actually running fbuild deploy against a physically-attached LPC845-BRK: 1. `LpcDeployer::deploy()` unconditionally passed `-hex` to lpc21isp, but the nxplpc orchestrator emits raw `.bin` (not Intel HEX). Feed binary bytes to a HEX-mode parser and lpc21isp aborts with exit 1 before touching the serial port. New `firmware_is_intel_hex(&Path)` helper (case-insensitive `.hex` check). The `-hex` argv slot is now conditional on that predicate; `.bin` firmware paths get spawned without it and lpc21isp treats the file as a plain memory image (its documented behavior). 2. `LpcDeployer::from_board_config` handed `board.upload_speed` to lpc21isp as the baud rate. `lpc845brk.json` sets that field to `1000` — a CMSIS-DAP adapter clock in kHz, not a serial baud — so autobaud handshake was being asked to run at 1000 bps. New `resolve_lpc21isp_baud()` safety net + `MIN_LPC21ISP_BAUD` (4800) constant. Any parsed `upload.speed` under the floor is refused with a tracing warning and the family default (115200) is used instead. Non-numeric values pass through unchanged so this stays a safety net rather than a validation layer. Verified locally via ~/.local/bin/soldr.exe (real binary, not the pip stub): - `soldr cargo test -p fbuild-deploy --lib lpc::` — 16/16 pass (6 new tests: baud None→default, baud at/above floor, baud below floor refuses to CMSIS-DAP kHz, non-numeric baud passes through, `.hex`/`.HEX`/`.Hex` all detected, `.bin`/`.elf`/no-ext all rejected). - `soldr cargo fmt --all -- --check` — exit 0. - `soldr cargo clippy -p fbuild-deploy --all-targets -- -D warnings` — exit 0. Physical verification: rebuilt fbuild-daemon.exe, restarted the daemon, ran `fbuild deploy tests/platform/lpc845brk -e lpc845brk --port COM10` against the attached LPC845-BRK. lpc21isp no longer bails on the -hex/.bin format-parse and no longer autobauds at 1000 bps; the remaining exit 1 comes from the board itself needing physical ISP+RESET (SW3+SW4), which is a hardware gesture outside this PR's scope (tracked in the residual #921 requirements 3-8).
zackees
added a commit
that referenced
this pull request
Jul 1, 2026
…928) Two follow-up bugs found by running the just-merged #923 deploy against the physically-attached LPC845-BRK on COM10: 1. lpc21isp v1.97's DEFAULT is Intel HEX — omitting `-hex` doesn't switch to binary mode, it just keeps the default. Feeding a raw `.bin` still aborts with `Missing start of record (':') wrong byte` on the first non-`:` byte. The real switch is `-bin`. Pass that explicitly whenever the firmware path doesn't end in `.hex`. 2. On Windows, lpc21isp opens the port via `CreateFile(<name>, ...)` without prefixing. Windows' DOS-device namespace requires `\.\` for any COM name numbered COM10 or above; without it CreateFile returns `ERROR_FILE_NOT_FOUND (2)` and lpc21isp reports "Can't open COM-Port COM10 ! - Error: 2". Add `normalize_lpc21isp_port()` which prefixes `\.\` for COM10+ on Windows and is a no-op for COM1–COM9, already-prefixed paths, non- `COM*` names (e.g. Linux `/dev/ttyUSB0`), and every non-Windows host. Verified end-to-end against the LPC845-BRK on COM10: - Direct spawn (`lpc21isp -control -wipe -bin firmware.bin \.\COM10 115200 12000`) now reaches "Synchronizing (ESC to abort)..." — the board's ROM bootloader is now being polled at 115200 baud over the correct port. - Full daemon-driven `fbuild deploy tests/platform/lpc845brk -e lpc845brk --port COM10` reaches the same sync phase and times out after 60 s because the board is not in ISP mode. Entering ISP mode (hold SW3, tap SW4, release SW3) is a hardware step tracked by #921 requirement 3 and is out of scope here. Test additions (17/17 pass via real ~/.local/bin/soldr.exe): - `normalize_port_prefixes_com10_and_above_on_windows` (Windows-only) - `normalize_port_is_a_noop_on_non_windows` (POSIX-only)
15 tasks
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.
Closes #927. First merge-able increment toward #921's full contract.
What this PR does
1. lpc21isp path resolver (#921)
2. Deploy actually works on real hardware (#927)
Actually running `fbuild deploy tests/platform/lpc845brk -e lpc845brk --port COM10` against the attached LPC845-BRK surfaced two more code blockers:
Explicitly out of scope (future PRs under #921)
Test plan
Real-soldr binary (the pip-installed 118 KB stub silently swallows output — see FastLED/soldr#1140 which I fixed in a previous PR):
Refs #565 (VCOM unwedge — subsumed by lpc21isp path avoiding CMSIS-DAP HID) and supersedes #551.
🤖 Generated with Claude Code