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

remote_id: open drone id plugin interface #327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

persuader72
Copy link
Contributor

Proto file for the remote_id protocol.

This proto file is need by the new remote_id MAVSDK plugin.

The goal of the plugin to use the companion computer to work with open drone id compatible hardware like this even is the autopilot is not compatible with such protocol.

protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @persuader72. I'm appalled by the OpenDrone ID MAVLink messages, now that I look at them in detail but a well, too late now, I suppose.

However, we have to figure out which things we want to expose raw and what we can hide and make easier.

protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
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.

A few nitpicks 😇.

protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
protos/remote_id/remote_id.proto Outdated Show resolved Hide resolved
@persuader72
Copy link
Contributor Author

With the "ArmStatus" stream i think that the feature set is completed

Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

I think this looks persuading 😄

Comment on lines +165 to +173
enum VerAcc {
VER_ACC_UNKNOWN = 0; // The vertical accuracy is unknown.
VER_ACC_METER_150 = 1; // The vertical accuracy is smaller than 150 meter.
VER_ACC_METER_45 = 2; // The vertical accuracy is smaller than 45 meter.
VER_ACC_METER_25 = 3; // The vertical accuracy is smaller than 25 meter.
VER_ACC_METER_10 = 4; // The vertical accuracy is smaller than 10 meter.
VER_ACC_METER_3 = 5; // The vertical accuracy is smaller than 3 meter.
VER_ACC_METER_1 = 6; // The vertical accuracy is smaller than 1 meter.
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still puzzles my why they wouldn't just use a uint8 to signal meter accuracy... but anyway, I think I can live with that.

Copy link
Contributor Author

@persuader72 persuader72 Jan 20, 2024

Choose a reason for hiding this comment

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

I think the reasons are historic. In some radio broadcasting ID protocols (like AIS or ADSB) the accuracy is an enumerative type or is rounded to less than 8 bits to save space (then save bandwidth)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, fair enough.

@persuader72
Copy link
Contributor Author

@julianoes, It is better to rewrite the history of this PR in one single commit?

@julianoes
Copy link
Collaborator

@persuader72 Feel free to rewrite the history and force push. If you don't, then I'll just squash-merge it.

@julianoes
Copy link
Collaborator

Sorry, this disappeared off my radar. I will review again ASAP!

@josephm28
Copy link

@julianoes @persuader72 Is it possible to merge this?

@julianoes
Copy link
Collaborator

Sorry yes, I need to work through this. I have it planned as soon as v3 is done (see mavlink/MAVSDK#2316).

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.

4 participants