Add missing qos_profile argument to publisher and subscriber in migration guide #3858
Conversation
There was a problem hiding this comment.
i am not sure if we want to add this to migration guide.
i think these migration guide is meant to show some basic API mapping between ROS 1 and ROS 2. probably it would be simpler if we do not add any other configuration details such as QoS?
i would like to have other opinion. thanks.
source/The-ROS2-Project/Contributing/Migration-Guide-Python.rst
Outdated
Show resolved
Hide resolved
|
In ROS 1 the queue_size argument was optional, but in ROS 2 it is required. The examples given will not run without my changes. I added the optional queue_size argument on the ROS 1 side to show a more direct correspondence to the ROS 2 functions. I think the short description (quoted from the API definition) is useful to explain the difference between the two possible choices. |
this is good point. probably we should fix ROS 2 implementation as optional? i am not sure why this is required arguments for the API, it seems to me that we can use default topic configuration if not specified? @clalancette what do you think? |
|
Agreed that it should be optional in the API. |
We had a lot of discussion about that particular part of the ROS 2 API back near the beginning of development. The original proposal was actually to make the QoS object a required part of the API, but we ended up compromising so just the queue size is required, with the QoS object being optional. The idea is that we should have the user specify these things so that they will be required to think about them; there may be different things a user does in their callbacks when the queue size is 1 vs. 10, for instance. All of that said, this isn't the right place to discuss this. If you'd like to make a proposal for changing the API, we can certainly discuss it again. Please open a new issue on https://github.com/ros2/ros2 and we can talk further there. |
|
what #3858 (comment) said,
this stands out for now. |
|
|
||
| or |
There was a problem hiding this comment.
I am not sure if the following explanation should be added here since this doc simply explains migration mapping between ROS 1 and ROS 2. but this does not block the PR.
There was a problem hiding this comment.
Yeah, agreed. My thinking is that we aren't necessarily trying to teach ROS 2 here (that is done elsewhere), but just give straightforward advice on how to port. From that perspective, I think we should just use the version above, since that is the most common.
|
|
||
| or |
There was a problem hiding this comment.
Yeah, agreed. My thinking is that we aren't necessarily trying to teach ROS 2 here (that is done elsewhere), but just give straightforward advice on how to port. From that perspective, I think we should just use the version above, since that is the most common.
| or | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| sub = node.create_subscription(String, 'chatter', callback, rclpy.qos.QoSProfile()) | ||
|
|
||
| The fourth parameter is "A QoSProfile or a history depth to apply to the subscription. In the case that a history depth is provided, the QoS history is set to KEEP_LAST, the QoS history depth is set to the value of the parameter, and all other QoS settings are set to their default values." |
There was a problem hiding this comment.
Same comment here; I think we can just drop this whole thing.
|
How about this? I removed the description text but I still think it's useful to show both forms of the functions. |
clalancette
added
the
backport-all
(cherry picked from commit 5c8a652)
(cherry picked from commit 5c8a652)
(cherry picked from commit 5c8a652) Co-authored-by: Matthew Foran <[email protected]>
(cherry picked from commit 5c8a652) Co-authored-by: Matthew Foran <[email protected]>
No description provided.