mavlink / mavlink

Marshalling / communication library for drones.
http://mavlink.io
Other
1.72k stars 1.91k forks source link

No way to get current ROI from vehicle #2049

Open nexton-winjeel opened 1 year ago

nexton-winjeel commented 1 year ago

We have a number of commands to set the ROI (e.g. DO_SET_ROI_LOCATION, DO_SET_ROI_WPNEXT_OFFSET, DO_SET_ROI_NONE). However, we don't have a way to get the current ROI.

QGC[^1] gets around this by assuming that the ROI is whatever it commanded, e.g.: https://github.com/mavlink/qgroundcontrol/blob/v4.2.9/src/Vehicle/Vehicle.cc#L3023

However, this doesn't work if:

[^1]: I have no idea what MissionPlanner does...

nexton-winjeel commented 1 year ago

I suspect we need a CURRENT_ROI message (similar to POSITION_TARGET_GLOBAL_INT), e.g.:

Field Name Type Units Values Description
gimbal_device_id uint8_t Component ID of gimbal device (or 1-6 for non-MAVLink gimbal), associated with this ROI, 0 for all gimbal device components.
coordinate_frame uint8_t MAV_FRAME Valid options are: MAV_FRAME_GLOBAL_INT = 5, MAV_FRAME_GLOBAL_RELATIVE_ALT_INT = 6, MAV_FRAME_GLOBAL_TERRAIN_ALT_INT = 11
lat_int int32_t degE7   X Position in WGS84 frame
lon_int int32_t degE7   Y Position in WGS84 frame
alt float m   Altitude (MSL, Relative to home, or AGL - depending on frame)
type uint8_t MAV_ROI Region of interest mode.
roi_data uint16_t ROI data, set based on type:
type=MAV_ROI_NONE: Unused
type=MAV_ROI_WPNEXT: Mission item sequence number of ROI target
type=MAV_ROI_WPINDEX: Mission item sequence number of ROI target
type=MAV_ROI_LOCATION: Unused
* type=MAV_ROI_TARGET: System ID of ROI target

Updates:

hamishwillee commented 1 year ago

This sounds sensible, but as always to be part of the standard it needs broad interest to implement from the key stakeholders.

@peterbarker Is ardupilot interested in having something like this, and if so, do you have an opinion on the design? @TSC21 Does RAS-A have anything already for emitting current ROI?

FWIW I think that "type" would be important, but not necessarily the set defined in MAV_ROI.

peterbarker commented 1 year ago

So ArduPilot doesn't currently actually keep the Region of Interest around. We throw it straight at our gimbal ("mount") library and have it deal with the problem.

We've just merged support for CAMERA_FOV_STATUS, which emits the camera's current target, at least for the usual case of pointing at a latitude/longitude.

So in a way we've just caught up to where we should've been a long time ago...

As far as I know the vehicle only has one ROI. Any message returned should probably be independent of whatever is done with that ROI, so I'm not sure why the gimbal ID is in there. Also, your ROI can be a specific system ID, or the next waypoint with a particular angular offset, neither of which is representable in the mooted message.

Somewhat related to this is ArduPilot's "location database" work (https://github.com/ArduPilot/ardupilot/pull/24429), which I hope to get in sometime this decade. We'll need a messaging set around that, and the ability to set (and perhaps report...), which object from the location database is the current ROI.

nexton-winjeel commented 1 year ago

@peterbarker:

We've just merged support for CAMERA_FOV_STATUS

Yep, that's related, but not enough. CAMERA_FOV_STATUS tells you what you're actually looking at. This proposed message tells you what we're trying to look at (which may not be the same thing due to vehicle orientation and mount limits).

As far as I know the vehicle only has one ROI. Any message returned should probably be independent of whatever is done with that ROI, so I'm not sure why the gimbal ID is in there.

Right now, ArduPilot only supports one ROI in the message handling in GCS_MAVLink. But both the AP_Mount library and the MAVLink ROI commands support an ROI per mount.

Also, your ROI can be a specific system ID, or the next waypoint with a particular angular offset, neither of which is representable in the mooted message.

Agreed. I'm coming at this from the GCS side: what I care about is a 3D point I can draw on the map to indicate where the ROI is. I'm less interested in how that point was derived, but I did put the type field in the message to partially address this.

Somewhat related to this is ArduPilot's "location database" work [...]

That's interesting. I'll try to get my head around it tomorrow.

On a related note, something else that I'll write up a proposal for one day: ROIs should be a MAV_MISSION_TYPE. It would allow you to pre-plan ROIs and easily switch between them while flying (e.g via a switch on an RC or a button on a GCS).

nexton-winjeel commented 1 year ago

@hamishwillee:

[...] but as always to be part of the standard it needs broad interest to implement from the key stakeholders.

Agreed, which is why I raised an issue. Once there is consensus, I'll attempt to get a solution into ArduPilot.

hamishwillee commented 1 year ago

Yes, there is the possibility of multiple ROIs that can be looked at at the same time, or that can be set but potentially not enabled. Further ROIs might be fixed, as we mostly imagine now, or mapped to a vehicle or some other moving target.

Looking at RAS-A it appears to define "point" of interest with point geometry, classification, and lots of other info. We should at least consider that.

hamishwillee commented 11 months ago

@nexton-winjeel Discussed this briefly in dev call last night.

  1. We should un-deprecate MAV_ROI - we need this information still. It may be useful to know if you're looking at a next waypoint or a sysid or what.
  2. Given that though, perhaps we should include an target id for the message - so if you're looking at a sys id or "some database value" you can display that in some meaningful way to the user?
  3. Gimbal ID makes sense to me, but what if you have a fixed camera?
  4. Should lat, lon, alt ALL be relative to MAV_FRAME? I.e. will there be indoor ROIs, or need to show ROI in a local frame? I tend to think we should specify them as relative to MAV_FRAME, but then in the MAV_FRAME field specify that it currently only may support frames x, y, z (list those you want).
  5. I'm going to assume that an ROI is a 3d point of camera focus. Do we need to think about regions? i.e. a POI with some kind of shape or area (I am not inclined to, just throwing it in).
  6. ROIs can exist in a mission right? Do we need to indicate that an ROI is mission set or command set? Note I am not sure how such things work - I THINK that it is last ROI set by any mechanism is used. So if you send a command to set ROI you get that, but if the mission were to have another ROI setting that would then overcome the command.
  7. If all ROIs are cleared what should be sent?

We're on hold until you have time to discuss this again.

nexton-winjeel commented 8 months ago

Back looking at this one. I've updated the message definition above to address the following:

  1. We should un-deprecate MAV_ROI - we need this information still. It may be useful to know if you're looking at a next waypoint or a sysid or what.

:+1:

  1. Given that though, perhaps we should include an target id for the message - so if you're looking at a sys id or "some database value" you can display that in some meaningful way to the user?

I've added an roi_data field to map to the MAV_ROI options.

  1. Gimbal ID makes sense to me, but what if you have a fixed camera?

Set it to 0? What is expected behaviour for MAV_CMD_DO_SET_ROI_LOCATION and friends in this case?

  1. Should lat, lon, alt ALL be relative to MAV_FRAME? I.e. will there be indoor ROIs, or need to show ROI in a local frame? I tend to think we should specify them as relative to MAV_FRAME, but then in the MAV_FRAME field specify that it currently only may support frames x, y, z (list those you want).

Restricted the allowed frames. The coordinate_frame, lat_int, lon_int and alt fields are a direct copy of the equivalent fields in SET_POSITION_TARGET_GLOBAL_INT.

  1. I'm going to assume that an ROI is a 3d point of camera focus. Do we need to think about regions? i.e. a POI with some kind of shape or area (I am not inclined to, just throwing it in).

We don't have a command to set a POI as a shape/area, so no point reporting it here.

  1. ROIs can exist in a mission right? Do we need to indicate that an ROI is mission set or command set? Note I am not sure how such things work - I THINK that it is last ROI set by any mechanism is used. So if you send a command to set ROI you get that, but if the mission were to have another ROI setting that would then overcome the command.

Yeah, ROIs can exist in a mission. But I don't think there is any value in reporting whether the ROI came from a command or a mission (no other messages make this distinction).

  1. If all ROIs are cleared what should be sent?

Set the type to MAV_ROI_NONE and the gimbal_device_id to 0.

hamishwillee commented 8 months ago

@nexton-winjeel I'm going to have to defer looking at this until next week sorry - spent too much time looking at MAVLink things and now have some higher priority stuff to look at.

nexton-winjeel commented 8 months ago

No worries. We're currently working through initial ArduPilot and QGC implementations for this, so we'll have something more concrete to discuss in the next week or two.

hamishwillee commented 7 months ago

OK, scanned through. I think it makes a lot of sense:

  1. When is this emitted?
  2. FYI ArduPilot are trying to deprecate the _INT frames. Not convinced yet https://github.com/mavlink/mavlink/pull/2092#top
  3. Can any of these values be "not set" and if so, what is the unused value? (I think the answer is either no, or those defaults already specified by the enums), just checking.
  4. This would go into development.xml first until broadly agreed and prototype. FWIW got my vote - its useful but no one has to implement this.
auturgy commented 7 months ago

Discussed at Mavlink devcall 13Mar24. Approved in principle, waiting on draft implementation/PR prior to merging to development.xml