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

Preallocate JTC variables to avoid resizing in realtime loops #340

Merged
merged 9 commits into from
Jul 6, 2022

Conversation

AndyZe
Copy link
Contributor

@AndyZe AndyZe commented Apr 26, 2022

  • 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 force-pushed the andyz/preallocate_jtc_variables branch from 672acbc to 4bd432d Compare May 2, 2022 20:21
Copy link
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!

@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
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
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
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
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
Contributor Author

AndyZe commented Jun 8, 2022

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
Copy link
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.

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
Member

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
Contributor Author

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
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 StoglRobotics-forks/ros2_controllers that referenced this pull request Aug 26, 2022
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.

5 participants