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

AP_Camera: add time based triggering support #24357

Merged
merged 1 commit into from Aug 5, 2023

Conversation

khanasif786
Copy link
Contributor

Tested in SiYi Zr10.

@rmackay9
Copy link
Contributor

Let's make sure that distance based triggering and time interval triggering cannot be active at the same time. So if one is activated it should disable the other.

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Jul 20, 2023
@khanasif786 khanasif786 force-pushed the time_trig branch 3 times, most recently from 8e4cc7a to ca354b1 Compare July 25, 2023 08:35
@khanasif786
Copy link
Contributor Author

khanasif786 commented Jul 25, 2023

Let's make sure that distance based triggering and time interval triggering cannot be active at the same time. So if one is activated it should disable the other.

@rmackay9 In the newer patch when time and distance both triggering is enabled at the same time then only time triggering will be done. Is it good?

@rmackay9
Copy link
Contributor

Ah, it seems in the mavlink spec, "Total Images: 0 to capture forever/until MAV_CMD_IMAGE_STOP_CAPTURE".. I think we're not handling this. This may mean that we DO need the enabled boolean. Or some way to indicate whether we're taking pictures forever or not. You could change num_remaining to be a signed integer (int16_t might be fine if we think 32K images is OK. and I think it is) and use "-1" to mean take pictures forever. Or you could change the enabled boolean to be an enum (DISABLED, ENABLED, ENABLED_FOREVER).. anyway, I'll leave this to you to decide.

@khanasif786
Copy link
Contributor Author

khanasif786 commented Jul 26, 2023

Or you could change the enabled boolean to be an enum (DISABLED, ENABLED, ENABLED_FOREVER).. anyway, I'll leave this to you to decide.

using -1 makes solving this easier and concise. so choosing that. let me know if you want the another method.

@khanasif786 khanasif786 force-pushed the time_trig branch 2 times, most recently from f41bded to 0630b5f Compare July 26, 2023 18:36
@rmackay9
Copy link
Contributor

This looks correct to me.

The use of -1 to specify take pictures forever is a little unusual in AP but it's OK I think.

@peterbarker how do you feel about this?

@khanasif786
Copy link
Contributor Author

khanasif786 commented Aug 2, 2023

Refrence to mavlink PR #2024 For further upgrading this PULL according to the new definition of MAV_CMD_IMAGE_START_CAPTURE

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.

We probably want to add an instance field to that message....

libraries/AP_Camera/AP_Camera.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 Show resolved Hide resolved
libraries/AP_Camera/AP_Camera_Backend.cpp Outdated Show resolved Hide resolved
@khanasif786
Copy link
Contributor Author

I think for MAV_CMD_IMAGE_STOP_CAPTURE we should have a separate PR. where we can add functionality to stop capture no matter what type of triggering we are doing (distance, time).

@rmackay9 rmackay9 dismissed peterbarker’s stale review August 4, 2023 06:14

I think PeterB's requests have been addressed

@rmackay9
Copy link
Contributor

rmackay9 commented Aug 4, 2023

Great stuff, let's merge after it passes CI.

@peterbarker peterbarker merged commit 2141f06 into ArduPilot:master Aug 5, 2023
81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants