Fix MSP callback on crc error

[haslinghuis]

• isolated fix from Fix port event handling #4661

Summary by CodeRabbit

• Bug Fixes
  • Ensures callbacks are always invoked after data processing, even when integrity checks fail, so the UI can surface errors instead of appearing unresponsive.
  • Improves robustness by safely handling exceptions thrown within callbacks, preventing crashes while logging issues for easier troubleshooting.
  • Reduces console noise by removing unnecessary warnings when no callback is present.

[coderabbitai]

Walkthrough

The callback handling in MSPHelper.prototype.process_data was changed to always invoke the callback with a result object that includes command, data, length, and crcError, irrespective of CRC status. Callback execution is wrapped in try-catch with errors logged. The prior branch that skipped callbacks on CRC error was removed.

Changes

Cohort / File(s)	Summary of Changes
MSP callback handling src/js/msp/MSPHelper.js	Always invoke callback post-parse with { command, data, length, crcError }; compute length via data.byteLength or 0; wrap callback in try-catch and log exceptions; remove branch that avoided callback on CRC error and related warning.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Parser as MSPHelper.process_data
  participant Callback as Client Callback
  Note over Parser: Incoming MSP frame processed
  Parser->>Parser: Validate CRC
  alt CRC valid
    Parser->>Callback: callback({command, data, length, crcError:false})
  else CRC error
    Note over Parser: New behavior: still invoke callback
    Parser->>Callback: callback({command, data, length|0, crcError:true})
  end
  rect rgba(230,240,255,0.5)
    Note right of Parser: Callback wrapped in try-catch
    Callback-->>Parser: May throw
    Parser->>Parser: Catch and console.log error
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Improve msp send #4510 — Adjusts MSPHelper.prototype.process_data CRC and callback invocation flow, similar to this change.

Suggested reviewers

• VitroidFPV
• nerdCopter
• blckmn

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request description only provides a single-line reference to another PR and does not adhere to the repository’s required template, missing the structured guideline section and details such as change summary, coding style considerations, and testing notes.	Please update the pull request description to include the required “## Important considerations when opening a pull request” section from the template, remove placeholder text, and provide a full summary of the changes, motivation, and any relevant testing or issue references.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly and accurately describes the main change to the MSP callback behavior regarding CRC errors, aligning directly with the modifications in the code.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Preview URL: https://pr4665.betaflight-app-preview.pages.dev

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/js/msp/MSPHelper.js (1)

1782-1793: LGTM! Callback handling correctly ensures error notification.

The changes correctly ensure callbacks are always invoked with the crcError flag, allowing them to handle CRC errors appropriately. The try-catch wrapper protects the MSP processing loop from callback errors, and the comment clearly explains the backward compatibility rationale.

The existing callback at line 2487 already checks response.crcError, confirming this approach is correct and aligns with expected usage patterns.

Based on learnings: This simple, straightforward approach aligns well with the preference for avoiding complex callback resolution mechanisms.

Optional improvement for error logging:

Consider including the command name in the error log for easier debugging, consistent with other logging in this file:

             } catch (e) {
-                console.error(`callback for code ${code} threw:`, e);
+                console.error(`callback for code ${code} (${getMSPCodeName(code)}) threw:`, e);
             }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6ecb566 and 2808ccc.

📒 Files selected for processing (1)

• src/js/msp/MSPHelper.js (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:41:44.286Z
Learning: When fixing MSP duplicate handling in Betaflight Configurator, avoid complex changes to callback resolution mechanisms as they can break tab switching functionality. Simple duplicate detection based on code and payload size is safer than complex requestKey-based approaches.

Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:384-391
Timestamp: 2025-09-19T20:42:20.332Z
Learning: Complex MSP duplicate handling fixes in Betaflight Configurator can cause infinite loading messages when changing tabs due to disruption of the callback resolution mechanism. Simple code-only duplicate detection (using this.callbacks.some((instance) => instance.code === code)) is the safer approach that preserves tab switching functionality.

Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.

Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.

📚 Learning: 2025-09-19T20:41:42.451Z

Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.

Applied to files:

• src/js/msp/MSPHelper.js

🧬 Code graph analysis (1)

src/js/msp/MSPHelper.js (2)

src/js/tabs/cli.js (1)

• data (411-411)

src/js/msp.js (2)

• data (74-74)
• data (355-355)

[nerdCopter]

• approving based on #4661 testing