Added support for MAV_CMD_REQUEST_MESSAGE for mavlink streams #13481
Conversation
AmeliaEScott
force-pushed
the
pr-request-msg-cmd
branch
from
450bbed to
71666ba
Compare
AmeliaEScott
force-pushed
the
pr-request-msg-cmd
branch
from
71666ba to
ee9cffa
Compare
|
I'm just curious, what's the desired use case for this? My initial thought is that it would primarily be useful for sending messages that aren't already streamed. |
|
|
||
| for (const auto &stream : _mavlink->get_streams()) { | ||
| if (stream->get_id() == message_id) { | ||
| stream->request_message(vehicle_command.param1, vehicle_command.param2, vehicle_command.param3, |
There was a problem hiding this comment.
Why are all the parameters passed on?
There was a problem hiding this comment.
For my use case (see below), I need parameters 2, 3, and 4 (although the specifications for this are still under discussion, so that might not be the case in the end). Based on this, it seems to be fine to use any set of the parameters. So for future implementations of this method, different messages might have to use any or all of the parameters from the command.
There was a problem hiding this comment.
I think a good idea. We could just update the mavlink message to define all these param values properly now if that will make it easy.
There was a problem hiding this comment.
That's indeed a bit odd. According to the message definition no params should be really used, right?
There was a problem hiding this comment.
What @ItsTimmy has done is the intention of the message, but not its current implementation.
Specifically, some message, like the proposed microservices requestor, depend on parameters. If we want to use this message to request it we need to be able to provide those parameters. So if we don't do this, we'd need a dedicated message (also a valid option).
I've updated the message so that it is more clear: mavlink/mavlink#1276
There was a problem hiding this comment.
@hamishwillee ok got it. I think in that case I just don't like the fact that we overload the REQUEST_MESSAGE with additional params. I can see how we got there but it makes it hard to understand from the outside, in my opinion.
There was a problem hiding this comment.
@julianoes That's a reasonable argument. Certainly having dedicated messages makes microservices more compartmentalised.
But it does mean we need to create at least one dedicated message for @ItsTimmy to use for this. Tim, can you propose a message in the spec?
|
I am working on implementing microservice versioning, which will include an option to request that the autopilot send an individual message for each supported microservice. I implemented a MavlinkStream for microservice version messages, which responds to the Request Message command. The intent there is to avoid clogging the mavlink channel by sending all of the messages at once. Instead, I am using the existing MavlinkStream infrastructure to space out the messages on a regular interval, then stop the stream when I have sent all of them. If you want to support other messages for the Request Message command in the future, it should be as easy as adding a new MavlinkStream and setting the interval very large. (Of course, that's a bit of a hack. A slightly better option is to change MavlinkStream such that an interval value of, say, |
@dagar Yes. Essentially it is a generic replacement for having to create a special message every time you want to do a one-shot requester for a particular message. For Tim it's a part of the microservice handshake. |
|
@dagar @julianoes @hamishwillee Will this get merged, or do you think it is unnecessary? I would like to have a decision within a few days, so that I can know whether I should start working on a different way to implement microservice versioning. |
The round needs to be updated, but otherwise this looks safe to merge. |
|
Looks good to me. I actually don't have rights to merge stuff in this repo (which is generally a good thing :-) ) |
AmeliaEScott
force-pushed
the
pr-request-msg-cmd
branch
from
b240f0c to
7eb8dd2
Compare
|
Since the spec for microservice versioning has been updated such that it no longer uses Also, since mavlink/mavlink#1276 was merged, should I leave this as it is, with all of the parameters included? |
|
@ItsTimmy my strong preference is to leave it in/merge this. The requestor is useful for many messages, not just microservices.
Yes |
Describe problem solved by this pull request
PX4 does not support the command
MAV_CMD_REQUEST_MESSAGEDescribe your solution
Added a function to MavlinkStream which, by default, calls
reset_last_sent(). It can be overridden for situations where a parameter should be used.Describe possible alternatives
This could be handled outside of MavlinkStream.
Test data / coverage
Used MavSDK to send
MAV_CMD_REQUEST_MESSAGEto PX4 running in SITL, and verified that the requested message was sent.