common.xml: add PARAM_ERROR message#2344
Conversation
|
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 |
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. |
|
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:
|
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! |
Just what MAVProxy does, +/- retry count. |
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 :-)
Not something ArduPilot would emit AFAICS, but enumeration entries are free :-) ... I'll just also state that I considered adding |
868b27d to
c50a0b9
Compare
|
I've updated the PR to add the suggested enumeration entries (and fix the naming on others) |
message_definitions/v1.0/common.xml
Outdated
| </entry> | ||
| </enum> | ||
| <enum name="MAV_PARAM_ERROR"> | ||
| <description>Specifies an error type being regarding the parameter protocol.</description> |
There was a problem hiding this comment.
| <description>Specifies an error type being regarding the parameter protocol.</description> | |
| <description>Parameter protocol error types (see PARAM_ERROR).</description> |
hamishwillee
left a comment
There was a problem hiding this comment.
Looks good.
- On a param set, if there is an error, would the current value still be emitted?
- 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?
|
@julianoes Any thoughts on this? |
That's what my current implementation does.
Yep. |
c50a0b9 to
240a2bc
Compare
|
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. |
|
On Sun, 7 Sep 2025, Julian Oes wrote:
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.
This essentially is adding a nak message. ack is accomplished by echoing
the new value.
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.
Correct. But if there *is* a problem then PARAM_ERROR means you *can* get
informed of it now! ie. you can be explicitly told that the parameter
you're tring to set doesn't exist, or that the value is out-of-range or
whatever.
Note that not receiving param_error isn't distuishable from the param-set
being lost, too :-)
|
|
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:
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? |
|
On Thu, 11 Sep 2025, Hamish Willee wrote:
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?
Which bits need *fixing* in the current protocol? String parameters,
obviously. Anything else?
|
Bulk parameter transfer. We should formalize compression + ftp |
Yes. Plus
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. |
|
So as per #2344 (comment)
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? |
|
@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 |
An old GCS won't know about PARAM_ERROR though either |
|
On Wed, 17 Sep 2025, Jacob Dahl wrote:
An old GCS won't know about PARAM_ERROR though either
Yes, so will end up with their existing behaviour, which in MAVPproxy's
case on a PARAM_GET is to ask for the parameter 5 times before giving up
(I believe QGC does likewise).
|
|
What advantage does the message PARAM_ERROR offer over extending PARAM_VALUE with a |
hamishwillee
left a comment
There was a problem hiding this comment.
Approved by Dev call.
|
@DonLakeFlyer Just ping me when someone implements this and I'll add to QGC. |
|
While I'm ok with this addition, shouldn't we have added this to development.xml first? We're breaking our process... |
Yes |
|
Extended with more param values in #2358 |
|
Can I always assume that |
|
On Fri, 17 Oct 2025, Don Gagne wrote:
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.
Yeah, the same is true of MAVProxy - there's no assumption made that
parameter indexes are persistent except during a complete parameter-set
fetch.
ArduPilot boomerangs both fields. So if you send the ID (name) then
you'll get that back. If you send the index you'll get that back. If you
send both... well, you'll get both back and we'll look at you strangely.
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? From what you were
saying that would *always* be by-name for parameter sets, for example, and
you'll get the name back...
One thing ArduPilot isn't doing is filling names in where we could. So if
you try to set an invalid value (currently inf and nan are considered
invalid values in the base C++ code), *and* the parameter exists we don't
fill the parameter name in. I don't think that's too bad thing... you
probably shouldn't be filling a parameter's value in if you don't already
know what its name is :-)
This is the ArduPilot regression test for PARAM_ERROR:
https://github.com/ArduPilot/ardupilot/pull/31062/files#diff-229d52fbfb155773ac224cb49ea9ac3adad7ec4addff82fb1d560b68ceced109R205
You can see exactly what the received values are expected to be there.
The PR adding support to MAVProxy is here:
https://github.com/ArduPilot/MAVProxy/pull/1629/files#diff-68655e9162d7f0884145944097120f52a93e1768f77ad8ada2a26badcf2b262fR251
|
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. |
|
FWIW the service docs strongly suggest using the id (name) for reading, and indicates that the index might change:
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. |
|
On Sun, 19 Oct 2025, Don Gagne wrote:
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.
Right, good :-)
As mentioned, ArduPilot just boomerangs the fields at the moment.
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.
Same is true of PARAM_VALUE :-)
|
|
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 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. |
|
Is this still true for ArduPilot: |
|
On Sat, 25 Oct 2025, Don Gagne wrote:
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).
That "not" seems out of place.
|
|
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. |
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. |
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. 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? |
allows an error regarding parameter sub-protocol to be passed around
Talking point only, let's not merge it right now...