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

Refactor MAVLinkParameters into client and server classes #1772

Merged
merged 7 commits into from
May 22, 2023

Conversation

julianoes
Copy link
Collaborator

@julianoes julianoes commented May 17, 2022

This splits the MAVLinkParameters class into MavlinkParameterClient and MavlinkParameterServer.

This brings:

  • Support to provide parameters in components.
  • Get/set parameters for a specific component.
  • Both with normal as well as extendended parameters.

Big thanks to @Consti10 for starting this work!

@julianoes
Copy link
Collaborator Author

The current state is that it compiles again and the tests seem to pass but now it needs simplifying and cleanup.

@julianoes julianoes force-pushed the pr-split-mavlink-params branch 2 times, most recently from 1402600 to e970618 Compare December 17, 2022 05:37
@julianoes julianoes mentioned this pull request Mar 15, 2023
13 tasks
@julianoes julianoes force-pushed the pr-split-mavlink-params branch 2 times, most recently from 3ae028c to d459571 Compare March 23, 2023 23:14
@julianoes julianoes marked this pull request as ready for review May 5, 2023 23:34
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Wow that's a big one, with quite some improvements! It depends on mavlink/MAVSDK-Proto#296, right?

Just a couple questions:

  1. Is that right that it is the responsibility of the drone to now have conflicting param names between components? It seems to me that we save them all together, so if a camera has a param called MODE and the autopilot also has it, then they will conflict. Correct?
  2. What happens when select_component is called? Does that impact the params already in the queue, or only the new ones? Say I select the autopilot, set a param (which puts it on the worker queue), then select the camera to set another param. Am I guaranteed that the first param will be sent to the autopilot?

src/mavsdk/core/mavlink_parameter_cache.cpp Outdated Show resolved Hide resolved
src/mavsdk/plugins/calibration/calibration_impl.cpp Outdated Show resolved Hide resolved
@julianoes
Copy link
Collaborator Author

julianoes commented May 20, 2023

@JonasVautherin thanks for the review!

  1. Is that right that it is the responsibility of the drone to now have conflicting param names between components? It seems to me that we save them all together, so if a camera has a param called MODE and the autopilot also has it, then they will conflict. Correct?

That's a good point I haven't thought about. Sounds like I ought to change it.

Edit: I had a look and the client is per target component ID, so it has separate caches and it should not conflict if I have that right.

  1. What happens when select_component is called? Does that impact the params already in the queue, or only the new ones? Say I select the autopilot, set a param (which puts it on the worker queue), then select the camera to set another param. Am I guaranteed that the first param will be sent to the autopilot?

Good point. I need to check again what I did but better would definitely be if it only affects the new ones. I'll see if I can change it.

Edit: yes, that's indeed how it works already. There are queues for all components in system_impl and when selecting the component, the next request will just go on another cache and queue instance.

@julianoes julianoes force-pushed the pr-split-mavlink-params branch 2 times, most recently from 7139352 to 3ced488 Compare May 20, 2023 22:59
JonasVautherin
JonasVautherin previously approved these changes May 21, 2023
julianoes and others added 7 commits May 22, 2023 12:19
This brings:

- Support to provide parameters in components.
- Get/set parameters for a specific component.
- Both with normal as well as extendended parameters.

Signed-off-by: Julian Oes <[email protected]>

Co-authored-by: consti10 <[email protected]>
Except for the camera discovery where we use the subscription directly.

Signed-off-by: Julian Oes <[email protected]>
@julianoes julianoes merged commit 5366e11 into main May 22, 2023
@julianoes julianoes deleted the pr-split-mavlink-params branch May 22, 2023 03:05
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.

3 participants