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.

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

Pl clarify targets in stream/request messages #1248

Closed
olliw42 opened this issue Oct 9, 2019 · 17 comments · Fixed by #1249
Closed

Pl clarify targets in stream/request messages #1248

olliw42 opened this issue Oct 9, 2019 · 17 comments · Fixed by #1249
Labels
Dev Call Issues to be discussed during the Dev Call

Comments

@olliw42
Copy link
Contributor

olliw42 commented Oct 9, 2019

a component can be made to emit a message (one-shot or streaming) with MAV_CMD_REQUEST_MESSAGE or MAV_CMD_SET_MESSAGE_INTERVAL. However, if a message is requested which has a target system/component, it is not clear how the target should be set.

Contradicting lines of arguments come to mind. One straight-forward "answer" could be that the target should be non-broadcast, i.e., set to the requester's source system/component IDs. Another straight-forward "answer" could be that the behavior should be similar to that of the non-target messages, i.e., be broadcast. One also could argue that for a MAV_CMD_REQUEST_MESSAGE the target should be the source while for a MAV_CMD_SET_MESSAGE_INTERVAL it should be broadcasting. One generally could envision situations where, especially for a stream, either a targeted or a broadcasting response would be more preferred and thus would want to have the option. And so on.

Currently the spec is unclear as to this point. I think it should not be left unspecified however, since different implementations could then behave very differently.

As food for discussion, three simple solutions:

  • it is specified that the target must be set to the source
  • it is specified that the target must be broadcasting
  • a param is used to hold a enum which allows to choose between options

Note that this is not totally academic: For instance, the ArduPilot specific message https://mavlink.io/en/messages/ardupilotmega.html#MOUNT_STATUS has a target, and is (at least currently) a main incredient of the gimbal support. It is not clear what the expected behavior should be.

@hamishwillee
Copy link
Collaborator

That's an interesting question.

I think it is a property of the message you request and should be documented by that message (if non-obvious).

It will be obvious in almost every case from the design of the message. If a message has no target fields it is broadcast. If it has target fields then it is almost always intended to go to the source of a specific request (e.g. MAV_CMD_LOGGING_START) and would not be streamed by default anyway.

If there is a real case where the message might sometimes be broadcast and sometimes be to a target then the option would be be documented in that message. The option could then be passed in a MAV_CMD_REQUEST_MESSAGE parameter.

@olliw42
Copy link
Contributor Author

olliw42 commented Oct 10, 2019

I hear you, but am not fully convinced :)

you largely follow my first line of argument. And I agree, for most messages it will be clear what it should be. I mean, for a TUNNEL it is clear that it doesn't even make sense to request it in the first place. However, with 100-eds of messages and growing there will always be hybrid cases, there it also may depend on whether it's one-shot or stream. There will even be cases there the use range of the message shifts over time. And there will be cases there a message wasn't most optimal designed or documented ...

ArduPilot's MOUNT_STATUS may serve as an example as each of these points can be argued to apply to it.

Finally, from a conceptional view, there is obviously a hole in the spec. It should not be left to the implementer to decide if she wants to pass an additional option in an parameter.

The simplest might be to say in the spec that a targeted message should be targeted when requested/streamed, unless explicitly spec-ed otherwise elsewhere, akin what you sugest in your 2nd last sentence.

I somewhat tend to prefer to adding a parameter to the spec, carrying an enum. Where entry = 1 could be targeted, 2 be sysId targeted but compId = 0 and 3 all broadcast. The entry = 0 could mean default, where default means that it is what it was before however it was chosen by the specific implementation. That way this modification would be nearly non-breaking (I think the spec says that command params should be defaulted to zero)

@hamishwillee
Copy link
Collaborator

hamishwillee commented Oct 11, 2019

@julianoes Do you have an opinion here? I'm pretty much happy to do nothing - as per #1248 (comment) ...

If we do feel that something has to be done then @olliw42 enum approach looks good as a way for system to specify how it wants the message returned.

@olliw42
Copy link
Contributor Author

olliw42 commented Oct 11, 2019

independent on the outcome, I'd like to say: I generally have the expectation that something which calls itself a specification gives me the directions or hints as regards to how to use and do the things which are within the context of the specification.

so, to literally do nothing I wouldn't consider appropriate. Even if the outcome should be to "do nothing" I think that at minimum the spec's text should be extended to say something like "for requested messages which have a target system, the target system is set as desired by the specific implementation" or such.

:)

@julianoes
Copy link
Collaborator

One also could argue that for a MAV_CMD_REQUEST_MESSAGE the target should be the source while for a MAV_CMD_SET_MESSAGE_INTERVAL it should be broadcasting.

That sounds appropriate. With the addition that if the message requested using MAV_CMD_REQUEST_MESSAGE does not have a target then it will be broadcast.

@olliw42
Copy link
Contributor Author

olliw42 commented Oct 11, 2019

With the addition that if the message requested using MAV_CMD_REQUEST_MESSAGE does not have a target then it will be broadcast.

if the requested message doesn't have a target it's broadcast per se, i.e., the whole issue doesn't exist for them in the first place

@olliw42
Copy link
Contributor Author

olliw42 commented Oct 12, 2019

One also could argue that for a MAV_CMD_REQUEST_MESSAGE the target should be the source while for a MAV_CMD_SET_MESSAGE_INTERVAL it should be broadcasting.

That sounds appropriate.

so, is this the final conclusion now, i.e., what the spec is now going to be?

@julianoes
Copy link
Collaborator

We probably need to talk through it in a MAVLink dev call but to me it sounds sane.

@olliw42
Copy link
Contributor Author

olliw42 commented Oct 14, 2019

ok, so I'll just wait :)

thx sir

@julianoes julianoes added the Dev Call Issues to be discussed during the Dev Call label Oct 14, 2019
@olliw42
Copy link
Contributor Author

olliw42 commented Oct 15, 2019

I'd like to mention this disadvantage of the "for a MAV_CMD_REQUEST_MESSAGE the target should be the source while for a MAV_CMD_SET_MESSAGE_INTERVAL it should be broadcasting" approach:

the point is that streams can be potentially of high rate, so that with this approach many components may be swamped with data which they themselves never had asked for. That is, one can argue that especially for streams it normally should be targeted, and only in cases there it is really desired be broadcasting

a realistic example would be case 2 "Standalone integrated camera/gimbal" in your "Proposal for new gimbal messages". The gimbal component would ask for e.g. some high rate attitude and maybe rc channels stream, but this would also have to be processed by the camera component (unless some dirty tricks would be played in that implementation)

I more strongly would lean towards the enum thing now

@julianoes
Copy link
Collaborator

Hm, that's a good point!

@hamishwillee
Copy link
Collaborator

hamishwillee commented Oct 16, 2019

Even if the message is set with a target it will still occupy bandwidth on the channel and at least minimal processing for each system to decide "is/is not for me". I guess there is potentially a lot more processing though if it does have to do some real handling.

I still see it as very much an edge case, but still potentially relevant. First attempt at PR: #1249

@olliw42
Copy link
Contributor Author

olliw42 commented Oct 16, 2019

the more I think about it the more I object this line of reasoning

this might be an edge-case from a conceptional point of view, but I tried to point out that it may not be an edge-case from a practical point of view. The integrated gimbal/cameras for instance might become much more frequent. I'm kind of convinced that all the gimbal manufacturers you are in contact with go into this direction. Which means that the situation I described might become rather the normal than an edge. And given that all efforts go into this direction, we actually should expect that this is not just a "might become" but a "will become". I don't buy that "edge-case" argument,.

Sure, high-rate data exchange does strain the links and components, but there is no way out of this, if the data needs to be exchanged itneeds to be exchanged. So, I don't think this needs to be disputed.

However, it can be done the smart or the stupid way. There are some things on can do to alleviate the strain:

  • one can design compact messages, which carry the required data but not also amounts of superfluous data
  • one can avoid that the same data are send twice or multiple times
  • one can avoid that not required data are send
  • ...

The smartest way isn't possible within the MAVLink framework. However, I clearly pledge for not doing it the stupiest way.

Consider e.g. this setup: A gimbal is connected to a companion which is connected to the autopilot, and the companion is connected per telemetry to a GCS. I would say, a clearly very realistic setup, see also the gimbal thread. Now the gimbal asks the autopilot for a high-rate attitude and velocity and location and rc channels and what else it wants at high rate. So the autopilot is broadcasting them all, and the companion is routing them all - since it wants to be a MAVlink spec conform device (!!!) - also to the GCS ... that is the usually most weakest link is swamped with totally unneeded data !!! You will find that you can construct several very realistic situations. And the only way out currently is to break MAVlink at some point.

You now can go on and consider multi-vehicle situations, which by means of the system ID MAVLink proudly supports. So, a GCS - if it wants to be MAVLink conform (!!!) - would follow the MAVLink routing rules and also send that high-rate data to all the other vehicles. And that's not yet all, there is also all the data coming from the other vehicles to the one gimbal, which just wanted to get data from its autopilot, and all other components which give a dam about this data. Sure, no one would allo the GCS to do this, for the obvious catastrophe. That is: No one would follow the MAVlink spec. In the gimbal thread julianoes in fact was just saying, let's hope that the gimbal messages never get out of their system.

A further interesting consequence btw, which I'm not sure you ever noted, is that in nearly all cases the targeting system ID should actually be better not 0, but the system ID of the system. In many cases this won't have consequences, but in many cases only because one manifestly would ignore the MAVlink spec. That is, target sysId = sysId & target compId = 0 should be the standard for broadcasting, not target sysId = 0 & target compId = 0.

So, with not taking the means which are at least possible within the MAVLink framework, one more strongly is saying to everyone: "Don't take the MAVLink spec serious!".

As said, it might be that not all is resolvable within MAVLink, but I pledge to at least do what is possible.

Providing means to chose for (target sysId, compId) = (sysId, 0) and (target sysId, compId) = (sysId, compID) is possible.

:) :) :)

@olliw42
Copy link
Contributor Author

olliw42 commented Oct 16, 2019

having said this, this can be closed, right?

so I do. Many thx folks.

@olliw42 olliw42 closed this as completed Oct 16, 2019
@hamishwillee
Copy link
Collaborator

hamishwillee commented Oct 16, 2019

When I say "edge case", I really mean "is this really a problem right now that is blocking your work, or just a flaw you see in the definition" - because my list of tasks is so long everything I do is a choice to let someone down and I'd really prefer to fix blocking problems first. I accept that there will be cases where this is a problem, and we should do something about it, but perhaps not now.

Thanks for closing. My preferred mode is that if there is a PR in place that looks like it will solve the problem we remove the associated issue. Others are not so trusting that the final PR will sort out the issue so leave it open. Whatever model works for you :-)

@olliw42
Copy link
Contributor Author

olliw42 commented Oct 16, 2019

When I say "edge case", I really mean "is this really a problem right now that is blocking your work, or just a flaw you see in the definition"

ah, I see, I indeed got this wrong. Many thx for the clarification.

And also thx for having addressed it so quickly.

@hamishwillee
Copy link
Collaborator

Well, you can't really guess my intent unless I make it clear :-). YOu're welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Call Issues to be discussed during the Dev Call
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants