Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions .github/workflows/dylint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,13 @@ jobs:
*"@"*) continue ;;
esac
alias_path="$lib_dir/${stem}@${toolchain}.${ext}"
if [ ! -e "$alias_path" ]; then
cp "$lib" "$alias_path"
# Refresh a stale alias too: cargo rebuilds the bare
# library when a lint source/allowlist changes, but a
# cache-restored `@<toolchain>` copy is what cargo-dylint
# actually loads — never overwriting it pins every lint
# to its first-ever compiled allowlist.
if [ ! -e "$alias_path" ] || [ "$lib" -nt "$alias_path" ]; then
cp -f "$lib" "$alias_path"
created_any=1
fi
done
Expand Down
3 changes: 1 addition & 2 deletions crates/fbuild-config/src/board/loaders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,7 @@ impl BoardConfig {
name: get("name", board_id),
mcu: get("mcu", "unknown"),
f_cpu: get("f_cpu", "16000000L"),
clock_source: Some(get("clock_source", ""))
.filter(|source| !source.is_empty()),
clock_source: Some(get("clock_source", "")).filter(|source| !source.is_empty()),
board: get("board", &board_id_to_board_define(board_id)),
core: get("core", "arduino"),
variant: get("variant", "standard"),
Expand Down
206 changes: 159 additions & 47 deletions crates/fbuild-daemon/src/handlers/operations/deploy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -803,21 +803,23 @@ pub async fn deploy(
baud_override,
no_probe_rs,
),
fbuild_core::Platform::Ch32v => {
match req.protocol.as_deref() {
Some("isp") => {
let mcu = board.as_ref().map(|b| b.mcu.as_str()).unwrap_or_default();
if !fbuild_deploy::wchisp::supports_mcu(mcu) {
return Err(fbuild_core::FbuildError::DeployFailed(format!(
"no ISP bootloader on {mcu}; use a WCH-LinkE probe"
)));
}
Box::new(fbuild_deploy::wchisp::WchispDeployer::new())
fbuild_core::Platform::Ch32v => match req.protocol.as_deref() {
Some("isp") => {
let mcu = board.as_ref().map(|b| b.mcu.as_str()).unwrap_or_default();
if !fbuild_deploy::wchisp::supports_mcu(mcu) {
return Err(fbuild_core::FbuildError::DeployFailed(format!(
"no ISP bootloader on {mcu}; use a WCH-LinkE probe"
)));
}
Some("wlink") | None => Box::new(fbuild_deploy::wlink::WlinkDeployer::new()),
Some(protocol) => return Err(fbuild_core::FbuildError::DeployFailed(format!("unsupported CH32V deploy protocol: {protocol}"))),
Box::new(fbuild_deploy::wchisp::WchispDeployer::new())
}
}
Some("wlink") | None => Box::new(fbuild_deploy::wlink::WlinkDeployer::new()),
Some(protocol) => {
return Err(fbuild_core::FbuildError::DeployFailed(format!(
"unsupported CH32V deploy protocol: {protocol}"
)));
}
},
_ => {
return Err(fbuild_core::FbuildError::DeployFailed(format!(
"deployer for {:?} not yet implemented",
Expand Down Expand Up @@ -892,12 +894,23 @@ pub async fn deploy(
&deploy_result,
Ok(r) if r.success && matches!(r.outcome, fbuild_deploy::DeployOutcome::VerifySkip)
);
let recovery_port = deploy_port_str.clone().or_else(|| {
// FastLED/fbuild#1147: when the deployer owns post-flash port discovery
// (RP2040), `DeploymentResult.port` is the only health-gated,
// open-verified endpoint. The historical `requested.or(result.port)`
// order re-blessed a stale pre-flash COM name (e.g. a retained
// CM_PROB_PHANTOM devnode whose serial still matches the board) whenever
// the runtime CDC failed to recover.
let port_discovery_owned = deployer_for_recovery
.as_ref()
.is_some_and(|deployer| deployer.owns_post_flash_port_discovery());
let recovery_port = resolve_recovery_port(
port_discovery_owned,
deploy_port_str.clone(),
deploy_result
.as_ref()
.ok()
.and_then(|result| result.port.clone())
});
.and_then(|result| result.port.clone()),
);
if let Some(ref p) = recovery_port {
ctx.serial_manager.clear_preemption(p).await;
Comment on lines +903 to 915

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -nP --type=rust -C4 '(fn\s+clear_preemption|fn\s+preempt_for_deploy|last_disconnect|remove.*preempt)' crates/fbuild-daemon/src

Repository: FastLED/fbuild

Length of output: 12569


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== deploy.rs relevant sections =="
sed -n '400,470p' crates/fbuild-daemon/src/handlers/operations/deploy.rs
sed -n '850,930p' crates/fbuild-daemon/src/handlers/operations/deploy.rs
sed -n '1210,1265p' crates/fbuild-daemon/src/handlers/operations/deploy.rs

echo "== serial_manager references in deploy and manager =="
rg -n --type=rust -C3 'serial_manager|preempt|clear_preemption|preemption' crates/fbuild-daemon/src/handlers/operations/deploy.rs crates/fbuild-daemon/src h 2>/dev/null || true

echo "== locate serial_manager type definition/mocks/tests =="
rg -n --type=rust -C3 'struct .*Serial|serial_manager|preempt_for_deploy|clear_preemption|MonitorLease|preemption' crates/fbuild-daemon/src

Repository: FastLED/fbuild

Length of output: 50370


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== shared serial manager files =="
fd -i 'serial' crates/fbuild-daemon/src -t f

echo "== serial manager outline and relevant preemption API =="
for f in $(fd -i 'serial' crates/fbuild-daemon/src -t f); do
  echo "--- $f"
  rg -n 'pub struct |mod |impl .*Serial|clear_preemption|preempt_for_deploy|has_clients|notify_port_renumbered|rebind_port_session' "$f" -C4
done

echo "== focused read files =="
sed -n '1,360p' crates/fbuild-daemon/src/handlers/operations/monitor.rs
sed -n '700,950p' crates/fbuild-daemon/src/handlers/operations/deploy.rs
sed -n '1220,1260p' crates/fbuild-daemon/src/handlers/operations/deploy.rs

Repository: FastLED/fbuild

Length of output: 26436


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files that define SharedSerialManager =="
rg -n --type=rust 'SharedSerialManager|impl .*SharedSerialManager|struct SharedSerialManager' crates -S

echo "== imports and usages with clearer boundary =="
rg -n --type=rust 'serial_manager|SerialManager|Monit' crates/fbuild-daemon/src/context.rs crates/fbuild-daemon/src/handlers/operations/deploy.rs crates/fbuild-daemon/src/handlers/operations/reset.rs crates/fbuild-daemon/src/handlers/operations/monitor.rs crates/fbuild-daemon/src/lib.rs crates/fbuild-daemon/src/main.rs -S

echo "== repository-wide serial manager exact matches =="
rg -n 'struct .*SerialManager|enum .*SerialManager|impl .*SerialManager|clear_preemption|preempt_for_deploy|notify_port_renumbered' crates -S

Repository: FastLED/fbuild

Length of output: 12021


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== manager preemption, client tracking, and renumber APIs =="
sed -n '50,140p' crates/fbuild-serial/src/manager.rs
sed -n '440,520p' crates/fbuild-serial/src/manager.rs
sed -n '640,730p' crates/fbuild-serial/src/manager.rs
sed -n '840,945p' crates/fbuild-serial/src/manager.rs
sed -n '1020,1105p' crates/fbuild-serial/src/manager.rs

echo "== monitor attach/detach/close preemption handling =="
rg -n --type=rust -C5 'attach_reader|detach_reader|release_writer|close_port_after_grace_if_idle|has_clients' crates/fbuild-serial/src/manager.rs

echo "== local behavioral verifier for preemption mapping logic =="
python3 - <<'PY'
requested = "COM12"
deployer_port = "COM27"
old_recovered = requested or deployer_port
new_recovered = deployer_port
preempted = requested
cleared_by_new = set(p for p in [deployer_port] if p)
print("preempted:", preempted)
print("ownership: requested", requested, "deployer_port", deployer_port)
print("old_recovery_port:", old_recovered)
print("new_recovery_port:", new_recovered)
print("cleared_preempted", preempted in cleared_by_new)
PY

Repository: FastLED/fbuild

Length of output: 22357


Release the pre-flash preemption after owned post-flash recovery. Owned deployers resolve recovery_port to the new port, so a renumbered pre-flash endpoint like COM12 never has its preempt_for_deploy() lease cleared when clear_preemption only runs on the recovered COM27 port. Clear deploy_port_str before falling back to recovery-port clearing.

🔒 Proposed fix
     let recovery_port = resolve_recovery_port(
         port_discovery_owned,
         deploy_port_str.clone(),
         deploy_result
             .as_ref()
             .ok()
             .and_then(|result| result.port.clone()),
     );
+    // Always release the preemption taken on the pre-flash port,
+    // even when owned discovery moved recovery to a renumbered endpoint.
+    if let Some(ref pre) = deploy_port_str {
+        if recovery_port.as_deref() != Some(pre.as_str()) {
+            ctx.serial_manager.clear_preemption(pre).await;
+        }
+    }
     if let Some(ref p) = recovery_port {
         ctx.serial_manager.clear_preemption(p).await;
📝 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 port_discovery_owned = deployer_for_recovery
.as_ref()
.is_some_and(|deployer| deployer.owns_post_flash_port_discovery());
let recovery_port = resolve_recovery_port(
port_discovery_owned,
deploy_port_str.clone(),
deploy_result
.as_ref()
.ok()
.and_then(|result| result.port.clone())
});
.and_then(|result| result.port.clone()),
);
if let Some(ref p) = recovery_port {
ctx.serial_manager.clear_preemption(p).await;
let port_discovery_owned = deployer_for_recovery
.as_ref()
.is_some_and(|deployer| deployer.owns_post_flash_port_discovery());
let recovery_port = resolve_recovery_port(
port_discovery_owned,
deploy_port_str.clone(),
deploy_result
.as_ref()
.ok()
.and_then(|result| result.port.clone()),
);
// Always release the preemption taken on the pre-flash port,
// even when owned discovery moved recovery to a renumbered endpoint.
if let Some(ref pre) = deploy_port_str {
if recovery_port.as_deref() != Some(pre.as_str()) {
ctx.serial_manager.clear_preemption(pre).await;
}
}
if let Some(ref p) = recovery_port {
ctx.serial_manager.clear_preemption(p).await;
🤖 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-daemon/src/handlers/operations/deploy.rs` around lines 901 -
913, Update the recovery cleanup around resolve_recovery_port and
clear_preemption so deploy_port_str is cleared first when present, before
falling back to clearing recovery_port. Preserve the existing recovery-port
behavior for cases without a deploy port, ensuring owned post-flash recovery
releases the original pre-flash preemption even when the port is renumbered.

if !deploy_skipped_bus_work {
Expand Down Expand Up @@ -933,42 +946,63 @@ pub async fn deploy(
}
}

let (deploy_success, deploy_stdout, mut deploy_stderr, deploy_outcome, deploy_post_port) =
match deploy_result {
Ok(r) if r.success => (true, Some(r.stdout), Some(r.stderr), r.outcome, r.port),
Ok(r) => {
return (
StatusCode::INTERNAL_SERVER_ERROR,
Json(OperationResponse {
success: false,
request_id,
message: r.message,
exit_code: 1,
output_file: Some(reported_output_file.clone()),
output_dir: reported_output_dir.clone(),
launch_url: None,
stdout: Some(r.stdout),
stderr: Some(r.stderr),
}),
);
}
Err(e) => {
return (
StatusCode::INTERNAL_SERVER_ERROR,
Json(deploy_error_response(request_id, &e)),
);
}
};
let (
deploy_success,
deploy_stdout,
mut deploy_stderr,
deploy_outcome,
deploy_post_port,
deploy_message,
) = match deploy_result {
Ok(r) if r.success => (
true,
Some(r.stdout),
Some(r.stderr),
r.outcome,
r.port,
r.message,
),
Ok(r) => {
return (
StatusCode::INTERNAL_SERVER_ERROR,
Json(OperationResponse {
success: false,
request_id,
message: r.message,
exit_code: 1,
output_file: Some(reported_output_file.clone()),
output_dir: reported_output_dir.clone(),
launch_url: None,
stdout: Some(r.stdout),
stderr: Some(r.stderr),
}),
);
}
Err(e) => {
return (
StatusCode::INTERNAL_SERVER_ERROR,
Json(deploy_error_response(request_id, &e)),
);
}
};
append_warning_to_stderr(&mut deploy_stderr, deploy_port_warning);
// Build the "deploy succeeded (...)" prefix used by every
// monitor-attached and non-monitor-attached response below. Stable
// wording — see GitHub issue #76 and the DeployOutcome::describe
// test in fbuild-deploy.
let deploy_prefix = format!(
let mut deploy_prefix = format!(
"deploy succeeded ({}); FBUILD_DEPLOY_PORT={}",
deploy_outcome.describe(),
deploy_post_port.as_deref().unwrap_or("")
);
// FastLED/fbuild#1147: a port-discovery-owning deployer distinguishes
// "firmware transferred" from "runtime CDC recovered". When the flash
// succeeded but no healthy endpoint returned, its `message` carries the
// structured recovery diagnostic, which the success arms previously
// discarded — surface it instead of a bare empty port.
if port_discovery_owned && deploy_post_port.is_none() {
deploy_prefix = format!("{deploy_prefix}; {deploy_message}");
}

// Post-deploy monitoring: if monitor_after is set, open the serial port
// and stream lines checking halt conditions (matching Python behavior).
Expand All @@ -977,10 +1011,39 @@ pub async fn deploy(
// Teensy state machine in #433 returns the freshly re-enumerated CDC
// ACM port, which can differ from the pre-flash `--port`). Fall back
// to the caller-supplied port, then to a platform default.
let monitor_port = deploy_post_port
.clone()
.or(deploy_port_str.clone())
.unwrap_or_else(|| "/dev/ttyUSB0".to_string());
// FastLED/fbuild#1147: when the deployer owns post-flash port
// discovery, an absent recovered port means there is nothing safe to
// monitor — attaching to the stale pre-flash name (or a guessed
// default) would target a known-dead endpoint.
let monitor_port = if port_discovery_owned {
match deploy_post_port.clone() {
Some(port) => port,
None => {
return (
StatusCode::OK,
Json(OperationResponse {
success: true,
request_id,
message: format!(
"{} but monitor was skipped: no healthy runtime CDC endpoint was recovered after the flash",
deploy_prefix
),
exit_code: 0,
output_file: Some(reported_output_file.clone()),
output_dir: reported_output_dir.clone(),
launch_url: None,
stdout: deploy_stdout,
stderr: deploy_stderr,
}),
);
}
}
} else {
deploy_post_port
.clone()
.or(deploy_port_str.clone())
.unwrap_or_else(|| "/dev/ttyUSB0".to_string())
};
if deploy_post_port
.as_deref()
.zip(deploy_port_str.as_deref())
Expand Down Expand Up @@ -1168,6 +1231,25 @@ pub async fn deploy(
)
}

/// Post-deploy recovery/monitor port choice (FastLED/fbuild#1147).
///
/// A deployer that owns post-flash port discovery returns the only endpoint
/// that passed a fresh health + openability gate; the requested pre-flash
/// name must never be substituted when that endpoint is absent, because on
/// Windows it can be a retained `CM_PROB_PHANTOM` devnode record. Legacy
/// deployers keep the historical requested-first order.
fn resolve_recovery_port(
port_discovery_owned: bool,
requested: Option<String>,
deployer_port: Option<String>,
) -> Option<String> {
if port_discovery_owned {
deployer_port
} else {
requested.or(deployer_port)
}
}

fn deploy_error_response(
request_id: String,
error: &fbuild_core::FbuildError,
Expand Down Expand Up @@ -1205,4 +1287,34 @@ mod tests {
);
assert_eq!(response.stderr.as_deref(), Some(response.message.as_str()));
}

#[test]
fn owned_port_discovery_never_substitutes_the_requested_preflash_port() {
// FastLED/fbuild#1147: UF2 success + phantom CDC must not resurrect
// the historical COM name for recovery/monitor propagation.
assert_eq!(
resolve_recovery_port(true, Some("COM12".to_string()), None),
None
);
assert_eq!(
resolve_recovery_port(true, Some("COM12".to_string()), Some("COM27".to_string())),
Some("COM27".to_string())
);
}

#[test]
fn legacy_deployers_keep_requested_first_recovery_order() {
assert_eq!(
resolve_recovery_port(false, Some("COM3".to_string()), None),
Some("COM3".to_string())
);
assert_eq!(
resolve_recovery_port(false, Some("COM3".to_string()), Some("COM4".to_string())),
Some("COM3".to_string())
);
assert_eq!(
resolve_recovery_port(false, None, Some("COM4".to_string())),
Some("COM4".to_string())
);
}
}
Loading
Loading