Microservice Versioning Proof of Concept #13565
Closed
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
5 tasks
|
@ItsTimmy Two points.
We didn't really resolve that - but clearly we need to identify things that are optional in some way and be able to query for support. |
That's your proposal but I don't see why we need to have two ways to do it. Yes, the original proposition needs one more message to be transmitted but it's clear and I don't understand why we should limit flexibility for an optimization that we don't know if we need it. |
I'm fine with either approach - they both make logical sense to me. But if I had to implement this I'd much rather do the second version. Anyway, the important thing is that Tim as implementer is convinced of the rightness of a particular approach. I don't mind so much what that is, as long as he tells me what was done, so I can make sure the docs match. |
julianoes
reviewed
Nov 26, 2019
src/modules/mavlink/mavlink_main.cpp
Outdated
| microservice_versions::service_status & | ||
| Mavlink::get_service_status(uint16_t service_id) | ||
| { | ||
| // TODO microservice versions: Maybe we don't have to search, and can just always place service ID 2 at index 2 |
There was a problem hiding this comment.
I would say that's easier.
| } | ||
| // if ((mavlink_mission_item->frame == MAV_FRAME_GLOBAL_INT || | ||
| // mavlink_mission_item->frame == MAV_FRAME_GLOBAL_RELATIVE_ALT_INT)) { | ||
| // int_mode() = true; |
There was a problem hiding this comment.
Here comes the catch: we somehow still need to be compatible to older ground stations which do not support micro services yet. So, we need some sort of "if no microservice version chosen, then try to guess if int or not".
| ****************************************************************************/ | ||
|
|
||
| /** | ||
| * @file mavlink_service_versions.c |
There was a problem hiding this comment.
Suggested change
| * @file mavlink_service_versions.c | |
| * @file mavlink_service_versions.cpp |
| @@ -0,0 +1,74 @@ | |||
| /**************************************************************************** | |||
| * | |||
| * Copyright (c) 2012-2019 PX4 Development Team. All rights reserved. | |||
There was a problem hiding this comment.
Suggested change
| * Copyright (c) 2012-2019 PX4 Development Team. All rights reserved. | |
| * Copyright (c) 2019 PX4 Development Team. All rights reserved. |
| { | ||
| .service_id = MAVLINK_SERVICE_ID_CAMERA, | ||
| .min_version = 3, | ||
| .max_version = 30 |
| /** | ||
| * This enum represents the current status of the microservice version handshake for one particular microservice. | ||
| */ | ||
| enum handshake_status { |
There was a problem hiding this comment.
Suggested change
| enum handshake_status { | |
| enum class HandshakeStatus { |
|
@hamishwillee ok we're lacking the context here. I suggest we discuss it on Slack or in the Google Sheet. |
|
My current implementation is as follows:
I think this does not put any significant burden on the autopilot. I can of course test this more thoroughly, but right now it only requires ~3 bytes of RAM per service per MAVLink connection, and in exchange we get the flexibility to support multiple versions on PX4. For example, right now, PX4 effectively already supports 2 versions of the mission protocol (as I have implemented it). As services are updated to newer versions, we can make the decision on a case-by-case basis whether to drop support for older versions of a service on PX4, if flash space or computational load demands it. I have also implemented the 'stream all services' functionality (QGC sends |
added 8 commits
February 6, 2020 16:33
AmeliaEScott
force-pushed
the
pr-microservice-versioning
branch
from
0556993
to
a2feda5
Compare
AmeliaEScott
changed the title
|
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
|
We can re-open this if we decide to make another effort for it. |
|
Here is the diff so we don't lose it: |
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.
Related:
This PR uses this common.xml, and this C library generated from it.
This is currently a minimum functioning prototype, meant to demonstrate the possible functionality of microservice versioning. If this is to be merged, there will likely be some necessary changes in response to community feedback.