Skip to content

Preallocate JTC variables to avoid resizing in realtime loops#340

Merged
destogl merged 9 commits intoros-controls:masterfrom
AndyZe:andyz/preallocate_jtc_variables
Jul 6, 2022
Merged

Preallocate JTC variables to avoid resizing in realtime loops#340
destogl merged 9 commits intoros-controls:masterfrom
AndyZe:andyz/preallocate_jtc_variables

Conversation

@AndyZe
Copy link
Copy Markdown
Contributor

@AndyZe AndyZe commented Apr 26, 2022 •
edited
Loading

  • Preallocate several variables
  • last_commanded_state_ was previously used for open-loop control. I think it's redundant with state_desired_ now, so delete it

@AndyZe AndyZe marked this pull request as draft April 26, 2022 14:43
@AndyZe AndyZe force-pushed the andyz/preallocate_jtc_variables branch from 1e0590e to dcb42a6 Compare April 26, 2022 14:56
@AndyZe AndyZe marked this pull request as ready for review April 26, 2022 14:59
@AndyZe AndyZe mentioned this pull request May 2, 2022
Merged
@AndyZe AndyZe force-pushed the andyz/preallocate_jtc_variables branch from 672acbc to 4bd432d Compare May 2, 2022 20:21
erickisos
erickisos approved these changes May 3, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@erickisos erickisos left a comment

Choose a reason for hiding this comment

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

LGTM!

mechwiz
mechwiz reviewed May 3, 2022
View reviewed changes
mechwiz
mechwiz reviewed May 3, 2022
View reviewed changes
@AndyZe AndyZe force-pushed the andyz/preallocate_jtc_variables branch 3 times, most recently from 7ca39b0 to d54e877 Compare May 5, 2022 18:25
@AndyZe
Copy link
Copy Markdown
Contributor Author

AndyZe commented May 8, 2022

I don't understand why quite a few unit tests are failing. Marking as draft until that's fixed.

@AndyZe AndyZe marked this pull request as draft May 8, 2022 12:41
@mechwiz
Copy link
Copy Markdown
Contributor

mechwiz commented May 8, 2022

I think #296 broke the JTC tests which is why many of them fail.
I think #334 is an attempt to fix the issue but it hasn't yet been resolved.

@AndyZe
Copy link
Copy Markdown
Contributor Author

AndyZe commented May 10, 2022

OK, it does look like this PR does not make the test failures worse. Re-marking it as ready for review.

@AndyZe AndyZe marked this pull request as ready for review May 10, 2022 14:00
@AndyZe AndyZe force-pushed the andyz/preallocate_jtc_variables branch from d54e877 to 5f17111 Compare June 1, 2022 03:19
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jun 8, 2022

This pull request is in conflict. Could you fix it @AndyZe?

@AndyZe AndyZe force-pushed the andyz/preallocate_jtc_variables branch 4 times, most recently from 8cc92fe to 2a6255c Compare June 8, 2022 17:20
@AndyZe
Copy link
Copy Markdown
Contributor Author

AndyZe commented Jun 8, 2022 •
edited
Loading

The only possibly-controversial thing I see is error handling in case dof_ isn't initialized, and that's just because I didn't see a better way to handle the error there. I could even delete it, if you'd rather.

  if (dof_ == 0)
  {
    fprintf(stderr, "During ros2_control interface configuration, degrees of freedom is not valid;"
      " it should be positive. Actual DOF is %zu\n", dof_);
    std::exit(EXIT_FAILURE);
  }

This could be turned into an assert() if you'd rather.

@AndyZe AndyZe force-pushed the andyz/preallocate_jtc_variables branch from 2a6255c to 77eef92 Compare June 17, 2022 16:54
destogl
destogl approved these changes Jul 5, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

I find this very good! Thanks! Just one small comment.

joint_trajectory_controller/src/joint_trajectory_controller.cpp Outdated
Comment on lines +830 to +833
resize_joint_trajectory_point(state_current_, dof_);
resize_joint_trajectory_point(state_desired_, dof_);
resize_joint_trajectory_point(state_error_, dof_);
resize_joint_trajectory_point(last_commanded_state_, dof_);
Copy link
Copy Markdown
Member

@destogl destogl Jul 5, 2022

Choose a reason for hiding this comment

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

can we resize those in on_configure? Since this should be, if possible, real-time capable method.

Copy link
Copy Markdown
Contributor Author

@AndyZe AndyZe Jul 6, 2022

Choose a reason for hiding this comment

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

That makes sense. Done and tested.

@AndyZe AndyZe force-pushed the andyz/preallocate_jtc_variables branch from 77eef92 to 001c703 Compare July 6, 2022 13:17
@destogl
Copy link
Copy Markdown
Member

destogl commented Jul 6, 2022

The failing CI things are OK. Merging this.

@destogl destogl merged commit fa1138e into ros-controls:master Jul 6, 2022
mamueluth pushed a commit to b-robotized-forks/ros2_controllers that referenced this pull request Aug 26, 2022
@mamueluth
Preallocate JTC variables to avoid resizing in realtime loops (ros-co…
879e922 
…ntrols#340)
@christophfroehlich christophfroehlich mentioned this pull request Oct 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@destogl destogl destogl approved these changes

@jackcenter jackcenter Awaiting requested review from jackcenter

@kasiceo kasiceo Awaiting requested review from kasiceo

@Serafadam Serafadam Awaiting requested review from Serafadam

@UsamaHamayun1 UsamaHamayun1 Awaiting requested review from UsamaHamayun1

@a10263790 a10263790 Awaiting requested review from a10263790

@bmagyar bmagyar Awaiting requested review from bmagyar

@jaron-l jaron-l Awaiting requested review from jaron-l

@TomoyaFujita2016 TomoyaFujita2016 Awaiting requested review from TomoyaFujita2016

@VX792 VX792 Awaiting requested review from VX792

+2 more reviewers

@mechwiz mechwiz mechwiz left review comments

@erickisos erickisos erickisos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AndyZe @mechwiz @destogl @bmagyar @erickisos