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

Fix various issues in TriggerRate #292

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

rhaschke
Copy link
Contributor

In order to actually reach the desired rate, the inequality needs to be replaced with greater equal.
Here is an example with publish_rate = 50 and Gazebo simulation (i.e. perfect sim time)

Before: rate = 47.619 Hz = 1/21ms

rostopic hz /franka_state_controller/joint_states
subscribed to [/franka_state_controller/joint_states]
WARNING: may be using simulated time
average rate: 47.619
        min: 0.019s max: 0.024s std dev: 0.00086s window: 143

After: rate = 50 Hz = 1/20ms (as expected)

average rate: 50.000
        min: 0.018s max: 0.022s std dev: 0.00063s window: 52

In order to actually reach the desired rate, the inequality needs to be replaced with greater equal.
Converting ros::Time toSec(), a double, introduces rounding errors
and additional computational effort.
When ros::Time is reset, e.g. simulation was reset, we should trigger
publishing the state (instead of waiting for the new timeline catching up with the old one).
@rhaschke rhaschke changed the title Fix TriggerRate: trigger already when period is reached (==) Fix various issues in TriggerRate Oct 27, 2022
@rhaschke
Copy link
Contributor Author

I found some more issues with this class:

  • Using ros::Time() conversion to double can introduce rounding errors, causing sporadic misses in state publishing
  • When ros::Time was reset (e.g. in simulation), there were no state updates at all, because now - time_stamp_ is negative.

@rickstaa
Copy link
Contributor

rickstaa commented Sep 4, 2023

@rhaschke Excellent observations! Much appreciated! I applied them to my fork.

@Maverobot Unfortunately, I couldn't test this PR on the real robot, but they look good. It would be great if these could be merged as they will significantly improve the performance ⚡.

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.

2 participants