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

GCS_MAVLink: Support requesting MESSAGE_INTERVAL via REQUEST_MESSAGE cmd #28353

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nexton-winjeel
Copy link
Contributor

Updates the MAV_CMD_REQUEST_MESSAGE handler with a special case for requesting the MESSAGE_INTERVAL message. The old method of using MAV_CMD_GET_MESSAGE_INTERVAL is deprecated.

Raised in this Discord message: https://discord.com/channels/674039678562861068/728017546313466047/1293342302261219329

@nexton-winjeel
Copy link
Contributor Author

FYI: @peterbarker

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Please create an autotest for this.

This won't be the final shape of this functionality - it should be inverted, with handle_command_get_message_interval implemented in terms of handle_command_request_message and eventually going away.

The method you're modifying will need to be a lot more involved to support the "instance" parameter generally - but it will be nice to be able to request a specific battery instance, for example.

... and per-instance message rates are going to be "interesting".

I'm happy with this implementation, it just needs a regression test - which is going to be maybe 3 lines in rover.py or something.

@nexton-winjeel
Copy link
Contributor Author

Please create an autotest for this.

Added an autotest to Rover.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Test is good.

Just needs to pass Python cleanliness. You can run flake8 on the command-line against the file. I run it as part of my git commit hooks to avoid pushing stuff which doesn't work. And recently using fly-check.

@nexton-winjeel nexton-winjeel force-pushed the upstream/support_MESSAGE_INTERVAL_via_REQUEST_MESSAGE branch from 026474a to 0761215 Compare October 9, 2024 09:31
@nexton-winjeel nexton-winjeel force-pushed the upstream/support_MESSAGE_INTERVAL_via_REQUEST_MESSAGE branch from 0761215 to 2f0c5f4 Compare October 20, 2024 22:20
@nexton-winjeel
Copy link
Contributor Author

@peterbarker: Can I get you to review this again?

@@ -3195,6 +3195,12 @@ uint8_t GCS::get_channel_from_port_number(uint8_t port_num)
MAV_RESULT GCS_MAVLINK::handle_command_request_message(const mavlink_command_int_t &packet)
{
const uint32_t mavlink_id = (uint32_t)packet.param1;

if (mavlink_id == MAVLINK_MSG_ID_MESSAGE_INTERVAL) {
const mavlink_command_int_t msg_interval_cmd = { packet.param2, };
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const mavlink_command_int_t msg_interval_cmd = { packet.param2, };
const mavlink_command_int_t msg_interval_cmd { command : packet.param2, };

Sorry, but I don't think we should rely on the magic packing stuff that mavlink does in its messages to do this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@nexton-winjeel nexton-winjeel force-pushed the upstream/support_MESSAGE_INTERVAL_via_REQUEST_MESSAGE branch from 2f0c5f4 to 034d94f Compare November 13, 2024 03:19
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