-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Add mount POI support to c++ (send camera fov status) #24657
Conversation
|
||
float dist_increment_m; | ||
float dist_max; | ||
AP_Param::get("TERRAIN_SPACING", dist_increment_m); |
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.
We shouldn't directly grab parameters by name like this because it is quite CPU intensive. Instead if necessary we should add an accessor to AP_Terrain to get the current spacing.
For POI_DIST_MAX we will probably need to add a new parameter or we could just hardcode it for now to some value like 10km.
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.
Currently Hardcoded. But i am not marking this a resolved so later i remember this to be done.
Great to see this, I've added some comments. thanks! |
40f2d0b
to
2f0ab17
Compare
f6688a0
to
612835e
Compare
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.
Also needs autotests.
2c719f5
to
332c698
Compare
Well i fixed all things. please ignore the current code. just need a test and a i will push here |
2fe43b3
to
e6f9aa1
Compare
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 reviews. Addressed most of the comments of @rmackay9 and @peterbarker. Still i dont have a clue on how to test the priority and stack size. however functionality is working.
libraries/AP_Camera/AP_Camera.cpp
Outdated
@@ -418,6 +418,17 @@ void AP_Camera::send_camera_settings(mavlink_channel_t chan) | |||
} | |||
} | |||
|
|||
#if AP_TERRAIN_AVAILABLE |
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 AP_TERRAIN_AVAILABLE | |
#if AP_CAMERA_SEND_FOV_STATUS_ENABLED |
... and all other relevant places.
You'll need an entry like this in AP_Camera_config.h
:
#ifndef AP_CAMERA_SEND_FOV_STATUS_ENABLED
#define AP_CAMERA_SEND_FOV_STATUS_ENABLED AP_TERRAIN_AVAILABLE
#endif
... and probably #include <AP_Terrain/AP_Terrain_config.h
at the top of that file
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.
done
libraries/AP_Camera/AP_Camera.h
Outdated
@@ -10,6 +10,8 @@ | |||
#include <GCS_MAVLink/GCS_MAVLink.h> | |||
#include "AP_Camera_Params.h" | |||
#include "AP_Camera_shareddefs.h" | |||
#include <AP_Terrain/AP_Terrain.h> |
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.
Will not need this include
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.
done
e6f9aa1
to
631f05d
Compare
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.
done changes according to reviews given. have a look
libraries/AP_Camera/AP_Camera.cpp
Outdated
@@ -418,6 +418,17 @@ void AP_Camera::send_camera_settings(mavlink_channel_t chan) | |||
} | |||
} | |||
|
|||
#if AP_TERRAIN_AVAILABLE |
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.
done
libraries/AP_Camera/AP_Camera.h
Outdated
@@ -10,6 +10,8 @@ | |||
#include <GCS_MAVLink/GCS_MAVLink.h> | |||
#include "AP_Camera_Params.h" | |||
#include "AP_Camera_shareddefs.h" | |||
#include <AP_Terrain/AP_Terrain.h> |
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.
done
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 is look really quite good now!
This is all minor stuff, 'though I have not really delved into your actual algorithm (Randy has, FWIU),
NaN, // currently setting hfov as NaN | ||
NaN // currently setting vfov as NaN | ||
); | ||
#endif |
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 don't think this is nested correctly.
Please add your new define to build_options.py
.
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.
Added. but should it be done for AP_MOUNT_POI_TO_LATLNGALT_ENABLED also ?
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.
Yes, please.
if (total_dist >= dist_max) { | ||
// POI: unable to find terrain within dist_max | ||
continue; | ||
} else if (is_negative(terrain_amsl_m)) { |
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.
} else if (is_negative(terrain_amsl_m)) { | |
} | |
if (is_negative(terrain_amsl_m)) { |
631f05d
to
ffdb5e5
Compare
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.
@peterbarker , done changes suggested by you. however i have concerns in some of them listed here.
NaN, // currently setting hfov as NaN | ||
NaN // currently setting vfov as NaN | ||
); | ||
#endif |
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.
Added. but should it be done for AP_MOUNT_POI_TO_LATLNGALT_ENABLED also ?
ffdb5e5
to
b8e81c9
Compare
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.
@peterbarker Done fixing. Have a look (specially at the one making const to get_poi in mount backend)
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.
LGTM, thanks!
b8e81c9
to
c4f94cc
Compare
Replaced by #25390 so I will close this PR. The new PR's commits preserve the contributors github account though so credit will still go where it belongs. Txs. |
Reference issue #24038 A Task from Camera Enhancements list #23151.
This PR adds support for sending the STATUS_FOV message to the ground station including the lat,lon,alt of the Location that the gimbal is pointing at. The calculations used are ported from the mount-poi lua script.
Tested With using V6x and SiYi zr10.