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

Changing to_chrono to use nanoseconds #507

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

danwahl
Copy link
Contributor

@danwahl danwahl commented Jan 21, 2023

Previously std::chrono::seconds, which rounds to 0 if action_monitor_period_ < 1

Previously std::chrono::seconds, which rounds to 0 if action_monitor_period_ < 1
@nbbrooks
Copy link
Contributor

nbbrooks commented Jan 21, 2023

A related bugfix with JTC also resets the timer pointer so you don't end up creating multiple ones. We should do the same here - it's a shame noone caught the pattern being used here in gripper controller.

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.

Thanks for this fix!

@codecov-commenter
Copy link

Codecov Report

Merging #507 (432ec83) into master (e7f9962) will decrease coverage by 3.31%.
The diff coverage is 26.60%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #507      +/-   ##
==========================================
- Coverage   35.78%   32.48%   -3.31%     
==========================================
  Files         189        7     -182     
  Lines       17570      665   -16905     
  Branches    11592      357   -11235     
==========================================
- Hits         6287      216    -6071     
+ Misses        994      157     -837     
+ Partials    10289      292    -9997     
Flag Coverage Δ
unittests 32.48% <26.60%> (-3.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ontroller/test/test_load_diff_drive_controller.cpp 12.50% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp 42.16% <11.11%> (ø)
diff_drive_controller/src/speed_limiter.cpp 46.55% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp 17.62% <12.08%> (ø)
...troller/include/diff_drive_controller/odometry.hpp 20.00% <20.00%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 39.22% <35.50%> (ø)
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <100.00%> (ø)
..._broadcaster/test/test_joint_state_broadcaster.cpp
...ontrollers/src/joint_group_position_controller.cpp
...ory_controller/src/joint_trajectory_controller.cpp
... and 191 more

@destogl destogl enabled auto-merge (squash) January 25, 2023 17:59
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks for the follow-up!

@bmagyar bmagyar disabled auto-merge January 26, 2023 10:54
@bmagyar bmagyar merged commit ec80aba into ros-controls:master Jan 26, 2023
@bmagyar
Copy link
Member

bmagyar commented Jan 26, 2023

@Mergifyio backport to humble

mergify bot pushed a commit that referenced this pull request Jan 26, 2023
* Changing to_chrono to use nanoseconds

Previously std::chrono::seconds, which rounds to 0 if action_monitor_period_ < 1

* Reset gripper action goal timer to match JTC impl

(cherry picked from commit ec80aba)
@mergify
Copy link
Contributor

mergify bot commented Jan 26, 2023

backport to humble

✅ Backports have been created

destogl pushed a commit that referenced this pull request Jan 26, 2023
* Changing to_chrono to use nanoseconds
* Reset gripper action goal timer to match JTC impl

(cherry picked from commit ec80aba)

Co-authored-by: Dan Wahl <drwahl@gmail.com>
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.

6 participants