Server component and server plugin infrastructure #1733
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julianoes
force-pushed
the
pr-component-camera-server
branch
from
668ed0f to
13c7bf4
Compare
|
@dlech I have a simple take picture integration test working: |
|
Nice! |
julianoes
force-pushed
the
pr-component-camera-server
branch
from
9597656 to
c9c2197
Compare
julianoes
force-pushed
the
pr-component-camera-server
branch
3 times, most recently
from
e041252 to
4969dd3
Compare
julianoes
force-pushed
the
pr-component-camera-server
branch
from
32b2554 to
dfe7f3e
Compare
This was referenced Apr 22, 2022
Merged
Merged
julianoes
force-pushed
the
pr-component-camera-server
branch
from
8b5f646 to
011d227
Compare
|
For anyone reading along, we now have some system_tests which exercise plugins against the server plugins: |
julianoes
force-pushed
the
pr-component-camera-server
branch
from
bcecd20 to
6f9dcb6
Compare
dayjaby
reviewed
May 6, 2022
| .horizontal_sensor_size_mm = 3.68, | ||
| .vertical_sensor_size_mm = 2.76, | ||
| .horizontal_resolution_px = 3280, | ||
| .vertical_resolution_px = 2464, |
There was a problem hiding this comment.
Can we add the following here?
.lens_id = 0,
.definition_file_version = 0,
.definition_file_uri = "https://raw.githubusercontent.com/mavlink/mavlink-devguide/master/en/services/camera_definition_example.xml"
These fields are already part of the proto.
There was a problem hiding this comment.
I think we want to do it properly with a locally hosted file that can be queried using MAVLink FTP.
| all_camera_servers.insert({system->get_system_id(), camera_server}); | ||
|
|
||
| // First add all subscriptions. This defines the camera capabilities. | ||
|
|
There was a problem hiding this comment.
Would be great to see your new mavsdk/core/mavlink_parameters here in action, maybe by subscribing to the parameters listed in https://raw.githubusercontent.com/mavlink/mavlink-devguide/master/en/services/camera_definition_example.xml
There was a problem hiding this comment.
Yes, agreed, we should. I think the camera_server plugin and example will require quite a bit more work for settings to work properly.
|
Thanks for the review @dayjaby, that's great! We'll get to it next week, probably. |
example: - fix more than one new system in subscribe_on_new_system() callback - fix use after free - publish photo to all systems plugin: - fix take photo logic - add support for interval flag in capture status message - add support for image count in capture status message
This introduces a ServerComponent(Impl) class which can be used by server plugins instead of the System(Impl) class. This means that the server is really tied to the Mavsdk instance rather than a System's representation.
We can now take advantage of the server components.
We actually need a default component ID, so that heartbeats are sent out.
Hopefully this fixes CI.
Instead of creating it in the desctructor, we create it right before sending out the first heartbeat.
We add the start_sending_heartbeats() call the thread queue to avoid deadlocks. What happens is that the mavlink receiver mutex is locked when a heartbeat arrives which then creates the server component to send out heartbeats. Creating the server component involves registering for mavlink messages which is what blocks.
I previously had commented out some code related to the autopilot version and heartbeat state messages. This should clean this up, however, it will need some testing.
julianoes
force-pushed
the
pr-component-camera-server
branch
from
1efd615 to
d722098
Compare
This was commented out during refactoring earlier.
This still needs a cleanup but should restore most functionality for now.
This moves the component information test from the integration tests over to system tests. However, it is currently disabled pending MAVLink FTP refactoring.
Not sure why this had been disabled.
julianoes
force-pushed
the
pr-component-camera-server
branch
from
f14183d to
a62c249
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I started some refactoring towards an infrastructure that better works for server plugins as mentioned in #1700.
The current state is just a rough try in that direction to see if it could be feasible.
FYI @dlech @dayjaby