Add Python examples for tf2_ros #161
Conversation
sloretz
added
in progress
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
sloretz
force-pushed
the
examples_tf2_py
branch
from
dc59df9
to
7a7dd24
Compare
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
sloretz
added
in review
|
Looks like the examples work with the latest eloquent debs. This PR is ready for review. |
| @@ -0,0 +1,64 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
I think we typically eschew the shebang nowadays, correct? In that case, please remove it. (the same comment applies several other times below)
There was a problem hiding this comment.
Replaced shebang with copyright comment in 48ca46e
| @@ -0,0 +1,64 @@ | |||
| #!/usr/bin/env python3 | |||
|
|
|||
| import math | |||
There was a problem hiding this comment.
It doesn't seem like we are actually using the math library here. Please remove.
| except KeyboardInterrupt: | ||
| pass | ||
| rclpy.shutdown() | ||
|
|
There was a problem hiding this comment.
Nit: trailing whitespace.
| try: | ||
| # Block until the transform is available | ||
| when = rclpy.time.Time() | ||
| self._tf_buffer.lookup_transform( |
There was a problem hiding this comment.
When not using coroutines, what's our recommendations for doing lookups like this? Is it really to block in the callback? Obviously this works with a MultiThreadedExecutor, but it seems like blocking up any of the executor threads is problematic. I'm just wondering whether we should have this example at all, or if we should suggest something else?
There was a problem hiding this comment.
When not using coroutines, what's our recommendations for doing lookups like this? Is it really to block in the callback?
I think so. Maybe someone could start a dedicated thread to poll for the transform? It would be nice if there was an api that called a callback when a transform became ready.
I'm just wondering whether we should have this example at all, or if we should suggest something else?
The API exists and this calls out in docstrings and comments that the blocking behavior is a problem. However, I would always use coroutines instead of the blocking API. I think the example should exist since the comments could steer someone to use the coroutine API instead.
There was a problem hiding this comment.
I think so. Maybe someone could start a dedicated thread to poll for the transform? It would be nice if there was an api that called a callback when a transform became ready.
Oh yeah, good call.
So I'm OK with adding this example, as this is something people will want to do. But I'll request two things here:
- Open an issue to have a callback-based reply to
lookup_transform. - Update the comment right above this call, or in the class header (your choice) to be a little more strong and suggest to always use the coroutine way instead.
There was a problem hiding this comment.
It looks like it should be possible to avoid blocking without coroutines, but I encountered an issues ros2/rclpy#460, but that should be fixed by ros2/rclpy#461. I added a second example in 9c74ebc, but it could be made simpler if the result included the transform, so I opened #194 too.
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
|
Ready for 2nd round of review |
| try: | ||
| # Block until the transform is available | ||
| when = rclpy.time.Time() | ||
| self._tf_buffer.lookup_transform( |
There was a problem hiding this comment.
I think so. Maybe someone could start a dedicated thread to poll for the transform? It would be nice if there was an api that called a callback when a transform became ready.
Oh yeah, good call.
So I'm OK with adding this example, as this is something people will want to do. But I'll request two things here:
- Open an issue to have a callback-based reply to
lookup_transform. - Update the comment right above this call, or in the class header (your choice) to be a little more strong and suggest to always use the coroutine way instead.
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
| self._lock = threading.Lock() | ||
| self._tf_future = None | ||
| self._from_frame = None | ||
| self._to_frame = None | ||
| self._when = None |
There was a problem hiding this comment.
Hm, I think there's a race condition here at startup time. I think it could be the case that on_timer fires before we initialize the lock and set these variables to None, in which case bad things may happen. I'll suggest moving this initialization near the top of the constructor.
There was a problem hiding this comment.
I think it's ok. The timer callback can't be called until the node is added to an executor, and that can't happen until after __init__() is finished.
There was a problem hiding this comment.
Oh, you are totally right. Sorry about that, will approve.
|
CI (only testing examples_tf2_py)
|
Requires #99✔️Requires #160✔️Requires ros2/rclpy#412✔️This adds a package with examples on how to use the tf2 Python API.