ros2 / ros2cli

ROS 2 command line interface tools
Apache License 2.0
172 stars 161 forks source link

add timeout option for ros2param to find node. #802

Closed fujitatomoya closed 1 year ago

fujitatomoya commented 1 year ago

address https://github.com/ros2/ros2cli/issues/799

Signed-off-by: Tomoya Fujita Tomoya.Fujita@sony.com

fujitatomoya commented 1 year ago

@audrow @gbiggs what do you think? I will add timeout option to other ros2 param sub-command as well.

fujitatomoya commented 1 year ago

@iuhilnehc-ynos it would be helpful if you can review.

iuhilnehc-ynos commented 1 year ago

If the wait_for_node is a helper code that will be used in somewhere else, what about using wait_for in the wait_for_node?

def wait_for_node(node, node_name, include_hidden_nodes=False, timeout=1.0) -> bool:
    def check_node_names():
        node_names = get_node_names(
            node=node, include_hidden_nodes=include_hidden_nodes)
        return True if node_name in {n.full_name for n in node_names} else False

    return wait_for(check_node_names, timeout)
iuhilnehc-ynos commented 1 year ago

BTW:

https://github.com/ros2/ros2cli/blob/1e3f1a0cb2a7686c9a5d01dea0622149c3b8c4a6/ros2cli/ros2cli/helpers.py#L40-L44 should be updated, because the predicate() is unnecessary to be called twice under some situations. (It reminded me of the logic similar to https://github.com/gcc-mirror/gcc/blob/83ffe9cde7fe0b4deb0d1b54175fd9b19c38179c/libstdc%2B%2B-v3/include/std/condition_variable#L348-L351.)

    while not predicate():
        if time.time() > deadline:
            return predicate()
        time.sleep(period)
    return True
fujitatomoya commented 1 year ago

@iuhilnehc-ynos thanks for the suggested fix, all addressed in https://github.com/ros2/ros2cli/pull/802/commits/776312d2b57dfb9bd2843e24013e0740b45331d8

fujitatomoya commented 1 year ago

I believe that same option can be applied at lease ros2param sub command, since other sub commands have possibly same problem.

fujitatomoya commented 1 year ago

CI:

fujitatomoya commented 1 year ago
fujitatomoya commented 1 year ago

@audrow @gbiggs friendly ping, requesting another review.

fujitatomoya commented 1 year ago

@audrow thanks for the review, i will go ahead to merge this.