ArduPilot / MissionPlanner

Mission Planner Ground Control Station for ArduPilot (c# .net)
http://ardupilot.org/planner/
GNU General Public License v3.0
1.81k stars 2.42k forks source link

Point Camera Here altitude is alt-above-home #2392

Open rmackay9 opened 4 years ago

rmackay9 commented 4 years ago

MP-roi When the user selects the right-mouse-button-menu item, "Point camera here" a message box pops up indicating that the altitude is above sea level but it's actually above home. The ArduPilot code is here.

I've verified the current behaviour is incorrect using a Gremsy PixyU camera which talks using mavlink the same as the SToRM32 gimbal. These calculations are all done in the ArduPilot AP_Mount library so the results should not be gimbal specific.

meee1 commented 4 years ago

i currently send DO_SET_ROI behavior changed here https://github.com/ArduPilot/ardupilot/commit/402f3ec09c41c873d3887a412e383334fdfe463b

meee1 commented 4 years ago

so question is, which way do i go

jagrandle commented 4 years ago

Setting the other altitudes, such as from the Action tab as shown in the image above, seem to have the convention of being above Home, rather than Absolute. It might be good to follow the same convention ?

I imagine people may know the height of an object such as a tower or building, which is typically relative to the ground, but I doubt they know it above Sea Level. Saves the mental gymnastics and the opportunity to make a mistake.

Just a thought.

meee1 commented 4 years ago

the other catch here is i think the point camera to coords now is also broken because of the breaking change to do_set_roi

rmackay9 commented 4 years ago

I agree with @jagrandle, I think most users will be able to come up with an altitude above home more easily than an altitude above sea-level... so I think the behaviour change introduced by @peterbarker was a good one although we should have thought this through and/or added an issue for MP at the time that change went in.

peterbarker commented 4 years ago

@meee1 can you send the command through as a COMMAND_INT and choose the correct frame?

peterbarker commented 4 years ago

@rmackay9 yes, that behaviour change is odd. The extended commit message for that change even calls the fact out, but is oddly truncated. So whatever reason we had at the time for making that change is lost in the mists of time, sadly. But it was, apparently, not an accident.

I have really vague recollections of the backends not all doing the same thing with that parameter - so it might be we just went for consistency and a "good choice"?

We could revert that by changing one line to be in absolute-altitude - and encourage GCS authors to use a COMMAND_INT to specify a frame. This hasn't made a Plane or Copter stable, so I think we'd not annoy too many people and save @meee1 a headache. Your point about people being able to think in the local coordinate frame would be addressed by using COMMAND_INT.

peterbarker commented 4 years ago

That MissionPlanner commit probably means we want to grab support for the COMMAND_INT back into the soon-to-be-released 4.0.x releases, @rmackay9

meee1 commented 4 years ago

yes ive converted to command_int. so now just depends on release cycles/users

PYBrulin commented 4 years ago

@meee1 , correct me if I'm wrong, but it seems to me that there is a unit conversion mistake in the altitude sent for camera commands. Currently, to be able to point the camera 10 meters above home with "Point Camera Here", the user should enter 0.1 as the targeted altitude. According to the COMMAND_INT definition, z should be sent in meters, however the altitude is currently multiplied by 100 and cast as an int, corresponding to the altitude in centimeters.

meee1 commented 4 years ago

when sending as an int you send lat/lng as 1e7 and alt as 1e2 so looks correct to me

PYBrulin commented 4 years ago

@meee1, I do agree with the lat/lng as 1e7. However for the altitude ArduPilot does the 1e2 and int cast when handling packet.z for MAV_CMD_DO_SET_ROI_* from a COMMAND_INT (GCS_Common.cpp#L4119). The message definition for COMMAND_INT defines z as the altitude in meters and as a float (mavlink_msg_command_int) so I believe the *1e2 is redundant on MP side. Currently, this makes the selection of the target alt in the pop-up window not in meters but in hectometers (selecting 1 gives a final ROI target 100 meters above home).

meee1 commented 4 years ago

yes i think your correct. waypoints use alt in CM's commandint uses m's i love the consistency here.....