Align with upstream common for a number of definitions (#2317) #409
Conversation
hamishwillee
force-pushed
the
mav_bool_cherry
branch
from
6377a1e to
77c0ac3
Compare
| <param index="2" label="Shutter" units="ms" minValue="-1" increment="1">Camera shutter integration time. -1 or 0 to ignore</param> | ||
| <param index="3" label="Trigger" enum="MAV_BOOL">Trigger camera once, immediately (MAV_BOOL_TRUE). Values not equal to 0 or 1 are invalid.</param> | ||
| <param index="4">Empty</param> | ||
| <param index="4" label="Target Camera ID" minValue="0" maxValue="255" increment="1">Target camera ID. 7 to 255: MAVLink camera component id. 1 to 6 for cameras attached to the autopilot, which don't have a distinct component id. 0: all cameras. This is used to target specific autopilot-connected cameras. It is also used to target specific cameras when the MAV_CMD is used in a mission.</param> |
There was a problem hiding this comment.
@peterbarker I know you don't love these, but this is what was agreed with @rmackay9 . There are good reasons for this approach - specifically that autopilots don't want to implement attached components such as cameras as having a unique mavlink identity. This allows addressing of autopilot-attached cameras and addressing of any camera from a mission.
hamishwillee
changed the title
| <param index="4">Forward moving aircraft this sets exit xtrack location: 0 for center of loiter wp, 1 for exit location. Else, this is desired yaw angle. NaN to use the current system yaw heading mode (e.g. yaw towards next waypoint, yaw to home, etc.).</param> | ||
| <description>Loiter at the specified latitude, longitude and altitude for a certain amount of time. Multicopter vehicles stop at the point (within a vehicle-specific acceptance radius). Forward-only moving vehicles (e.g. fixed-wing) circle the point with the specified radius/direction. If the Heading Required parameter (2) is non-zero forward moving aircraft will only leave the loiter circle once heading towards the next waypoint.</description> | ||
| <param index="1" label="Time" units="s" minValue="0">Loiter time (only starts once Lat, Lon and Alt is reached).</param> | ||
| <param index="2" label="Heading Required" enum="MAV_BOOL">Leave loiter circle only when track heading towards the next waypoint (MAV_BOOL_FALSE: Leave on time expiry). Values not equal to 0 or 1 are invalid.</param> |
There was a problem hiding this comment.
Please revert the patch here giving meaning to this field. AP doesn't support it, PX4-Autopilot does, it's a mess to sort out later.
There was a problem hiding this comment.
As discussed (offline)
- I won't revert these because we did not have a shared understanding of the scope of this PR.
Specifically, you wanted a PR limited to only MAV_BOOL related changes but have then rejected the two MAV_BOOL changes on the basis that ArduPilot doesn't support them.
That would be fine, except there isn't any point removing them because then there would be nothing to review. - Rejecting meanings for empty/reserved parameters added in mavlink/mavlink is "invalid", whether or not they are supported in ArduPilot/mavlink.
- The responsibility of mavlink/mavlink is to ensure that:
- default values aren't likely to have unintended side effects.
- any consumers of this are consulted before additions.
- The responsibility of consumers is to return a COMMAND_ACK with MAV_RESULT_DENIED for non implemented params that receive non-default values.
- BUT we have unclear meaning of those. I think we need to standardize on those used for COMMAND_INT by default -NaN for floats, INT max for the others.
- Note that we could compare against either not NAN or not INT_MAX by default. This would be easier for ArduPilot due to work in https://github.com/ArduPilot/ardupilot/pull/28866/files#top
- The responsibility of mavlink/mavlink is to ensure that:
SO this thing needs to be reviewed as "did this change the meaning of anything that is harmful/semantic change", and as a trigger to making sure that if these commands are received they are rejected by ArduPilot/PX4 if not supported. Note, PX4 is poor at this too. We are not allowed to break consumers, but we are allowed to extend them safely.
We also need to agree that default value as a matter of urgency
| <param index="2">Empty</param> | ||
| <param index="3">Empty</param> | ||
| <param index="4">Empty</param> | ||
| <param index="2" label="Roll" units="deg" minValue="-180" maxValue="180">Roll angle (of surface). Range: -180..180 degrees. NAN or 0 means value not set. 0.01 indicates zero roll.</param> |
There was a problem hiding this comment.
Please revert this change. It's unrelated to the BOOL stuff.
It might be good, but we don't want to hold things up for evaluation.
There was a problem hiding this comment.
Invalid because of https://github.com/ArduPilot/mavlink/pull/409/files#r2241619487
| <param index="2" label="Shutter" units="ms" minValue="-1" increment="1">Camera shutter integration time. -1 or 0 to ignore</param> | ||
| <param index="3" label="Trigger" enum="MAV_BOOL">Trigger camera once, immediately (MAV_BOOL_TRUE). Values not equal to 0 or 1 are invalid.</param> | ||
| <param index="4">Empty</param> | ||
| <param index="4" label="Target Camera ID" minValue="0" maxValue="255" increment="1">Target camera ID. 7 to 255: MAVLink camera component id. 1 to 6 for cameras attached to the autopilot, which don't have a distinct component id. 0: all cameras. This is used to target specific autopilot-connected cameras. It is also used to target specific cameras when the MAV_CMD is used in a mission.</param> |
There was a problem hiding this comment.
Let's keep this one as Randy's OK'd it.
There was a problem hiding this comment.
Cool. But its the same thing really.
| <param index="2">Reserved (all remaining params)</param> | ||
| <param index="2" label="Target Camera ID" minValue="0" maxValue="255" increment="1">Target camera ID. 7 to 255: MAVLink camera component id. 1 to 6 for cameras attached to the autopilot, which don't have a distinct component id. 0: all cameras. This is used to target specific autopilot-connected cameras. It is also used to target specific cameras when the MAV_CMD is used in a mission.</param> |
There was a problem hiding this comment.
Don't care about this one as we don't support it...
There was a problem hiding this comment.
Ditto https://github.com/ArduPilot/mavlink/pull/409/files#r2241619487 - I won't close this though, so you have your note when reviewing the rest that this one is OK.
hamishwillee
changed the title
--------- Co-authored-by: PX4BuildBot <[email protected]>
hamishwillee
force-pushed
the
mav_bool_cherry
branch
from
77c0ac3 to
1436d5b
Compare
3 participants
This aligns the fork with upstream common for a number of definitions in mavlink/mavlink (this is the set of things to which we just added MAV_BOOL, since we're currently familiar with these messages).
The changes primarily:
I believe this should make it easy to see where there are clashes, if any.
Notes:
MAV_CMD_DO_SET_MISSION_CURRENT - incompatible (need to look at, and perhaps revert in this, or do separately.