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

Always trigger all cameras #21684

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

potaito
Copy link
Contributor

@potaito potaito commented Jun 5, 2023

Solved Problem

The camera triggering logic is currently "random" when more than one camera sending a mavlink heartbeat is connected to PX4. The trigger logic in PX4 currently stores the last camera mavlink component ID it saw a heartbeat from, and then sends trigger commands to that camera. If there are multiple cameras sending heartbeats, it is more or less random which camera gets triggered when a capture command is sent to PX4. This includes missions that have capture waypoints, or surveys that capture a photo periodically.

Solution

This PR changes the behaviour such that always all mavlink cameras are triggered, making the photo capture process deterministic, which is the main improvement.

This changes nothing for systems that have only one camera, as it will still trigger this one camera.

The only scenario where PX4 users would be negatively affected by this change, is if somebody were to use a camera that understands mavlink, receives mavlink messages, but does not send out its own heartbeat and is therefore invisible to PX4. Before this PR, such an invisible camera would not get triggered. After this PR it will. This is a very hypothetical scenario which I hope does not exist in reality. It would also be non-compliant with the mavlink protocol to have a component participate in the mavlink network without sending its own heartbeat.

Changelog Entry

For release notes:
Bugfix: When multiple mavlink cameras are present, they will now all be triggered by PX4

Test coverage

Tested on real hardware

@potaito potaito requested review from dagar and tstastny June 5, 2023 16:07
@potaito
Copy link
Contributor Author

potaito commented Jun 5, 2023

@tstastny @dagar this is not the perfect solution, but I would argue that any deterministic solution is better than undefined behaviour :)

Copy link

@tstastny tstastny left a comment

Choose a reason for hiding this comment

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

lgtm - just need to run make format

tstastny
tstastny previously approved these changes Jun 5, 2023
@tstastny tstastny force-pushed the potaito/trigger-all-cameras-deterministically branch from fc45513 to e844792 Compare June 8, 2023 15:17
@tstastny
Copy link

tstastny commented Jun 9, 2023

@dagar Any idea why we're getting this message on jenkins for cuav_x7pro and px4fmu_v4?

Running command: 'param save'
ERROR [parameters] param_save_default: file lock failed (already locked)
ERROR [param] Param save failed (-1)

I don't think this PR would have an effect on this.. but didnt know if this was an error you were tracking already?

@tstastny tstastny merged commit f951055 into main Jun 9, 2023
82 of 84 checks passed
@tstastny tstastny deleted the potaito/trigger-all-cameras-deterministically branch June 9, 2023 11:17
@tstastny
Copy link

tstastny commented Jun 9, 2023

@dagar Any idea why we're getting this message on jenkins for cuav_x7pro and px4fmu_v4?

Running command: 'param save'
ERROR [parameters] param_save_default: file lock failed (already locked)
ERROR [param] Param save failed (-1)

I don't think this PR would have an effect on this.. but didnt know if this was an error you were tracking already?

ok @bkueng told me this is related to the lock free param thing

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

2 participants