-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix hardcoded cam footprint field of view and AGL #2103
fix hardcoded cam footprint field of view and AGL #2103
Conversation
I mention of few of these fixes in a comment for issue #172 |
sw/ground_segment/cockpit/live.ml
Outdated
@@ -326,19 +333,6 @@ let mark = fun (geomap:G.widget) ac_id track plugin_frame -> | |||
let png = sprintf "%s/var/logs/%s" Env.paparazzi_home name in | |||
GdkPixbuf.save png "png" dest; | |||
incr i; | |||
|
|||
(* Computing the footprint: front_left and back_right *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part is actually not related to the camera footprint but to the "mark" button and screenshot tool. See http://wiki.paparazziuav.org/wiki/GCS#Actions
please add back this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops my bad.
sw/ground_segment/cockpit/live.ml
Outdated
@@ -1521,6 +1532,45 @@ let get_shapes = fun (geomap:G.widget)_sender vs -> | |||
let listen_shapes = fun (geomap:G.widget) -> | |||
safe_bind "SHAPE" (get_shapes geomap) | |||
|
|||
let get_cam_status = fun (geomap:MapCanvas.widget) _sender vs -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point the camera is pointing at (taking into account the bank angles and the ground alt) is already computed in the server program: https://github.com/paparazzi/paparazzi/blob/master/sw/ground_segment/tmtc/server.ml#L165
Your current position and this cam point should be enough to compute your polygon without extra information (maybe AGL).
Also be careful that at large bank angle, the projection of the footprint may send some corner very "far". I don't know how GTK canvas are dealing with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there some ambiguity if the camera is on a gimbal? It is unknown if the camera has any rotation in the direction it is pointing. Would it be better to move all the calculations into the server program and modify the cam status message to transmit the 4 corners of footprint? My current code also does not take into account any gimbal effects that the original may have, because the gimbal information is only being read on the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a point can't be plotted, the GCS will stop updating the footprint and retain the last plotted footprint. Once the aircraft has leveled off and a footprint can be plotted, then the footprint continues to update. I have performed multiple flights with this code without any GCS error messages or crashing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even without a gimbal just a camera point is not enough. An aircraft traveling north at a 30deg pitch, will point a camera is the same direction as an aircraft traveling east with a 30deg roll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gimbal information (pan and tilt angle) are currently taken from the telemetry CAM message (if sent by the drone, assuming 0 by default). With this, the server
agent is computing a target point. Isn't this enough with altitude above ground to compute the footprint ? I don't really like the idea of sending the coordinates of the corners of the footprint. I prefer either a target point or absolute angles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The horizontal field of view and the vertical field of view of the camera may not be the same. We need one program to read the fields from airframe.xml. If the camera is pointed straight down(tilt angle of 0), the target point will be directly under the aircraft no matter what angle the camera is panned at. Due to pan angle the footprint may be a wide rectangle, a skinny rectangle, or a diamond shape. This test case can then be applied to a variety of tilt angle. Am I missing something with the target point? The pieces of information we need to fully calculate a footprint are ac.pitch, ac.roll, ac.yaw, ac.agl, ac.pos, cam.pan, cam.tilt, cam.hfv, and cam.vfv. IMHO the server should send cam.pan and cam.tilt messages to the GCS, GCS will then compute the footprint (having all the information), GCS will then send the the information to mapTrack.
sw/ground_segment/cockpit/live.mli
Outdated
@@ -54,6 +55,12 @@ type aircraft = private { | |||
mutable in_kill_mode : bool; | |||
mutable speed : float; | |||
mutable alt : float; | |||
mutable agl : float; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that in fact only agl
is needed for your needs
match last, cam_on with | ||
Some last_ac, true -> | ||
let (cam_xw, cam_yw) = geomap#world_of cam_wgs84 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the math that you have in your get_cam_status
function could go here as well
sw/lib/ocaml/mapTrack.ml
Outdated
`OUTLINE_COLOR color]; | ||
cam#affine_absolute (affine_pos_and_angle 1.0 cam_xw cam_yw cam_heading); | ||
(*cam#affine_absolute points; *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove if not needed anymore
Im going to close this pull request and make the suggested changes and open it again |
@rijesha sorry I couldn't help much, I'm really busy these days |
@gautierhattenberger I have moved all of the code to the server e3abd26. I have also used quaternions to handle a gimbal. Commit a6126a0 is how the cam footprint code would look like it it were in the GCS. I decided to put it in the server so that the cam footprint information is broadcasted out for other programs to easily use. I still need to clean up some of the code, but I figured I should get your opinion on whether the code is better in the server or the gcs before proceeding. |
sw/ground_segment/tmtc/server.ml
Outdated
"cam_lat_bl", PprzLink.Float ((Rad>>Deg)geo_3.posn_lat); | ||
"cam_lon_bl", PprzLink.Float ((Rad>>Deg)geo_3.posn_long); | ||
"cam_lat_br", PprzLink.Float ((Rad>>Deg)geo_4.posn_lat); | ||
"cam_lon_br", PprzLink.Float ((Rad>>Deg)geo_4.posn_long); | ||
"cam_target_lat", PprzLink.Float ((Rad>>Deg)twgs84.posn_lat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautierhattenberger Is there a better way of doing this? My OCaml is not good enough to figure out how to make arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can send comma-separated values like DL_VALUES
(https://github.com/paparazzi/paparazzi/blob/master/sw/ground_segment/tmtc/server.ml#L204 and https://github.com/paparazzi/paparazzi/blob/master/sw/ground_segment/cockpit/live.ml#L956)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you manage to use an array to send the cam footprint, that would be better I think, so more complex shapes could be used in the future if needed.
sw/ground_segment/tmtc/server.ml
Outdated
and br_rotated = multiply_quaternion acRot (multiply_quaternion gimRot br) | ||
and bl_rotated = multiply_quaternion acRot (multiply_quaternion gimRot bl) in | ||
|
||
let absF (f:float) = if f > 0.0 then f else (f *. -1.0) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it already exists with abs_float
sw/ground_segment/tmtc/server.ml
Outdated
"cam_lat_bl", PprzLink.Float ((Rad>>Deg)geo_3.posn_lat); | ||
"cam_lon_bl", PprzLink.Float ((Rad>>Deg)geo_3.posn_long); | ||
"cam_lat_br", PprzLink.Float ((Rad>>Deg)geo_4.posn_lat); | ||
"cam_lon_br", PprzLink.Float ((Rad>>Deg)geo_4.posn_long); | ||
"cam_target_lat", PprzLink.Float ((Rad>>Deg)twgs84.posn_lat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can send comma-separated values like DL_VALUES
(https://github.com/paparazzi/paparazzi/blob/master/sw/ground_segment/tmtc/server.ml#L204 and https://github.com/paparazzi/paparazzi/blob/master/sw/ground_segment/cockpit/live.ml#L956)
sw/ground_segment/cockpit/live.ml
Outdated
@@ -406,6 +414,16 @@ let get_speech_name = fun af_xml def_name -> | |||
fvalue "SPEECH_NAME" default_speech_name | |||
with _ -> default_speech_name | |||
|
|||
let get_cam_aov = fun af_xml -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need the cam field of view in the GCS since you are doing the computation in server agent ? It doesn't seemed to be used here anymore. Some question for roll, pitch, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I will clean up all of my code and removed any extra variables/functions I no longer need.
|
||
let target_wgs84 = { posn_lat = (Deg>>Rad)(a "cam_target_lat"); posn_long = (Deg>>Rad)(a "cam_target_long") } in | ||
|
||
let geo_1 = { posn_lat = (Deg>>Rad)(List.nth lats 0); posn_long = (Deg>>Rad)(List.nth longs 0) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautierhattenberger In the example you gave me for DL_Settings Arrays were used. I don't think there should be too much performance degradation keeping it in lists. Please correct me if I am wrong. Do i actually need to put exception handling in for List.nth? Will safe_bind catching it and notifying the user be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the sizes involved, list or arrays should not make a big difference.
The safe_bind
should catch exceptions and print them.
Ok. I think this is now ready to be pushed. Pprzlink PR paparazzi/pprzlink#59 |
please update for the correct PPRZLINK version |
you should also rebase on top of master to solve the conflicts, otherwise I can't merge it |
Oops I didn't rebase. I just merged master in. It should still work though? |
The cam footprint angle of view can now be adjusted in the airframe.xml file. The old cam footprint math used height in MSL not AGL. In addition the footprint did not adjust properly when the aircraft pitched or rolled.