Fix invalid tick frequency in core library. #130
Conversation
Signed-off-by: David Oberacker <[email protected]>
Oberacda
added
bug
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #130 +/- ##
==========================================
+ Coverage 49.60% 49.62% +0.01%
==========================================
Files 59 59
Lines 7678 7675 -3
Branches 1647 1647
==========================================
Hits 3809 3809
+ Misses 3805 3802 -3
Partials 64 64 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I am still not sure if we want to calculate it this way, because the reported tick frequency can be faster than the set rate with this measurement (as we check after the tick is done, not after the rate.sleep is)
While this is great for benchmarking it could also confuse people in the future when they look at the tick rate and ask themselves (why is it this fast? I thought it was 10 Hz, why is the parameter not doing anything). Maybe we should publish both an tick_freq_theory and tick_freq_real to combat this
| @@ -371,8 +371,6 @@ def __init__( | |||
| if tick_frequency_hz == 0.0: | |||
| tick_frequency_hz = 10.0 | |||
|
|
|||
| self.tick_sliding_window = [tick_frequency_hz] * 10 | |||
There was a problem hiding this comment.
I like removing the sliding window stuff, that is something for the monitoring GUI to do if we even want it
| tick_start_timestamp = self.ros_node.get_clock().now() | ||
| while True: | ||
| tick_start_timestamp = self.ros_node.get_clock().now() |
There was a problem hiding this comment.
I am not sure if I understand this change? Why not get the new timestamp at the beginning of each iteration instead of before the loop and at the end of each iteration?
Signed-off-by: David Oberacker <[email protected]>
| tick_start_timestamp = self.ros_node.get_clock().now() | ||
| self.rate.sleep() |
There was a problem hiding this comment.
Does this make sense? We first start our new timer and then sleep the rest of the rate of the old one?
There was a problem hiding this comment.
I think we can also switch this around, but we need to include the rate time in the tick calculation. Indeed it might be more correct to the reporting after wakeing up from the rate.
There was a problem hiding this comment.
I would switch it around and start at the beginning of the loop and end after the rate
Related to #129
This PR should fix two issues: