Skip to content

Conversation

demvlad
Copy link
Contributor

@demvlad demvlad commented Oct 1, 2025

The additions for MAVLink serial RX provider PR

Summary by CodeRabbit

  • New Features

    • MAVLINK can now be selected as a serial receiver protocol on devices whose firmware/API exposes the new option; it will appear in serial RX lists on supported devices.
  • Style

    • Reformatted telemetry condition checks for readability; no behavioral change.
    • Minor whitespace cleanup in debug update logic; no impact on runtime behavior.

Copy link
Contributor

coderabbitai bot commented Oct 1, 2025

Walkthrough

Adds MAVLINK as a serial RX type behind an API version and build-option gate: imports API_VERSION_1_47, adds FIRMWARE_BUILD_OPTIONS.USE_SERIALRX_MAVLINK = 4109, appends "MAVLINK" in getSerialRxTypes for API >= 1.47 and in getSupportedSerialRxTypes when the build option is present. Also minor formatting in src/js/Features.js and a whitespace cleanup in src/js/debug.js.

Changes

Cohort / File(s) Summary
MAVLINK serial RX support
src/js/fc.js
Import API_VERSION_1_47; add FIRMWARE_BUILD_OPTIONS.USE_SERIALRX_MAVLINK = 4109; getSerialRxTypes now appends "MAVLINK" when API version >= 1_47; getSupportedSerialRxTypes appends "MAVLINK" when build options include USE_SERIALRX_MAVLINK.
Code style cleanup
src/js/Features.js, src/js/debug.js
Features.js: reformatted TELEMETRY OR condition across multiple lines (no logic change). debug.js: removed an extra blank line between two delete statements (no logic change).

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant FC
  participant API as API Version
  participant Build as Firmware Options

  App->>FC: getSerialRxTypes()
  FC->>API: read apiVersion
  alt apiVersion >= 1.47
    rect #E6F4EA
      Note over FC: include "MAVLINK"
      FC-->>App: ["SBUS","CRSF","...","MAVLINK"]
    end
  else
    FC-->>App: ["SBUS","CRSF","..."]
  end

  App->>FC: getSupportedSerialRxTypes(options)
  FC->>Build: inspect options
  alt options include USE_SERIALRX_MAVLINK
    rect #FFF4E6
      Note over FC: add "MAVLINK"
      FC-->>App: ["SBUS","...","MAVLINK"]
    end
  else
    FC-->>App: ["SBUS","..."]
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Suggested reviewers

  • nerdCopter
  • VitroidFPV

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The provided description only contains a brief reference to a related PR and does not remove the default template instructions or include required sections such as a summary of changes, motivation, testing instructions, and related issue links. Therefore, it does not adhere to the repository’s pull request description template. Please remove the template instructions and expand the description to include a detailed summary of changes, the motivation for the update, testing instructions, and links to related issues or pull requests.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed This title clearly summarizes the primary change of adding support for MAVLink serial RX provider settings and is concise and specific to the pull request’s main focus.
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

📜 Recent 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 81cb261 and c75f07d.

📒 Files selected for processing (1)
  • src/js/fc.js (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/fc.js (1)
src/js/data_storage.js (2)
  • API_VERSION_1_47 (5-5)
  • API_VERSION_1_47 (5-5)
🔇 Additional comments (4)
src/js/fc.js (4)

3-3: LGTM!

The import of API_VERSION_1_47 is correctly added and is used in the conditional check on line 809.


87-87: LGTM!

The USE_SERIALRX_MAVLINK build option is correctly defined with sequential value 4109, following the pattern of other serial RX provider options.


808-811: LGTM!

The syntax errors from the previous review have been correctly fixed. The conditional logic properly gates the MAVLINK serial RX type behind API version 1.47.

Minor observation: The blank line on line 808 is slightly inconsistent with the formatting pattern at line 803, but this is purely a stylistic nitpick.


859-861: LGTM!

The implementation follows the same pattern as other serial RX providers (e.g., IRC GHOST on lines 856-858). The build option check is correctly implemented and the string values are consistent with the constant defined on line 87.


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

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

@demvlad demvlad marked this pull request as draft October 1, 2025 20:20
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 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 d5a3604 and de4914c.

📒 Files selected for processing (4)
  • src/js/Features.js (1 hunks)
  • src/js/debug.js (1 hunks)
  • src/js/fc.js (3 hunks)
  • src/js/tabs/firmware_flasher.js (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/js/fc.js (2)
src/js/serial_backend.js (1)
  • options (546-546)
src/js/tabs/receiver.js (1)
  • supportedRxTypes (254-254)
src/js/Features.js (1)
src/js/data_storage.js (2)
  • API_VERSION_1_46 (4-4)
  • API_VERSION_1_46 (4-4)
src/js/debug.js (1)
src/js/utils/array.js (2)
  • addArrayElement (65-69)
  • replaceArrayElement (78-83)
🔇 Additional comments (6)
src/js/debug.js (1)

858-858: LGTM!

Minor whitespace improvement that enhances code readability.

src/js/Features.js (1)

64-74: LGTM!

The TELEMETRY enablement logic correctly includes MAVLINK alongside existing protocols (CRSF, GHST, FPORT, JETI). The implementation is consistent with the established pattern and properly gated by API version checks.

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

294-294: LGTM!

MAVLINK is correctly added to the list of radio protocols with built-in telemetry support. This ensures the UI properly disables telemetry protocol selection when MAVLINK is selected, consistent with other protocols like CRSF and GHST.

src/js/fc.js (3)

87-87: LGTM!

The USE_SERIALRX_MAVLINK constant is properly defined with sequential value 4109, following the established pattern for serial RX protocol build options.


807-807: LGTM!

MAVLINK is correctly added to the serial RX types list for API version 1.46+, following the same pattern as other RX type additions in this block.


856-858: LGTM!

The build option check for MAVLINK support is correctly implemented, following the established pattern for other serial RX protocols. The logic properly adds MAVLINK to supported types when the USE_SERIALRX_MAVLINK build option is present.

@demvlad
Copy link
Contributor Author

demvlad commented Oct 2, 2025

@haslinghuis
I am looking, the congigurator is opened at Android smartphone. Does that mean it can work by Android now?
It had opening error some time ago.

@haslinghuis
Copy link
Member

Only BT for now

@demvlad
Copy link
Contributor Author

demvlad commented Oct 3, 2025

What is interested.
The telemetries type options are disabled in uart setup list when I use mavlink rx provider. Even if i've used manual define in auto generated build config files. IIRC, its can be enabled if DUSE_TELEMETRY_XXXX is defined in firmware build.
How can I set auto generated build config files to debug code at local build? Are it's auto generated by using cloud build option or can I do something? Probably I can have some script for this.

I will add mavlink debug in BBExplorer for the curent versions compatibility

@haslinghuis
Copy link
Member

  • Local build should work
  • When using cloud build + custom defines, it won't be catched 😭
  • Cloud build server options needs to be updated before we can run the script to include this.

@haslinghuis haslinghuis changed the title Added MAVLIK serial RX provider settings Added MAVLink serial RX provider settings Oct 3, 2025
@haslinghuis
Copy link
Member

@demvlad when merged you can use this define - betaflight/betaflight#14693

Copy link
Member

@haslinghuis haslinghuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@demvlad this is ready ?

@demvlad
Copy link
Contributor Author

demvlad commented Oct 4, 2025

I am testing right now.

@demvlad demvlad marked this pull request as ready for review October 4, 2025 16:26
@demvlad
Copy link
Contributor Author

demvlad commented Oct 4, 2025

@demvlad this is ready ?

The MAVLink telemetry and RX provider option are disabled in menues in my local run and in web preview too.
But there are not my problem, i think.
The DEBUG is released in yours PR, it is UNKNOWN, this is normal.
Yes, the PR is ready.

@demvlad demvlad requested a review from haslinghuis October 4, 2025 17:14
demvlad and others added 2 commits October 4, 2025 21:49
demvlad and others added 2 commits October 4, 2025 21:50
@demvlad
Copy link
Contributor Author

demvlad commented Oct 4, 2025

Seems, the Configurator and Blackbox explorer use different Node.js version

@haslinghuis
Copy link
Member

@demvlad

Seems, the Configurator and Blackbox explorer use different Node.js version

Have a PR ready - will take some time to test as official v24 LTS will be available in two weeks from now

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 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 7d44028 and 9f56a1e.

📒 Files selected for processing (1)
  • src/js/fc.js (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/fc.js (2)
src/js/data_storage.js (2)
  • API_VERSION_1_47 (5-5)
  • API_VERSION_1_47 (5-5)
src/js/tabs/receiver.js (1)
  • supportedRxTypes (254-254)
🪛 Biome (2.1.2)
src/js/fc.js

[error] 809-809: expected ( but instead found semver

Remove semver

(parse)

🪛 GitHub Actions: Deployment (PR and Push)
src/js/fc.js

[error] 809-809: Syntax error in build: 'if semver.gte(apiVersion, API_VERSION_1_47))' should be 'if (semver.gte(apiVersion, API_VERSION_1_47))'.


[error] 807-811: There was an error during the build caused by a parsing failure in fc.js (vite-plugin-pwa).

🔇 Additional comments (3)
src/js/fc.js (3)

3-3: LGTM!

The import of API_VERSION_1_47 is correct and the constant is properly defined in src/js/data_storage.js.


87-87: LGTM!

The USE_SERIALRX_MAVLINK build option is correctly added with the next sequential value (4109).


860-862: LGTM!

The logic to check for USE_SERIALRX_MAVLINK build option and add "MAVLINK" to supported types follows the established pattern correctly.

Copy link

sonarqubecloud bot commented Oct 4, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
23.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link
Contributor

github-actions bot commented Oct 4, 2025

@haslinghuis haslinghuis merged commit e65dc56 into betaflight:master Oct 4, 2025
7 of 8 checks passed
@github-project-automation github-project-automation bot moved this from App to Done in 2025.12.0 Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants