Clarify target address for requested message/stream #1249

hamishwillee · 2019-10-16T11:04:42Z

As discussed in #1248 many messages are intended to be broadcast and if you request them, they will be broadcast. However some messages have target address in them so can either be broadcast or sent to a specific target.

This update repurposes parameters such that a user can explicitly specify that the target address of the response is broadcast or the address of the component that requested the message/stream.

By default the message will be sent with 0, which is "whatever the flight stack wants to do".

hamishwillee · 2019-10-16T11:06:12Z

message_definitions/v1.0/common.xml

@@ -1831,11 +1831,13 @@
        <description>Set the interval between messages for a particular MAVLink message ID. This interface replaces REQUEST_DATA_STREAM.</description>
        <param index="1" label="Message ID" minValue="0" maxValue="16777215" increment="1">The MAVLink message ID</param>
        <param index="2" label="Interval" units="us" minValue="-1" increment="1">The interval between two messages. Set to -1 to disable and 0 to request default rate.</param>
+        <param index="4" label="Response Target" minValue="0" maxValue="2" increment="1">Target address of message stream (if message has target address fields). 0: Flight-stack default (recommended), 1: address of requestor, 2: broadcast.</param>


@julianoes

I used index 4 rather than 3 to give space for another user-specified value. Why not 6 - dunno!

I used numbers rather than an enum because it didn't seem worth creating one for what is probably not going to be a user-selectable value.

I didn't try capture any of the discussion around how the default might be selected. That is up to the flight stack, and IMO will mostly be obvious.

I would probably call it target system and component instead of address.

please provide 4 options

0: Flight-stack default
1: system and component ID of requestor
2: system ID of requestor and component ID broadcasting
3: both system and component ID broadcasting

I also would not indicate 0 as recommended. The most recommended values would be 1 or 2. 0 is there "only" for backwards compatibility.

I personally would prefer an enum, but I see your reasoning.

btw: not 6 in order to have a better chance to take advantage of trailing zeros (I know, there is the target and confirmation, yet...). Not sure if 4 instead of 3 makes sense - eventually it might end up being in the middle anyways.

OK, how about (modified in line with above comments). Note, not recommendation, though IMO in practice you'd be fine to accept the flight stack default in 99.9% of cases.

Target system/component of message stream (only if message has target fields). 0: Flight-stack default, 1: Requestor system/component ID, 2: Requestor system ID and broadcast component ID, 3: Broadcast system and component ID.

I'll leave it in param 4 - I can see us needing another "arbitrary" param at some point.

looks great

I think it mostly will depend on when flight stacks would start to support the option ...

olliw42 · 2019-10-16T19:32:15Z

thx for taking the action.

hamishwillee · 2019-10-20T22:51:06Z

message_definitions/v1.0/common.xml

      </entry>
      <entry value="512" name="MAV_CMD_REQUEST_MESSAGE" hasLocation="false" isDestination="false">
        <description>Request the target system(s) emit a single instance of a specified message (i.e. a "one-shot" version of MAV_CMD_SET_MESSAGE_INTERVAL).</description>
        <param index="1" label="Message ID" minValue="0" maxValue="16777215" increment="1">The MAVLink message ID of the requested message.</param>
        <param index="2" label="Index ID" minValue="0" increment="1">Index id (if appropriate). The use of this parameter (if any), must be defined in the requested message.</param>
+        <param index="4" label="Response Target" minValue="0" maxValue="2" increment="1">Target address for requested message (if message has target address fields). 0: Flight-stack default, 1: address of requestor, 2: broadcast.</param>


@julianoes @olliw42 I'm thinking about moving this to the final index and defining all these index values as "optional". My reasoning being that I have a message that has 3 possible params :-0. All would be zero by default.

OK?

I think it would be weird to have it in the last param. Shouldn't it just be at index="3"?

@julianoes Perhaps I wasn't very clear.

So we currently have an optional configurable parameter (2). In future we might have multiple optional configurable parameters and having them surrounding param 3 is even more odd. For example consider the command I suggested in the Microservices versioning, which has 3 params - service, max version, min version. It would seem odd to have param 2, 4, 5, as configurable and 3 as the response target.

The "nice" think to do here would be to make param 2 the response target, and make the optional ones from 3. But given 3 has been defined for a while I was thinking we might explicitly define param 3, 4,5 6, as configurable params, and just have 7 as the response target.

Does that make sense?

I think it depends on how many more commands there are for which such a need of specifying the target of a returned message is needed. From scratch I only see the two commands under discussion.
If it is really just few it's maybe overkill to wrap a general strategy around them, and I would think any place might be considered as good as any other. My natural feeling would be to just fill from bottom to top. But anything would IMHO be equally good.

Not sure. Was going to just fill. But then realised that there other questions like "should we put contstraints on the fields if we fill them. Which makes me tend to agree with you - fill from bottom as we need and live with odd structure.

hamishwillee · 2019-10-21T01:42:56Z

@julianoes It also occurs to me that I haven't properly got my head around the expected COMMAND_ACK.

The success case is fine - as usual we just return MAV_RESULT_ACCEPTED.

I'm just not sure what we return if MAV_CMD_REQUEST_MESSAGE is supported but the particular message we want to act on is not. i.e. I'd like to return MAV_RESULT_UNSUPPORTED but normally this would be be interpreted as MAV_CMD_REQUEST_MESSAGE is not supported. Or am I over-thinking this?

julianoes · 2019-10-21T09:09:18Z

The success case is fine - as usual we just return MAV_RESULT_ACCEPTED.

I'm just not sure what we return if MAV_CMD_REQUEST_MESSAGE is supported but the particular message we want to act on is not. i.e. I'd like to return MAV_RESULT_UNSUPPORTED but normally this would be be interpreted as MAV_CMD_REQUEST_MESSAGE is not supported. Or am I over-thinking this?

Good question. I would say.
MAV_RESULT_ACCEPTED: default, all is good.
MAV_RESULT_UNSUPPORTED: according to spec "Command UNKNOWN/UNSUPPORTED"
MAV_RESULT_DENIED: not possible for this message because there are reasons
MAV_RESULT_FAILED: not possible because something went wrong

hamishwillee · 2019-10-21T10:08:56Z

@julianoes So what do we return if MAV_CMD_REQUEST_MESSAGE is supported but someone requests a message that can't be sent?

olliw42 · 2019-10-21T10:42:40Z

very good question ...

NOTE: This situation doesn't only apply/occur for this command but in fact is quite general in nature, i.e., a similar situation can happen for other cmds too. That is, is could and should be addressed by a generally valid specification. !!

As suggestion, here the pattern I decided to follow in my implementation:

MAV_RESULT_ACCEPTED: Command ACCEPTED and EXECUTED
MAV_RESULT_TEMPORARILY_REJECTED: Command was accepted, but could currently not be executed. Implementations supports this command, so it can make sense to retry later.
MAV_RESULT_DENIED: Command was accepted, but cannot be executed, e.g. because of inappropriate params. It doesn't make sense to retry as outcome will be the same.
MAV_RESULT_UNSUPPORTED | Command UNKNOWN/UNSUPPORTED
MAV_RESULT_FAILED | Command executed, but failed. It can make sense to retry.
MAV_RESULT_IN_PROGRESS

julianoes · 2019-10-21T12:19:02Z

So what do we return if MAV_CMD_REQUEST_MESSAGE is supported but someone requests a message that can't be sent?

MAV_RESULT_DENIED I'd say.

hamishwillee · 2019-10-22T01:08:21Z

@olliw42 @julianoes You're both right. I'll try clarify the specification here: #1253 and MAV_RESULT_DENIED would be used for a request to send back a message that hasn't been set up for this purpose.

PUt this on hold for now.

olliw42 · 2019-10-31T13:37:05Z

the MAV_RESULT_XXX topic has been settled, so this doesn't need to be on hold anymore and could be settled too, right?

hamishwillee · 2019-10-31T23:58:44Z

I changed the param for the response target to 7. Otherwise IMO this is good to merge. @julianoes Any last comments?

hamishwillee · 2019-11-03T22:41:00Z

Done. Thanks all.

Clarify target address for requested message/stream

2a16f6c

hamishwillee requested a review from julianoes October 16, 2019 11:04

hamishwillee commented Oct 16, 2019

View reviewed changes

hamishwillee mentioned this pull request Oct 16, 2019

Pl clarify targets in stream/request messages #1248

Closed

hamishwillee commented Oct 20, 2019

View reviewed changes

Change response target to param7

6badad1

julianoes approved these changes Nov 1, 2019

View reviewed changes

Merge branch 'master' into hamishwillee-requested_message

f5afaed

hamishwillee merged commit 47ead18 into master Nov 3, 2019

hamishwillee deleted the hamishwillee-requested_message branch November 3, 2019 22:40

amilcarlucas pushed a commit to amilcarlucas/mavlink that referenced this pull request Dec 10, 2019

Clarify target address for requested message/stream (mavlink#1249)

737aec3

tridge pushed a commit to amilcarlucas/mavlink that referenced this pull request Dec 22, 2019

Clarify target address for requested message/stream (mavlink#1249)

0d34365

tridge pushed a commit to ArduPilot/mavlink that referenced this pull request Dec 22, 2019

Clarify target address for requested message/stream (mavlink#1249)

ba89eeb

hamishwillee mentioned this pull request May 4, 2020

Implement MAV_CMD_REQUEST_MESSAGE PX4/PX4-Autopilot#14815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify target address for requested message/stream #1249

Clarify target address for requested message/stream #1249

hamishwillee commented Oct 16, 2019

hamishwillee Oct 16, 2019 •

edited

julianoes Oct 16, 2019

olliw42 Oct 16, 2019 •

edited

hamishwillee Oct 16, 2019 •

edited

olliw42 Oct 16, 2019

olliw42 commented Oct 16, 2019 •

edited

hamishwillee Oct 20, 2019

julianoes Oct 21, 2019

hamishwillee Oct 21, 2019

olliw42 Oct 21, 2019

hamishwillee Oct 22, 2019 •

edited

hamishwillee commented Oct 21, 2019

julianoes commented Oct 21, 2019

hamishwillee commented Oct 21, 2019 •

edited

olliw42 commented Oct 21, 2019

julianoes commented Oct 21, 2019

hamishwillee commented Oct 22, 2019 •

edited

olliw42 commented Oct 31, 2019

hamishwillee commented Oct 31, 2019

hamishwillee commented Nov 3, 2019

Clarify target address for requested message/stream #1249

Clarify target address for requested message/stream #1249

Conversation

hamishwillee commented Oct 16, 2019

hamishwillee Oct 16, 2019 • edited

Choose a reason for hiding this comment

julianoes Oct 16, 2019

Choose a reason for hiding this comment

olliw42 Oct 16, 2019 • edited

Choose a reason for hiding this comment

hamishwillee Oct 16, 2019 • edited

Choose a reason for hiding this comment

olliw42 Oct 16, 2019

Choose a reason for hiding this comment

olliw42 commented Oct 16, 2019 • edited

hamishwillee Oct 20, 2019

Choose a reason for hiding this comment

julianoes Oct 21, 2019

Choose a reason for hiding this comment

hamishwillee Oct 21, 2019

Choose a reason for hiding this comment

olliw42 Oct 21, 2019

Choose a reason for hiding this comment

hamishwillee Oct 22, 2019 • edited

Choose a reason for hiding this comment

hamishwillee commented Oct 21, 2019

julianoes commented Oct 21, 2019

hamishwillee commented Oct 21, 2019 • edited

olliw42 commented Oct 21, 2019

julianoes commented Oct 21, 2019

hamishwillee commented Oct 22, 2019 • edited

olliw42 commented Oct 31, 2019

hamishwillee commented Oct 31, 2019

hamishwillee commented Nov 3, 2019

hamishwillee Oct 16, 2019 •

edited

olliw42 Oct 16, 2019 •

edited

hamishwillee Oct 16, 2019 •

edited

olliw42 commented Oct 16, 2019 •

edited

hamishwillee Oct 22, 2019 •

edited

hamishwillee commented Oct 21, 2019 •

edited

hamishwillee commented Oct 22, 2019 •

edited