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

Reduce memory used by DDS client #26095

Merged
merged 3 commits into from Mar 12, 2024

Conversation

srmainwaring
Copy link
Contributor

@srmainwaring srmainwaring commented Jan 29, 2024

Remove unused TFMessage variable and reduce size of pre-allocate arrays in XRCE DDS message types.

Dependencies

Requires upstream change:

Which are applied to the ArduPilot forks in:

Tasks

  • Require a submodule update in this PR before merging.

Testing

Before

AP: DDS: ********** DDS message sizes **********
AP: DDS: ardupilot_msgs_srv_ArmMotors_Request:   1
AP: DDS: ardupilot_msgs_srv_ArmMotors_Response:  1
AP: DDS: builtin_interfaces_msg_Time:            8
AP: DDS: geographic_msgs_msg_GeoPoseStamped:     320
AP: DDS: geometry_msgs_msg_PoseStamped:          320
AP: DDS: geometry_msgs_msg_TwistStamped:         312
AP: DDS: sensor_msgs_msg_Joy:                    1072
AP: DDS: sensor_msgs_msg_NavSatFix:              376
AP: DDS: rosgraph_msgs_msg_Clock:                8
AP: DDS: tf2_msgs_msg_TFMessage:                 57608
AP: DDS: AP_DDS_Client:                          119768
AP: DDS: ********** DDS message sizes **********

After

AP: DDS: ********** DDS message sizes **********
AP: DDS: ardupilot_msgs_srv_ArmMotors_Request:   1
AP: DDS: ardupilot_msgs_srv_ArmMotors_Response:  1
AP: DDS: builtin_interfaces_msg_Time:            8
AP: DDS: geographic_msgs_msg_GeoPoseStamped:     320
AP: DDS: geometry_msgs_msg_PoseStamped:          320
AP: DDS: geometry_msgs_msg_TwistStamped:         312
AP: DDS: sensor_msgs_msg_BatteryState:           920
AP: DDS: sensor_msgs_msg_Joy:                    336
AP: DDS: sensor_msgs_msg_NavSatFix:              376
AP: DDS: rosgraph_msgs_msg_Clock:                8
AP: DDS: tf2_msgs_msg_TFMessage:                 4616
AP: DDS: AP_DDS_Client:                          9168
AP: DDS: ********** DDS message sizes **********

Run on SITL and on MatekH743 / plane.

Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

LGTM. Hopefully upstream merges the dependencies soon with our endorsement. Once that's done, this can go in.

@tridge
Copy link
Contributor

tridge commented Jan 30, 2024

Nice memory reduction!

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Feb 19, 2024

Can you squash the commits together? Each one looks good.

@srmainwaring
Copy link
Contributor Author

Can you squash the commits together? Each one looks good.

Could do, though I kept them separate on purpose to make it clear what was changing: 1. removal of unused variable, 2. change in upstream dep, and 3. an unrelated bug fix.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Feb 19, 2024

Can you squash the commits together? Each one looks good.

Could do, though I kept them separate on purpose to make it clear what was changing: 1. removal of unused variable, 2. change in upstream dep, and 3. an unrelated bug fix.
Alright, I'll request a merge in dev call as-is.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Feb 20, 2024

We agreed in dev call a final test on hardware would be a good idea before merging. The plan is this can be backported to 4.5 with the testing complete. I'll test on Pixhawk 6X with serial.

@srmainwaring
Copy link
Contributor Author

srmainwaring commented Feb 20, 2024

I'll test on Pixhawk 6X with serial.

Ok. I'm running on a MatekH743-WING using MatekH743-bdshot with DDS and PPP enabled and it's working well. Will be good to have coverage of other boards.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Mar 4, 2024

I think the testing Rhys has done is sufficient. Are we ok to merge this so it can be part of 4.5?

@tridge tridge merged commit dac291c into ArduPilot:master Mar 12, 2024
92 checks passed
@srmainwaring srmainwaring deleted the prs/pr-dds-reduce-mem-use branch March 13, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 4.5.0-beta3
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants