dronecan: log cumulative bus-off count to blackbox slow frame

[daijoubu]

Summary

Adds droneCANBusOffCount to the blackbox slow (S) frame, sourced from the existing dronecanGetBusOffCount() counter, so intermittent CAN bus faults are diagnosable from flight logs instead of requiring a live dronecan CLI session.

Changes

• New droneCANBusOffCount field in the blackbox S-frame (UNSIGNED_VB, PREDICT(0)), gated behind USE_DRONECAN
• Follows the same pattern as the existing escRPM/escTemperature fields (field table row, struct member, write, load)

Testing

• Built KAKUTEH7WING (H7), SPEEDYBEEF405WING (F4), MATEKF765SE (F7), IFLIGHT_BLITZ_ATF435 (AT32), and SITL
• Flashed to KAKUTEH7WING hardware and recorded a blackbox log during a bench test session
• Decoded the log with blackbox_decode --debug: the new S-frame field decodes correctly and at the expected byte length across all 214 slow frames in the log
• Confirmed the counter tracked real CAN bus-off events during the session, incrementing 0 → 1 → 2 monotonically in step with actual bus faults

Code Review

Reviewed with inav-code-review agent — no CRITICAL or IMPORTANT issues found.

[sensei-hacker]

Just an FYI for contributors: The tentative schedule for INAV 10 is to have a full release in mid December. That means RC2 needs to be in early to mid November, which places INAV 10.0RC1 at September 1. Please plan to have any new features for INAV 10.0 ready for RC1 no later than September 1. After that, 10.1 will follow about six to seven months later.

[github-actions]

RAM / Flash usage vs. base branch — commit 94b3925

Target	Flash Δ	RAM Δ
MATEKF405	±0 B (±0.00%)	±0 B (±0.00%)
MATEKF722	±0 B (±0.00%)	±0 B (±0.00%)
MATEKF765	±0 B (±0.00%)	±0 B (±0.00%)
MATEKH743	+40 B (+0.01%)	±0 B (±0.00%)

See RAM/flash optimization guide for techniques to reduce usage.

[github-actions]

Test firmware build ready — commit 94b3925

Download firmware for PR #11729

247 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Blackbox: log DroneCAN cumulative bus-off count in slow (S) frame

✨ Enhancement 🕐 10-20 Minutes

AI Description

• Add a DroneCAN bus-off counter field to the blackbox slow (S) frame.
• Source the field from the existing dronecanGetBusOffCount() cumulative counter.
• Gate logging behind USE_DRONECAN to avoid impacting non-DroneCAN builds.

Diagram

graph TD
  A["DroneCAN driver"] --> B["Bus-off counter"] --> C["loadSlowState()"] --> D["slowHistory (S-state)"] --> E["writeSlowFrame()"] --> F["Blackbox S-frame"] --> G["Log decoder"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Log bus-off transition events (event frame)

• ➕ Captures precise timestamps of each bus-off transition
• ➕ Avoids changing the always-present slow-frame schema for DroneCAN builds
• ➖ Requires introducing/standardizing an event-frame concept and decoder support
• ➖ More implementation and compatibility surface area than a simple cumulative counter

2. Expose via configurable debug fields instead of S-frame

• ➕ No slow-frame schema change; uses existing debug plumbing
• ➕ Allows users to opt-in only when diagnosing
• ➖ Harder to discover in logs and requires correct debug mode selection
• ➖ Consumes limited debug channels and complicates analysis across flights

Recommendation: Current approach (logging the cumulative counter in the slow S-frame) is the best trade-off: it is monotonic, low-overhead, and naturally triggers an S-frame write on change due to slow-state memcmp-based change detection. The main caution is log-format variability across builds (USE_DRONECAN gated fields), which is consistent with existing conditional S-frame fields and should be acceptable as long as header-based decoders are used.

Files changed (1) +17 / -1

Enhancement (1) +17 / -1

blackbox.cAdd 'droneCANBusOffCount' to blackbox slow (S) frame +17/-1

Add 'droneCANBusOffCount' to blackbox slow (S) frame

• Adds a 'USE_DRONECAN'-gated blackbox S-frame field definition and corresponding packed slow-state member. Populates the field from 'dronecanGetBusOffCount()' and serializes it into slow-frame output using unsigned variable-byte encoding.

src/main/blackbox/blackbox.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Inefficient predictor for counter 🐞 Bug ➹ Performance

Description

droneCANBusOffCount is defined with PREDICT(0), so every slow frame re-encodes the full
cumulative count instead of (usually) a 0/1 delta, increasing blackbox log size and write bandwidth
for no benefit. Since dronecanGetBusOffCount() increments monotonically, using PREDICT(PREVIOUS)
will keep encoded values tiny and more robust on constrained logging backends.

Code

src/main/blackbox/blackbox.c[476]

+    {"droneCANBusOffCount",   -1, UNSIGNED, PREDICT(0),      ENCODING(UNSIGNED_VB)},

Evidence

The PR adds the new field with PREDICT(0) in the slow field table, while the codebase already uses
PREDICT(PREVIOUS) for slowly-changing fields in the same S-frame. The underlying source value is a
monotonically increasing counter (busOffCount++), so delta-to-previous encoding will usually be
0/1 and compress far better than absolute values.

src/main/blackbox/blackbox.c[443-447]
src/main/blackbox/blackbox.c[471-477]
src/main/drivers/dronecan/dronecan.c[176-180]
src/main/drivers/dronecan/dronecan.c[241-244]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new Blackbox slow-frame field `droneCANBusOffCount` is encoded with `PREDICT(0)`, which forces the encoder to store the full cumulative counter value every time it is logged. Since this value is a monotonically increasing counter, the delta versus the previous slow frame is typically 0 or 1; encoding the delta improves compression and reduces blackbox bandwidth/log size.
### Issue Context
Other slow-frame fields already use `PREDICT(PREVIOUS)` where it improves compression (e.g. `rxUpdateRate`, `escTemperature`). The DroneCAN bus-off counter is incremented over time, so it is a good candidate for `PREDICT(PREVIOUS)`.
### Fix Focus Areas
- src/main/blackbox/blackbox.c[470-478]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[sensei-hacker]

Thanks for the PR — logging the cumulative DroneCAN bus-off count to the blackbox slow frame is a nice, small addition.

While reviewing I noticed the Qodo finding and wanted to weigh in: the new field is declared with PREDICT(0), which re-encodes the full cumulative count every slow frame. Since dronecanGetBusOffCount() increments monotonically, PREDICT(PREVIOUS) would keep the encoded values tiny (usually a 0/1 delta) and reduce blackbox log size/write bandwidth — the codebase already uses PREDICT(PREVIOUS) for slowly-changing fields in the same S-frame. Could you switch it to PREDICT(PREVIOUS)?

One question from me: is the raw cumulative count what you want to log, or would a delta/rate also be useful for diagnosing bus-off events in the field? The cumulative count is great for "has this ever happened", but for "when is it happening" a per-frame delta in a faster frame might complement it. Happy to defer to your intent — just curious.

Also: the PR targets maintenance-10.x, which is correct for a 10.0 enhancement.

Other than the predictor question, this looks clean — +17/−1, no RAM/flash impact. Would you be able to make the PREDICT(PREVIOUS) change and push?

[daijoubu]

Thanks for taking a look, and for the FYI on the 10.0 RC1 date.

On the predictor: pushing back on the premise rather than repeating my earlier trace (full analysis here) — PREDICT(PREVIOUS) wouldn't actually shrink this field, and it turns out that's by design, not just an artifact of the two decoders I checked. Per docs/development/Blackbox Internals.md: "All Slow frames are logged as intraframes. An interframe encoding scheme can't be used for Slow frames...", while PREDICT(PREVIOUS) ("Predict last value") is documented as "the most common predictor in interframes." Slow frames can never use interframe encoding at all, so declaring an interframe predictor on an S-frame field is a spec-level mismatch, not just a missed optimization — and matches exactly what writeSlowFrame() does (writes every field's full absolute value, no delta, regardless of declared predictor).

That actually means the two existing S-frame fields declared PREDICT(PREVIOUS) (rxUpdateRate, escTemperature) are already misdeclared per the format's own rules — not a bug introduced here, but I'd rather not add droneCANBusOffCount as a third. If you're up for it, I think the right fix is reclassifying those two to PREDICT(0) to match both the doc and the actual encoding; happy to write that up as a follow-up issue.

On cumulative vs. delta/rate: cumulative-only, deliberately. Bus-off fires when the CAN TEC (Tx error counter) hits 256, incrementing 8 per transmit error — at today's DroneCAN transmit rate that's 30+ seconds of accumulated errors before bus-off actually trips (it'll shorten once actuator traffic increases, but the accumulation window doesn't go away). That window means a bus-off event can't be cleanly tied back to the specific environmental condition that caused the underlying transmit errors regardless of how finely we log it — a delta/rate field wouldn't fix that. What this field is actually for is coarser: a blackbox record that the CAN hardware was bad, so if sensor data went bad or servo control was lost in the same flight, that's attributable to a hardware fault rather than a software bug. I also considered logging the raw TEC/REC counters, but they're too fast-changing for the slow frame to begin with — they change faster than the DroneCAN service even dispatches, so any sample would be a near-random snapshot rather than meaningful state, and some nonzero TEC/REC is normal anyway. That's better suited to live CLI inspection during bench testing than blackbox logging.

Given RC1 is close, my suggestion is to merge this as-is (PREDICT(0), cumulative count) and track the rxUpdateRate/escTemperature predictor mismatch as a separate follow-up rather than block here.