Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mavlink] Parameter to always start on USB #22234

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

dakejahl
Copy link
Contributor

@dakejahl dakejahl commented Oct 17, 2023

Added parameter and logic to always start mavlink on USB instead of auto-detecting the protocol. The mavlink mode can now be selected as well. I also refactored the code into a module and moved it into the LP px4 work queue. I refactored everything into functions and a state machine.

Added parameters
SYS_USB_AUTO
USB_MAV_MODE

The problem being solved
When running mavlink-router on a companion computer, no data will be routed until a GCS connects. This is because in typical mavlink-router configuration there is only one Server endpoint which corresponds to the GCS. The GCS will send a heartbeat over the mavlink network which triggers the FMU to start mavlink on USB.

PX4 Behavior before PR
PX4 waits to receive data on USB and auto-detects the protocol (mavlink, nsh, ublox serial passthru).

PX4 Behavior after PR
SYS_USB_AUTO is set to Mavlink by default. The mavlink instance will be started as soon as USB is available rather than wait and auto-detect the protocol. This is better default behavior for users and end products.

How it was tested
ARK Jetson (v6x) : Validated mavlink-routerd initates comms after immediately receiving data on USB.
Pixhawk4 (v5): Validated connection to QGC, picocom, and u-center

  • (SYS_USB_AUTO=2) Start mavlink
  • (SYS_USB_AUTO=1) Detect mavlink
  • (SYS_USB_AUTO=1) Detect nsh
  • (SYS_USB_AUTO=1) Detect ublox

Closes #20631

@dagar
Copy link
Member

dagar commented Oct 17, 2023

Thanks @dakejahl.

How about we simply handle sercon + mavlink USB start in rcS if the parameter is set? Then the relatively low level autostart code doesn't need to poke into parameters, and we just need to make sure it stays quiet and out of the way if sercon is already started.

@dagar dagar marked this pull request as draft October 18, 2023 17:29
@dakejahl
Copy link
Contributor Author

dakejahl commented Oct 24, 2023

I've updated the PR. I moved the autoselect driver into a runtime work_queue module and added the mavlink startup into rcS. I also introduced another parameter to allow selecting the mode for the USB mavlink.

Tested succesfully:

  • mavlink force start w/ multiple modes
  • autostart mavlink
  • autostart nsh

I did not test ublox serial passthrough.

@dakejahl dakejahl requested a review from dagar October 24, 2023 20:05
@dakejahl dakejahl marked this pull request as ready for review October 24, 2023 20:05
@dakejahl
Copy link
Contributor Author

dakejahl commented Oct 27, 2023

Starting the mavlink stream in rcS actually won't work on boards that don't have VBUS connected when the flight controller boots (ark jetson carrier). The linux side enables the VBUS after it boots, which means USB shows up later after the start mavlink /dev/ttyACM0 in rcS has already failed. I didn't catch this initially because I was just rebooting the FC and not the entire board.
If you try to start it in rcS before VBUS is high sercon && sleep 5 && mavlink start -d /dev/ttyACM0 fails to open the port

ERROR [mavlink] could not open /dev/ttyACM0

@dakejahl dakejahl marked this pull request as draft October 27, 2023 23:26
@mcsauder
Copy link
Contributor

This can only be done if the driver is started in the rcs and runs an retries continously. I know this because of the year I spent rewriting rcs.

@dakejahl dakejahl force-pushed the pr-force_mavlink_usb branch 3 times, most recently from 9782ccb to a16232b Compare October 28, 2023 04:26
@mcsauder
Copy link
Contributor

Rcs runs once. You get one chance at boot. If you want anything to start after boot you need to kick off a process that monitors and doesn't die until shutdown.

@dakejahl dakejahl marked this pull request as ready for review October 28, 2023 21:52
@dakejahl
Copy link
Contributor Author

The only piece I can't figure out is how to make the cdcacm_autostart/Kconfig depend on SYSTEM_CDCAMC and MODULES_MAVLINK. It would be nice to use the depends on property but it doesn't seem to work. The alternative is going through every .px4board and adding DRIVERS_CDCACM_AUTOSTART=y where appropriate, but this seems fragile and a PITA.

This doesn't work

menuconfig DRIVERS_CDCACM_AUTOSTART
	bool "cdcacm_autostart"
	default y
	depends on SYSTEM_CDCACM && MODULES_MAVLINK
	---help---
		Enable support for cdcacm_autostart 

@dagar do you know how to do this properly?

@dakejahl dakejahl marked this pull request as draft October 29, 2023 00:12
@PetervdPerk-NXP
Copy link
Member

Your KConfig sample looks to okay. @dakejahl in boardconfig you can select the symbol and type the ? key. There you can see how the dependencies are resolved.

@dakejahl
Copy link
Contributor Author

I think it doesn't work because SYSTEM_CDCACM is part of the nuttx Kconfig not PX4

@PetervdPerk-NXP
Copy link
Member

I think it doesn't work because SYSTEM_CDCACM is part of the nuttx Kconfig not PX4

Yeah NuttX kconfig isn't exposed to PX4 Kconfig.

@dakejahl
Copy link
Contributor Author

@dagar let me know if there's any further changes you want to see here! Otherwise this is good to go.

@dakejahl dakejahl changed the title Parameter to force start mavlink on USB [mavlink] Parameter to always start on USB Nov 20, 2023
@AlexKlimaj
Copy link
Member

@dagar Testing this now on the HaLow radios and its needed. Working well. onboard default mavlink usb brings the HaLow radios CPU to 100%. With this PR and minimal mavlink mode on the usb it works well.

@dakejahl
Copy link
Contributor Author

@dagar mentioned there is a script that auto sorts the KConfig files and suggested I make sure this PR matches that. I'm not sure where said script is to be found. The DRIVERS_CDCACM_AUTOSTART only needs to be applied to boards with USB. I can remove the alphabetization of the KConfig files if that would make it easier to merge.

@dakejahl
Copy link
Contributor Author

This continues to be a problem. Honestly I'd say we backport to 1.14 so that newbies don't struggle so hard with this.
https://discord.com/channels/1022170275984457759/1169554115827876001

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/issue-with-waiting-for-heartbeat-message-in-uav-project/36858/2

    Refactored cdcacm_init into a module and added a paramter to allow always starting mavlink on
    USB, also added a paramter to choose the mode. The current default behavior is to wait and listen
    for data on USB and auto-detect the protocol (mavlink, nsh, ublox). This results in the mavlink
    stream not starting until something else on the mavlink network sends a packet first. The new
    default behavior is to always start mavlink.

    Added parameters
    MAV_USB_ENABLE -- default 1 (always start mavlink on USB)
    MAV_USE_MODE -- default 3 (onboard)
@dakejahl
Copy link
Contributor Author

dakejahl commented Apr 4, 2024

I removed the PGA460 driver from COMMON_DISTANCE_SENSOR and the LIS2MDL from COMMON_MAGNETOMETER to save 7.6KB flash. The PGA460 driver was only ever used on the Teal One. The LIS2MDL is not used on any other boards.

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-may-15-2024/38773/1

@mrpollo
Copy link
Contributor

mrpollo commented May 15, 2024

Let's make sure the old parameters are still there.

@dagar will check

@dakejahl
Copy link
Contributor Author

I took care of it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review/test
Development

Successfully merging this pull request may close these issues.

Add parameter to enable mavlink stream over USB
7 participants