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

[TricycleController] Removed “publish period” functionality ⏱ #abi-break #behavior-break #468

Conversation

Robotgir
Copy link
Contributor

@Robotgir Robotgir commented Nov 17, 2022

Per discussion in Control WG meeting from Nov. 16th 2022

@destogl destogl force-pushed the tricycle_controller-removing-state_publish_period branch from 4c938e9 to d103ada Compare November 17, 2022 19:05
@destogl
Copy link
Member

destogl commented Nov 17, 2022

Can you please check the documentation of the tricycle controller (if any in doc folder) and remove the parameter from there? Also, I guess that the formatting will fail. Please setup pre-commit in your local repository.

@destogl destogl changed the title removed publish_period, publish_rate previous_publish_timestamp_ [TricycleController] Removed “publish period” functionality ⏱ Dec 3, 2022
@destogl destogl force-pushed the tricycle_controller-removing-state_publish_period branch from d103ada to 589c3f0 Compare December 3, 2022 14:04
@destogl destogl changed the title [TricycleController] Removed “publish period” functionality ⏱ [TricycleController] Removed “publish period” functionality ⏱ #abi-break #behavior-break Dec 3, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #468 (380c2dc) into master (e7f9962) will decrease coverage by 6.07%.
The diff coverage is 20.15%.

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
- Coverage   35.78%   29.70%   -6.08%     
==========================================
  Files         189        7     -182     
  Lines       17570      744   -16826     
  Branches    11592      429   -11163     
==========================================
- Hits         6287      221    -6066     
+ Misses        994      163     -831     
+ Partials    10289      360    -9929     
Flag Coverage Δ
unittests 29.70% <20.15%> (-6.08%) ⬇️

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

Impacted Files Coverage Δ
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <ø> (ø)
...ontroller/test/test_load_diff_drive_controller.cpp 12.50% <0.00%> (ø)
diff_drive_controller/src/odometry.cpp 42.16% <11.11%> (ø)
...ive_controller/test/test_diff_drive_controller.cpp 17.62% <12.08%> (ø)
diff_drive_controller/src/speed_limiter.cpp 46.55% <13.33%> (ø)
...troller/include/diff_drive_controller/odometry.hpp 20.00% <20.00%> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 32.04% <24.01%> (ø)
...ntroller/test/test_trajectory_controller_utils.hpp
..._state_broadcaster/src/joint_state_broadcaster.cpp
...ectory_controller/test/test_trajectory_actions.cpp
... and 192 more

@destogl destogl enabled auto-merge (squash) December 3, 2022 19:43
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.

Only flaky tests are breaking.

@destogl destogl merged commit 4c0d58e into ros-controls:master Dec 4, 2022
@tonynajjar
Copy link
Contributor

tonynajjar commented Dec 6, 2022

Per discussion in Control WG meeting from Nov. 16th 2022

I can't find any comments on that topic in the minutes of that meeting, could someone quickly explain why this change is needed?

@destogl destogl deleted the tricycle_controller-removing-state_publish_period branch December 6, 2022 14:12
@destogl
Copy link
Member

destogl commented Dec 6, 2022

I can't find any comments on that topic in the minutes of that meeting, could someone quickly explain why this change is needed?

Here is the discussion on this topic on JTC: https://docs.google.com/document/d/1818AoYucI2z82awL_-8sAA5pMCV_g_wXCJiM6SQmhSQ/edit?disco=AAAAlmxhM3k

I also wrote the decision just now into the comments to make it clear (@bmagyar can you please confirm?)

The reason is that we had issues on the test with this functionality, and is repeatable across the controller. So, the idea would be to make this somehow reusable in helpers.hpp header or somewhere. But then actually people at the meeting realized that the functionality is rather confusing and not actually necessary since there is a data loss on the topic which is not what one usually needs/wants. Therefore, we decided to make this logic simpler, reduce the code and remove features which value is not clear.

@bmagyar
Copy link
Member

bmagyar commented Dec 6, 2022

#473

@tonynajjar
Copy link
Contributor

Thanks for the references, I commented my follow up question in #473

tonynajjar added a commit to tonynajjar/ros2_controllers that referenced this pull request Feb 20, 2023
tonynajjar pushed a commit to tonynajjar/gazebo_ros2_control that referenced this pull request Feb 20, 2023
bmagyar pushed a commit that referenced this pull request Feb 20, 2023
bmagyar pushed a commit to ros-controls/gazebo_ros2_control that referenced this pull request May 16, 2023
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