fix: warn and skip gyro plots when unfiltered data unavailable - #164
Conversation
Older Betaflight logs with only gyroADC (no gyroUnfilt, no debug[0-2] GYRO_SCALED fallback) silently fell through parse_log_file() with no console message, leaving users to guess why spectrum/PSD/comparison plots were skipped. Add a warning for that case, matching the existing messages for the gyroUnfilt-found and debug-fallback branches. Also fix a related mislabeling bug found while making this change: main.rs recomputed using_debug_fallback from header presence only, ignoring the debug_mode == GYRO_SCALED (6) check parse_log_file() already performs. This could label plots as using debug-mode fallback even when the parser rejected the fallback and never populated gyroUnfilt from debug[0-2]. parse_log_file() now returns its own validated using_debug_fallback value instead of main.rs recomputing it. Also fix 5 pre-existing clippy::needless_range_loop errors (unrelated to the above, blocking the pre-commit hook on this branch) in filter_response.rs and log_parser.rs by switching to iterator-based loops. IT #126
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (1)All constants go in `src/constants.rs` — no hardcoded magic numbers in function code📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (4)
📝 WalkthroughWalkthroughThe parser now determines and returns whether debug channels provide gyro fallback data. ChangesParser fallback and filter cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change adds clearer warnings for unsupported gyro data and corrects fallback labeling without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant process_file
participant parse_log_file
participant PlotAndReportPaths
process_file->>parse_log_file: Parse log and detect gyro fallback
parse_log_file-->>process_file: Return using_debug_fallback
process_file->>PlotAndReportPaths: Use fallback state for plots and report
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The PR implements the warning and fallback-status correction for issue ✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…back have_gyro_unfilt used .any() across the 3 axes, so a log with gyroUnfilt present on even one axis disabled debug[0-2] fallback globally, starving axes that lack gyroUnfilt of a fallback they could otherwise use. The per-axis merge in the row-parsing loop already picks gyroUnfilt over debug per axis when both exist, so gating on .all() instead lets the fallback still activate for logs with partial gyroUnfilt coverage. Found by local CodeRabbit CLI review (coderabbit review --agent) on PR #164. IT #126
|
@coderabbitai — AI generated comment Analysis requested on this PR's changes:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files and diff ==='
git diff --unified=80 HEAD^ HEAD -- \
src/data_input/log_parser.rs \
src/main.rs \
src/types.rs \
src/data_analysis/filter_response.rs
echo '=== Relevant file outlines ==='
ast-grep outline src/data_input/log_parser.rs --items all --type function --match 'parse_log_file'
ast-grep outline src/main.rs --items all --type function --match 'process_file|main'
echo '=== parse_log_file definitions and call sites ==='
rg -n -C 5 '\bparse_log_file\s*\(' --type rust src
echo '=== Fallback-state definitions and independent computations ==='
rg -n -C 6 '\b(using_debug_fallback|have_gyro_unfilt|have_debug_axes|gyro_unfilt_header_found|debug_header_found)\b' \
--type rust srcLength of output: 38376 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== parse_log_file documentation and return type ==='
sed -n '45,75p' src/data_input/log_parser.rs
sed -n '20,40p' src/types.rs
echo '=== Parser tests and fixture references ==='
rg -n -C 5 '\b(parse_log_file|gyroUnfilt|GYRO_SCALED|using_debug_fallback|debug\[0\])\b' \
--type rust -g '*test*' -g '*tests*' .
echo '=== Gyro plot availability checks ==='
rg -n -C 12 '\b(gyro_unfilt|unfilt_series_data|all\(|any\(|filter_map)\b' \
--type rust \
src/plot_functions/plot_gyro_vs_unfilt.rs \
src/plot_functions/plot_gyro_spectrums.rs \
src/plot_functions/plot_psd.rs \
src/plot_functions/plot_psd_db_heatmap.rs
echo '=== Previous parser behavior around fallback ==='
git show HEAD^:src/data_input/log_parser.rs | sed -n '465,625p'Length of output: 3297 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Rust test files ==='
fd -t f -e rs . | rg '(^|/)(tests?|.*_tests?)\.rs$' || true
echo '=== Parser and fallback test references ==='
rg -n -C 5 '\b(parse_log_file|gyroUnfilt|GYRO_SCALED|using_debug_fallback|debug\[0\])\b' \
--type rust . || true
echo '=== Gyro plot availability checks ==='
for file in \
src/plot_functions/plot_gyro_vs_unfilt.rs \
src/plot_functions/plot_gyro_spectrums.rs \
src/plot_functions/plot_psd.rs \
src/plot_functions/plot_psd_db_heatmap.rs
do
echo "--- $file ---"
rg -n -C 12 '\b(gyro_unfilt|unfilt_series_data|all\(|any\(|filter_map)\b' \
"$file" || true
done
echo '=== Previous parser fallback and merge behavior ==='
git show HEAD^:src/data_input/log_parser.rs | sed -n '465,625p'Length of output: 68257
I confirmed the requested control flow.
You are interacting with an AI system. |
Superseded the .all()-across-3-axes fix from the previous commit: some firmware configs legitimately omit Yaw from gyroUnfilt logging (matching the existing ROLL_PITCH_AXIS_COUNT convention used elsewhere in this codebase for Yaw-exclusion), so requiring all 3 axes present would incorrectly treat a normal Roll+Pitch-only gyroUnfilt log as missing data and route it into the debug-fallback/warning path. have_gyro_unfilt now requires only the first ROLL_PITCH_AXIS_COUNT (2) axes; Yaw still independently falls back to debug[2] via the existing per-axis merge when gyroUnfilt[2] is absent and using_debug_fallback is true. Also reworded the neither-source warning: it no longer claims "gyroUnfilt[0-2]" is entirely absent, since the gate is now Roll/Pitch-specific and Yaw's gyroUnfilt status doesn't factor in. Found via pi second-opinion review (openrouter/nvidia/nemotron-3-ultra-550b-a55b) on PR #164; two other findings from that pass (missing debug_mode!=6 warning, iterator truncation risk in the clippy cleanup) were verified as false positives — the debug_mode!=6 warning already exists unchanged, and config.dterm/config.gyro are fixed [T; 3] arrays that cannot be shorter than the take() bound. IT #126
Extracted the have_gyro_unfilt/have_debug_axes/debug_mode branching into resolve_gyro_unfilt_fallback(), a pure function taking header flags and the debug_mode value, returning (using_debug_fallback, Option<warning message>). parse_log_file() now calls it and prints the returned message, with identical message text to before. parse_log_file() itself is file-I/O-coupled (reads from a &Path) with no existing fixture-based test precedent in this codebase, so this extraction makes the header-coverage decision logic directly unit testable without CSV fixtures. Added 7 tests covering: full gyroUnfilt, Roll+Pitch-only gyroUnfilt (Yaw omitted, no warning), debug_mode=6 fallback, debug_mode absent (assumed GYRO_SCALED), debug_mode present but wrong value, neither source, and partial Roll-only coverage. Also fixed a stale doc comment above parse_log_file: it still listed header_metadata as return-tuple field 8; using_debug_fallback is field 8 and header_metadata is field 9 now. Found via CodeRabbit GitHub bot analyze pass on PR #164. IT #126
|
AI Generated comment Addressed findings from local CodeRabbit CLI (
All commits pushed. |
…ssage
The "Skipping '<file>' plot saving" message was misleading: the file was
written regardless, because BitMapBackend writes a best-effort fallback
on Drop even when .present() is never explicitly called (see
plotters-bitmap's Drop impl: `if !self.saved { let _ = self.present(); }`).
So every plot type that could have zero axes with real data (Gyro
Spectrums, D-term Spectrums, PSD, PSD heatmap, Motor Spectrums, Step
Response, D-term heatmap) was still producing a PNG containing nothing
but "Data Unavailable" placeholder panels.
Per the actual goal: skip producing the file only when *all 3* axes
have no data; if even 1 axis has data, render normally (unchanged —
this already worked, since any_axis_plotted/any_plot_drawn/
any_motor_plotted already gate on "at least one axis had real data").
Fix: present() must still run unconditionally (it's what marks the
backend `saved`, preventing the Drop-triggered fallback write), but the
resulting placeholder-only file is now deleted immediately after when
no axis had data — net result is no file on disk, not a suppressed
write. Applies to all 3 shared plot_framework.rs drawing functions
(draw_stacked_plot, draw_dual_spectrum_plot, draw_dual_heatmap_plot)
plus plot_motor_spectrums.rs's separate hand-rolled implementation,
which had the identical bug.
Also fixed a report.md correctness issue this surfaces: main.rs built
the report's PNG-links list by unconditionally formatting the expected
filename for every enabled plot type, regardless of whether the file
was actually written. With plots now genuinely skipped, that produced
broken links to non-existent files. png_links now only includes a
filename that actually exists on disk (step_response already did this
correctly via directory scan; the other 13 plot types now check
Path::exists() via a small push_if_exists helper).
Console message reworded per feedback: uses the "⚠️ " prefix already
established elsewhere in this codebase, and prints the short plot-type
name (e.g. "Gyro Spectrums") instead of the full output filename.
Manually verified: re-ran against the neither-gyroUnfilt-nor-debug
EmuFlight test log — Gyro_Spectrums_comparative.png and
Step_Response_stacked_plot_0.5s.png are now genuinely absent from the
output directory (confirmed via fresh output-dir + file listing, not
just console output), while the 5 plot types that do have real data
(SetpointVsGyro, GyroVsUnfilt, D_Term_Spectrums, Motor_Spectrums,
RC_Command_Activity) are present and correctly linked in the report.
Re-verified the other 2 required cases (gyroUnfilt present, debug_mode=6
fallback) still produce all plots normally — no regression.
IT #126
|
AI Generated comment Found and fixed a deeper issue while manually testing this PR against real logs in Per the actual goal for this issue — skip producing the file only when all 3 axes have no data, render normally when even 1 axis has data — fixed by always calling This surfaced a second bug: Verified end-to-end with a fresh output directory (not just console output): |
Console output mixed zero, single, and double blank lines between analysis sections, found via a full console.log capture. Standardized on exactly one blank line between sections: - Removed two redundant bare println!() calls in main.rs that doubled up with the next section header's own leading "\n" (after PID Tuning Analysis, and after the Step Response P:D recommendations intro) — these produced two consecutive blank lines whenever the following content had nothing to print in between (e.g. no step response data). - Added a leading "\n" to 3 section-style headers that previously had zero separation from whatever printed before them: the Gyro Data Availability Diagnostic block (filter_delay.rs), the D-term delay analysis diagnostic block (d_term_delay.rs), and the filter configuration detection messages (filter_response.rs, all 3 branches: EmuFlight/Betaflight/none-recognized). - Added a blank line between "[OK] Report written." and "--- Finished processing file ---" in main.rs — that header intentionally has no leading "\n" of its own, since the next thing printed (either the next file's "--- Processing file ---" or the final "All files processed successfully.") already supplies one; giving it its own leading blank too would have created a new double at file-loop boundaries. Verified against a fresh console.log run over all 4 files in ~/SYNC/XPS/BlackBoxLogs-and-diffs/testy/ (gyroUnfilt case, debug_mode=6 case x2, neither case x2): 0 remaining double-blank-line runs anywhere in 808 lines of output, and all 70 section-style headers across the full run now have exactly one blank line before them (0 misses). IT #126
The per-file analysis console output mixed three header styles: "---" (10 headers, the majority), a lone "===" outlier (Gyro Data Availability Diagnostic), and unbracketed plain labels (filter-config detection messages, D-term delay diagnostic). Standardized on "---" throughout this output, chosen over dropping markers entirely: this tool batch-processes multiple files per run producing long console output, and a human scanning/grepping for phase boundaries benefits more from a consistent visual anchor than from undecorated text — this outweighs the minor "bracket fatigue" of a few more dashed lines. The --help screen's "===" category headers are a separate namespace (static usage documentation, not sequential analysis output) and are intentionally left unchanged, per explicit instruction. Changed: - filter_delay.rs: "=== Gyro Data Availability Diagnostic ===" -> "--- Gyro Data Availability Diagnostic ---" - filter_response.rs: bracketed all 3 branches of the filter-config detection message (EmuFlight/Betaflight/none-recognized) - d_term_delay.rs: "D-term delay analysis diagnostic:" -> "--- D-term delay analysis diagnostic ---" Left unmarked (continuation/sub-detail lines within an already-marked block, not new top-level sections): "Parsed filter configuration:", "Detected PTn filters.../Detected EmuFlight pseudo-Kalman filter...", per-axis sub-lines like "Gyro Filtering delay analysis for Roll axis:". Verified against a fresh console.log run over all 4 files in ~/SYNC/XPS/BlackBoxLogs-and-diffs/testy/: 0 remaining "===" or unbracketed diagnostic-block headers, 0 double-blank-line runs, all 70 section headers have exactly 1 blank line before them. IT #126
|
@coderabbitai — AI generated comment Analysis requested on everything since your last review/analysis on this PR: the skip-render fix (BitMapBackend now only writes a file when at least one axis has real data, present()-then-delete-if-unwanted pattern in plot_framework.rs and plot_motor_spectrums.rs), the report.rs png_links correctness fix (only links files that actually exist via Path::exists()), and the two console-output formatting commits (single blank line between sections, unified "---" section markers replacing the "===" outlier and unbracketed diagnostic-block headers). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Recent commits ==='
git log --oneline --decorate -20
echo '=== Changed files since the earlier parser/fallback changes ==='
git diff --name-status HEAD~12..HEAD || true
echo '=== Current PR diff summary against merge base ==='
base="$(git merge-base HEAD origin/master 2>/dev/null || git merge-base HEAD HEAD^)"
git diff --stat "$base"..HEAD
git diff --unified=35 "$base"..HEAD -- \
src/plot_functions/plot_framework.rs \
src/plot_functions/plot_motor_spectrums.rs \
src/report.rs \
src/main.rs
echo '=== Outline candidate changed Rust files ==='
for file in $(git diff --name-only "$base"..HEAD -- '*.rs'); do
echo "--- $file ---"
ast-grep outline "$file" --items all --type function --view compact || true
done
echo '=== Render/save and PNG-link call sites ==='
rg -n -C 5 '\b(present|remove_file|BitMapBackend|png_links|Path::new|exists)\b' \
--type rust srcLength of output: 72148 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Individual recent commit diffs ==='
for commit in a0e8bdb 4e237f2 5b813e1; do
echo "--- $commit ---"
git show --format=fuller --stat "$commit"
git show --format= --unified=25 "$commit" -- '*.rs'
done
echo '=== Plot framework function bodies ==='
ast-grep outline src/plot_framework.rs --items all --type function --view signatures
sed -n '580,750p' src/plot_framework.rs
sed -n '800,930p' src/plot_framework.rs
echo '=== Motor spectrum function body ==='
ast-grep outline src/plot_functions/plot_motor_spectrums.rs --items all --type function --view signatures
sed -n '120,315p' src/plot_functions/plot_motor_spectrums.rs
echo '=== Project platforms and output-file tests ==='
rg -n -i -C 3 '\b(windows|linux|macos|cross-platform|output-dir|png_links|remove_file)\b' \
README.md OVERVIEW.md Cargo.toml src
echo '=== All shared plot-framework callers ==='
rg -n -C 4 '\b(draw_stacked_plot|draw_dual_spectrum_plot|draw_stacked_heatmap_plot)\s*\(' \
--type rust srcLength of output: 105069 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Numbered deletion sites ==='
nl -ba src/plot_framework.rs | sed -n '650,671p;726,744p;910,928p'
nl -ba src/plot_functions/plot_motor_spectrums.rs | sed -n '288,306p'
echo '=== Relevant dependency versions ==='
rg -n -C 2 '^(plotters|plotters-bitmap)\s*=' Cargo.toml Cargo.lockLength of output: 696 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Numbered deletion sites ==='
awk 'NR >= 650 && NR <= 671 { printf "%6d %s\n", NR, $0 }
NR >= 726 && NR <= 744 { printf "%6d %s\n", NR, $0 }
NR >= 910 && NR <= 928 { printf "%6d %s\n", NR, $0 }' src/plot_framework.rs
awk 'NR >= 288 && NR <= 306 { printf "%6d %s\n", NR, $0 }' \
src/plot_functions/plot_motor_spectrums.rs
echo '=== Relevant dependency versions ==='
rg -n -C 2 '^(plotters|plotters-bitmap)\s*=' Cargo.toml Cargo.lock || trueLength of output: 4344
I found one correctness issue in the skip-render fix.
Drop all drawing areas before deletion. Also propagate or report a deletion failure. This change makes the skip result correct on platforms that do not permit deletion of an open file. The remaining reviewed changes are correct.
You are interacting with an AI system. |
nerdCopter
changed the title
CodeRabbit GitHub bot analysis flagged that remove_file() ran while root_area/margined_root_area/sub_plot_areas (which may hold a shared reference to the BitMapBackend) were still in scope, raising a Windows file-lock concern, and that a deletion failure was silently swallowed (`let _ = ...`), which would leave a stale placeholder-only PNG that Path::exists() then incorrectly links in the report. Verified against the actual plotters-bitmap source: present() calls img.save(&path), the image crate's synchronous open+write+close, and Target::File only stores a path (not an open File handle) — so the file is fully closed well before remove_file() runs regardless of Rust-level scope, on any platform. The specific mechanism CodeRabbit described doesn't hold for this version, but explicitly dropping the drawing-area hierarchy before deletion is a harmless, essentially free belt-and-suspenders change, and surfacing a deletion failure instead of ignoring it closes an independent, real correctness gap (any deletion failure, for any reason — permissions, disk, antivirus lock on Windows — previously left a stale placeholder file that would still get linked in the report with no visible warning). Applied to all 4 occurrences: plot_framework.rs's 3 shared drawing functions plus plot_motor_spectrums.rs's separate implementation. Manually re-verified against the neither-gyroUnfilt-nor-debug test log: Gyro_Spectrums_comparative.png and Step_Response_stacked_plot_0.5s.png are still correctly absent, no "Failed to remove" warning printed, no regression. IT #126
|
@coderabbitai — AI generated comment Addressed the correctness issue from the skip-render analysis:
Note on the stated mechanism: verified against the actual |
|
Tip For best results, initiate chat on the files or code changes.
Your correction to the mechanism is valid. In this The explicit 🐇✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
@coderabbitai — AI generated comment Analysis requested on a design alternative for the skip-render fix (commits a0e8bdb, 81c86a4). Current approach: Proposed alternative: from a user/developer perspective, "skip" should mean the file is never written at all, not written-then-deleted. That would require a two-pass restructure: first call the data-fetching closure for every axis (and column, for the dual-spectrum/heatmap variants) and cache results in a What's your assessment of the two-pass restructure vs. the current write-then-delete approach — correctness, risk, and whether the added complexity (buffering |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Framework function outline ==='
ast-grep outline src/plot_framework.rs --items all --type function --match 'draw_stacked_plot|draw_dual_spectrum_plot|draw_dual_heatmap_plot' --view expanded
echo '=== Motor spectrum function outline ==='
ast-grep outline src/plot_functions/plot_motor_spectrums.rs --items all --type function --view expanded
echo '=== Relevant framework bodies ==='
rg -n -C 10 'fn (draw_stacked_plot|draw_dual_spectrum_plot|draw_dual_heatmap_plot)\b|\.present\(\)|remove_file|data_fn|FnMut|FnOnce|Fn\(' \
src/plot_framework.rs
echo '=== Relevant motor-spectrum body ==='
rg -n -C 10 'fn .*motor|\.present\(\)|remove_file|data_fn|FnMut|FnOnce|Fn\(' \
src/plot_functions/plot_motor_spectrums.rs
echo '=== Call sites and closure shapes ==='
rg -n -C 8 '\b(draw_stacked_plot|draw_dual_spectrum_plot|draw_dual_heatmap_plot)\s*\(' \
--type rust src
echo '=== Report PNG-link existence checks ==='
rg -n -C 6 '\b(png_links|Path::new|exists)\b' --type rust src/report.rs src/main.rsLength of output: 42803 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Axis data-type definitions ==='
rg -n -C 8 '\b(struct|type)\s+(AxisSpectrum|AxisHeatmapSpectrum)\b' --type rust src
echo '=== Shared drawing loops ==='
sed -n '589,677p' src/plot_framework.rs
sed -n '680,755p' src/plot_framework.rs
sed -n '834,945p' src/plot_framework.rs
echo '=== Motor spectrum extraction and drawing loop ==='
sed -n '44,312p' src/plot_functions/plot_motor_spectrums.rs
echo '=== Closures with non-clone extraction or observable work ==='
rg -n -C 5 'draw_(stacked_plot|dual_spectrum_plot|dual_heatmap_plot)\(' \
--type rust src/plot_functions
echo '=== Error and save behavior in callers ==='
rg -n -C 4 'Stacked plot saved|Skipping .*no .* data|No valid .*data found' \
--type rust src/plot_functionsLength of output: 41040
I recommend the cache-then-render design. The current write-then-delete design is functionally correct when The cache-then-render design provides a stronger invariant:
This is not a true two-evaluation design. The implementation should call The inspected signatures make this practical:
The main correctness requirement is to use the existing validity rules during the pre-render check. Do not test only
This keeps the current behavior for mixed data. The renderer still creates unavailable panels for missing or invalid axes when at least one other panel has valid data.
The refactor risk is low to moderate. The loops are short and bounded. The principal risk is duplicating the validity logic in the pre-render check and drawing loop. Avoid that risk by extracting small shared predicates, for example I consider the additional complexity justified if “skip” is a user-visible contract that means “no file was written.” If the current PR should remain narrowly scoped, the existing approach is acceptable because it reports deletion errors. The cache-then-render approach is the cleaner long-term design. You are interacting with an AI system. |
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
…PNGs Fixes IT #165 (follow-up to PR #164 / IT #126): replaces the write-then-delete skip-render approach with cache-then-render, so a skipped plot's PNG is never written at all instead of written-then-deleted. - plot_framework.rs's 3 shared drawing functions plus plot_motor_spectrums.rs's own copy now cache every axis's data first and only create the BitMapBackend when something is plottable. - Fixed a real pre-existing plot_bode.rs bug: create_bode_grid_plot wrote an incomplete placeholder PNG on the coherence-skip path via plotters-bitmap's Drop-triggered fallback save. - Fixed a real heatmap-validity-predicate gap: is_heatmap_plot_config_valid and its render-loop counterpart accepted a non-empty values row without checking x_bins/y_bins length, which could draw zero cells while still saving a background-only PNG. - New "## Skipped Plots" section in the generated .md report, naming any enabled plot type with no plottable data this run. - Reverted an intermediate remove_stale_output_file() mechanism — this application renders CSV into PNG/.md only and never deletes files; stale-output cleanup is an operator task, not this tool's responsibility. - Follow-up IT #167 tracks moving pre-existing hardcoded numeric literals into named constants. Full output-equivalence verified against the pre-refactor cd1976a baseline: byte-identical PNGs and reports on the happy path, and identical final file set on a genuine skip case.
1 participant
AI Generated pull-request
Summary
Fixes IT #126: gyro spectrum/PSD/comparison plots silently produced generic "Data Unavailable" output on logs lacking
gyroUnfiltand a validdebug[0-2]fallback, with no clear reason given and no way to distinguish this from other failure modes.Core fix
parse_log_file()now warns clearly when neithergyroUnfiltnor a validdebug[0-2]fallback (debug_mode=GYRO_SCALED) is available, instead of failing silently.ROLL_PITCH_AXIS_COUNT); Yaw is treated as optional, since some firmware configs legitimately omit it fromgyroUnfiltlogging.debug_modedecision logic into a pureresolve_gyro_unfilt_fallback()function with 7 new unit tests (no fixture-based test precedent existed for the file-I/O-coupledparse_log_file()).Related bug fixed
main.rswas recomputing ausing_debug_fallbackflag independently ofparse_log_file()'s owndebug_mode-validated value, so plot labels could claim debug-mode fallback even when the parser had rejected it.parse_log_file()now returns its own validated value instead.Skip-render fix
BitMapBackend(plotters-bitmap) writes a best-effort fallback onDropeven when.present()is never called. Fixed by always calling.present(), then deleting the placeholder-only file immediately after when no axis had data.Console output cleanup (found via manual testing)
---(was a mix of---, a lone===outlier, and unbracketed diagnostic-block headers). The--helpscreen's===category headers are a separate, unrelated namespace and are unchanged.Also fixed: 5 pre-existing
clippy::needless_range_looperrors that were blocking every commit's pre-commit hook (unrelated to the above, but the repo's clippy gate was already red onmasterbefore this PR).Test plan
cargo clippy --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— cleancargo test --verbose— all tests passing (including 7 new unit tests for the fallback-decision logic)cargo build --release— clean~/SYNC/XPS/BlackBoxLogs-and-diffs/(viabbl_parser), one per data-source case:gyroUnfiltpresent (EmuFlight master, gyroUnfilt-by-default log)debug[0-2]fallback,debug_mode=6(GYRO_SCALED)gyroUnfiltnordebug[0-2]present (real EmuFlight log)===/unbracketed section headers, all 70 section headers have exactly 1 blank line before thempisecond-opinion review (openrouter/nvidia/nemotron-3-ultra-550b-a55b:free) + CodeRabbit GitHub bot analyze — findings addressed or verified as false positives