perf(deploy): sector-selective write-flash on verify mismatch (#67)#71
perf(deploy): sector-selective write-flash on verify mismatch (#67)#71
Conversation
Closes #67. When esptool verify-flash reports a mismatch, parse its per-region output and write only the regions that actually differ. For the common case of "only firmware changed", this skips the ~1s bootloader + partitions rewrite and avoids unnecessary flash wear. Changes: - `parse_verify_regions()` maps esptool's `Verifying ... at 0x{addr}` / `Verification successful|failed` output to per-region `FlashRegion` verdicts by matching the address against the deployer's three known offsets. Unknown offsets and unparseable output fall back to an empty result, which the daemon treats as "flash everything". - `VerifyOutcome::Mismatch` carries a `regions: Vec<RegionVerifyResult>` so the daemon can decide between full and selective writes. - `Esp32Deployer::deploy_regions()` and `build_write_flash_args()` issue a `write_flash` call that includes only the requested offset/file pairs. - The daemon operations handler calls `deploy_regions()` when parsing identified fewer than three mismatched regions; otherwise it falls through to the existing full-flash path. Tests cover the parser (success, unknown format, unknown offsets) and the selective argv builder (firmware-only, all-regions default, empty slice rejection). The real-hardware verify tests in `try_verify_deployment_real_esp32*` remain ignored by default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 47 minutes and 47 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 (1)
📝 WalkthroughWalkthroughThe changes implement sector-selective write-flash on ESP32 deployment verification mismatch. When Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Daemon Handler
participant Device as ESP32 Device
participant Deployer as Esp32Deployer
Handler->>Deployer: deploy(firmware_path, port)
Deployer->>Device: esptool verify-flash (all regions)
Device-->>Deployer: stdout/stderr with region results
Deployer->>Deployer: parse_verify_regions(stdout)
Deployer-->>Handler: VerifyOutcome::Mismatch {regions: Vec<RegionVerifyResult>}
Handler->>Handler: Extract mismatched regions
alt Fewer than 3 regions differ
Handler->>Deployer: deploy_regions(firmware_path, port, mismatched_regions)
Deployer->>Deployer: build_write_flash_args (selective regions)
Deployer->>Device: esptool write-flash (selective offsets)
Device-->>Deployer: success/stderr
else 3+ regions or parse failed
Handler->>Deployer: deploy(firmware_path, port)
Deployer->>Device: esptool write-flash (all regions)
Device-->>Deployer: success/stderr
end
Deployer-->>Handler: DeploymentResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The changes introduce new public data structures ( Possibly related issues
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-deploy/src/esp32.rs`:
- Around line 817-832: deploy_regions currently can be asked to deploy specific
FlashRegion(s) but will silently drop Bootloader/Partitions when their files
(bootloader_path, partitions_path) are missing; update deploy_regions to
validate requested non-Firmware regions before calling build_write_flash_args
and return an explicit Err describing which region file is missing (reference
FlashRegion::Bootloader and FlashRegion::Partitions), leaving
build_write_flash_args unchanged (argv-only). Ensure the check iterates the
incoming regions slice (or matches the Some(&[...]) case), tests path.exists()
for the corresponding bootloader_path/partitions_path, and returns a clear error
(including the missing region name) instead of letting esptool receive no region
args.
🪄 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: da4bd90c-50ff-4954-a50b-12a90316d580
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/fbuild-daemon/src/handlers/operations.rscrates/fbuild-deploy/src/esp32.rs
Address CodeRabbit review on #71. If a caller passed Some(&[Bootloader]) but bootloader.bin wasn't on disk, build_write_flash_args silently emitted zero offset/file pairs and esptool failed with an opaque usage error. We now verify each requested region's file exists up front and return a clear error naming the missing file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nse message (#76) Previously `/api/deploy` always returned `message: "deploy succeeded"` regardless of whether the deploy was a full write, a verify-skip (MD5 match), or a selective rewrite of only the differing ESP32 regions. The distinction existed internally (from #67/#71) but was lost at the handler layer. Adds a `DeployOutcome` enum to `fbuild-deploy` carried on `DeploymentResult::outcome`, populated by every deployer path: - Esp32Deployer::deploy -> FullFlash - Esp32Deployer::deploy_regions -> SelectiveFlash { regions } - AvrDeployer / TeensyDeployer -> FullFlash - Daemon VerifyOutcome::Match short-circuit -> VerifySkip The daemon handler composes a stable prefix `"deploy succeeded (<describe>)"` and uses it at every response site (plain success + four monitor-attached variants), so consumers can distinguish a 6s MD5 skip from a 25s full write. Covered by new unit tests in `fbuild-deploy::outcome_tests` and `fbuild-daemon::handlers::operations::deploy_message_tests`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes #67.
When
esptool verify-flashreports a mismatch, parse its per-region output and write only the regions that actually differ. For the common case of "only firmware changed", this skips the ~1s bootloader + partitions rewrite and avoids unnecessary flash wear.Changes
parse_verify_regions()maps esptool'sVerifying ... at 0x{addr}/Verification successful|failedoutput to per-regionFlashRegionverdicts by matching the address against the deployer's three known offsets. Unknown offsets and unparseable output fall back to an empty result, which the daemon treats as "flash everything".VerifyOutcome::Mismatchcarries aregions: Vec<RegionVerifyResult>so the daemon can decide between full and selective writes.Esp32Deployer::deploy_regions()andbuild_write_flash_args()issue awrite_flashcall that includes only the requested offset/file pairs.deploy_regions()when parsing identified fewer than three mismatched regions; otherwise it falls through to the existing full-flash path.Test plan
cargo clippy --workspace --all-targets -- -D warningscleancargo fmt --all -- --checkcleantry_verify_deployment_real_esp32*tests remain ignored by default; maintainer should run one with an attached board when convenient)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes