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

Arm authorization #2267

Merged
merged 2 commits into from
Apr 6, 2024
Merged

Arm authorization #2267

merged 2 commits into from
Apr 6, 2024

Conversation

ddatsko
Copy link
Contributor

@ddatsko ddatsko commented Apr 1, 2024

This PR adds the implementation of arm authorization server functionality, and is an extension of this PR in MAVSDK-Proto. It implements a server that allows user to register a custom callback on arm authorization request.

It also adds an example of plugins usage, which was tested with a PX4 autopilot and it's implementation of arm authorization functionality

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 a lot for the contribution! This looks very clean and well done!

One thing I don't fully understand is the system_id passed to the subscription.

I assume the arm_authorizer_server is meant to filter for the correct system ID that it is responsible for?

@ddatsko
Copy link
Contributor Author

ddatsko commented Apr 2, 2024

Thanks for the comment! Regarding the system_id, I was following the logic documented in MavLink MAV_CMD_ARM_AUTHORIZATION_REQUEST. As I understand, arm_authorizer_server can be a separate instance, responsible for arm authorization for multiple systems with different ids. So the user callback can make a decision based on the system_id and its knowledge about that system (e.g. weather conditions are suitable for one system, but not for another)

@julianoes
Copy link
Collaborator

I think you're right in terms of system id. A component server doesn't have a notion of "connected system" like a normal system plugin does, by design, so it could potentially decide for any system, and this should indeed work as you describe.

Fixed spelling for arm_authorizer_server

Changed component type in arm_authorizer_server example

Changes to comply with updates in proto repository

Deleted forgotten file

proto: update submodule now that PR is merged

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

update submodule again

Signed-off-by: Julian Oes <[email protected]>
@julianoes julianoes force-pushed the arm_authorization branch 3 times, most recently from 37892ce to c80b212 Compare April 6, 2024 18:04
@julianoes
Copy link
Collaborator

julianoes commented Apr 6, 2024

@JonasVautherin I'm puzzled why this is still broken.

@JonasVautherin
Copy link
Collaborator

Oh, I think the subscribe response is wrong?

See e.g. here:

rpc SubscribePosition(SubscribePositionRequest) returns(stream PositionResponse) {}

vs

rpc SubscribeArmAuthorization(SubscribeArmAuthorizationRequest) returns(stream SubscribeArmAuthorizationResponse) { option (mavsdk.options.async_type) = ASYNC; }

I guess it should be

rpc SubscribeArmAuthorization(SubscribeArmAuthorizationRequest) returns(stream ArmAuthorizationResponse) { option (mavsdk.options.async_type) = ASYNC; }

i.e. SubscribeArmAuthorizationResponse -> ArmAuthorizationResponse 🤔

@julianoes
Copy link
Collaborator

julianoes commented Apr 6, 2024

Ah thanks. I'll check that. However, why does it not show up when I build it locally?

Oh, it does, after a clean build.

@julianoes julianoes force-pushed the arm_authorization branch 2 times, most recently from 26eddfa to 6592706 Compare April 6, 2024 21:27
Signed-off-by: Julian Oes <[email protected]>
@julianoes julianoes merged commit 6113b03 into mavlink:main Apr 6, 2024
27 checks passed
Copy link

sonarcloud bot commented Apr 6, 2024

Please retry analysis of this Pull-Request directly on SonarCloud

ddatsko added a commit to ddatsko/MAVSDK that referenced this pull request Aug 6, 2024
* Added arm authorization server functionality
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.

3 participants