Add support for topic QOS for ros2topic bw, delay and hz #935
Conversation
Signed-off-by: Anthony Welte <[email protected]>
TonyWelte
force-pushed
the
feature/expand-ros-topic-qos
branch
from
7f6a506
to
392e320
Compare
| @@ -253,3 +256,78 @@ def add_qos_arguments(parser: ArgumentParser, subscribe_or_publish: str, default | |||
| help=( | |||
| f'Quality of service liveliness lease duration setting to {subscribe_or_publish} ' | |||
| 'with (overrides liveliness lease duration value of --qos-profile option')) | |||
|
|
|||
| def extract_qos_arguments(args): | |||
There was a problem hiding this comment.
I do not understand why this is needed here. it does not do anything but passing all variables into another class object, can you elaborate this?
There was a problem hiding this comment.
It's definitely useless. I had a few alternatives in mind but none that I was satisfied with:
- just pass
argsto the_rostopic_verbfunctions as theqosparameter. But in that case I might as well remove the other arguments from_rostopic_verbsinceargshas all of them - split each qos argument into their own function parameters but that would have added 6 parameters to both
_rostopic_verbandchoose_qos
I'm not sure what would best fit the style of this package. What do you think ?
|
|
||
| return qos | ||
|
|
||
| def choose_qos(node, topic_name: str, qos_args): |
There was a problem hiding this comment.
i think moving this to ros2topic/api is nice to do here aligned with add_qos_arguments.
There was a problem hiding this comment.
besides that, i see choose_qos function is doing like adaptive qos configuration based on the concerned publisher information for durability and reliability to avoid incompatible QoS setting when it starts echo. I think this can be also applied to ros2 topic pub without this adaptive qos checking for the publisher? that does a bit more refactoring and cleaning for ros2topic qos functions.
There was a problem hiding this comment.
I'm not sure what you mean. From what I understand apart from the adaptive QoS configuration choose_qos just calls qos_profile_from_short_keys which ends up calling the same function profile_configure_short_keys that is called by pub ?
| 'topic', | ||
| 'topic_name', |
There was a problem hiding this comment.
Why do we need to change this? Is this related to what this PR is fixing? just checking.
There was a problem hiding this comment.
It's not needed but I initially implemented this feature by removing all parameters from _rostopic_verb and just passing it args and noticed that different verbs used different argument names so I harmonized them.
| '--window', '-w', type=positive_int, default=DEFAULT_WINDOW_SIZE, | ||
| '--window', '-w', dest='window_size', type=positive_int, default=DEFAULT_WINDOW_SIZE, |
There was a problem hiding this comment.
same here, is this required for this PR?
Signed-off-by: Anthony Welte <[email protected]>
|
I'm planning on adding tests once I have time and figure out how to run the tests without getting 13 failures after over 7 min of runtime |
|
I’ve explored adding tests for the QoS parameters but couldn’t find a clear starting point. Since the other verbs, Additionally, I made a small change to the existing tests to address an instability in |
Signed-off-by: Anthony Welte <[email protected]>
TonyWelte
force-pushed
the
feature/expand-ros-topic-qos
branch
from
3b31907
to
399ef65
Compare
This PR adds support for QoS configuration for the verbs: bw, delay and hz.
It moves the logic to decide on the QoS from
echointoros2topic.apito keep the same logic for the 4 verbs.I'm not really satisfied with my
extract_qos_argumentsfunction. It's a bit useless, it might be cleaner to just pass the full args to each_rostopic_verbfunctionResolves: #719