feat: detect firmware vendor transitions within a BBL file#41
feat: detect firmware vendor transitions within a BBL file#41nerdCopter wants to merge 3 commits into
Conversation
When a BBL file contains sessions from multiple firmware vendors (e.g. Betaflight sessions left on flash before reflashing to EmuFlight), bbl_parser now detects the transition and: - Prints a WARNING listing session ranges per vendor with the suggestion to erase flash before reflashing - Corrects the output filename prefix for sessions whose firmware vendor does not match the BBL filename prefix (e.g. EMUF_* file + Betaflight session → output named BTFL_*), preventing EMUF_BTFL_* or BTFL_BTFL_* duplicate prefixes New public API in export module: - `firmware_prefix_for_revision(revision)` — maps a firmware revision string to its canonical filename prefix (EMUF_, BTFL_, INAV_) - `corrected_session_base_name(bbl_path, firmware_revision)` — returns a corrected base name when the session vendor differs from the BBL filename prefix, or None when they match All export functions (`export_to_csv`, `export_to_gpx`, `export_to_event`, `compute_export_paths`) accept an optional `base_name_override` parameter. Existing callers pass `None` to preserve prior behaviour. Known firmware prefixes are defined in one place (`KNOWN_FIRMWARE_PREFIXES`) and extended by adding a single `(&str, &str)` entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)src/{main,lib}.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/{lib.rs,parser/**/*.rs,export.rs}📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/lib.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/main.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/export.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-12-19T21:17:26.889ZApplied to files:
📚 Learning: 2025-12-19T21:17:26.889ZApplied to files:
📚 Learning: 2025-12-19T21:17:26.889ZApplied to files:
📚 Learning: 2025-12-19T21:17:26.889ZApplied to files:
📚 Learning: 2025-12-19T21:17:26.889ZApplied to files:
🔇 Additional comments (19)
📝 WalkthroughWalkthroughThis PR adds firmware vendor detection and session-aware base-name correction to export paths. A new ChangesFirmware-Vendor Export Path Correction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Register Quicksilver and Kiss FC in the KNOWN_FIRMWARE_PREFIXES table so their sessions are correctly identified and prefixed when detected in mixed-firmware BBL files. Keywords are best-effort guesses based on firmware names; update if actual revision strings differ. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/export.rs (1)
145-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
base_name_overridebefore path construction (path traversal/write-outside-output-dir risk).Line 157 and Line 246 trust
base_name_overridedirectly in filenames. A caller can pass absolute paths or../segments and escape the intended output directory.🔧 Suggested hardening
+use std::path::Component; + +fn sanitize_base_name_override(base_name_override: Option<&str>) -> Option<&str> { + let raw = base_name_override?; + let p = Path::new(raw); + if p.is_absolute() { + return None; + } + if p.components().any(|c| { + matches!( + c, + Component::ParentDir | Component::RootDir | Component::Prefix(_) + ) + }) { + return None; + } + p.file_name().and_then(|s| s.to_str()) +} + pub fn compute_export_paths( @@ - let base_name = base_name_override.unwrap_or_else(|| extract_base_name(input_path)); + let base_name = sanitize_base_name_override(base_name_override) + .unwrap_or_else(|| extract_base_name(input_path)); @@ pub fn export_to_csv( @@ - let base_name = base_name_override.unwrap_or_else(|| extract_base_name(input_path)); + let base_name = sanitize_base_name_override(base_name_override) + .unwrap_or_else(|| extract_base_name(input_path));Also applies to: 240-271
🤖 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 `@src/export.rs` around lines 145 - 176, compute_export_paths currently trusts base_name_override directly which allows absolute paths or path traversal; validate and sanitize base_name_override before constructing filenames: when base_name_override is Some, convert it to a Path and extract only the final file-stem/file-name component (e.g. via Path::new(base_name_override).file_name() or file_stem) and reject or fallback if it contains separators, parent segments, or is absolute; ensure the sanitized base name contains no path separators or leading dots and only safe characters (or else use extract_base_name(input_path)); keep using export_options.output_dir/output_dir for directory placement so callers cannot escape the intended output directory.
🧹 Nitpick comments (2)
src/export.rs (1)
985-1005: ⚡ Quick winAdd coverage for the newly added QUIC/KISS prefix mappings.
KNOWN_FIRMWARE_PREFIXESincludes Quicksilver/Kiss, buttest_firmware_prefix_for_revisioncurrently verifies only EMUF/BTFL/INAV plus unknown cases.✅ Minimal test additions
assert_eq!(firmware_prefix_for_revision("INAV 7.1.2"), Some("INAV_")); + assert_eq!( + firmware_prefix_for_revision("Quicksilver 0.1.0"), + Some("QUIC_") + ); + assert_eq!(firmware_prefix_for_revision("Kiss Ultra"), Some("KISS_")); assert_eq!(firmware_prefix_for_revision(""), None); assert_eq!(firmware_prefix_for_revision("Unknown firmware"), None);🤖 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 `@src/export.rs` around lines 985 - 1005, The test test_firmware_prefix_for_revision is missing assertions for the newly added QUIC/KISS mappings; update this test to include cases that exercise the QUIC and KISS entries in KNOWN_FIRMWARE_PREFIXES by calling firmware_prefix_for_revision with representative revision strings (e.g., "Quicksilver ..." and "KISS ..." or uppercase variants) and asserting they return the expected prefixes (e.g., "QUIC_" and "KISS_"); ensure you add both typical and uppercase/lowercase variations similar to the existing EMUF/BTFL checks so the mapping logic is covered.src/main.rs (1)
942-949: ⚡ Quick winAvoid duplicating vendor mapping in CLI warning output.
This
matchduplicates prefix/vendor knowledge already maintained insrc/export.rs. Future prefix additions can silently drift and printUnknownhere.🤖 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 `@src/main.rs` around lines 942 - 949, The inline match that sets vendor_name duplicates the prefix->vendor mapping maintained elsewhere; replace this block by calling the shared lookup in export.rs (e.g., a function like vendor_for_prefix or get_vendor_by_prefix) instead of re-implementing the mapping. Update the code that currently computes vendor_name (using prefix) to call that shared function, and ensure the shared function is made public if necessary so the CLI warning uses the single source of truth for prefix->vendor mappings.
🤖 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.
Outside diff comments:
In `@src/export.rs`:
- Around line 145-176: compute_export_paths currently trusts base_name_override
directly which allows absolute paths or path traversal; validate and sanitize
base_name_override before constructing filenames: when base_name_override is
Some, convert it to a Path and extract only the final file-stem/file-name
component (e.g. via Path::new(base_name_override).file_name() or file_stem) and
reject or fallback if it contains separators, parent segments, or is absolute;
ensure the sanitized base name contains no path separators or leading dots and
only safe characters (or else use extract_base_name(input_path)); keep using
export_options.output_dir/output_dir for directory placement so callers cannot
escape the intended output directory.
---
Nitpick comments:
In `@src/export.rs`:
- Around line 985-1005: The test test_firmware_prefix_for_revision is missing
assertions for the newly added QUIC/KISS mappings; update this test to include
cases that exercise the QUIC and KISS entries in KNOWN_FIRMWARE_PREFIXES by
calling firmware_prefix_for_revision with representative revision strings (e.g.,
"Quicksilver ..." and "KISS ..." or uppercase variants) and asserting they
return the expected prefixes (e.g., "QUIC_" and "KISS_"); ensure you add both
typical and uppercase/lowercase variations similar to the existing EMUF/BTFL
checks so the mapping logic is covered.
In `@src/main.rs`:
- Around line 942-949: The inline match that sets vendor_name duplicates the
prefix->vendor mapping maintained elsewhere; replace this block by calling the
shared lookup in export.rs (e.g., a function like vendor_for_prefix or
get_vendor_by_prefix) instead of re-implementing the mapping. Update the code
that currently computes vendor_name (using prefix) to call that shared function,
and ensure the shared function is made public if necessary so the CLI warning
uses the single source of truth for prefix->vendor mappings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63d03f66-064b-49af-885f-60be10440394
📒 Files selected for processing (11)
examples/csv_export.rsexamples/event_export.rsexamples/export_demo.rsexamples/gpx_export.rsexamples/multi_export.rsexamples/multi_flight_export.rssrc/conversion.rssrc/export.rssrc/lib.rssrc/main.rstests/export_integration_tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Do not remove or modify comments unless the related code is changed; only add comments that explain code functionality, no AI instructional comments
Never embed or call external binaries from Rust code
Ensurecargo build --releasehas no errors or warnings
Only commit if:cargo clippy --all-targets --all-features -- -D warnings,cargo fmt --all -- --check,cargo test --verbose,cargo test --features=cli --verbose, andcargo build --releaseall pass with no errors or warnings
BEFORE ANY CODE CHANGES: Runcargo clippy --all-targets --all-features -- -D warningsto catch all issues
BEFORE ANY CODE CHANGES: Runcargo fmt --all -- --checkto ensure formatting compliance
Ifcargo fmt --all -- --checkfails, IMMEDIATELY runcargo fmt --allto fix formatting
Code must passcargo fmt --all -- --checkwithout any formatting issues
Never skip clippy checks or formatting checks; never allow warnings to pass
ALWAYS runcargo fmt --allafter making ANY code changes before moving to next steps
After runningcargo fmt --all, ALWAYS verify withcargo fmt --all -- --checkbefore proceeding
Files:
src/conversion.rssrc/lib.rssrc/main.rssrc/export.rs
src/{main,lib}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Maintain CLI source as
src/main.rsand library core assrc/lib.rs
Files:
src/lib.rssrc/main.rs
src/{lib.rs,parser/**/*.rs,export.rs}
📄 CodeRabbit inference engine (AGENTS.md)
Core logic in
src/lib.rswith parser modules (src/parser/) and export functions (src/export.rs) shared by both library and CLI
Files:
src/lib.rssrc/export.rs
src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public API must include:
parse_bbl_file(),parse_bbl_bytes(),BBLLog,ExportOptions,export_to_csv(),export_to_gpx(),export_to_event(), conversion utilities, parser helpers
Files:
src/lib.rs
src/main.rs
📄 CodeRabbit inference engine (AGENTS.md)
CLI (
src/main.rs) uses library export functions (export_to_csv,export_to_gpx,export_to_event) with CLI-specific status messages as thin wrapper with zero public functions
Files:
src/main.rs
src/export.rs
📄 CodeRabbit inference engine (AGENTS.md)
The CSV output must precisely match the format and header order of blackbox_decode CSV files
Files:
src/export.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 0
File: :0-0
Timestamp: 2025-12-08T21:15:02.434Z
Learning: Repo nerdCopter/bbl_parser: For outside-diff review notes, prefer a compact, single-block comment with flat bullets (file:line → note), no nested <details> or admonitions; overflow should be linked via gist. Maintainer: nerdCopter.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: LICENSE_COMMERCIAL:1-4
Timestamp: 2025-08-29T19:52:05.099Z
Learning: nerdCopter prefers to avoid publishing personal information in license files for privacy and security reasons, as they are an individual maintainer rather than a company.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: README.md:520-521
Timestamp: 2025-08-29T19:53:41.354Z
Learning: nerdCopter uses AGPL-3.0-or-later licensing for the bbl_parser project with a dual-licensing approach that includes a separate commercial license option.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: CONTRIBUTING.md:9-14
Timestamp: 2025-08-21T20:25:45.741Z
Learning: nerdCopter prefers to keep CLA language general using "project maintainer" rather than specifying a legal entity name, as they are an individual maintainer without an associated company.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: LICENSE_COMMERCIAL:0-0
Timestamp: 2025-08-29T20:15:04.624Z
Learning: nerdCopter prefers clear positive indicators (✅) when describing fixes rather than using ❌ symbols which can be confusing when describing what was corrected in a positive context.
📚 Learning: 2025-12-19T21:17:26.889Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T21:17:26.889Z
Learning: Applies to src/main.rs : CLI (`src/main.rs`) uses library export functions (`export_to_csv`, `export_to_gpx`, `export_to_event`) with CLI-specific status messages as thin wrapper with zero public functions
Applied to files:
examples/csv_export.rsexamples/multi_flight_export.rsexamples/gpx_export.rssrc/lib.rsexamples/event_export.rsexamples/multi_export.rsexamples/export_demo.rstests/export_integration_tests.rssrc/main.rssrc/export.rs
📚 Learning: 2025-12-19T21:17:26.889Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T21:17:26.889Z
Learning: Applies to src/lib.rs : Public API must include: `parse_bbl_file()`, `parse_bbl_bytes()`, `BBLLog`, `ExportOptions`, `export_to_csv()`, `export_to_gpx()`, `export_to_event()`, conversion utilities, parser helpers
Applied to files:
examples/csv_export.rsexamples/multi_flight_export.rsexamples/gpx_export.rssrc/lib.rsexamples/event_export.rsexamples/multi_export.rsexamples/export_demo.rstests/export_integration_tests.rssrc/main.rssrc/export.rs
📚 Learning: 2025-12-19T21:17:26.889Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T21:17:26.889Z
Learning: Applies to src/export.rs : The CSV output must precisely match the format and header order of blackbox_decode CSV files
Applied to files:
examples/csv_export.rsexamples/multi_flight_export.rssrc/lib.rsexamples/multi_export.rsexamples/export_demo.rstests/export_integration_tests.rssrc/main.rs
📚 Learning: 2025-12-19T21:17:26.889Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T21:17:26.889Z
Learning: Applies to Cargo.toml : Feature flags `csv`, `cli`, `json`, `serde` control optional dependencies; default: `csv` + `cli`
Applied to files:
examples/csv_export.rs
📚 Learning: 2025-12-19T21:17:26.889Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T21:17:26.889Z
Learning: Applies to src/{lib.rs,parser/**/*.rs,export.rs} : Core logic in `src/lib.rs` with parser modules (`src/parser/`) and export functions (`src/export.rs`) shared by both library and CLI
Applied to files:
examples/csv_export.rssrc/lib.rssrc/main.rs
🔇 Additional comments (13)
src/conversion.rs (1)
346-346: LGTM!src/export.rs (2)
86-131: LGTM!
467-578: LGTM!Also applies to: 629-638, 917-926
src/main.rs (2)
9-903: LGTM!
908-941: LGTM!Also applies to: 950-960
examples/gpx_export.rs (1)
43-52: LGTM!examples/multi_export.rs (1)
70-71: LGTM!Also applies to: 79-88, 105-112
examples/multi_flight_export.rs (1)
63-64: LGTM!examples/csv_export.rs (1)
57-57: LGTM!examples/event_export.rs (1)
50-50: LGTM!examples/export_demo.rs (1)
87-87: LGTM!Also applies to: 112-112, 131-131
src/lib.rs (1)
39-39: LGTM!tests/export_integration_tests.rs (1)
39-39: LGTM!Also applies to: 88-88, 127-127, 156-156, 188-188, 249-249
|
@coderabbitai review.
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat src/main.rsLength of output: 42877 🏁 Script executed: #!/bin/bash
cat src/export.rsLength of output: 35544
On the "log 1 not repaired" concern: after tracing through the code, log 1 is actually handled correctly. Here's why: In for (log_index, &start_pos) in log_positions.iter().enumerate() {
// ...
let (should_skip, reason) = should_skip_export(&log, export_options.force_export);
if should_skip { continue; }
// ← called for log 1 (log_index=0) too
let base_name_override =
corrected_session_base_name(file_path, &log.header.firmware_revision);For your exact scenario (
One edge case worth noting: if log 1 is filtered out by One minor real concern I do see: the transition warning message always says "Consider excluding earlier sessions", which assumes old firmware always appears first. That holds for the reflash scenario (flash not erased before upgrade), but if someone reflashed back to an older firmware, the message could be misleading. Not a blocker but worth a Triggering the full review now. ✅ Action performedFull review finished. |
|
@coderabbitai, what is better verbiage to fix your reported (output unified-diff format): |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n "earlier sessions\|firmware transition\|Consider\|WARNING\|vendor" src/main.rs -A2 -B2Length of output: 347 🏁 Script executed: #!/bin/bash
rg -n "WARNING\|transition\|erase\|earlier\|session" src/main.rs -A2 -B2Length of output: 327 🏁 Script executed: #!/bin/bash
grep -n "WARNING\|transition\|erase\|earlier\|Consider\|firmware" src/main.rs | head -60Length of output: 1688 🏁 Script executed: #!/bin/bash
sed -n '908,965p' src/main.rsLength of output: 1983
--- a/src/main.rs
+++ b/src/main.rs
@@ -954,7 +954,8 @@ fn print_firmware_transition_warning(file_path: &Path, session_firmware: &[(usiz
}
}
- println!(" The flash was not erased before reflashing. Consider excluding earlier sessions.");
+ println!(" This typically occurs when flash is not erased before reflashing.");
+ println!(" Consider excluding sessions from the unintended firmware vendor listed above.");
}Why this is better:
The new phrasing refers to the vendor name (already printed above in the grouped list) rather than a positional word, so it stays correct regardless of which direction the firmware transition went. |
1 participant
AI Generated pull-request
Summary
WARNINGwith per-vendor session ranges and the hint to erase flash before reflashingEMUF_BLACKBOX_LOG_...01.csvfor a Betaflight session in anEMUF_*file becomesBTFL_BLACKBOX_LOG_...01.csv, preventingEMUF_BTFL_*orBTFL_BTFL_*duplicatesNew public API (
exportmodule)firmware_prefix_for_revision(revision: &str) -> Option<&'static str>— maps a firmware revision string to its canonical filename prefix (EMUF_,BTFL_,INAV_)corrected_session_base_name(bbl_path: &Path, firmware_revision: &str) -> Option<String>— returns a corrected base name when the session vendor differs from the BBL filename prefix, orNonewhen they matchAll export functions (
export_to_csv,export_to_gpx,export_to_event,compute_export_paths) gain an optionalbase_name_override: Option<&str>parameter. Existing callers passNone— no behaviour change.Known firmware prefixes are defined in one
consttable (KNOWN_FIRMWARE_PREFIXES). Adding support for a new firmware (Quicksilver, Kiss, etc.) requires adding a single(&str, &str)entry.Example output on
EMUF_BLACKBOX_LOG_STELLARH7DEV_20260531_172007.BBL(mixed BF + EF flash)Test plan
cargo test— all unit + integration tests pass (4 new tests forfirmware_prefix_for_revisionandcorrected_session_base_name)cargo clippy -- -D warnings— cleanEMUF_BLACKBOX_LOG_STELLARH7DEV_20260531_172007.BBL— sessions 1–3 renamed toBTFL_*, sessions 4–5 kept asEMUF_*, warning printed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests