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

[v4.4] Gimbal fixes #11706

Merged
merged 5 commits into from
Aug 2, 2024
Merged

[v4.4] Gimbal fixes #11706

merged 5 commits into from
Aug 2, 2024

Conversation

julianoes
Copy link
Contributor

This includes several gimbal fixes. For more information, please check the individual commit descriptions.

Fixes #11686.

Otherwise, we potentially work with garbage.
This changes how the found gimbals are referenced. Instead of only using
the gimbal device ID for the gimbal map, we now also use the gimbal
manager compid.

This assumes that it is valid to have more than one gimbal manager with
non-MAVLink gimbals attached, which means the gimbal_device_id would
clash in that case, e.g. both would be 1.

Therefore, we use the gimbal manager compid as well as the associated
gimbal_device_id as the map key.
We should use the new yaw value, not the previous one when calculating
the missing frame.
We shouldn't just send the commands to the vehicle because the gimbal
manager might be implemented on any component, not just the autopilot.
@julianoes
Copy link
Contributor Author

@Davidsastresas would be good to get your testing, so I can get release v4.4.1 soon.

@Davidsastresas
Copy link
Member

Davidsastresas commented Aug 1, 2024

I just tested with Ardupilot 4.5.4 SITL, and it seems everything works alright. I tested all the functionality on following setup:

  • Single gimbal, where gimbal manager compid is autopilot compid.
  • Dual gimbal, where both gimbal managers compid are autopilot compid ( gimbal device id 1 and 2 ).

I have not tested other combinations than that. But I think the other interesting scenario to test after such changes is precisely what was reported to be fixed in #11686, so I think we should be good.

I saw you caught some sneaky bugs like ddeb2b5 how we were updating gimbal yaw from gimbal_device_attitude_status message, we were using the status of the last message instead of the one we were processing, very nice catches!

And overall, very nice fixes too, it is great that we keep covering all the scenarios we didn't thought about at the beginning, very nice work.

Whenever this goes in please let me know and I will leverage it in #11696, so hopefully we can keep updating master from now on with the updates we keep doing on 4.x.

Thanks!

@julianoes
Copy link
Contributor Author

Great, thanks so much for testing @Davidsastresas. And I'm impressed that you have a test setup with 2 gimbals, something I don't have yet.

@julianoes julianoes merged commit 70daedb into Stable_V4.4 Aug 2, 2024
4 of 6 checks passed
@julianoes julianoes deleted the pr-v4.4-gimbal-compid branch August 2, 2024 01:11
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.

2 participants