add timeout option for ros2param to find node.

[fujitatomoya]

address #799

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

[fujitatomoya]

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

[fujitatomoya]

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

[iuhilnehc-ynos]

Can we just use

ros2cli/ros2cli/ros2cli/helpers.py

Line 25 in 1e3f1a0

def wait_for(predicate, timeout, period=0.1):

instead of adding wait_for_node?
and then use it similar to

ros2cli/ros2cli/ros2cli/daemon/daemonize.py

Lines 141 to 147 in 1e3f1a0

	def daemon_ready():
	try:
	pickler.dump(None)
	return False
	except OSError:
	return True
	if not wait_for(daemon_ready, timeout):

.

Just a tiny minor suggestion.

[iuhilnehc-ynos]

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]

BTW:

ros2cli/ros2cli/ros2cli/helpers.py

Lines 40 to 44 in 1e3f1a0

	while not predicate():
	if time.time() > deadline:
	break
	time.sleep(period)
	return predicate()

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]

@iuhilnehc-ynos thanks for the suggested fix, all addressed in 776312d

[fujitatomoya]

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

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

• Linux-aarch64

[fujitatomoya]

@audrow @gbiggs friendly ping, requesting another review.

[destogl]

Thanks for following up on this!

I have just a small proposal to increase the default timeout. As you see in the tests, 1, second could be too low.

[audrow]

Looks good to me - thanks, @fujitatomoya!

[fujitatomoya]

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