Allow using system ids other than 1 #1581

Williangalvani · 2023-04-06T23:13:52Z

This goes on the assumption that the vehicle is only one system with multiple components.
it relies on mavlink2rest to detect the most recent message received and locks on to that ID

Relevant to #1466

core/libs/commonwealth/commonwealth/mavlink_comm/MavlinkComm.py

ES-Alexander

Mostly looks good to me, although I haven't tested it.

I've left a couple of comments / suggestions :-)

core/libs/commonwealth/commonwealth/mavlink_comm/MavlinkComm.py

ES-Alexander · 2023-04-07T10:23:52Z

core/frontend/src/components/health/HealthTrayMenu.vue

@@ -205,7 +205,7 @@ export default Vue.extend({
  },
  mounted() {
    mavlink2rest.startListening('HEARTBEAT').setCallback((message) => {
-      if (message?.header.system_id !== 1 || message?.header.component_id !== 1) {
+      if (message?.message.mavtype.type === 'MAV_TYPE_GCS' || message?.header.component_id !== 1) {


I'm not certain what this mounted function is for. Is it supposed to indicate that the vehicle is connected to a topside computer, or is it supposed to show that an autopilot is connected and running well, or something else?

Does the current non-zero component ID check mean that being connected to the onboard camera manager counts as being mounted?

this makes the Heartbeat icon beat. This change makes it beat with anything sending a heartbeat that is not a GCS.
We should filter for all valid types here, too.

Ahh, I think I misunderstood the logic - your description clarified what it's actually doing, thanks :-)

That said, I don't think the heartbeat icon should beat if only the camera manager is present, so it should only be beating if there's a valid autopilot connected. Presumably that's what you meant by filtering for valid types?

You should also remove:
MAV_TYPE_GENERIC
MAV_TYPE_ONBOARD_CONTROLLER
MAV_TYPE_GIMBAL
MAV_TYPE_ADSB
and everything after:
MAV_TYPE_CAMERA

core/libs/commonwealth/commonwealth/mavlink_comm/MavlinkComm.py

This goes on the assumption that the vehicle is only one system with multiple components. it relies on mavlink2rest to detect the most recent message received and locks on to that ID

patrickelectric · 2023-04-07T13:14:16Z

core/frontend/src/components/health/HealthTrayMenu.vue

@@ -205,7 +205,7 @@ export default Vue.extend({
  },
  mounted() {
    mavlink2rest.startListening('HEARTBEAT').setCallback((message) => {
-      if (message?.header.system_id !== 1 || message?.header.component_id !== 1) {
+      if (message?.message.mavtype.type === 'MAV_TYPE_GCS' || message?.header.component_id !== 1) {


You should also remove:
MAV_TYPE_GENERIC
MAV_TYPE_ONBOARD_CONTROLLER
MAV_TYPE_GIMBAL
MAV_TYPE_ADSB
and everything after:
MAV_TYPE_CAMERA

patrickelectric · 2023-04-07T16:01:29Z

There are some points where the system id is not defined globally.
Other extensions will fail, services that are not based on python may have the same logic, but identify different system ids.
What should we do with mavlink2rest, that needs the system id when starting from the command line: https://github.com/bluerobotics/BlueOS-docker/blob/537aa6d9f840e5af7b3489d872ef03e691056d98/core/start-blueos-core#L39

Isn't safer and simpler to use the MAV_SYSTEM_ID and use bootstrap to define such variable before starting all dockers ? With that there is a defined and global value for it where the user can configure the system. We could also do that once the parameter SYSID_THISMAV is changed by the user in BlueOS.

Williangalvani · 2023-04-07T16:29:10Z

Isn't safer and simpler to use the MAV_SYSTEM_ID and use bootstrap to define such variable before starting all dockers ? With that there is a defined and global value for it where the user can configure the system. We could also do that once the parameter SYSID_THISMAV is changed by the user in BlueOS.

Things get messy as the parameter can be changed in multiple places, such as QGC.
But I'm thinking of actually doing both:

Have a interface where the user can change the environment variable and SYSID_THISMAV at the same time. This will be a reboot required for blueos due to using environment variables
At the same time, keep the automatic detection, additionally pushing a notification in case it gets triggered, so the user can fix things. Note that this only gets triggered after something goes on with getting data from the configured MAV_SYSTEM_ID

Williangalvani · 2023-04-07T19:49:07Z

I'd like to get this in as is (scope-wise), then as I get the dialog in #1582, I intend to introduce some other vehicle-related configuration in there, as the expected system id

Williangalvani force-pushed the sysid branch from 7a32064 to 00866ca Compare April 6, 2023 23:24

ES-Alexander reviewed Apr 6, 2023

View reviewed changes

core/libs/commonwealth/commonwealth/mavlink_comm/MavlinkComm.py Outdated Show resolved Hide resolved

Williangalvani force-pushed the sysid branch 5 times, most recently from 12f9d6c to ff91798 Compare April 7, 2023 02:33

ES-Alexander reviewed Apr 7, 2023

View reviewed changes

patrickelectric requested changes Apr 7, 2023

View reviewed changes

core: commonwealth: cope with changing vehicle ID

c8638bf

This goes on the assumption that the vehicle is only one system with multiple components. it relies on mavlink2rest to detect the most recent message received and locks on to that ID

Williangalvani force-pushed the sysid branch from ff91798 to c8b9e43 Compare April 7, 2023 12:05

patrickelectric requested changes Apr 7, 2023

View reviewed changes

patrickelectric mentioned this pull request Apr 7, 2023

core: frontend: Implement motor test #1499

Merged

core: frontend: filter heartbeats by vehicle type instead of sysid

593e831

Williangalvani force-pushed the sysid branch from c8b9e43 to 593e831 Compare April 7, 2023 17:07

patrickelectric approved these changes Apr 7, 2023

View reviewed changes

patrickelectric merged commit 083ef95 into bluerobotics:master Apr 7, 2023
5 checks passed

Williangalvani deleted the sysid branch April 7, 2023 20:37

ES-Alexander mentioned this pull request Jun 28, 2023

core: frontend: multi-vehicle support: allow configuring system ID #1829

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using system ids other than 1 #1581

Allow using system ids other than 1 #1581

Williangalvani commented Apr 6, 2023 •

edited by ES-Alexander

ES-Alexander left a comment

ES-Alexander Apr 7, 2023

Williangalvani Apr 7, 2023

ES-Alexander Apr 7, 2023

patrickelectric Apr 7, 2023

Williangalvani Apr 7, 2023

patrickelectric Apr 7, 2023

patrickelectric commented Apr 7, 2023

Williangalvani commented Apr 7, 2023

Williangalvani commented Apr 7, 2023 •

edited

Allow using system ids other than 1 #1581

Allow using system ids other than 1 #1581

Conversation

Williangalvani commented Apr 6, 2023 • edited by ES-Alexander

ES-Alexander left a comment

Choose a reason for hiding this comment

ES-Alexander Apr 7, 2023

Choose a reason for hiding this comment

Williangalvani Apr 7, 2023

Choose a reason for hiding this comment

ES-Alexander Apr 7, 2023

Choose a reason for hiding this comment

patrickelectric Apr 7, 2023

Choose a reason for hiding this comment

Williangalvani Apr 7, 2023

Choose a reason for hiding this comment

patrickelectric Apr 7, 2023

Choose a reason for hiding this comment

patrickelectric commented Apr 7, 2023

Williangalvani commented Apr 7, 2023

Williangalvani commented Apr 7, 2023 • edited

Williangalvani commented Apr 6, 2023 •

edited by ES-Alexander

Williangalvani commented Apr 7, 2023 •

edited