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

Add support for non-default (1) vehicle ID (SYSID_THISMAV) #740

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

goasChris
Copy link
Contributor

@goasChris goasChris commented Feb 16, 2024

Motivation:
Adds the possibility to use cockpit in multivehicle environments, where people may be using multiple tools to monitor/track different robots and be connected to several mavlink sessions.

I personally use 20+ robots, where I need to gather and track information from multiple at the same time, and our setup and system is built around identifying robots by their ID. It will also allow us to use cockpit to start testing more smoothly without having to tear down entire existing system.

Changes:

  • Add detection, storage, and handling of system_id
    • Ardupilot has protected/export where we store the ID for later use
    • Updated all functions I could find where I believe it to be required
  • Remove/adapt code that excludes all vehicles except for 1
  • Update the VehicleFactory class to account for system_id, and use it when creating vehicles
  • Updated all vehicles to be created with the system_id detected

Devices tested:

  • Tested on SITL with our custom firmware, using mavlink router
  • Tested on physical pi4 running custom rover variant firmware

Tests:

  • Arm/Disarm with a vehicle id of 200
  • Mode changes back and forth
  • Messages, data/info/mavlink packets get displayed

@CLAassistant
Copy link

CLAassistant commented Feb 16, 2024

CLA assistant check
All committers have signed the CLA.

@rafaellehmkuhl rafaellehmkuhl self-requested a review February 17, 2024 16:33
@rafaellehmkuhl
Copy link
Member

Cool! I will take a better look at it on Monday!

And I think @patrickelectric and @Williangalvani will be helpful about your question regarding the target_system: 0.

@goasChris
Copy link
Contributor Author

I got some input from an Ardupilot Dev. 0 is "Broadcast ID"

If the ids are omitted or set to zero then the message is considered a broadcast (intended for all systems/components).
Ref: https://mavlink.io/en/guide/routing.html

Based on this, I believe the correct action is to change the 0 (broadcast) to a specific system, as it is probably what is intended, and it's safer.

@goasChris goasChris marked this pull request as ready for review February 19, 2024 07:19
@rafaellehmkuhl
Copy link
Member

I got some input from an Ardupilot Dev. 0 is "Broadcast ID"

If the ids are omitted or set to zero then the message is considered a broadcast (intended for all systems/components). Ref: https://mavlink.io/en/guide/routing.html

Based on this, I believe the correct action is to change the 0 (broadcast) to a specific system, as it is probably what is intended, and it's safer.

Totally. This was actually a bug by itself.

src/libs/vehicle/ardupilot/arducopter.ts Show resolved Hide resolved
src/libs/vehicle/ardupilot/arducopter.ts Outdated Show resolved Hide resolved
src/libs/vehicle/ardupilot/ardupilot.ts Outdated Show resolved Hide resolved
src/libs/vehicle/ardupilot/ardupilot.ts Outdated Show resolved Hide resolved
src/libs/vehicle/ardupilot/ardupilot.ts Outdated Show resolved Hide resolved
src/libs/vehicle/ardupilot/ardupilot.ts Outdated Show resolved Hide resolved
src/libs/vehicle/ardupilot/ardupilot.ts Outdated Show resolved Hide resolved
@goasChris
Copy link
Contributor Author

Thank you very much for your patience @rafaellehmkuhl
What you said in the your code review made a lot of sense, and I have incorporated the requested changes.
Please give it another look when you have time, and I hope I didn't miss or misunderstand any of the requested changes :)

I've re-tested the basics (arm/dism, mode changes, messages, etc), and it seems to still work after making the changes.

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

Nice!!

I think with those last changes we are good to go!

Excited to have this merged :)

src/libs/vehicle/ardupilot/ardupilot.ts Outdated Show resolved Hide resolved
src/libs/vehicle/ardupilot/ardupilot.ts Outdated Show resolved Hide resolved
src/libs/vehicle/ardupilot/ardupilot.ts Show resolved Hide resolved
Add detection, storage, and handling of system_id
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

That seems perfect, Chris!!

Did you test this last version? If so, we are ready to merge!

@goasChris
Copy link
Contributor Author

Sorry, wrong account.

Tests:
Yeah, again, just the basics. It builds, passes CI, mode changes and arm/dism, messages etc. Checked both vid 1 and 200.

@patrickelectric patrickelectric merged commit d000292 into bluerobotics:master Feb 21, 2024
7 checks passed
@rafaellehmkuhl
Copy link
Member

Perfect Chris! Thanks a lot for the contribution!

@rafaellehmkuhl rafaellehmkuhl added the docs-needed Change needs to be documented label Feb 21, 2024
@goasChris goasChris deleted the non-default-vid branch February 21, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants