esp32: default to native espflash with esptool fallback#197
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR shifts native espflash features from opt-in to default-enabled with automatic fallback to esptool. It enables Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Deploy Handler
participant Deployer as Esp32Deployer
participant Native as Native Espflash
participant Esptool as Esptool Subprocess
participant Result as Deployment Result
Handler->>Deployer: deploy() / verify()
Deployer->>Native: Attempt native operation
alt Native succeeds
Native-->>Deployer: Success
Deployer-->>Handler: Return result
else Native fails or error
Native-->>Deployer: Error / Failure
Deployer->>Deployer: Log warning
Deployer->>Esptool: Fallback: run esptool command
Esptool-->>Deployer: Parse stdout/stderr
Deployer-->>Handler: Return esptool result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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: 1
🧹 Nitpick comments (2)
crates/fbuild-daemon/Cargo.toml (1)
17-22: LGTM — daemon default aligns with deploy crate.Forwarding
espflash-nativetofbuild-deploy/espflash-nativeand enabling it by default keeps the daemon's feature set consistent with the deploy crate. Runtime env-var opt-out plus automatic esptool fallback on native failure makes this a safe default flip.Note for release comms: this pulls the
espflashcrate (and itsserialporttransitive deps) into the default daemon build. If you track binary size or cold-compile time deltas, worth calling out in the changelog so downstream packagers know to expect the bump.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-daemon/Cargo.toml` around lines 17 - 22, No code change required: the Cargo.toml feature table correctly forwards espflash-native to fbuild-deploy/espflash-native and enables it in default; just keep the existing default = ["espflash-native"] and espflash-native = ["fbuild-deploy/espflash-native"] entries as-is and consider calling out the transitive espflash/serialport deps in release notes for size/compile-time impact.crates/fbuild-deploy/src/esp32.rs (1)
1145-1163: Optional: factor out the "native → esptool fallback" match into a helper.The two call sites (
deploy_regionsat 1145-1163 anddeployat 1232-1250) have essentially identical match/log shape. If this grows (e.g., metrics, structured fallback reasons), a small helper like:#[cfg(feature = "espflash-native")] fn native_write_or_fallback<F>(port: &str, label: &str, native: F) -> Option<DeploymentResult> where F: FnOnce() -> Result<DeploymentResult>, { match native() { Ok(result) if result.success => Some(result), Ok(result) => { tracing::warn!(port, "native {} failed ({}); falling back to esptool", label, result.message); None } Err(e) => { tracing::warn!(port, "native {} failed ({}); falling back to esptool", label, e); None } } }…would keep both sites to a single line. Not essential — readable as-is.
Also applies to: 1232-1250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-deploy/src/esp32.rs` around lines 1145 - 1163, Factor out the duplicated "native → esptool fallback" match into a helper (e.g., native_write_or_fallback) and call it from both deploy_regions and deploy: extract the match over try_deploy_regions_native/try_deploy_native into a small #[cfg(feature = "espflash-native")] function that accepts port, a label (like "selective write-flash" or "write-flash") and a closure returning Result<DeploymentResult>, returns Option<DeploymentResult> (Some on success where result.success is true, None on any fallback) and emits the same tracing::warn messages including result.message or the error; then replace the inline match blocks in deploy_regions and deploy with a single call to this helper and act on the Option result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fbuild-daemon/src/handlers/operations.rs`:
- Around line 2239-2275: Replace the plain .unwrap() on ENV_LOCK.lock() in the
espflash_env_tests module with .unwrap_or_else(|e| e.into_inner()) to survive
mutex poisoning (reference ENV_LOCK and the test functions
native_verify_defaults_on_and_allows_opt_out /
native_write_defaults_on_and_allows_opt_out which call native_verify_enabled and
native_write_enabled). Also add a brief comment above the tests noting that the
mutex only serializes these unit tests and does not protect production callers
of native_verify_enabled() and native_write_enabled(), warning future
maintainers that other tests or integration tests reading those env vars could
still race.
---
Nitpick comments:
In `@crates/fbuild-daemon/Cargo.toml`:
- Around line 17-22: No code change required: the Cargo.toml feature table
correctly forwards espflash-native to fbuild-deploy/espflash-native and enables
it in default; just keep the existing default = ["espflash-native"] and
espflash-native = ["fbuild-deploy/espflash-native"] entries as-is and consider
calling out the transitive espflash/serialport deps in release notes for
size/compile-time impact.
In `@crates/fbuild-deploy/src/esp32.rs`:
- Around line 1145-1163: Factor out the duplicated "native → esptool fallback"
match into a helper (e.g., native_write_or_fallback) and call it from both
deploy_regions and deploy: extract the match over
try_deploy_regions_native/try_deploy_native into a small #[cfg(feature =
"espflash-native")] function that accepts port, a label (like "selective
write-flash" or "write-flash") and a closure returning Result<DeploymentResult>,
returns Option<DeploymentResult> (Some on success where result.success is true,
None on any fallback) and emits the same tracing::warn messages including
result.message or the error; then replace the inline match blocks in
deploy_regions and deploy with a single call to this helper and act on the
Option result.
🪄 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: ef8cd214-d28a-4ae4-8fe4-df1a81b6d636
📒 Files selected for processing (5)
crates/fbuild-daemon/Cargo.tomlcrates/fbuild-daemon/src/handlers/operations.rscrates/fbuild-deploy/Cargo.tomlcrates/fbuild-deploy/README.mdcrates/fbuild-deploy/src/esp32.rs
|
Addressed the CodeRabbit feedback in
Validation:
|
1 participant
Summary
espflash-nativeby default infbuild-deployandfbuild-daemonFBUILD_USE_ESPFLASH_VERIFY=0andFBUILD_USE_ESPFLASH_WRITE=0esptoolautomatically when native verify or write fails, and update docs/tests accordinglyTesting
cargo test -p fbuild-daemon espflash_env_testscargo test -p fbuild-deploySummary by CodeRabbit
New Features
Changes
0/false/no/offto prefer esptool.Documentation