Add QoS Profile override to CLI #347
Conversation
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
piraka9011
force-pushed
the
override-subscription-cli
branch
from
3cfefaa
to
a085ecb
Compare
rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
There was a problem hiding this comment.
This one's also looking a lot better.
The one open missing feature is providing enum values by name/string, rather than integer value. I think that's critical to the feature, we can't ask users to provide reliability: 2, those values aren't documented anywhere and are error prone.
rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
piraka9011
force-pushed
the
override-subscription-cli
branch
from
c7bd42c
to
7beb584
Compare
Signed-off-by: Anas Abou Allaban <[email protected]>
piraka9011
force-pushed
the
override-subscription-cli
branch
from
752aa32
to
019e9dc
Compare
| 'reliability': rclpy.qos.QoSReliabilityPolicy.get_from_short_key, | ||
| 'durability': rclpy.qos.QoSDurabilityPolicy.get_from_short_key, | ||
| 'liveliness': rclpy.qos.QoSLivelinessPolicy.get_from_short_key | ||
| } |
There was a problem hiding this comment.
How do we make sure those two will stay in-sync? Can't we just get the user to pass the short key?
There was a problem hiding this comment.
We can have the user specify in the override yaml file the enum integer for each policy (ex. reliability: 0 which is the same as reliability: system_default) but that's not good UX.
I don't think there's a 'better' way to get these values beside using these helper methods provided by each policy.
Maybe using importlib to import all modules with the name *Policy in rclpy.qos but that's hacky IMO.
There was a problem hiding this comment.
I see, we should at least add a comment explaining this needs to be updated when rclpy.qos is updated.
Consider also using a frozenset() here as you don't really need a map to store x => x.foo for multiple values of x, unless you plan to have elements that would break this pattern, but it's probably YAGNI.
rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp
Outdated
Show resolved
Hide resolved
| @@ -52,6 +78,7 @@ rosbag2_transport_record(PyObject * Py_UNUSED(self), PyObject * args, PyObject * | |||
| "max_cache_size", | |||
| "topics", | |||
| "include_hidden_topics", | |||
| "qos_profile_overrides", | |||
There was a problem hiding this comment.
Alphasorting these argument will introduce irrelevant changes that I'd prefer not to have as a part of this PR.
There was a problem hiding this comment.
Sure, sgtm. Please link the PR back here once it's done.
rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
Signed-off-by: Anas Abou Allaban <[email protected]>
|
@ros2/aws-oncall - please run this CI job
|
|
@emersonknapp Looks like the same issue as before here. Let me know when it's ready to merge (or feel free to do so). |
piraka9011
changed the title
--qos-profile-overridesCLI optionQoSProfileThis PR as it stands is non-functional.
However, it ties in to the PR's related to #125 by overriding the
qos_profilesintroduced in theRecordOptions.Part of #125
Part of ros-tooling/aws-roadmap#218
Signed-off-by: Anas Abou Allaban [email protected]