Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

khanasif786
Copy link
Contributor

@khanasif786 khanasif786 commented Aug 15, 2023

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.
Screenshot from 2023-08-29 15-04-22


float dist_increment_m;
float dist_max;
AP_Param::get("TERRAIN_SPACING", dist_increment_m);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@rmackay9
Copy link
Contributor

Great to see this, I've added some comments. thanks!

@khanasif786 khanasif786 force-pushed the mount_poi branch 2 times, most recently from 40f2d0b to 2f0ab17 Compare August 22, 2023 07:45
@khanasif786
Copy link
Contributor Author

khanasif786 commented Aug 22, 2023

I have tested this. the version without threading works. Now in next push it will be done with threading. Some other parts also need work for eg: new parameter for max distance Screenshot from 2023-08-22 12-44-31

@khanasif786 khanasif786 force-pushed the mount_poi branch 3 times, most recently from f6688a0 to 612835e Compare August 22, 2023 19:32
@khanasif786 khanasif786 marked this pull request as ready for review August 22, 2023 19:33
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/GCS_MAVLink/GCS_Common.cpp Show resolved Hide resolved
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs autotests.

@khanasif786 khanasif786 force-pushed the mount_poi branch 3 times, most recently from 2c719f5 to 332c698 Compare August 24, 2023 19:32
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
@khanasif786
Copy link
Contributor Author

khanasif786 commented Aug 28, 2023

Well i fixed all things. please ignore the current code. just need a test and a i will push here

libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@khanasif786 khanasif786 left a 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_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
@@ -418,6 +418,17 @@ void AP_Camera::send_camera_settings(mavlink_channel_t chan)
}
}

#if AP_TERRAIN_AVAILABLE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -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>
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@khanasif786 khanasif786 left a 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

@@ -418,6 +418,17 @@ void AP_Camera::send_camera_settings(mavlink_channel_t chan)
}
}

#if AP_TERRAIN_AVAILABLE
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -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>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@peterbarker peterbarker left a 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),

libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
NaN, // currently setting hfov as NaN
NaN // currently setting vfov as NaN
);
#endif
Copy link
Contributor

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please.

libraries/AP_Camera/AP_Camera_config.h Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_config.h Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_config.h Outdated Show resolved Hide resolved
if (total_dist >= dist_max) {
// POI: unable to find terrain within dist_max
continue;
} else if (is_negative(terrain_amsl_m)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (is_negative(terrain_amsl_m)) {
}
if (is_negative(terrain_amsl_m)) {

libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.h Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_config.h Show resolved Hide resolved
libraries/GCS_MAVLink/GCS_Common.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@khanasif786 khanasif786 left a 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.

libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
NaN, // currently setting hfov as NaN
NaN // currently setting vfov as NaN
);
#endif
Copy link
Contributor Author

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 ?

libraries/AP_Camera/AP_Camera_config.h Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_config.h Outdated Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_config.h Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.h Outdated Show resolved Hide resolved
libraries/GCS_MAVLink/GCS_Common.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_config.h Show resolved Hide resolved
libraries/AP_Mount/AP_Mount.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_config.h Show resolved Hide resolved
Copy link
Contributor Author

@khanasif786 khanasif786 left a 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)

libraries/AP_Mount/AP_Mount.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@rmackay9
Copy link
Contributor

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.

@rmackay9 rmackay9 closed this Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants