Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify target address for requested message/stream #1249

Merged
merged 3 commits into from
Nov 3, 2019
Merged

Clarify target address for requested message/stream #1249

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a16f6c
Clarify target address for requested message/stream
hamishwillee Oct 16, 2019
6badad1
Change response target to param7
hamishwillee Oct 31, 2019
f5afaed
Merge branch 'master' into hamishwillee-requested_message
hamishwillee Nov 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions message_definitions/v1.0/common.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Collaborator Author

@hamishwillee hamishwillee Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@julianoes

  1. I used index 4 rather than 3 to give space for another user-specified value. Why not 6 - dunno!
  2. 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.
  3. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@olliw42 olliw42 Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@hamishwillee hamishwillee Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks great

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

</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>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@hamishwillee hamishwillee Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

</entry>
<entry value="519" name="MAV_CMD_REQUEST_PROTOCOL_VERSION" hasLocation="false" isDestination="false">
<description>Request MAVLink protocol version compatibility</description>
Expand Down