Fix for MAV_CMD_DO_SET_ROI_LOCATION issue

[Puni2001]

@rmackay9, This pull request resolves the issue with the MAV_CMD_DO_SET_ROI_LOCATION command in the simulated vehicle SITL instance.

According to the MAVLink docs, the latitude and longitude fields should be in 1e7 form, however, the firmware and also MAVProxy implemented this feature with float numbers, causing the simulated vehicle SITL instance to immediately stop working.

This pull request changes the code so that the lat, lon, and alt fields are correctly specified in 1e7 form, ensuring that the simulated vehicle SITL instance will function correctly.

Changes Made The cmd_set_roi function was updated to convert the lat, lon fields to int and multiply them by 1e7 before sending the MAV_CMD_DO_SET_ROI_LOCATION command.

Types of changes Bug fix

[rmackay9]

This is related to this issue https://github.com/ArduPilot/ardupilot/issues/22849

[rmackay9]

Thanks for looking into the above linked issue. I'm not sure this is a correct change. If command_long is used I think the lat,lon is in regular form (e.g. degrees), if command_int is used then I think lat,lon is multiplied by 10e7.

[Puni2001]

Thanks for looking into the above linked issue. I'm not sure this is a correct change. If command_long is used I think the lat,lon is in regular form (e.g. degrees), if command_int is used then I think lat,lon is multiplied by 10e7.

[Puni2001]

please guide me as a beginner i think the merging is blocked please review and send me the feedback

[hamishwillee]

is wrong (@hamishwillee ). Those units are only valid if the command is sent via COMMAND_INT, but AFAIK sending this via COMMAND_LONG is perfectly valid.

Yes you can send it in a COMMAND_LONG, but given that this is significantly less precise, why would you? As we can only specify one set of units we've indicated the one that we'd expect people to use. This would be much easier if there was just one message :-(.

The only general solution I can see is to clarify the units and behaviour in the message or field description. That will be a bit of a pain in the bum given how widely lat/lon are used in messages

For example, we could have in every single description "This message should be sent in a COMMAND_INT for greater precision in position fields. If sent in a COMMAND_LONG the units of the lat/lon are degrees".

If there is strong support for that and an agreed approach I can roll it out.

[peterbarker]

On Wed, 22 Feb 2023, Hamish Willee wrote:

  is wrong ***@***.*** ). Those units are only valid if the command is sent via COMMAND_INT, but AFAIK sending this via COMMAND_LONG is perfectly valid.

Yes you can send it in a COMMAND_LONG, but given that this is significantly less precise, why would you?

Primarily because the autopilot doesn't support it, I would imagine :-)

As we can only specify one set of units we've indicated the one that we'd expect people to use. This would be much easier if there was just one message :-(.

tridge pointed out that being able to transport 7 floats can be useful for non-COMMAND_LONG messages. Perhaps we could start to deprecate sending of any position-field-using command via COMMAND_LONG?

The only general solution I can see is to clarify the units and behaviour in the message or field description. That will be a bit of a pain in the bum given how widely lat/lon are used in messages For example, we could have in every single description "This message should be sent in a COMMAND_INT for greater precision in position fields. If sent in a COMMAND_LONG the units of the lat/lon are degrees".

That works.

If there is strong support for that and an agreed approach I can roll it out.

Thanks - I'll ask for this to be discussed at DevCall to make sure of that "deprecation" thing and see what people think of the comments.

[hamishwillee]

tridge pointed out that being able to transport 7 floats can be useful for non-COMMAND_LONG messages.

So he's right in theory, but perhaps not so much in practice. If you look through https://mavlink.io/en/messages/common.html#mav_commands the vast majority use param 5, 6 for latitude and longitude. Most of the rest don't use the params. A couple more use the params for values, but these are fine to send as integers.

The main exceptions are things like https://mavlink.io/en/messages/common.html#MAV_CMD_NAV_TAKEOFF_LOCAL that could perhaps have been better designed. It is pretty rare you really need more than 5 integers.

Perhaps we could start to deprecate sending of any position-field-using command via COMMAND_LONG?

IMO we already do "among ourselves". The question is, how do we show this?

I think it is a no-brainer that the description of https://mavlink.io/en/messages/common.html#COMMAND_LONG should be updated to note that

• messages with lat/lon should be sent in COMMAND_INT instead for greater precision.
• This is required for commands that set param 5 or param 6 to float values.

Within commands I'm not so sure. My leaning would be to add attribute flag to indicate the status "should be sent in command_long". This would be applied to anything where it makes sense to send in command long and would otherwise default false. We could then auto-generate a line of text for this. Thoughts?

[peterbarker]

Deprecation not going to happen right now
Float is good enough for a lot of location commands
But recommendation is fine
As command_int is better :-)
There are commands like NAV_SCRIPTING which really, really do need all of the values to be floating point, so COMMAND_LONG is here to stay

[hamishwillee]

Fair enough. I've proposed some wording that might make this more clear in linked items.

[peterbarker]

@Puni2001 I think Hamish and I kind of hijacked your PR here.

I've been working in this area for the last couple of days, and I'm afraid that even if you were to get this right we couldn't actually merge it as ArduPilot doesn't accept this command via COMMAND_INT at the moment. Even if we were to fix that we would need to make sending as INT optional in case people were using older autopilots which didn't have the INT version.

Did you still want to work on this?