Skip to content

common.xml: add PARAM_ERROR message#2344

Merged
hamishwillee merged 4 commits intomavlink:masterfrom
peterbarker:pr/param-error
Sep 25, 2025
Merged

common.xml: add PARAM_ERROR message#2344
hamishwillee merged 4 commits intomavlink:masterfrom
peterbarker:pr/param-error

Conversation

@peterbarker
Copy link
Copy Markdown
Contributor

allows an error regarding parameter sub-protocol to be passed around

Talking point only, let's not merge it right now...

@dakejahl
Copy link
Copy Markdown
Contributor

dakejahl commented Sep 5, 2025

Makes sense. What is the current behavior in Ardupilot? Emit the unchanged parameter in PARAM_VALUE and the user doesn't know why it didn't update? Or does it not emit PARAM_VALUE and the GCS just keeps retrying and eventually tells the user the operation failed? @DonLakeFlyer would probably have some good input

@DonLakeFlyer
Copy link
Copy Markdown
Contributor

GCS just keeps retrying and eventually tells the user the operation failed

That's what QGC does. Timeout on not getting the value back. Retries up to 5 times. If still doesn't get value back it notifies the user of the write failure.

@DonLakeFlyer
Copy link
Copy Markdown
Contributor

As far as the message itself. Seems like a reasonable thing to add for better feedback to the user. I think there might be more cases to add:

  • Read only param (sort of) - some params are dynamic aren't they. I think ArduPilot has a temperature one something like that which is constantly updated on the vehicle side? PX4 has things like this as well. Settting it from a GCS doesn't make sense since it will just get stomped by the vehicle side.
  • Also maybe a "component id not found" error?

@peterbarker
Copy link
Copy Markdown
Contributor Author

Makes sense. What is the current behavior in Ardupilot? Emit the unchanged parameter in PARAM_VALUE and the user doesn't know why it didn't update? Or does it not emit PARAM_VALUE and the GCS just keeps retrying and eventually tells the user the operation failed? @DonLakeFlyer would probably have some good input

Yeah, if the parameter exists but isn't writable we emit the current value.

If the parameter exists we just pity the poor caller as we currently can't tell them anything!

@peterbarker
Copy link
Copy Markdown
Contributor Author

GCS just keeps retrying and eventually tells the user the operation failed

That's what QGC does. Timeout on not getting the value back. Retries up to 5 times. If still doesn't get value back it notifies the user of the write failure.

Just what MAVProxy does, +/- retry count.

@peterbarker
Copy link
Copy Markdown
Contributor Author

As far as the message itself. Seems like a reasonable thing to add for better feedback to the user. I think there might be more cases to add:

* Read only param (sort of) - some params are dynamic aren't they. I think ArduPilot has a temperature one something like that which is constantly updated on the vehicle side? PX4 has things like this as well. Settting it from a GCS doesn't make sense since it will just get stomped by the vehicle side.

Yeah, we have a whole bunch of read-only variables. Also, we recently added the facility for a Lua script to act as a mediator for setting parameters, which allows really flexible control over what users can fat-finger and what they can't. Adding in a "yeah, that exists but it is read-only" enumeration option should have been obvious in hind-sight, thanks :-)

* Also maybe a "component id not found" error?

Not something ArduPilot would emit AFAICS, but enumeration entries are free :-)

... I'll just also state that I considered adding value as a field but couldn't see how that would work sensibly across multiple different failure cases.

@peterbarker
Copy link
Copy Markdown
Contributor Author

I've updated the PR to add the suggested enumeration entries (and fix the naming on others)

</entry>
</enum>
<enum name="MAV_PARAM_ERROR">
<description>Specifies an error type being regarding the parameter protocol.</description>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<description>Specifies an error type being regarding the parameter protocol.</description>
<description>Parameter protocol error types (see PARAM_ERROR).</description>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

Looks good.

  1. On a param set, if there is an error, would the current value still be emitted?
  2. An error doesn't stop the sequence right? So if someone doesn't have permission they'd get an error, but they could continue to read other params even if you'd normally infer that they don't generally have permissions - it would be up to the GCS to decide globally if it wanted to continue?

@hamishwillee
Copy link
Copy Markdown
Contributor

@julianoes Any thoughts on this?

@peterbarker
Copy link
Copy Markdown
Contributor Author

1. On a param set, if there is an error, would the current value still be emitted?

That's what my current implementation does.

2. An error doesn't stop the sequence right? So if someone doesn't have permission they'd get an error, but they could continue to read other params even if you'd normally infer that they don't generally have permissions - it would be up to the GCS to decide globally if it wanted to continue?

Yep.

@julianoes
Copy link
Copy Markdown
Contributor

julianoes commented Sep 8, 2025

My understanding is that this is a workaround for the fact that the param protocol is poorly specced and doesn't return with an ack/nack but instead sends back the updated (or not updated) value.

PARAM_EXT has addressed this problem using PARAM_EXT_ACK. However, PARAM_EXT is not usable in general because of the poor layout which makes every message 128 byte no matter what's used.

My fear is that we make things somewhat complex by adding this message on top of it. While we make the behavior somewhat better in case both ends support the new PARAM_ERROR, we still can't rely on it: e.g. I'm trying to set a param but I don't get a result. In this case, I don't know whether the param set worked or not because I don't know whether the counterpart doesn't support sending the PARAM_ERROR or whether the PARAM_ERROR message was lost, and I have to retry anyway.

I think the better fix is to specc the PARAM_V3 in a similar way to PARAM_EXT but without the layout problem, and while at it we add the missing error cases that @peterbarker suggests here.

@peterbarker
Copy link
Copy Markdown
Contributor Author

peterbarker commented Sep 8, 2025 via email

@julianoes
Copy link
Copy Markdown
Contributor

I'm just worried that we make the protocol more complex and harder to document while still having the bifurcation with EXT. Consider poor @hamishwillee having to document it (or you 😄). We need v3 anyway, so this would be the chance!

@hamishwillee
Copy link
Copy Markdown
Contributor

I'm just worried that we make the protocol more complex and harder to document while still having the bifurcation with EXT. Consider poor @hamishwillee having to document it (or you 😄). We need v3 anyway, so this would be the chance!

Poor Hamish thinks that this would be relative easy to document because it is a strict addition. I'd be tempted to add this even if we do create a better protocol.

That said:

  • I know @tridge has a cunning plan to create a clean migration path for handling the encoding of the param in the param message itself, as I understand it, by encoding the type in the https://mavlink.io/en/messages/common.html#MAV_PARAM_TYPE. I don't fully understand the plan but pretty sure this is intended a path to getting consistent behaviour.
  • We also know that we'd like to support string parameters, which was a major impetus for the v2 API.

So there is a lot of convoluted things we are doing for compatibility, and it makes sense to consider if we can agree something that works for us and for component manufacturers.

@peterbarker @tridge At what point would you consider "biting the bullet" and agreeing a new parameter protocol given the things we have in play?
This isn't just about ArduPilot and PX4 - it's also to support components that are unlikely to implement things like the mavftp. I'm thinking of cameras and wifi points. Or is it the case that ArduPilot strongly feels it is better to extend the parameter protocol more or less forever?

@peterbarker
Copy link
Copy Markdown
Contributor Author

peterbarker commented Sep 11, 2025 via email

@dakejahl
Copy link
Copy Markdown
Contributor

Which bits need fixing in the current protocol? String parameters,
obviously. Anything else?

Bulk parameter transfer. We should formalize compression + ftp

@hamishwillee
Copy link
Copy Markdown
Contributor

Which bits need fixing in the current protocol? String parameters, obviously. Anything else?

Yes. Plus

  • whatever it is that is the reason for tridges enum thingy for indicating the encoding type.

Bulk parameter transfer. We should formalize compression + ftp

That's a different thing. My take is that it is "a good thing" for the same reasons as component metadata - a framework for transferring arbitrary data very quickly. But there is some more documentation to be done to make this easier for the implementers and to make it clear why certain things had been done.

By way of example, when we first talked about this the first reaction was why have custom compression given the existence of compression routines that are already in flight stacks. Tridge says this is to allow/enforce that every chunk contains at least a complete parameter and value. In other words there are reasons around how you handle partial file transfer etc. That stuff isn't clear in the existing RFC and hence can't be argued.

But yes we should. And formalise the virtual drive prefix for mavtfp.

@hamishwillee
Copy link
Copy Markdown
Contributor

hamishwillee commented Sep 17, 2025

So as per #2344 (comment)

@peterbarker @tridge At what point would you consider "biting the bullet" and agreeing a new parameter protocol given the things we have in play?

I'm not opposed to this change either way, but it would be good to understand just how far we'll push this.

EDIT PS. Request to extend Param value to have ability to set persistence here mavlink/rfcs#27 (comment). Is this a good enough reason to have another protocol? Can we extend the PARAM_VALUE, SET_PARAM to support this?

@hamishwillee
Copy link
Copy Markdown
Contributor

@peterbarker As suggested in mavlink/rfcs#27 (comment) , any reason why having a new message for the error is better than just adding a new extension field to PARAM_VALUE?

@peterbarker
Copy link
Copy Markdown
Contributor Author

@peterbarker As suggested in mavlink/rfcs#27 (comment) , any reason why having a new message for the error is better than just adding a new extension field to PARAM_VALUE?

As discussed elsewhere, this doesn't work as an old GCS will take any PARAM_VALUE message it receives as the value for the parameter, ignoring the extension field as it doesn't know about it.

@dakejahl
Copy link
Copy Markdown
Contributor

@peterbarker As suggested in mavlink/rfcs#27 (comment) , any reason why having a new message for the error is better than just adding a new extension field to PARAM_VALUE?

As discussed elsewhere, this doesn't work as an old GCS will take any PARAM_VALUE message it receives as the value for the parameter, ignoring the extension field as it doesn't know about it.

An old GCS won't know about PARAM_ERROR though either

@peterbarker
Copy link
Copy Markdown
Contributor Author

peterbarker commented Sep 17, 2025 via email

@dakejahl
Copy link
Copy Markdown
Contributor

What advantage does the message PARAM_ERROR offer over extending PARAM_VALUE with a result field? The downside of PARAM_ERROR is that the GCS response handling is made more complex -- it needs to look for both PARAM_VALUE and PARAM_ERROR as part of the response. Whereas the extension of PARAM_VALUE guarantees the response is handled completely with just PARAM_VALUE. In either case an old GCS will retain the existing behavior.

Copy link
Copy Markdown
Contributor

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

Approved by Dev call.

@hamishwillee hamishwillee merged commit 7817511 into mavlink:master Sep 25, 2025
19 checks passed
peterbarker added a commit to peterbarker/mavlink that referenced this pull request Sep 25, 2025
@DonLakeFlyer
Copy link
Copy Markdown
Contributor

@DonLakeFlyer Just ping me when someone implements this and I'll add to QGC.

peterbarker added a commit to peterbarker/mavlink that referenced this pull request Sep 30, 2025
@julianoes
Copy link
Copy Markdown
Contributor

While I'm ok with this addition, shouldn't we have added this to development.xml first? We're breaking our process...

@hamishwillee
Copy link
Copy Markdown
Contributor

While I'm ok with this addition, shouldn't we have added this to development.xml first? We're breaking our process...

Yes

tridge pushed a commit to ArduPilot/mavlink that referenced this pull request Oct 6, 2025
@hamishwillee
Copy link
Copy Markdown
Contributor

Extended with more param values in #2358

@DonLakeFlyer
Copy link
Copy Markdown
Contributor

Can I always assume that PARAM_ERROR.param_index would only be valid if the request was originally made with an index? Same thing for param_id. The docs read like the way the call was made doesn't impact whether you get back a string or an index to identify the param. If that's the case it's gonna be a real PITA to implement in QGC. Because indices are only used during initial download, before the names are known. After that, QGC has no idea what the relationship between parameter index and parameter name is.

@peterbarker
Copy link
Copy Markdown
Contributor Author

peterbarker commented Oct 18, 2025 via email

@DonLakeFlyer
Copy link
Copy Markdown
Contributor

Are you saying that you can't deal with ACKs which are returning with the same fields which you're sending up to the autopilot?

I'm saying the opposite. If I send name, I can only deal with name coming back.

If you take the spec literally it says nothing about a request being made with a string and responding to it with an index being incorrect. Or the opposite.

@hamishwillee
Copy link
Copy Markdown
Contributor

FWIW the service docs strongly suggest using the id (name) for reading, and indicates that the index might change:

The param_id is used to read parameters where possible (the mapping of param_index to a particular parameter might change on systems where parameters can be added/removed).

I think the name should always be returned. The index should be considered only safe during the request for params. @peterbarker Any objections to me updating the spec to saying that? It is IMO more reliable.

@peterbarker
Copy link
Copy Markdown
Contributor Author

peterbarker commented Oct 19, 2025 via email

@peterbarker
Copy link
Copy Markdown
Contributor Author

peterbarker commented Oct 19, 2025 via email

@hamishwillee
Copy link
Copy Markdown
Contributor

On Sun, 19 Oct 2025, Hamish Willee wrote: I think the name should always be returned. The index should be considered only safe during the request for params. @peterbarker Any objections to me updating the spec to saying that? It is IMO more reliable.
Sounds good to me. So say an autopilot MUST return the same identifier in PARAM_VALUE and PARAM_ERROR as was used in the original request?

Yes, but it must also/always return the param_id (name) if that exists.

The autopilot must include the param identity from the request because in some cases the other identity won't exist - say the index is outside the range and so there is no param_id, but you still need to know what request you're talking about. Similarly, if you set an invalid param_id, then no corresponding index will exist.

It makes sense to include the name too if it exists, because that's what we're really after - its the only thing guaranteed to be invariant.

I've tried to capture this in #2365.

@DonLakeFlyer
Copy link
Copy Markdown
Contributor

Is this still true for ArduPilot:
PARAM_VALUE is not emitted after the parameter update with the new value (or the old value if update failed).

@peterbarker
Copy link
Copy Markdown
Contributor Author

peterbarker commented Oct 27, 2025 via email

@DonLakeFlyer
Copy link
Copy Markdown
Contributor

That sentence is straight from the official mavlink parameter protocol docs.

@peterbarker
Copy link
Copy Markdown
Contributor Author

That sentence is straight from the official mavlink parameter protocol docs.

Huh. That's weird.

So it's complicated. We send the current value if the parameter is read-only.

If we send a param_error, or the value is set, then we don't send the current value. That's new behaviour, 'though.

Suggestions welcome....

Thanks @DonLakeFlyer for pointing this out.

@DonLakeFlyer
Copy link
Copy Markdown
Contributor

If we send a param_error, or the value is set, then we don't send the current value.

I would say that if the value is set the new value should be sent back. Otherwise there is no "ack" to the fact that it worked.

I think not sending the value back on PARAM_ERROR makes sense since that works for back compat on systems which don't yet support PARAM_ERROR.

@hamishwillee I think the paramater protocol spec needs to updating to indicate the behavior on PARAM_ERROR.

@hamishwillee
Copy link
Copy Markdown
Contributor

hamishwillee commented Oct 29, 2025

@DonLakeFlyer @peterbarker

So it's complicated. We send the current value if the parameter is read-only.

If we send a param_error, or the value is set, then we don't send the current value. That's new behaviour, 'though.

The behaviour as documented in the XML should be considered canonical. Which is to say that it is more likely to be right because it gets more thorough review.

The specific parts in https://mavlink.io/en/services/parameter.html#ardupilot usually come from you @peterbarker, but even if you supplied the correct information, we might have been playing chinese whispers.

What the XML says is that the current value should always be sent if the flight stack is able to - so if you set a param and this fails because it is read only, the PARAM_VALUE should be sent AND a PARAM_ERROR.
If however you set a param using an invalid id there would be no PARAM_VALUE to return, so you would instead return the error.

The reason for this is that it means a non-updated flight stack can continue to work well with newer flight stacks (which would behave better). I.e. it is a completely complementary change.

Thoughts?

peterbarker added a commit to peterbarker/mavlink that referenced this pull request Dec 8, 2025
TomasTwardzik pushed a commit to Auterion/mavlink that referenced this pull request Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants