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

[ros2topic] pub frequency lower then expect #140

Closed
yechun1 opened this issue Aug 28, 2018 · 5 comments
Closed

[ros2topic] pub frequency lower then expect #140

yechun1 opened this issue Aug 28, 2018 · 5 comments
Labels
bug Something isn't working

Comments

@yechun1
Copy link
Contributor

yechun1 commented Aug 28, 2018

publish msg period as 1 minutes, but actually the result is lower then expect.

$ ros2 topic pub /image sensor_msgs/Image -r 10 -p 10
actually period: 1.004s.

$ ros2 topic pub /image sensor_msgs/Image -r 100 -p 100
actually period: 1.040s.

$ ros2 topic pub /image sensor_msgs/Image -r 1000 -p 1000
actually period: 1.387s.

reference debug code:

diff --git a/ros2topic/ros2topic/verb/pub.py b/ros2topic/ros2topic/verb/pub.py
index e07bdeb..431877d 100644
--- a/ros2topic/ros2topic/verb/pub.py
+++ b/ros2topic/ros2topic/verb/pub.py
@@ -101,9 +101,13 @@ def publisher(

     print('publisher: beginning loop')
     count = 0
+    prev = time.time()
     while rclpy.ok():
         count += 1
         if print_nth and count % print_nth == 0:
+            interval = time.time() - prev
+            print('interval:%s' %interval)
+            prev = time.time()
@dirk-thomas
Copy link
Member

dirk-thomas commented Aug 28, 2018

In the current code in the loop it sleeps for the duration specified by the rate argument:

time.sleep(period)

So any time spend in the actual publish call will add to the time of one cycle and therefore reduce the actual rate. I would suggest to change the main loop to use a time so that the publish time doesn't affect the rate (as long as publishing doesn't take longer than the rate). Please consider to provide a pull request for this.

The secondary issue is that the publish call does take that long for large messages in Python but I think that is already covered by other tickets (e.g. ros2/common_interfaces#55).

@dirk-thomas dirk-thomas added the enhancement New feature or request label Aug 28, 2018
@yechun1
Copy link
Contributor Author

yechun1 commented Aug 29, 2018

PR #141 use timer to fix rate loss of time.sleep()

@dirk-thomas dirk-thomas added bug Something isn't working and removed enhancement New feature or request labels Aug 29, 2018
@dirk-thomas
Copy link
Member

The first problem of the loop accumulating delays in each cycle is been addressed by #141 and ros2/rcl#291.

The slow performance of the publish() invocation is still pending.

@yechun1
Copy link
Contributor Author

yechun1 commented Sep 6, 2018

I have investigated publish issue, here is the status updates:

  1. When only pub topic with sub. the rate is normal for example 1000hz.
  2. When pub topic with sub(patch Enable ros2tpoic hz command #133), sometimes the rate is keep normal.(1000hz),
    sometimes, the publish rate will continually decrease. for example 1000hz -> 800hz -> 600hz ... (cpu > 100%)

the reproducing probability is about 50%. see the cpu occupy increased, not sure if there is memory leak on low level functions called by publish. I try to use valgrind, the log has memory issue of rmw and rcl but trace only to .so, not code line. I don't find the way to build ros2 with -g for valgrind analytics.

@yechun1
Copy link
Contributor Author

yechun1 commented Sep 7, 2018

I have sync-up the latest ros2.repo code (include rcl updates), have not reproduced the publish rate decrease issue.

On test:
the publish(msg) cost about 0.06s on every 1s print, using timer could keep the expected pub rate (with merged patches: #141 and ros2/rcl#291) .
$ ros2 topic pub /image sensor_msgs/Image -r 1000 -p 1000
actually period: 0.999994s.

the issue should be fixed.

@yechun1 yechun1 closed this as completed Sep 7, 2018
esteve pushed a commit to esteve/ros2cli that referenced this issue Dec 16, 2022
esteve pushed a commit to esteve/ros2cli that referenced this issue Dec 16, 2022
…os2#140)

This is a follow-up to ros2/launch_ros#122.

If 'node_executable' is used then a deprecation warning is issued.
If 'node_executable' is used at the same time as 'executable', then a
runtime error is raised. This is similar behaviour to the other
parameters deprecated in ros2/launch_ros#122.

Signed-off-by: Jacob Perron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants