UI broken when editing multiple non-default component params

[edwinhayes]

Expected Behaviour

When connected to a system comprised of multiple MAVLink components, the advanced parameter view should display a list of component IDs on the LHS (underneath the category groups for the default component). Clicking on each component ID in turn should populate the table on the RHS with the parameters for that component.

Current Behaviour

Clicking on the first non-default component shows the table of its parameters as expected. Clicking on another non-default component does not change the table to show the parameters for the new component, the table appears not to update, so continues to show parameters for the first non-default component.

Clicking on one of the categories for the default component still has the expected behaviour, after which you can then click on another non-default component as expected. So it's possible to view the parameters for many components by alternating between viewing parameters for the default component, and one of the non-default components.

Steps to Reproduce:

Please provide an unambiguous set of steps to reproduce the current behavior

1. Be connected to a system with at least three MAVLink components (the default/autopilot, plus two others).
2. Navigate to the advanced parameter view.
3. Click on the menu item for one of the categories on the default component. Works as expected.
4. Click on the menu item for non-default component A (say cID = 2). Works as expected.
5. Click on the menu item for non-default component B (say cID = 3). Table fails to update.
6. Click on the menu item for one of the categories on the default component. Works as expected.
7. Click on the menu item for non-default component B. Works as expected.
8. Click on the menu item for non-default component A. Table fails to update.

System Information

When posting bug reports, include the following information

• Operating System: macOS
• QGC Version: 3.4.x
• QGC build: Daily from earlier this week.
• Flight Controller: Pixhawk2 + Other avionics
• Autopilot (with version): ArduCopter 3.6.0

Detailed Description

Log Files and Screenshots

I will capture screenshots to explain this better when back in the office on Monday.

[DonLakeFlyer]

Thanks for the detailed report

[edwinhayes]

Now the UI updates as expected, but for some reason, some components parameters are displayed correctly, but for others, all the parameter values show up as zero.

See the two pics below; cID 240 and cID 38 are two instances of the same code running on a companion computer. 38 has two additional parameters defined (at runtime), but they are otherwise same, and its parameters are definitely not all zero.

The param messages are presumably getting to QGC, since it populates the list, but then something weird seems to happen.

[edwinhayes]

I can capture some of the QGC log messages if you tell me which would be helpful; turning on all the ones related to "parameter manager" creates about a million pages of text which you probably don't want.

[DonLakeFlyer]

Is it just the parameter editor list display that is wrong. Can you click on one to open the editor dialog? That will show the value for the parameter. If that is 0 as well, then there is some sort of protocol problem.

[edwinhayes]

The affected values also appear as zero in the editor dialog.

[DonLakeFlyer]

Can you capture a ParameterManagerLog and attach.

[edwinhayes]

Attached, but a cursory search doesn't show anything related to any component other than cID 1?

This was captured by first connecting with the logging disabled, enabling it, then refreshing the parameters, to minimise the amount of unrelated stuff in the log.

paramLog.txt

[DonLakeFlyer]

Wow, some really strange things going on with parameter download. Can you also turn on ParameterManagerVerbose1Log

[DonLakeFlyer]

It looks like component ids = -1 are getting through the system somehow.

[DonLakeFlyer]

No there is a bug in the logging. Let me fix that, and then log again with the next build.

[edwinhayes]

Assuming this makes it in, I will test against the next daily build.

[edwinhayes]

Here you go; with ParameterManagerVerbose1Log.

paramLog.txt

[DonLakeFlyer]

If you look in the log you can see that for example the majority of the AIRSIDE_PORT values are coming across as 0. The log should you the raw values from the mavlink message. So not a QGC side problem.

[edwinhayes]

The values are definitely not being sent as zeroes, because my software receives and decodes as expected; it must be some kind of protocol incompatibility between QGC and my software. I suspect something to do with casting the value to the right type? Hmm, but no idea why one of the instances works when the others don't since they should be the same binary. I'll investigate on my end.

[DonLakeFlyer]

Param values are stored in a struct based on type: mavlink_param_union_t

[edwinhayes]

I've learned something new: Ardupilot has a different interpretation of the parameter protocol (as referenced in APMFirmwarePlugin.cc - line 247).

On the system I'm testing against, while the flight controller is Ardupilot, all my companion computer modules should implement the parameter protocol per the original spec.

So I'm guessing the issue is that, for whatever reason, cID38 (which doesn't work) is being treated as an 'Ardupilot-like' device, while cID240 (which does work) is being treated as a 'PX4-like' device.

[DonLakeFlyer]

That's shouldn't be the case. A FirmwarePlugin is associated with a Vehicle. All mavlink traffic should be routing through that single vehicle to the same FirmwarePlugin for futzing with on ArduPilot vehicles.

[DonLakeFlyer]

all my companion computer modules should implement the parameter protocol per the original spec.

You need to match the ArduPilot parameter spec in your companion code as well.

[edwinhayes]

Is there some way I can efficiently obtain the raw messages which the parameter manager is decoding? Because as far as I can tell, what I'm sending over the wire is identical except for cID, yet one component works and the other doesn't.

[DonLakeFlyer]

Telemetry logs have the raw data in them.

[hamishwillee]

The main and obvious difference is that ardupilot stores and transmits ALL parameters as floats, and that if you're using pymavlink I'm not sure it properly encodes other types. @auturgy might have more useful information about the differences.

Probably not helpful for this case ...

• https://mavlink.io/en/services/parameter.html
• https://github.com/mavlink/mavlink-devguide/pull/140/files

[edwinhayes]

@DonLakeFlyer - Yep, I was just hoping there might be some secret way of just getting the param messages in question, rather than trawling thought the telemetry log file!

So basically:

This message works:
fd 19 00 00 c0 63 f0 16 00 00 02 00 00 00 08 00 07 00 4c 4f 47 5f 4c 45 56 45 4c 00 00 00 00 00 00 00 01 c1 f5

And this one doesn't:
fd 19 00 00 b3 63 25 16 00 00 02 00 00 00 0a 00 09 00 4c 4f 47 5f 4c 45 56 45 4c 00 00 00 00 00 00 00 01 8d f4

Can anyone tell me why? I feel like I'm going insane.

@hamishwillee - I'm talking to some of the AP devs about it currently.

[auturgy]

I’ll ping @peterbarker to make it easier for him to find this.

[hamishwillee]

Great. Just FMI, the component id in the first case is f0 (UDP bridge) and the second is Hex 25 - what sort of component is that?

[DonLakeFlyer]

The scope of the adjustment seems incorrect: it should only be adjusting messages from the flight controller (and any other components which is definitely known to have the same quirk) - it's not reasonable to go modifying messages from components you know nothing about.

I guess. But who knows what other ArduPilot bits expose themselves as another component and do params they same way main component does. Cameras? The rPi wifi APSync stuff? If not, then it could be spec'ed to say other components always follow spec. But I'm not sure if that is currently the case.

[DonLakeFlyer]

@hamishwillee Can you discuss in the next mavlink dev meeting? If it works I would propose specifically saying only MAV_COMP_ID_AUTOPILOT1 has special handling and everything else follows spec. Then it's an easy change in QGC to deal with that.

[edwinhayes]

Re the MAV_COMP_ID_USER thing: I swear at some point in the past, common.xml actually had comments which denoted parts of the comp ID range as being reserved for vendor specific messages (I think maybe there was more like two ranges, each denoted as reserved for distinct purposes). But that seems to be gone now?

If there are multiple devices out there with the other behaviour, seems like a pretty fundamental issue that needs to be addressed, since it breaks the concept of MAVLink compatibility pretty badly?

[DonLakeFlyer]

since it breaks the concept of MAVLink compatibility pretty badly?

Welcome to the world of QGroundControl...

[hamishwillee]

@hamishwillee There is no need; these other cIDs fall within the gaps left in the MAV_COMPONENT enumeration for vendor specific component definitions, and they don't require any special treatment.

Absolutely disagree. Please get an ID for components you plan to use because whatever gap you use may well be filled at some point.

The component id is used for routing and filtering; so even if nothing is broken by this misuse, it is still likely that messages will be pushed around the network that don't need to be.

I have not seen "MAV_COMP_ID_USER "

[edwinhayes]

@hamishwillee It would be unreasonable for common.xml to include component definitions for things which have no meaning to most other users: "UDP loopback bridge to mysterious bit of proprietary software 12". Per my comment to @DonLakeFlyer, I am sure there used to be ranges reserved for vendor definitions, what happened to those?

In my case, all my MAVLink components send heartbeat messages as required by the spec, so routing and filtering works just fine.

[hamishwillee]

@edwinhayes Yes, but when component X uses that ID, they're going to get all your messages forwarded even if their at the end of a separate interface, even if they can't do anything about them.

I don't know about user components. I have a sneaking feeling I've seen them though. Not sure. Have to think about whether they are a good idea. Probably OK as long as there are rules for how they are treated/not treated.

[DonLakeFlyer]

Probably OK as long as there are rules for how they are treated/not treated.

Forward, but don't touch.

[edwinhayes]

@hamishwillee True, but that's my problem: as an aircraft manufacturer, I can control the presence of other MAVLink devices. If some new device appears in the common message set which uses an ID I'm already using, it's up to me to rearrange my allocations, or my products stop working properly. However, the presence of defined ranges for vendor definitions does make this easier.

[hamishwillee]

@DonLakeFlyer

Can you discuss in the next mavlink dev meeting?

That is planned for 3.5 weeks. We're skipping one because no one wants to have meetings on Xmas or Xmas eve. Can you wait?

I think I need more context in order to represent your "case"/what you expect.
I would expect any special handling for a component to be based on the particular component - irrespective of autopilot. If it needs to depend on a service/protocol, then it should be based on the specification.

If something isn't implementing the standard protocol(s) I would have though that would be based on the autopilot in use, as inferred from its heartbeat and via sysid to associated components.

So I don't understand what special handling should be allowed on MAV_COMP_ID_AUTOPILOT1.

And I'd be leaning towards @edwinhayes view that the trigger for managing behaviour for the parameter should not be component id for UDP bridge.

[DonLakeFlyer]

So I don't understand what special handling should be allowed on MAV_COMP_ID_AUTOPILOT1.

ArduPilot and PX4 send PARAM_VALUE mavlink messages in a different format.

[hamishwillee]

@DonLakeFlyer Yes, but don't they both use MAV_COMP_ID_AUTOPILOT1?

Oh, I see, you're saying that you only want to allow modifications to the spec for messages that originate from an autopilot - not from a (say) Camera or UDP bridge?

Does that mean that a UDP bridge would need to do something special if passing info to QGC from ArduPilot (say)

[DonLakeFlyer]

A ground station needs to know that and handle accordingly. Whereas what to do with component != MAV_COMP_ID_AUTOPILOT1 is unclear.

Two options:

1. Follow the PARAM_VALUE format that is used for MAV_COMP_ID_AUTOPILOT1
2. Follow the PARAM_VALUE format which is in the mavlink spec

QGC Currently does 1 except for the UDP_BRIDGE component where it does 2. Which is a bit strange.

[hamishwillee]

@edwinhayes

@hamishwillee True, but that's my problem: as an aircraft manufacturer, I can control the presence of other MAVLink devices. If some new device appears in the common message set which uses an ID I'm already using, it's up to me to rearrange my allocations, or my products stop working properly.
However, the presence of defined ranges for vendor definitions does make this easier.

Well, to be a bit cheeky, you couldn't control the use of QGC on your network.

But hopefully to be less annoying, if you think user ranges will help (and I can see they might) then let's allocate a few items for this purpose and discuss in a PR in the mavlink/mavlink repo. How many IDs do you think you might reasonably need?

No guarantees they will get through, but that is the right channel for getting this change.

[DonLakeFlyer]

Yes, but don't they both use MAV_COMP_ID_AUTOPILOT1?

Yes. Firmware differences are identified from MAV_AUTOPILOT.

[DonLakeFlyer]

The point of user ranges is to not conflict with known device types which may have special handling (like UDP_BRIDGE). The only possible thing on user range component is support for the parameter protocol.

[hamishwillee]

@DonLakeFlyer So you think that if we resolve this issue we won't need user ranges?

[DonLakeFlyer]

I think both need to be resolved. There should be a user range for components ids and the parameter protocol handling for != MAV_COMP_ID_AUTOPILOT1 needs to be resolved.

[DonLakeFlyer]

For now I'll change QGC to not translate PARAM_VALUE message for anything other than MAV_COMP_ID_AUTOPILOT1, which is the most likely solution. And then it can be tweaked as needed when the final thing is decided. That will get Edwin working and then we'll see who else complains that we just broke them!

[hamishwillee]

@DonLakeFlyer Sounds like a pragmatic solution.

@edwinhayes Can you please create PR for the user range in mavlink/mavlink so we can have right forum to discuss it.

Don, thanks, #7049 (comment) is the clarification I needed.
So to confirm, once you've made your change, parameters from ArduPilot will have to come in with a component ID of MAV_COMP_ID_AUTOPILOT1 to be properly handled - if they come in with the component ID of the router they will be treated exactly as to the protocol spec. Correct?

[DonLakeFlyer]

Yup

[edwinhayes]

Really, I would like the APM vs PX4 param thing solved somehow; it erodes the whole attraction of 'MAVLink compatibility': will talk to @tridge and @peterbarker about it.

@hamishwillee Sure, I can probably do something tomorrow.

@DonLakeFlyer Thanks, that would be great.

[hamishwillee]

OK, so this will be in the next Dev Meeting agenda 20190102

The most relevant bit is where I suggest the text:

A GCS may provide support for different versions/variants of a protocol.
A GCS is only expected to support versions for messages originating from an autopilot (component ID: MAV_COMP_ID_AUTOPILOT1). Messages from other components should be treated according to the most recent supported version of the specification.

@edwinhayes

Really, I would like the APM vs PX4 param thing solved somehow; it erodes the whole attraction of 'MAVLink compatibility': will talk to @tridge and @peterbarker about it.

I could not agree more/please do. Along with Mission protocol it is the most damaging incompatibility between the two stacks for MAVLink. I know that ArduPilot need to evolve the parameter protocol for their needs. This should be done collaboratively with PX4 to represent the next (common) iteration of the protocol. I'm hoping @auturgy will be able to help make that happen.

Sure, I can probably do something tomorrow.

Thanks very much.

[DonLakeFlyer]

A GCS may provide support for different versions/variants of a protocol.

I would not say that. Anything new should follow spec. The fact that ArduPilot does not is just history which I don't know the background of. I would just mention how ArduPilot works and the fact that anything moving forward should not work that way.

[hamishwillee]

You misunderstand. I'm trying to say that this is how GCS should support spec evolution (spec deviations are a fact, but not something that can ever be "supported"). Even though this doesn't cover the deviation case per-se, it allows me to make it clear that a GCS only needs to support variation for MAV_COMP_ID_AUTOPILOT1.

Even so, this is challenging because:

• the version/component filtering feels a bit "out of scope" for the MAVLink definition.
• A GCS might not implement the most recent version of the spec either, so this feels "false"

How about:

Different autopilots may support a particular iteration of a microservice specification. A GCS that provides autopilot-specific support for a particular iteration will typically restrict this to only autopilot components (MAV_COMP_ID_AUTOPILOT1); for other components the GCS should use the most recent version of the specification it supports.

I'm also trying to capture what ArduPilot does different for params in mavlink/mavlink-devguide#140 - what else should I say?

[hamishwillee]

Perhaps this isn't MAVLink scope? It is something that should be send in the QGC dev guide.

[edwinhayes]

Just confirmed that #7094 has fixed the issue which this issue originally described: I now have correct behaviour for both flight controller parameters (with their special ArduPilot treatment) and other component parameters (per the spec). I will chase up the ongoing aspects (incompatibility between stacks etc) separately.