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

Autopilot server and command server fixes #2423

Merged
merged 3 commits into from
Oct 13, 2024
Merged

Conversation

julianoes
Copy link
Collaborator

@julianoes julianoes commented Oct 13, 2024

See commits.

The command fix gets rid of these confusing warnings:

[02:27:01|Debug] Received ack from 1/1 for not-existing command: 512! Ignoring... (mavlink_command_sender.cpp:285)

Finally...

- Remove parameters that are now unused.
- Clean up thread properly.
- Use udpout syntax.
It turns out we respond with two acks to a MAV_REQUEST_MESSAGE when we
should only respond once with the successful one.

Alternatively, we could first grab all the results and then do some
magic to determine which one is the best ack but that's a bit too magic
and complex, so I'm not doing that yet.
Copy link

sonarcloud bot commented Oct 13, 2024

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.

LGTM!

Comment on lines +101 to +104
// Let's hope someone else answers and keep silent. In an ideal world we would
// explicitly deny all the ones that we ought to answer but haven't implemented
// yet.
return std::optional<mavlink_command_ack_t>{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the issue that multiple components (e.g. autopilot_server or telemetry_server) may process the request, and they can't send MAV_RESULT_DENIED because they don't know if another component will answer? Or do I misunderstand it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly!

@julianoes julianoes merged commit ea06648 into main Oct 13, 2024
36 checks passed
@julianoes julianoes deleted the pr-autopilot-server-fixes branch October 13, 2024 18:41
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.

2 participants