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

This adds MAVLink2 support and removes MAVLink 0.9 support #535

Merged
merged 81 commits into from May 16, 2016

Conversation

Projects
None yet
4 participants
@tridge
Member

tridge commented Apr 5, 2016

@tridge

This comment has been minimized.

Show comment
Hide comment
@tridge

tridge Apr 5, 2016

Member

See this PR for the matching patches in ArduPilot to support MAVLink2, signing and automatic protocol detection:
ArduPilot/ardupilot#3859

Member

tridge commented Apr 5, 2016

See this PR for the matching patches in ArduPilot to support MAVLink2, signing and automatic protocol detection:
ArduPilot/ardupilot#3859

@vooon

This comment has been minimized.

Show comment
Hide comment
@vooon

vooon Apr 6, 2016

Member

@tridge could you please add void * user_data for mavlink_accept_unsigned_t?

Maybe into mavlink_status_t?

Member

vooon commented Apr 6, 2016

@tridge could you please add void * user_data for mavlink_accept_unsigned_t?

Maybe into mavlink_status_t?

@tridge

This comment has been minimized.

Show comment
Hide comment
@tridge

tridge Apr 7, 2016

Member

@vooon, the callback already has the status pointer which can be used to match up to a data structure in the caller. A void * is both not type safe and also isn't enough to do C++ functors properly.
If there is a need for more context then I'd far rather add a field into mavlink_status_t. That is the only place the void* in the callback could come from anyway.
I'd want to understand what the need is for that void* is though.

Member

tridge commented Apr 7, 2016

@vooon, the callback already has the status pointer which can be used to match up to a data structure in the caller. A void * is both not type safe and also isn't enough to do C++ functors properly.
If there is a need for more context then I'd far rather add a field into mavlink_status_t. That is the only place the void* in the callback could come from anyway.
I'd want to understand what the need is for that void* is though.

@tridge

This comment has been minimized.

Show comment
Hide comment
@tridge

tridge Apr 7, 2016

Member

Please see this link for the detailed design of the signing protocol:
https://docs.google.com/document/d/1ETle6qQRcaNWAmpG2wz0oOpFKSF_bcTmYMQvtTGI8ns/edit?usp=sharing

Member

tridge commented Apr 7, 2016

Please see this link for the detailed design of the signing protocol:
https://docs.google.com/document/d/1ETle6qQRcaNWAmpG2wz0oOpFKSF_bcTmYMQvtTGI8ns/edit?usp=sharing

@vooon

This comment has been minimized.

Show comment
Hide comment
@vooon

vooon Apr 7, 2016

Member

I think that common way to hook C callback to C++ class is via wrapper:

extern "C" void callback_wrapper(void *user /* args... */) {
    auto p = dynamic_cast<MyClass *>(user);
    p->callback(/* args... */);
}
Member

vooon commented Apr 7, 2016

I think that common way to hook C callback to C++ class is via wrapper:

extern "C" void callback_wrapper(void *user /* args... */) {
    auto p = dynamic_cast<MyClass *>(user);
    p->callback(/* args... */);
}
@vooon

This comment has been minimized.

Show comment
Hide comment
@vooon

vooon Apr 7, 2016

Member

Oh, and obvious things: sha256 and sign looks not endianness safe; headless pack not implemented (w/o finalize), but mentioned.

Member

vooon commented Apr 7, 2016

Oh, and obvious things: sha256 and sign looks not endianness safe; headless pack not implemented (w/o finalize), but mentioned.

@tridge

This comment has been minimized.

Show comment
Hide comment
@tridge

tridge Apr 8, 2016

Member

@vooon, yes, its a common method, but not actually portable. To make it portable needs a bit more stuff (and more storage space than a single pointer).
The sha256 code could certainly be improved to make it more portable.
headerless pack is a bit tricky as the pack can now depend on the protocol options (ie. if you have any extensions in the packet). Do you have a specific use case in mind?

Member

tridge commented Apr 8, 2016

@vooon, yes, its a common method, but not actually portable. To make it portable needs a bit more stuff (and more storage space than a single pointer).
The sha256 code could certainly be improved to make it more portable.
headerless pack is a bit tricky as the pack can now depend on the protocol options (ie. if you have any extensions in the packet). Do you have a specific use case in mind?

@vooon

This comment has been minimized.

Show comment
Hide comment
@vooon

vooon Apr 8, 2016

Member

Do you have a specific use case in mind?

Not, just that may help with sequence numbers when multiple threads generate messages.
I.e. producer thread -msg-> queue -msg-> comm thread (finalize; byte buffer) -bytes-> comm channel.

Member

vooon commented Apr 8, 2016

Do you have a specific use case in mind?

Not, just that may help with sequence numbers when multiple threads generate messages.
I.e. producer thread -msg-> queue -msg-> comm thread (finalize; byte buffer) -bytes-> comm channel.

@tridge

This comment has been minimized.

Show comment
Hide comment
@tridge

tridge Apr 10, 2016

Member

I just wanted to note that in my testing of MAVLink2 the biggest issue I have found is problems with the support for MAVLink2 in the SiK radio firmware. I have a set of patches for the SiK firmware, but it isn't stable yet.

Member

tridge commented Apr 10, 2016

I just wanted to note that in my testing of MAVLink2 the biggest issue I have found is problems with the support for MAVLink2 in the SiK radio firmware. I have a set of patches for the SiK firmware, but it isn't stable yet.

@LorenzMeier

This comment has been minimized.

Show comment
Hide comment
@LorenzMeier

LorenzMeier May 13, 2016

Member

Can you rebase this on master? I'd like to get it in now.

Member

LorenzMeier commented May 13, 2016

Can you rebase this on master? I'd like to get it in now.

@tridge

This comment has been minimized.

Show comment
Hide comment
@tridge

tridge May 16, 2016

Member

I have rebased and added your commits, plus added a new set of commits to allow auto-generation of the information needed to do routing (ie. the target_system offset and target_component offset).
See this code for an example of how that can be used:
https://github.com/ArduPilot/ardupilot/blob/pr-mavlink2/libraries/GCS_MAVLink/MAVLink_routing.cpp#L305

Member

tridge commented May 16, 2016

I have rebased and added your commits, plus added a new set of commits to allow auto-generation of the information needed to do routing (ie. the target_system offset and target_component offset).
See this code for an example of how that can be used:
https://github.com/ArduPilot/ardupilot/blob/pr-mavlink2/libraries/GCS_MAVLink/MAVLink_routing.cpp#L305

@LorenzMeier LorenzMeier merged commit 057c3ab into mavlink:master May 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@LorenzMeier

This comment has been minimized.

Show comment
Hide comment
@LorenzMeier

LorenzMeier May 16, 2016

Member

Nice! I've merged this now to master. Now on to deploy this!

Member

LorenzMeier commented May 16, 2016

Nice! I've merged this now to master. Now on to deploy this!

@vooon vooon referenced this pull request May 16, 2016

Closed

Support MAVLink 2.0 #543

5 of 5 tasks complete
@magicrub

This comment has been minimized.

Show comment
Hide comment
@magicrub

magicrub May 16, 2016

Contributor

Woo hoo!
On May 16, 2016 1:29 AM, "Lorenz Meier" notifications@github.com wrote:

Nice! I've merged this now to master. Now on to deploy this!


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#535 (comment)

Contributor

magicrub commented May 16, 2016

Woo hoo!
On May 16, 2016 1:29 AM, "Lorenz Meier" notifications@github.com wrote:

Nice! I've merged this now to master. Now on to deploy this!


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#535 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment