Update names for rc_smoothing, rx_timing and feedforward #851
Conversation
|
Warning Rate limit exceeded@ctzsnooze has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 12 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughAdds FEEDFORWARD, FEEDFORWARD_LIMIT, RX_TIMING, and RC_SMOOTHING debug groups with per-index friendly labels and shapes, extends decodeDebugFieldToFriendly mappings, and applies index-specific unit/scale conversions in ConvertDebugFieldValue/ConvertFieldValue; no public API signatures changed. (48 words) Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant Presenter as FlightLogFieldPresenter
participant Decoder as decodeDebugFieldToFriendly
participant Converter as ConvertDebugFieldValue/ConvertFieldValue
UI->>Presenter: Request label/value for (group, index, rawValue, toFriendly)
Presenter->>Decoder: Map group+index -> friendly label / shape
Decoder-->>Presenter: friendly label (+MinMax/shape)
Presenter->>Converter: Convert rawValue using group+index (+toFriendly)
Converter-->>Presenter: converted value (+units)
Presenter-->>UI: Display label + converted value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
ctzsnooze
force-pushed
the
Update-names-for-4.6-RC-Smoothing
branch
2 times, most recently
from
4de9598 to
e4cd348
Compare
to match PR 14411
ctzsnooze
force-pushed
the
Update-names-for-4.6-RC-Smoothing
branch
from
e4cd348 to
8cce9ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/flightlog_fields_presenter.js (1)
2014-2020: RC_SMOOTHING decode is inconsistent with 4.6 mapping (wrong unit and offset)For 4.6, RC_SMOOTHING debug[0] is “Rx Rate Current” (Hz), but this path adds 1500 and prints “us” (legacy semantics for “Raw RC Command”). This is misleading on 4.6.
Gate the RC_SMOOTHING decode by firmware version:
- case "debug[0]": - return `${(value + 1500).toFixed(0)} us`; + case "debug[0]": + if (semver.gte(flightLog.getSysConfig().firmwareVersion, "4.6.0")) { + return `${value.toFixed(0)} Hz`; + } + return `${(value + 1500).toFixed(0)} us`; - case "debug[3]": // rx frame rate [us] - return `${(value / 1000).toFixed(1)} ms`; + case "debug[3]": + if (semver.gte(flightLog.getSysConfig().firmwareVersion, "4.6.0")) { + // Throttle Cutoff Hz on 4.6 + return `${value.toFixed(0)} Hz`; + } + // Legacy: rx frame rate [us] + return `${(value / 1000).toFixed(1)} ms`;Also consider showing “Hz” for debug[1]/[5] on 4.6 to match the new labels.
🧹 Nitpick comments (1)
src/flightlog_fields_presenter.js (1)
2754-2765: RC_SMOOTHING conversion only handles debug[4]; others added in 4.6 have implicit unitsGiven new 4.6 semantics:
- debug[0], [1], [5] are rates (Hz) and don’t need scaling; that’s fine.
- debug[2], [3] are cutoffs (Hz); also no scaling needed.
- debug[6], [7] are counts; no scaling.
However, consider mirroring unit handling in decode for clarity and future maintainability (e.g., comments + explicit “return value; // Hz” for the Hz fields), and ensure debug[4] scaling matches firmware (see earlier note).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flightlog_fields_presenter.js(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/flightlog_fields_presenter.js (3)
src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)src/graph_spectrum_calc.js (1)
value(466-466)
🪛 Biome (2.1.2)
src/flightlog_fields_presenter.js
[error] 2313-2314: This code is unreachable
(lint/correctness/noUnreachable)
🔇 Additional comments (3)
src/flightlog_fields_presenter.js (3)
1444-1487: Action: standardize debug field labels & confirm FEEDFORWARD[1] semanticsQuick summary — the snippet at src/flightlog_fields_presenter.js (lines 1444–1487) shows mixed axis placeholders and potential label/units mismatches that need confirmation and standardization.
Points to address:
- Mixed axis placeholders: "[debug_axis]" vs "[debugAxis]" (and other placeholders used elsewhere). Pick and apply a single placeholder format (or remove it from the top-level label).
- FEEDFORWARD[1] label vs semantics: label reads "Delta, smoothed" but decode/conversion appears to treat debug[1] as setpoint speed (°/s/s). Align the label with the actual units/meaning or adjust conversion.
- RX_TIMING and RC_SMOOTHING add fields (Rx Rate, Smoothed Rx Rate, LQ Percent) — verify the decode/conversion logic applies correct units consistently.
Suggested minimal diffs:
- "debug[all]": "Feedforward [debug_axis]", + "debug[all]": "Feedforward", ... - "debug[1]": "MaxRcRate [debugAxis]", + "debug[1]": "Max RC Rate",Files/locations to check:
- src/flightlog_fields_presenter.js — DEBUG_FRIENDLY_FIELD_NAMES.FEEDFORWARD, FEEDFORWARD_LIMIT, RX_TIMING, RC_SMOOTHING (lines ~1444–1487)
- Search the file for other axis placeholder usages (e.g., "[dbg-axis]") and any decode/conversion functions that map debug[] values to units.
Can you confirm the intended units/semantics for FEEDFORWARD[1] (Delta vs Setpoint Speed) from firmware PR14411 for 4.6 so we can lock the labels and placeholders consistently?
1444-1487: Verify v4.6 (PR #14411) debug-field scalings before locking friendly names/unitsQuick: web results are inconclusive about exact integer scalings — please confirm the firmware-side encodings in PR #14411 and adjust the friendly names/units in this repo accordingly.
Files to inspect/update
- src/flightlog_fields_presenter.js — lines ~1444–1487 (DEBUG_FRIENDLY_FIELD_NAMES.FEEDFORWARD)
Also review the other occurrences you listed: 2014–2020, 2286–2319, 2754–2765, 2815–2826, 3033–3062.Verify these specific debug indices/scale questions in the Betaflight PR/source that defines the debug arrays (look for code that writes debug[0..7] for each mode):
- FEEDFORWARD
- [1] — is this setpoint speed (deg/s²) and encoded as raw ×0.01 (i.e., raw/100 → deg/s²)?
- [2] — boost factor: unitless (no integer scaling)?
- [3] — rcCommand Delta: is unit microseconds with scaling raw = us/10 (i.e., raw ÷10 = μs)?
- [4] — jitter attenuator: is this percent encoded as raw = percent (×100)?
- FEEDFORWARD_LIMIT
- [0] — jitter attenuator raw scaling (×100?)
- [6] — PT1K: is the raw stored multiplied by 1000 (or by 10)?
- RX_TIMING
- [0] — packet interval: is this μs/100 (or another divisor)?
- [1], [3] — are these μs/10 (or another divisor)?
- RC_SMOOTHING
- [0] — is this “Rx Rate Current” in Hz (raw ×1) on v4.6?
- [3] — confirm what internal smoothing state/unit is being emitted
How to verify
- Inspect Betaflight PR #14411 (betaflight/betaflight) and search for the code that populates debug[] for FEEDFORWARD, FEEDFORWARD_LIMIT, RX_TIMING, RC_SMOOTHING (the writer that assigns integers to debug array fields will show any multipliers/divisions).
- If you cannot access firmware source, keep the UI labels generic (don’t assert units) until confirmed.
Action requested
- Confirm the scalings above against PR #14411 (or point to the exact firmware lines) and then update the friendly names/units in src/flightlog_fields_presenter.js accordingly.
2815-2826: RX_TIMING: do not change debug[1] conversion; fix comment typos onlyShort: I inspected src/flightlog_fields_presenter.js. The RX_TIMING conversions are internally consistent — debug[0]/[3] use “hundredths of ms” (value/100 → ms) and debug[1] is labelled "us/100" and converted with value/10 → ms (us/100 = 0.1 ms/unit → divide by 10 to get ms). The proposed change to make debug[1] value/100 would be incorrect. There are, however, comment typos that should be fixed.
Files/locations to update
- src/flightlog_fields_presenter.js — RX_TIMING (around lines 2815–2826): no conversion change required.
- src/flightlog_fields_presenter.js:2823 — fix typo "wuality".
- src/flightlog_fields_presenter.js:2132 and 2881 — fix typo "Pphase".
Suggested minimal diff to apply (typo fixes only):
// debug 6 is link wuality
// debug 6 is link quality
// debug 3 = Pphase shift in us
// debug 3 = Phase shift in usIf you want I can search firmware (PR/commit) for RX_TIMING mappings if you provide the PR link or commit hash, but based on the repo here the conversion change in the original comment is incorrect.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/flightlog_fields_presenter.js (6)
2014-2023: Avoid duplicate scaling logic in RC_SMOOTHING decode — call ConvertDebugFieldValue and then format unitsYou’re performing numeric scaling in both decode and Convert paths. To keep a single source of truth, route decode through ConvertDebugFieldValue for the numeric part, then append units here.
Apply this diff:
- case "debug[0]": // current Rx Rate Hz - case "debug[1]": // smoothed but stepped Rx Rate Hz - case "debug[2]": // setpoint cutoff Hz - case "debug[3]": // throttle cutoff Hz - case "debug[5]": // smoothed Rx Rate Hz, without steps - return `${value.toFixed(0)} Hz`; + case "debug[0]": // current Rx Rate Hz + case "debug[1]": // smoothed but stepped Rx Rate Hz + case "debug[2]": // setpoint cutoff Hz + case "debug[3]": // throttle cutoff Hz + case "debug[5]": // smoothed Rx Rate Hz, without steps + { + const scaled = FlightLogFieldPresenter.ConvertDebugFieldValue(flightLog, fieldName, true, value); + return `${scaled.toFixed(0)} Hz`; + } case "debug[4]": // Feedforward PT1k range 0-1000 show as 0.nnn - return `${(value / 1000).toFixed(3)}`; + { + const scaled = FlightLogFieldPresenter.ConvertDebugFieldValue(flightLog, fieldName, true, value); + return `${scaled.toFixed(3)}`; + }Note: Please confirm the intended PT1K display convention (0.nnn vs 0–100.0). Earlier discussion suggested 0–100 with one decimal; current code shows 0.nnn. Choose one convention and make it consistent across decode and convert.
2296-2312: FEEDFORWARD decode: add unit to debug[3] and reuse Convert for scaling
- debug[3] currently renders a bare number. Append units.
- Avoid duplicating the /10 scaling here; use ConvertDebugFieldValue for numeric conversion.
Apply this diff:
case "debug[3]": // rcCommand Delta integer * 10 - return `${(value / 10).toFixed(1)}`; + { + const scaled = FlightLogFieldPresenter.ConvertDebugFieldValue(flightLog, fieldName, true, value); + return `${scaled.toFixed(1)} °/s`; + }Also, please verify raw scaling for debug[1] and debug[4] in 4.6. If either is logged with implicit scaling (e.g., *0.01 or *100), add the corresponding cases in ConvertDebugFieldValue so chart values and friendly text remain consistent.
2296-2312: Answering precedence question: Convert controls chart scaling; decode controls text/units — consolidate to avoid drift
- Charts/axis scaling: FlightLogFieldPresenter.ConvertFieldValue → ConvertDebugFieldValue determine numeric conversion for plotting.
- Text/hover/legend values: FlightLogFieldPresenter.decodeFieldToFriendly → decodeDebugFieldToFriendly format strings and units.
To avoid divergence, do all numeric conversions in ConvertDebugFieldValue and make decodeDebugFieldToFriendly call it for the numeric part before appending units. You’ve partially duplicated scaling (e.g., FEEDFORWARD debug[3]) — refactor as suggested in comments above.
I can prepare a cohesive refactor PR that routes all debug scaling through ConvertDebugFieldValue and updates decode paths to only format units. Want me to draft it?
Also applies to: 3042-3069
3057-3069: FEEDFORWARD_LIMIT: missing percent conversion for debug[0] (charts will be off)decode divides by 100 for debug[0], but Convert lacks a matching conversion. Graphs will show raw values and be mis-scaled vs the legend.
Apply this diff:
case "FEEDFORWARD_LIMIT": switch (fieldName) { - // case "debug[0]": // jitter attenuator percent + case "debug[0]": // jitter attenuator percent (value = pct * 100) + return toFriendly ? value / 100 : value * 100; // case "debug[1]": // max setpoint rate for the axis // case "debug[2]": // setpoint // case "debug[3]": // feedforward // case "debug[4]": // setpoint speed un-smoothed // case "debug[5]": // setpoint speed smoothed case "debug[6]": //feedforward smoothing PT1K * 1000 show as 0.nnn return toFriendly ? value / 1000 : value * 1000; // case "debug[7]": // smoothed RxRateHz default: return value; }
2081-2086: RX_TIMING decode: delegate math to Convert to avoid driftForward the transform to ConvertDebugFieldValue; format units here. This keeps graphs and labels in sync.
Apply this diff:
- case "debug[0]": // Packet interval us/10 - case "debug[3]": // Constrained packet interval us/10 - return `${(value / 100).toFixed(2)} ms`; + case "debug[0]": // Packet interval us/10 + case "debug[3]": // Constrained packet interval us/10 + { + const scaled = FlightLogFieldPresenter.ConvertDebugFieldValue(flightLog, fieldName, true, value); + return `${scaled.toFixed(2)} ms`; + } case "debug[1]": // Packet time stamp us/100, divide by 10 to ms - return `${(value / 10).toFixed(2)} ms`; + { + const scaled = FlightLogFieldPresenter.ConvertDebugFieldValue(flightLog, fieldName, true, value); + return `${scaled.toFixed(2)} ms`; + }
2029-2033: RC_SMOOTHING_RATE decode: route numeric via Convert for consistencyMinimize duplication by delegating the numeric conversion to ConvertDebugFieldValue, then format with units here.
Apply this diff:
- case "debug[0]": // current Rx Interval [us] - return `${(value / 1000).toFixed(2)} ms`; + case "debug[0]": // current Rx Interval [us] + { + const scaled = FlightLogFieldPresenter.ConvertDebugFieldValue(flightLog, fieldName, true, value); + return `${scaled.toFixed(2)} ms`; + } case "debug[2]": // smoothed Rx Rate [Hz] - return `${value.toFixed(0)} Hz`; + { + const scaled = FlightLogFieldPresenter.ConvertDebugFieldValue(flightLog, fieldName, true, value); + return `${scaled.toFixed(0)} Hz`; + }
🧹 Nitpick comments (3)
src/flightlog_fields_presenter.js (3)
1444-1465: Unify placeholder token: use one of [dbg-axis] or [debug_axis] consistentlyMinor naming inconsistency:
- FEEDFORWARD uses “Feedforward [debug_axis]”.
- FEEDFORWARD_LIMIT uses “MaxRcRate [debugAxis]”.
Elsewhere in this file, we commonly use “[dbg-axis]”. Recommend standardizing on a single token for consistency.
Apply this diff:
- "debug[all]": "Feedforward [debug_axis]", + "debug[all]": "Feedforward [dbg-axis]", @@ - "debug[1]": "MaxRcRate [debugAxis]", + "debug[1]": "MaxRcRate [dbg-axis]",
2314-2328: FEEDFORWARD_LIMIT decode: reuse Convert for PT1K to avoid duplicationYou already implement the numeric scaling in ConvertDebugFieldValue for debug[6]. Use it here to avoid drift and keep behavior centralized.
Apply this diff:
case "debug[6]": // feedforward smoothing PT1K * 1000 - return `${(value / 1000).toFixed(3)}`; + { + const scaled = FlightLogFieldPresenter.ConvertDebugFieldValue(flightLog, fieldName, true, value); + return `${scaled.toFixed(3)}`; + }
2825-2835: Typo in comments (“wuality”)Nit: fix spelling for clarity.
Apply this diff:
- // debug 6 is link wuality + // debug 6 is link quality
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flightlog_fields_presenter.js(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T02:03:17.493Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:2304-2318
Timestamp: 2025-08-14T02:03:17.493Z
Learning: PT1K values in Betaflight debug output have a range of 0-1, and are intentionally scaled by dividing by 10 to display as 0-100 with one decimal place for better readability in the blackbox viewer.
Applied to files:
src/flightlog_fields_presenter.js
🧬 Code Graph Analysis (1)
src/flightlog_fields_presenter.js (3)
src/graph_spectrum_calc.js (1)
value(466-466)src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)
🪛 Biome (2.1.2)
src/flightlog_fields_presenter.js
[error] 2771-2771: Expected a case but instead found '='.
Expected a case here.
(parse)
🪛 GitHub Actions: Preview Deployment
src/flightlog_fields_presenter.js
[error] 2771-2771: vite-plugin-pwa:build: Expression expected in src/flightlog_fields_presenter.js at line 2771.
🔇 Additional comments (2)
src/flightlog_fields_presenter.js (2)
1444-1487: New debug groups and labels look good for 4.6 — nice coverageThe additions for FEEDFORWARD, FEEDFORWARD_LIMIT, RX_TIMING, and RC_SMOOTHING are comprehensive and align with the firmware naming. This will greatly improve discoverability in the UI.
3042-3055: FEEDFORWARD conversions: consider adding cases for debug[1]/[4] if they’re scaled in firmwareOnly debug[3] has explicit conversion. If 4.6 firmware logs debug[1] as “*0.01f” and debug[4] as percent “*100”, charts will be mis-scaled without conversions here. If they’re already in final units, keep as-is.
I recommend verifying the raw units for FEEDFORWARD debug[1] and debug[4] in 4.6. If scaling exists, apply:
case "FEEDFORWARD": switch (fieldName) { + case "debug[1]": // setpoint speed * 0.01f (if applicable) + return toFriendly ? value / 100 : value * 100; case "debug[3]": // rcCommand Delta integer * 10 return toFriendly ? value / 10 : value * 10; + case "debug[4]": // jitter attenuator percent (if raw = pct*100) + return toFriendly ? value / 100 : value * 100; default: return value; }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/flightlog_fields_presenter.js (2)
2296-2328: Which path controls decimals/units? Short answer + recommended approach
- Charts and axis scaling: ConvertFieldValue/ConvertDebugFieldValue.
- Textual display (hover/legend): decodeFieldToFriendly.
Keep all numeric scaling in ConvertDebugFieldValue and keep decodeFieldToFriendly for formatting and appending units, reusing ConvertDebugFieldValue for the numeric part to avoid drift.
If you want, I can outline a minimal pattern to call ConvertDebugFieldValue from decodeFieldToFriendly for these FEEDFORWARD/FEEDFORWARD_LIMIT cases only, without a broad refactor.
Also applies to: 3041-3068
3056-3068: FEEDFORWARD_LIMIT: missing conversion for percent (debug[0]) causes mis-scaled chartsdecode shows debug[0] as (value/100) %, but ConvertDebugFieldValue lacks the matching ÷100 conversion. Charts will show 0–10000 instead of 0–100.
Apply:
case "FEEDFORWARD_LIMIT": switch (fieldName) { - // case "debug[0]": // jitter attenuator percent + case "debug[0]": // jitter attenuator percent (value = pct * 100) + return toFriendly ? value / 100 : value * 100; // case "debug[1]": // max setpoint rate for the axis // case "debug[2]": // setpoint // case "debug[3]": // feedforward // case "debug[4]": // setpoint speed un-smoothed // case "debug[5]": // setpoint speed smoothed case "debug[6]": //feedforward smoothing PT1K * 1000 show as 0.nnn return toFriendly ? value / 1000 : value * 1000; // case "debug[7]": // smoothed RxRateHz default: return value; }
🧹 Nitpick comments (6)
src/flightlog_fields_presenter.js (6)
1444-1465: Unify axis placeholder token for consistencyYou mix "[debug_axis]" and "[debugAxis]" across these new labels. Standardize to a single token to avoid future grep/refactor misses and to keep UI copy consistent.
Apply:
- "debug[all]": "Feedforward [debug_axis]", + "debug[all]": "Feedforward [debug_axis]", "debug[0]": "Setpoint, un-smoothed", "debug[1]": "Delta, smoothed", "debug[2]": "Boost, smoothed", "debug[3]": "rcCommand Delta", "debug[4]": "Jitter Attenuator", "debug[5]": "Duplicate Flag", "debug[6]": "Yaw FF BaseAmt", "debug[7]": "Yaw FF HoldAmt", }; DEBUG_FRIENDLY_FIELD_NAMES.FEEDFORWARD_LIMIT = { "debug[all]": "Feedforward Limit", "debug[0]": "Jitter Attenuator", - "debug[1]": "MaxRcRate [debugAxis]", + "debug[1]": "MaxRcRate [debug_axis]", "debug[2]": "Setpoint", "debug[3]": "Feedforward", "debug[4]": "Setpoint Speed", "debug[5]": "Setpoint Speed Smoothed", "debug[6]": "FF PT1K", "debug[7]": "Rx Rate", };
2029-2034: RC_SMOOTHING_RATE decode is fine; minor nit on explicit fallthroughDecode logic is fine. For clarity, consider adding a comment or explicit return for debug[3] if you intend fallthrough.
Apply either:
- // case "debug[3]": // flag to update smoothing + // case "debug[3]": // flag to update smoothing (falls through)or:
+ case "debug[3]": // flag to update smoothing + return value.toFixed(0);
2296-2312: FEEDFORWARD decode: add unit for rcCommand Delta and verify scaling for [1]/[2]
- debug[3] is an angular rate; add unit to avoid unitless legend/tooltips.
- Please verify raw scaling for debug[1]/[2] against firmware PR 14411. If they’re logged as “*0.01f”, you likely want matching numeric conversion in ConvertDebugFieldValue to keep charts in real units.
Apply:
case "debug[3]": // rcCommand Delta integer * 10 - return `${(value / 10).toFixed(1)}`; + return `${(value / 10).toFixed(1)} °/s`;Follow-up: If debug[1]/[2] are scaled in firmware (e.g., *0.01f), add corresponding conversions in ConvertDebugFieldValue so charts match the displayed text.
2780-2783: Empty case labels: clarify fallthrough or return explicitlyThe case labels for debug[2]/[3] don’t change the value and fall through to default. Consider adding an explicit return to document intent or a “falls through” comment to satisfy linters.
Example:
- case "debug[2]": // smoothed RxRate [Hz] - case "debug[3]": // boolean indicating need to update smoothing + case "debug[2]": // smoothed RxRate [Hz] (falls through) + case "debug[3]": // boolean indicating need to update smoothing (falls through)
2824-2834: Typo in comment: “wuality” → “quality”Fix the comment to avoid confusion.
- // debug 6 is link wuality + // debug 6 is link quality
3041-3054: FEEDFORWARD chart conversions: confirm scaling for debug[1]/[2]/[4]Only debug[3] is converted. If firmware 4.6 logs:
- debug[1]/[2] as “*0.01f” → add ÷100 to toFriendly (and inverse for fromFriendly).
- debug[4] as percent or percent100? Your decode shows it as a percent without dividing; if firmware emits percent100, add ÷100 here; if it emits 0–100 already, leave as-is.
I’m not proposing a diff here to avoid guessing firmware units; please confirm against PR 14411 and update ConvertDebugFieldValue accordingly so charts match the decode text.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flightlog_fields_presenter.js(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.698Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
📚 Learning: 2025-08-14T02:50:17.698Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.698Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
Applied to files:
src/flightlog_fields_presenter.js
📚 Learning: 2025-08-14T02:03:17.493Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:2304-2318
Timestamp: 2025-08-14T02:03:17.493Z
Learning: PT1K values in Betaflight debug output have a range of 0-1, and are intentionally scaled by dividing by 10 to display as 0-100 with one decimal place for better readability in the blackbox viewer.
Applied to files:
src/flightlog_fields_presenter.js
📚 Learning: 2025-08-14T02:49:32.927Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:49:32.927Z
Learning: User ctzsnooze prefers to learn and implement code changes manually rather than having complete refactor solutions provided, as they want to understand the codebase through hands-on experience.
Applied to files:
src/flightlog_fields_presenter.js
🧬 Code Graph Analysis (1)
src/flightlog_fields_presenter.js (3)
src/graph_spectrum_calc.js (1)
value(466-466)src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)
🔇 Additional comments (3)
src/flightlog_fields_presenter.js (3)
2014-2023: PT1K: 0.xxx formatting looks good and matches conversionsUsing value/1000 with toFixed(3) for RC_SMOOTHING debug[4] aligns with the agreed 0.xxx representation for PT1K and with ConvertDebugFieldValue. No issues.
2081-2086: RX_TIMING: unit conversions are coherent
- debug[0]/[3] as hundredths of ms → divide by 100 to ms.
- debug[1] as us/100 → divide by 10 to ms.
This matches the comments and expected math.
2764-2773: RC_SMOOTHING PT1K numeric conversion: LGTMThe toFriendly/value↔/1000 mapping is consistent with decode and the decided 0.xxx format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/flightlog_fields_presenter.js (7)
1444-1466: New 4.6 debug group labels added — consistency nitsNice addition of FEEDFORWARD/FEEDFORWARD_LIMIT/RX_TIMING/RC_SMOOTHING group labels. Minor consistency nits:
- Use the same axis placeholder everywhere. The rest of the file predominantly uses “[dbg-axis]”; here you mix “[debug_axis]” and “[debugAxis]”.
Suggested tweak for consistency:
- "debug[all]": "Feedforward [debug_axis]", + "debug[all]": "Feedforward [dbg-axis]", ... - "debug[1]": "MaxRcRate [debugAxis]", + "debug[1]": "MaxRcRate [dbg-axis]",
2081-2085: RX_TIMING: add units for Rx Rate and Smoothed Rx RateYou already convert 0/1/3 to ms; consider adding “Hz” for 4/5 to avoid ambiguity and match the friendly names.
case "RX_TIMING": switch (fieldName) { case "debug[0]": // Packet interval us/10 case "debug[3]": // Constrained packet interval us/10 return `${(value / 100).toFixed(2)} ms`; case "debug[1]": // Packet time stamp us/100, divide by 10 to ms return `${(value / 10).toFixed(2)} ms`; + case "debug[4]": // current Rx Rate, Hz + return `${value.toFixed(0)} Hz`; + case "debug[5]": // smoothed Rx Rate, Hz + return `${value.toFixed(0)} Hz`; default: return value.toFixed(0); }
2296-2312: FEEDFORWARD: add unit to rcCommand DeltaSmall clarity improvement: append “°/s” to debug[3] to match other rate displays.
- case "debug[3]": // rcCommand Delta integer * 10 - return `${(value / 10).toFixed(1)}`; + case "debug[3]": // rcCommand Delta integer * 10 + return `${(value / 10).toFixed(1)} °/s`;
2314-2328: FEEDFORWARD_LIMIT: verify jitter attenuator raw units and chart scalingFriendly decode shows “%” without scaling. Please confirm whether raw is:
- 0–100 (already percent): then no Convert case needed, or
- percent×100 (common in other modes): then charts will be off unless you add a matching conversion in ConvertDebugFieldValue.
If it’s percent×100, add:
case "FEEDFORWARD_LIMIT": switch (fieldName) { - // case "debug[0]": // jitter attenuator percent + case "debug[0]": // jitter attenuator percent (value = pct * 100) + return toFriendly ? value / 100 : value * 100;Also: FF PT1K shown as 0.nnn is consistent with your chosen format — looks good.
2824-2834: RX_TIMING Convert: fix comment typo and keep conversionsThe conversions are correct. Minor comment typo:
- // debug 6 is link wuality + // debug 6 is link quality
3041-3054: FEEDFORWARD Convert: consider adding conversions for scaled fields if applicableOnly debug[3] is converted. If your firmware logs for FEEDFORWARD use scaled values:
- debug[1]/[2] might be “*0.01f”
- debug[4] might be percent×100
Then charts will be mis-scaled without matching conversions. If that’s the case, extend conversions:
case "FEEDFORWARD": switch (fieldName) { - // case "debug[0]": // setpoint - // case "debug[1]": // setpoint speed - // case "debug[2]": //feedforward boost + // case "debug[0]": // setpoint + case "debug[1]": // setpoint speed * 0.01 (if applicable) + return toFriendly ? value / 100 : value * 100; + case "debug[2]": // feedforward boost * 0.01 (if applicable) + return toFriendly ? value / 100 : value * 100; case "debug[3]": // rcCommand Delta integer * 10 return toFriendly ? value / 10 : value * 10; - // case "debug[4]": // jitter attenuator percent + case "debug[4]": // jitter attenuator percent (value = pct * 100) + return toFriendly ? value / 100 : value * 100; // case "debug[5]": // boolean indicating packet duplicate // case "debug[6]": // yaw feedforward // case "debug[7]": // yaw feedforward hold element default: return value; }If firmware now emits raw values already in “human” units, keep as-is. Please confirm.
1444-1487: Precedence/behavior note: where decimals/units are controlledFor clarity as you proceed:
- Chart scaling/min-max uses ConvertFieldValue → ConvertDebugFieldValue (numeric only).
- Tooltips/legend textual units/precision come from decodeFieldToFriendly → decodeDebugFieldToFriendly.
So when you change numeric scaling, add the conversion in ConvertDebugFieldValue; then only append units in decodeDebugFieldToFriendly to avoid drift. Your current PT1K updates follow this pattern; keep applying it to the remaining FEEDFORWARD/FEEDFORWARD_LIMIT indices as needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flightlog_fields_presenter.js(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.698Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
📚 Learning: 2025-08-14T02:50:17.698Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.698Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
Applied to files:
src/flightlog_fields_presenter.js
📚 Learning: 2025-08-14T02:03:17.493Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:2304-2318
Timestamp: 2025-08-14T02:03:17.493Z
Learning: PT1K values in Betaflight debug output have a range of 0-1, and are intentionally scaled by dividing by 10 to display as 0-100 with one decimal place for better readability in the blackbox viewer.
Applied to files:
src/flightlog_fields_presenter.js
📚 Learning: 2025-08-14T02:49:32.927Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:49:32.927Z
Learning: User ctzsnooze prefers to learn and implement code changes manually rather than having complete refactor solutions provided, as they want to understand the codebase through hands-on experience.
Applied to files:
src/flightlog_fields_presenter.js
🧬 Code Graph Analysis (1)
src/flightlog_fields_presenter.js (3)
src/graph_spectrum_calc.js (1)
value(466-466)src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)
🔇 Additional comments (5)
src/flightlog_fields_presenter.js (5)
2014-2022: RC_SMOOTHING decoding looks correct; PT1K shown as 0.nnnThe Hz fields are labeled and PT1K is displayed as 0.nnn via /1000 — aligns with the chosen 0.xxx format and improves readability.
2029-2033: RC_SMOOTHING_RATE units/formatting LGTM
- debug[0] exposed as ms via /1000 is clear.
- debug[2] includes “Hz” in decode; good.
2764-2773: RC_SMOOTHING Convert: PT1K /1000 matches displayNumeric conversion matches decode; charts will be consistent. Good.
2780-2784: RC_SMOOTHING_RATE Convert: ms/Hz paths are consistent
- debug[0] us→ms via /1000
- debug[2]/[3] pass-through as intended
3056-3068: FEEDFORWARD_LIMIT Convert: PT1K /1000 looks good; add jitter percent only if raw is pct×100The PT1K scaling matches the 0.nnn display. If FEEDFORWARD_LIMIT debug[0] raw is percent×100, add a matching conversion (see prior comment). Otherwise, no change needed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/flightlog_fields_presenter.js (1)
3073-3086: Consolidate scaling in ConvertDebugFieldValue; keep decode only for units (duplicate of earlier guidance)To avoid drift, perform all numeric scaling here and let decodeDebugFieldToFriendly only append units. You’ve done this for several fields; finishing the consolidation will prevent future inconsistencies.
🧹 Nitpick comments (4)
src/graph_config.js (1)
797-821: FEEDFORWARD MinMax: consider boolean/flag scaling and verify yaw ranges
- debug[5] “Packet Duplicate boolean” uses 0–10. Consider 0–1 for clarity, unless you intentionally amplify booleans on charts.
- debug[6]/[7] “Yaw feedforward” and “Yaw feedforward hold” are 0–1000. Please confirm typical magnitude in 4.6 logs to avoid constant pegging.
Optional diff for the boolean:
case "debug[5]": // Packet Duplicate boolean return { power: 1.0, MinMax: { min: 0, - max: 10, + max: 1, }, };src/flightlog_fields_presenter.js (3)
1444-1466: Nit: keep label placeholders consistent (“debug_axis” vs “debugAxis”)You use “debug_axis” elsewhere but “MaxRcRate [debugAxis]” here. Harmonizing these improves UX consistency.
Apply this diff:
DEBUG_FRIENDLY_FIELD_NAMES.FEEDFORWARD_LIMIT = { "debug[all]": "Feedforward Limit", "debug[0]": "Jitter Attenuator", - "debug[1]": "MaxRcRate [debugAxis]", + "debug[1]": "MaxRcRate [debug_axis]", "debug[2]": "Setpoint", "debug[3]": "Feedforward", "debug[4]": "Setpoint Speed", "debug[5]": "Setpoint Speed Smoothed", "debug[6]": "FF PT1K", "debug[7]": "Rx Rate", };
2328-2344: Add unit to FEEDFORWARD debug[3] displayrcCommand Delta is scaled (÷10) but displayed without unit. Add “°/s” for clarity.
Apply this diff:
- case "debug[3]": // rcCommand Delta integer * 10 - return `${(value / 10).toFixed(1)}`; + case "debug[3]": // rcCommand Delta integer * 10 + return `${(value / 10).toFixed(1)} °/s`;
2856-2866: Typos in RX_TIMING commentsMinor comment polish:
- “Frame time stamp us/100” returns ms (÷10), which is correct — comment is fine.
- “link wuality” → “link quality”
Apply this diff:
- // debug 6 is link wuality + // debug 6 is link quality
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/flightlog_fields_presenter.js(7 hunks)src/graph_config.js(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.745Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
📚 Learning: 2025-08-14T02:50:17.745Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.745Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
Applied to files:
src/flightlog_fields_presenter.js
📚 Learning: 2025-08-14T02:03:17.510Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:2304-2318
Timestamp: 2025-08-14T02:03:17.510Z
Learning: PT1K values in Betaflight debug output have a range of 0-1, and are intentionally scaled by dividing by 10 to display as 0-100 with one decimal place for better readability in the blackbox viewer.
Applied to files:
src/flightlog_fields_presenter.js
📚 Learning: 2025-08-14T02:49:32.956Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:49:32.956Z
Learning: User ctzsnooze prefers to learn and implement code changes manually rather than having complete refactor solutions provided, as they want to understand the codebase through hands-on experience.
Applied to files:
src/flightlog_fields_presenter.js
🧬 Code Graph Analysis (2)
src/graph_config.js (2)
src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)
src/flightlog_fields_presenter.js (4)
src/graph_spectrum_calc.js (1)
value(466-466)src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)src/graph_config.js (1)
getCurveForMinMaxFields(270-291)
🔇 Additional comments (9)
src/graph_config.js (3)
580-592: Good: unified Hz ranges for RC_SMOOTHING indicesUsing 0–1200 Hz for the rate/cutoff-related indices looks appropriate and consistent with the rest of the viewer.
593-599: Good: PT1K shown as 0–1 rangeMinMax 0–1 here aligns with the 0.xxx display choice for PT1K values elsewhere.
827-880: Per-index FEEDFORWARD_LIMIT ranges look sensible; verify feedforward amplitude and “speed” boundsThe tailored ranges improve readability. Two items to double-check with real logs:
- debug[3] feedforward at [-50, 50] might be narrow depending on user setups.
- debug[4]/[5] setpoint speed at [-100, 100] assumes a specific normalization. Confirm this matches 4.6 firmware scaling so curves don’t saturate.
src/flightlog_fields_presenter.js (6)
1466-1476: Good: RX_TIMING friendly labelsClear, per-index naming here will make logs easier to interpret.
1477-1487: Good: RC_SMOOTHING friendly labels align with new firmware semanticsThe PT1K and Rx Rate fields are clearly labeled for 4.6.
2014-2023: Good: RC_SMOOTHING display formatting
- Hz for indices [0,1,2,3,5] and 0.nnn for PT1K matches the intended presentation.
2029-2033: Good: RC_SMOOTHING_RATE units
- Showing the interval in ms for debug[0], and rate in Hz for debug[2], matches the ConvertDebugFieldValue mapping.
2346-2360: Good: FEEDFORWARD_LIMIT formatted values
- PT1K as 0.nnn and RxRate in Hz match the chosen conventions.
2796-2805: Good: RC_SMOOTHING PT1K conversion uses 0.xxx scalingThe symmetric toFriendly/back conversion (÷1000/×1000) aligns with how it’s presented in decode and GraphConfig.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/graph_config.js (1)
600-608: RC_SMOOTHING outlier/valid counts: use 0–3 instead of 0–50.These are count fields with range 0–3. Max=50 compresses them unnecessarily and misleads the axis.
Apply:
case "debug[6]": // outlier count 0-3 case "debug[7]": // valid count 0-3 return { power: 1.0, MinMax: { - min: 0, - max: 50, // put them at the very bottom + min: 0, + max: 3, }, };src/flightlog_fields_presenter.js (1)
3047-3060: Single-source numeric scaling: keep all numeric conversions in ConvertDebugFieldValue.There’s still duplication risk between decode and convert for FEEDFORWARD (only debug[3] is converted here). Consider routing decode through ConvertDebugFieldValue for the numeric part and only appending units in decode.
I can help outline the pattern if you’d like to move incrementally and keep learning-by-doing.
🧹 Nitpick comments (7)
src/graph_config.js (2)
805-813: “Packet Duplicate” is a boolean; consider MinMax 0–1 instead of 0–10.A tighter range improves readability without forcing it to the bottom artificially.
case "debug[5]": // Packet Duplicate boolean return { power: 1.0, MinMax: { min: 0, - max: 10, + max: 1, }, };
828-835: FEEDFORWARD_LIMIT jitter attenuator range: confirm scale (0–100 vs 0–1000).Presenter shows it as a percent; this block uses max: 1000. If raw is percent (0–100), set max 100; if raw is percent×10 or ×100, keep as-is but align friendly formatting and ConvertDebugFieldValue.
- case "debug[0]": // Jitter Attenuator + case "debug[0]": // Jitter Attenuator (percent) return { power: 1.0, MinMax: { min: 0, - max: 1000, + max: 100, }, };src/flightlog_fields_presenter.js (5)
2081-2094: Add explicit units/format for RX_TIMING LQ and booleans.LQ is a percentage; booleans can remain numeric, but a clearer label helps tooltips.
case "RX_TIMING": switch (fieldName) { case "debug[0]": // Packet interval us/10 case "debug[3]": // Constrained packet interval us/10 return `${(value / 100).toFixed(2)} ms`; case "debug[1]": // Packet time stamp us/100, divide by 10 to ms return `${(value / 10).toFixed(1)} ms`; case "debug[4]": // Rx Rate Hz case "debug[5]": // Smoothed Rx RateHz return `${value.toFixed(0)} Hz`; - case "debug[6]": // LQ Percent - case "debug[2]": // isRateValid boolean - case "debug[7]": // Is Receiving Signal boolean + case "debug[6]": // LQ Percent + return `${value.toFixed(0)} %`; + case "debug[2]": // isRateValid boolean (0/1) + case "debug[7]": // Is Receiving Signal boolean (0/1) default: return value.toFixed(0); }
2302-2318: FEEDFORWARD decode: consider adding unit to debug[3] (‘°/s’).For consistency with other angle-rate values.
case "debug[3]": // rcCommand Delta integer * 10 - return `${(value / 10).toFixed(1)}`; + return `${(value / 10).toFixed(1)} °/s`;
2319-2334: FEEDFORWARD_LIMIT decode vs chart scaling: confirm jitter attenuator units.Presenter returns “value %” for debug[0], while graph_config uses MinMax 0–1000. If raw is percent, use 0–100 in graph_config; if raw is percent×10 or ×100, add matching conversion in ConvertDebugFieldValue and update the decode to divide accordingly.
1444-1465: Friendly names: tighten FEEDFORWARD yaw labels for consistency.Current: “Yaw FF BaseAmt”, “Yaw FF HoldAmt”. Suggested: “Yaw FF Base”, “Yaw FF Hold”.
DEBUG_FRIENDLY_FIELD_NAMES.FEEDFORWARD = { "debug[all]": "Feedforward [debug_axis]", "debug[0]": "Setpoint, un-smoothed", "debug[1]": "Delta, smoothed", "debug[2]": "Boost, smoothed", "debug[3]": "rcCommand Delta", "debug[4]": "Jitter Attenuator", "debug[5]": "Duplicate Flag", - "debug[6]": "Yaw FF BaseAmt", - "debug[7]": "Yaw FF HoldAmt", + "debug[6]": "Yaw FF Base", + "debug[7]": "Yaw FF Hold", };
3062-3074: FEEDFORWARD_LIMIT: add conversion for jitter attenuator (debug[0]) if raw is percent×100 or ×10.Currently only PT1K (debug[6]) is converted. If jitter is encoded (e.g., pct×100), add the symmetric conversion to keep charts aligned with decode and MinMax.
case "FEEDFORWARD_LIMIT": switch (fieldName) { - // case "debug[0]": // jitter attenuator percent + case "debug[0]": // jitter attenuator percent (confirm raw scale) + // If raw = percent×100 -> use /100; if raw = percent (0–100) -> return value unchanged. + return toFriendly ? value / 100 : value * 100; // case "debug[1]": // max setpoint rate for the axis // case "debug[2]": // setpoint // case "debug[3]": // feedforward // case "debug[4]": // setpoint speed un-smoothed // case "debug[5]": // setpoint speed smoothed case "debug[6]": //feedforward smoothing PT1K * 1000 show as 0.nnn return toFriendly ? value / 1000 : value * 1000; // case "debug[7]": // smoothed RxRateHz default: return value; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/flightlog_fields_presenter.js(7 hunks)src/graph_config.js(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.745Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
📚 Learning: 2025-08-14T02:50:17.745Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.745Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
Applied to files:
src/flightlog_fields_presenter.jssrc/graph_config.js
📚 Learning: 2025-08-14T02:03:17.510Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:2304-2318
Timestamp: 2025-08-14T02:03:17.510Z
Learning: PT1K values in Betaflight debug output have a range of 0-1, and are intentionally scaled by dividing by 10 to display as 0-100 with one decimal place for better readability in the blackbox viewer.
Applied to files:
src/flightlog_fields_presenter.js
📚 Learning: 2025-08-14T02:49:32.956Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:49:32.956Z
Learning: User ctzsnooze prefers to learn and implement code changes manually rather than having complete refactor solutions provided, as they want to understand the codebase through hands-on experience.
Applied to files:
src/flightlog_fields_presenter.js
🧬 Code Graph Analysis (2)
src/flightlog_fields_presenter.js (3)
src/graph_spectrum_calc.js (1)
value(466-466)src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)
src/graph_config.js (2)
src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)
🔇 Additional comments (5)
src/graph_config.js (3)
592-599: PT1K scaling for RC_SMOOTHING debug[4] looks correct (0–1).Matches the 0.xxx PT1K convention and aligns with presenter’s /1000 conversion.
961-971: RX_TIMING intervals: 0–30 ms cap is sensible; confirm timestamp vs interval semantics for debug[1].debug[1] is “Frame time stamp us/100” (a timestamp proxy). If it’s not bounded to ~30 ms deltas, the 0–30 cap may clip. Otherwise LGTM.
770-822: AI summary inconsistency: ranges differ from code.The AI summary claims:
- RC_SMOOTHING debug[6]/[7] 0–3. Code uses 0–50.
- FEEDFORWARD_LIMIT debug[0] 0–100. Code uses 0–1000.
Flagging for awareness; code takes precedence.
Also applies to: 828-871, 961-999, 580-608
src/flightlog_fields_presenter.js (2)
2014-2023: RC_SMOOTHING decode: PT1K 0.nnn and Hz labels look good.Consistent with /1000 PT1K scaling and graph ranges.
2029-2034: RC_SMOOTHING_RATE decode: units are consistent.
- debug[0] correctly converted to ms.
- debug[2] labeled Hz.
All good.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/graph_config.js (1)
772-821: FEEDFORWARD debug[6]/[7] are PT1K (non-negative) — don't make them signed; unify scaling & presentationShort: I verified the repo. src/graph_config.js currently sets FEEDFORWARD debug[6]/[7] MinMax {min: 0, max: 1000}, while FEEDFORWARD_LIMIT and RC_SMOOTHING use PT1K (0..1) and the presenter converts by /1000. These yaw FF fields are non-negative PT1K values — we should not change them to allow negatives. Instead make scaling/presentation consistent (use 0..1 in graph config and format as 0.nnn in the presenter).
Files to update
- src/graph_config.js — FEEDFORWARD case, debug[6]/debug[7] (current block around lines ~812–820)
- src/flightlog_fields_presenter.js — FEEDFORWARD display & conversion (presentation around ~2302–2317; value conversion around ~3048–3060)
Suggested changes
- graph_config.js: treat yaw FF fields as PT1K 0..1 (update MinMax max to 1 and add comment).
case "debug[6]": // Yaw feedforward case "debug[7]": // Yaw feedforward hold element return { power: 1.0, MinMax: { - min: 0, - max: 1000, + min: 0, + max: 1, // PT1K scaling (0..1) }, };
- flightlog_fields_presenter.js: present FEEDFORWARD debug[6]/[7] as 0.nnn and add to/from conversion so friendly display matches graphs.
Presentation (string formatting):
case "debug[4]": // jitter attenuator percent return `${value.toFixed(0)} %`; + case "debug[6]": // Yaw FF BaseAmt (PT1K) + return `${(value / 1000).toFixed(3)}`; + case "debug[7]": // Yaw FF HoldAmt (PT1K) + return `${(value / 1000).toFixed(3)}`;Conversion (toFriendly / fromFriendly parity):
- // case "debug[6]": // yaw feedforward - // case "debug[7]": // yaw feedforward hold element + case "debug[6]": // yaw feedforward (PT1K) + return toFriendly ? value / 1000 : value * 1000; + case "debug[7]": // yaw feedforward hold element (PT1K) + return toFriendly ? value / 1000 : value * 1000;Rationale: keeps FEEDFORWARD consistent with FEEDFORWARD_LIMIT and RC_SMOOTHING PT1K handling and avoids misleading charts if values are shown as 0..1000 while intended to be 0.000–1.000.
♻️ Duplicate comments (1)
src/graph_config.js (1)
600-608: Outlier/Valid Count should likely be 0–3; avoid “burying” with max: 50Comments and firmware semantics indicate counts in the 0–3 range. Setting max to 50 will compress the signal and make values harder to distinguish. If the intent is to keep them visually unobtrusive, consider a separate axis or a toned color rather than an exaggerated MinMax range.
Proposed patch (use 0–3 as previously discussed):
case "debug[6]": // outlier count 0-3 case "debug[7]": // valid count 0-3 return { power: 1.0, MinMax: { min: 0, - max: 50, // put them at the very bottom + max: 3, }, };If you still prefer to “pin” them low, add an inline comment explaining the rationale to avoid future “fixes” back to 0–3.
🧹 Nitpick comments (1)
src/graph_config.js (1)
961-999: RX_TIMING ranges: please confirm units/semantics for debug[1]
- 0–30 ms for intervals (debug[0] and [3]) is sensible.
- debug[1] is labeled “Frame time stamp us/100” but is also scaled to 0–30. If this is an absolute timestamp (us/100), it will exceed 30 quickly; if it’s an inter-frame delta already converted to ms, the label should reflect that. Consider either:
- relabeling debug[1] to clarify it’s an interval/delta, or
- switching debug[1] to getCurveForMinMaxFields(fieldName) if it truly is an absolute, rolling timestamp.
Boolean (debug[2], [7]) at 0–10, Rx Rate (debug[4], [5]) at 0–1200, and LQ (debug[6]) at 0–100 look consistent.
If debug[1] is a delta in ms, adjust the comment; if it’s absolute us/100, use an auto range:
- case "debug[1]": // Frame time stamp us/100 - case "debug[3]": // constrained interval in ms + case "debug[1]": // frame interval in ms (confirm) + case "debug[3]": // constrained interval in ms return { // start at bottom, scale up to 30ms power: 1.0, MinMax: { min: 0, max: 30, }, };Or, if absolute timestamp:
- case "debug[1]": // Frame time stamp us/100 - return { - // start at bottom, scale up to 30ms - power: 1.0, - MinMax: { min: 0, max: 30 }, - }; + case "debug[1]": // Frame time stamp us/100 (absolute) + return getCurveForMinMaxFields(fieldName);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/graph_config.js(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.745Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
📚 Learning: 2025-08-14T02:50:17.745Z
Learnt from: ctzsnooze
PR: betaflight/blackbox-log-viewer#851
File: src/flightlog_fields_presenter.js:0-0
Timestamp: 2025-08-14T02:50:17.745Z
Learning: In the Betaflight blackbox log viewer, PT1K debug fields should use 0.xxx format with /1000 scaling for proper chart display, as confirmed by ctzsnooze for the FEEDFORWARD and FEEDFORWARD_LIMIT debug modes.
Applied to files:
src/graph_config.js
🧬 Code Graph Analysis (1)
src/graph_config.js (2)
src/graph_legend.js (1)
fieldName(191-191)src/graph_minmax_setting_menu.js (1)
fieldName(896-896)
🔇 Additional comments (4)
src/graph_config.js (4)
580-591: RC_SMOOTHING rate/cutoff ranges (Hz) look correct0–1200 Hz for current/smoothed Rx rate and setpoint/throttle cutoffs aligns with typical ELRS/CRSF/DSMX rates and maintains consistency with other places in this file.
592-599: PT1K mapped to 0–1 is correct for chart unitsThe 0–1 MinMax for the pt1K field matches the expected /1000 scaling and will display as 0.xxx on the chart (in line with prior guidance).
827-873: FEEDFORWARD_LIMIT per-index ranges are a strong improvementPer-field ranges (±maxDegreesSecond for setpoint-related, ±200 for speed/FF, 0–1 for pt1K, and 0–1200 Hz for smoothed Rx rate) align with the new debug indices and should render well without manual tweaks. Nice work.
270-291: Conclusion: ConvertFieldValue → ConvertDebugFieldValue is authoritative for numeric MinMax conversion; decodeDebugFieldToFriendly is presentation-only — no mismatch found.Short summary: GraphConfig calls FlightLogFieldPresenter.ConvertFieldValue(...) to convert logged MinMax into chart units. ConvertFieldValue delegates debug[...] to ConvertDebugFieldValue (numeric scaling), while decodeDebugFieldToFriendly only formats strings for UI. Both ConvertDebugFieldValue and decodeDebugFieldToFriendly implement the 4.6 special-case (FFT_FREQ) consistently.
Verified locations / examples:
- src/graph_config.js — getCurveForMinMaxFields (≈270–291) calls FlightLogFieldPresenter.ConvertFieldValue(...) for mm -> chart units.
- src/flightlog_fields_presenter.js
- decodeDebugFieldToFriendly (≈1830–2030) — presentation formatting (e.g. RC_SMOOTHING debug[4] shown as (value/1000).toFixed(3)).
- FlightLogFieldPresenter.ConvertFieldValue (≈2360–2395) — numeric conversion entrypoint.
- FlightLogFieldPresenter.ConvertDebugFieldValue (≈2570–3095) — per-index numeric mappings, e.g.:
- RC_SMOOTHING debug[4]: toFriendly ? value/1000 : value*1000 (matches GraphConfig MinMax 0..1)
- FEEDFORWARD debug[3]: toFriendly ? value/10 : value*10 (matches GraphConfig MinMax 0..100)
- FEEDFORWARD_LIMIT debug[6]: toFriendly ? value/1000 : value*1000 (matches GraphConfig MinMax 0..1)
Result: no changes required — the numeric conversions in FlightLogFieldPresenter align with the hard-coded MinMax values in GraphConfig and the 4.6 handling is consistent.
|
|
Preview URL: https://eff8e480.betaflight-blackbox.pages.dev |
Updating names to match PR 14411 for debugs
RC_SMOOTHING
RC_SMOOTHING_RATE
RX_TIMING
FEEDFORWARD
FEEDFORWARD_LIMIT
Intent is that these debug parameters in firmware PR14411 (for 4.6) will show up correctly, with proper value scaling, in Blackbox.
I have a question about the 'duplication' of older style value scaling methods as in
FlightLogFieldPresenter.decodeDebugFieldToFriendly, which allowed setting the number of decimals and the 'units' of the value. And now there is a newer method inFlightLogFieldPresenter.ConvertDebugFieldValue, which contains a duplication of most of the older things, but has a different syntax, and doesn't appear to be able to set units.Just not sure which to use, and how to control the number of decimals and set the units in the new method.
For example, in the case here there is an original configuration of a debug already present in
FlightLogFieldPresenter.decodeDebugFieldToFriendly, if this is left alone, and a new '4.6' set of names is added, will the 'new' configuration inFlightLogFieldPresenter.ConvertDebugFieldValuetake precedence for 4.6?To do:
Summary by CodeRabbit