Wrong target component ID

[ljalves]

Target component should be the vehicle's comp_id (same as received on its heartbeat) and not MAV_COMP_ID_MISSIONPLANNER.

Example:
https://github.com/mavlink/qgroundcontrol/blob/master/src/MissionManager/MissionManager.cc#L123

MAV_COMP_ID_MISSIONPLANNER is the component ID for the GCS MissionPlanner.

[DonLakeFlyer]

Huh? According to who? QGC has used this for years as does MissionPlanner.

[ljalves]

That is not true.
MissionPlanner does the right thing.
It sends the messages targeting the vehicle sysid and compid as received in its heartbeat:

https://github.com/diydrones/MissionPlanner/blob/master/Mavlink/MAVLinkInterface.cs#L1688

[DonLakeFlyer]

What is this for then: https://github.com/diydrones/MissionPlanner/blob/master/Mavlink/MAVLinkInterface.cs#L613?

[DonLakeFlyer]

And here is APM Planner: https://github.com/diydrones/apm_planner/blob/master/src/uas/UASWaypointManager.cc#L331

[ljalves]

You are mixing target and source systems definition.
What you linked in MissionPlanner is the packet generator function, which correctly sets its system ID to (usually) 255 and component ID to MAV_COMP_ID_MISSIONPLANNER (=190).
Meaning that MissionPlanner is identified in the mavlink bus as (255:190)

Example:
MissionPlanner waits for a heartbeat from systems on the mavlink bus.
Imagining that one vehicle is alive, then his heartbeat identifies the vehicle as (sysid; compid) = (1;1)
Then when MissionPlanner requests the mission list to the vehicle it sends the message as:
mission_request_list: src(255:190) -> target(1;1)
And the vehicle responds to the message with:
mission_list: src(1;1) -> target(255;190)

What qgroundcontrol or APMplanner do is:
mission_request_list: src(255:190) -> target(1;190)
Target responds with:
mission_list: src(1;1) -> target(255;190)

Right now I'm not sure about the usage of the MAV_COMP_ID_MISSIONPLANNER.
Was it added specifically to be used only by MissionPlanner (the GCS)...?

[DonLakeFlyer]

I have no idea. QGC code has done it that way for years. Way before my time. Is there some bug you are running into? QGC currently works fine with missions on both PX4 and ArduPilot stack. PX4 firmware has an explicit check for MAV_COMP_ID_MISSIONPLANNER as well as component all. ArduPilot seems to ignore it. What is the reason for the change?

@LorenzMeier Is there some problem here?

[ljalves]

@DonLakeFlyer not really a problem - just noted that inconsistency while coding the mavlink packet router on AlceOSD - there wasn't consistency on the source and targets (sysID:compID) combo.

I just confirmed on the PX4 code and indeed it does check compID. But it accepts the command if one of these are true:
((_msg.target_component == mavlink_system.compid) ||
(_msg.target_component == MAV_COMP_ID_MISSIONPLANNER) ||
(_msg.target_component == MAV_COMP_ID_ALL))

So I could say that the inconsistency is on PX4 :)
The reply to the mission_request_list should come from the same compID that accepted the message - like this:

mission_request_list: src_gcs(255:190) -> target_uav(1;190)
mission_list: src_uav(1;190) -> target_gcs(255:190)

So I guess no bug here... Sorry for the "noise".

[DonLakeFlyer]

Sorry for the "noise".

No problem. It is odd in that no other messaging uses a special component id like this. So can't hurt to straighten out what the standard is supposed to. Not really sure what it is supposed to be. I'll get the answered.

[edwinhayes]

Sorry to reopen this. In view of the MAVLink routing approach utilised in newer ArduPilot, is this not a problem for non-trivial-but-still-valid configurations?

Based on the rules outlined here: http://ardupilot.org/dev/docs/mavlink-routing-in-ardupilot.html, if the flight controller (cID1) is connected directly to QGroundControl by a dumb link, this would work, because the flight controller falls back to locally processing messages targeting MAV_COMP_ID_MISSIONPLANNER (cID190) since normally it would have no routing table entry for that target.

But if the flight controller is instead located behind another device implementing the same routing behaviour, the message would not be forwarded to the flight controller since there will not be a routing table entry for that cID.

At present, I believe this issue is what prevents missions being transferred correctly across my companion computer (which implements the aforementioned routing rules). As referenced by mrshannon above, a workaround is to always pass messages targeting cID190, but as far as I see, this is just a hack to work around for the underlying issue.

[dogmaphobic]

But if the flight controller is instead located behind another device

What other device? If it's something in between the GCS and the Autopilot, it should be a "bridge" (passes everything through regardless of compID/sysID).

[edwinhayes]

Well, in my case it's a companion computer. But why do you say that is what the behaviour 'should' be? That is explicitly not how the flight controller itself behaves (at least, in the case of ArduPilot - I don't know what PX4 does):

http://ardupilot.org/dev/docs/mavlink-routing-in-ardupilot.html

[edwinhayes]

In fact, I believe that if your companion computer bridges messages blindly as you suggest, then if you are running ArduPilot, and have a complex system with multiple MAVLink Components, issues arise because the fall-back behaviour exhibited by ArduPilot causes it to execute commands that were not intended for the flight controller.

For instance, if you send a reboot command to a device which is connected to your companion computer and which isn't the flight controller, if your bridge sends it to the flight controller as well, the fall back behaviour will result in the flight controller performing a reboot even though the message was not targeted at it.

(Personally, I think this fall back behaviour is horrible, and I have no idea why it's like that).

[hamishwillee]

FWIW I think this is a bug. The component target of a mission request should be an autopilot component - it logically makes no sense for a vehicle to have a component MAV_COMP_ID_MISSIONPLANNER. There is no clear information on what a GCS should use for its component id, but I would assume that MAV_COMP_ID_MISSIONPLANNER makes sense (ie "a mission planner" not "the mission planner")

As to routing, the current official info is empty (more or less) and the pending docs are based on the ardupilot docs ... because they make sense. Upshot @dogmaphobic , your review / counter to this PR would be appreciated because the docs are supposed to reflect what a MAVLink system should/must do.

Another problem example of forwarding everything is the message for sharing message signing keys. If you forward everything then a key sent over USB to an autopilot with an active telemetry path over wifi would automatically be forwarded over radio. Not a good idea.

[edwinhayes]

I agree.

A quick check: Mission Planner uses cID190 for messages it sends. But QGC uses cID0 instead.

From a cursory search, I think the reason for targeting MAV_COMP_ID_MISSIONPLANNER could be that there isn't really a canonical definition for what the intended function of each of the assigned cIDs is; generally you'd think it would be self-explanatory, but in the case of MISSIONPLANNER, I think perhaps someone guessed that it meant some sort of companion computer node which was responsible for management of missions on board the aircraft? Or it could just be a typo.

Regardless, I think it can be fixed by just changing the target to cID1 without breaking anything? Implementations which are successfully parsing messages targeted at cID190 now will very likely already also be listening to cID1?

[hamishwillee]

Regardless, I think it can be fixed by just changing the target to cID1 without breaking anything? Implementations which are successfully parsing messages targeted at cID190 now will very likely already also be listening to cID1?

I would think so. Why don't you rebuild QGC with the change and try it out?

P.S. I will try find out what these ids are are all for: mavlink/mavlink#999

[DonLakeFlyer]

If this is going to change it needs PX4 firmware folks involved. All this ID stuff is ancient history in QGC which I know nothing about as to why it was that way originally. @dagar Any opinions here?

Also the thread is quite confusing as to what change is actually requested. Can someone detail exactly what is requested to be change? Showing it in the QGC code itself would be even better.

[dogmaphobic]

While discussing message forwarding, for documentation purposes, it's also important to point out that if something is in the "middle", messages should be forwarded as is. That is, if this "middle" process inspects a message for any reason, it should forward the original message and not to repackage it. I've seen this and this causes the message counters to get messed up.

[edwinhayes]

@hamishwillee Unfortunately, I don't personally have a suitable setup for either making/building changes, or testing them thoroughly, and since I'm currently down a couple of engineers, I won't be able to get anyone to look at it for a while. Anyway, at this point I don't have that pressing a need for it to be fixed, I just wanted to confirm that people are in agreement as to what the correct behaviour should be, and thus what the problem is.

@DonLakeFlyer Sorry, I haven't actually worked with the QGC codebase before, so I wouldn't be confident to point to the correct thing that needs to be changed. To a cursory glance, I think it's that every instance of "MAV_COMP_ID_MISSIONPLANNER" in PlanManager.cc needs to be replaced with "MAV_COMP_ID_AUTOPILOT1" (or if you want to be pedantic, replaced with something like vehicle.get_compid_for_flight_controller() which usually evaluates to same)...

Regarding involvement from the devs, certainly. Ideally, I'd like some input from whoever was responsible for designing the routing approach in the first place, to understand why the fallback behaviour even exists - on the face of it, it seems kind of insane to me, so I'd like to understand what the justification is.

[DonLakeFlyer]

@dagar Do you know anything about this?

[hamishwillee]

@dogmaphobic

While discussing message forwarding, for documentation purposes, it's also important to point out that if something is in the "middle", messages should be forwarded as is.

Yes indeed. In the PR I say "Forwarded messages must not be changed/repackaged by the forwarding system (the original message is passed to the new link)."

@edwinhayes Understood. There is no way to be certain this is the problem without testing.

Thinking about the above a bit more, I am not certain that this is the cause. In #2907 (comment) you say:

But if the flight controller is instead located behind another device implementing the same routing behaviour, the message would not be forwarded to the flight controller since there will not be a routing table entry for that cID.

But the rules indicate that it would be forwarded - because the vehicle system id is in the routing table (bullet 2). The component id only matters for routing for the current system (bullet 4).

Systems should forward messages to another link if any of these conditions hold:
- It is a broadcast message (`target_system` field omitted or zero).
- The `target_system` does not match the system id *and* the system knows the link of the target system (i.e. it has previously seen a message from `target_system` on the link).
- The `target_system` matches its system id and has a `target_component` field, and the system has seen a message from the `target_system`/`target_component` combination on the link.

@DonLakeFlyer @dagar To be compliant with MAVLink I still think QGC should set the target component id as MAV_COMP_ID_AUTOPILOT1 (1) rather than MAV_COMP_ID_MISSIONPLANNER (190) unless PX4 is advertising a MAV_COMP_ID_MISSIONPLANNER component.

[edwinhayes]

@hamishwillee

Assume Router is located in between FlightController and the ground station, with Router having address S1C2, and FlightController having address S1C1.

If Router receives a message (SRC S255C0 TGT S1C190), this message will not satisfy any of the routing rules:

• It is not a broadcast message.
• The target system does match the sID of Router.
• The target system matches the sID of Router, but Router has not seen any messages from C190 on the link between Router and FlightController.

Hence the message is not forwarded.

It would be a different story if FlightController had previously advertised as C190 (as you mention in the last line of your comment), but in the case of Ardupilot at least, this is not the case.

Oh, I realise re-reading this that perhaps I should have said explicitly; the intermediary node in the problematic case is onboard the aircraft (part of the companion computer), hence shares the same sID: hence (bullet 2) does not apply.

[hamishwillee]

@edwinhayes Thanks for clarification. You are right for this (expected) configuration. Someone would still need to build with the change to verify it is actually the cause.

@DonLakeFlyer @dagar So yes, I think the solution is that QGC should set the target component id as MAV_COMP_ID_AUTOPILOT1 (1) rather than MAV_COMP_ID_MISSIONPLANNER (190). As above, even if not, this is more correct than what QGC does now.