[timer] Fix malfunction when reset cancelled timer

[DongheeYe]

Signed-off-by: Donghee Ye donghee.ye@samsung.com

Related to #1012

To reset wait_set after reset a cancelled timer, there is needed node handle or context handle. When constructing a timer, there are context handle parameter. To trigger guard condition to reset wait_set, save context handle in constructor and trigger when reset timer.

[ivanpauno]

@DongheeYe Thanks for opening the PR.
There seems to be a bunch of issues here:

• Methods of the Timer class should be thread safe, and they don't seem to be.
• Timers should trigger a guard condition when being reset/cancelled, but that should be done in rcl and not here. Probably, here and here. This same guard condition should be triggered. I'm not sure about if should be triggered when it's cancelled/reset or both.

[ivanpauno]

See above comment #1013 (comment).

[DongheeYe]

@ivanpauno Thank to comments. I'd considered to use the rcl timer guard condition, either. (here). However, I thought, the purpose of current rcl timer guard condition is to wake up of waitset when waiting timer is expired. In this case, cancelled timer guard condition is not added to the node waitset rcl_wait. So, additional guard condition is needed to wake up node waitset to add timer waitset again like. I will try to make a rcl patch that add additional guard condition to rcl timer and trigger when rcl_timer_reset.

[ivanpauno]

In this case, cancelled timer guard condition is not added to the node waitset rcl_wait

I think you can change the logic to make sure is always added, instead of adding a new guard condition.

[wjwwood]

Interrupting all wait set associated with the context is overkill. It should only interrupt the wait set/executor of which the timer is associated. Pub/sub accomplish this by being lazily removed when the executor notices that the publisher or subscription have gone out of scope (it stores them as weak pointers), but they don't have a "reset" like function.

I think the best thing to do would be to get the node's node_base_interface and trigger the guard condition there, like NodeTimers::add_timer() does:

rclcpp/rclcpp/src/rclcpp/node_interfaces/node_timers.cpp

Lines 42 to 46 in 96ebf59

	if (rcl_trigger_guard_condition(node_base_->get_notify_guard_condition()) != RCL_RET_OK) {
	throw std::runtime_error(
	std::string("Failed to notify wait set on timer creation: ") +
	rmw_get_error_string().str);
	}

You could add a method to the NodeTimersInterface that does this and call it from NodeTimers::add_timer() as well as TimerBase::reset().

[wjwwood]

See my other comment for how it should change.

[ivanpauno]

Closed in favor of ros2/rcl#589 (I added the reviewers here by error, the intention was to add them to the rcl PR).

[ivanpauno]

Methods of the Timer class should be thread safe, and they don't seem to be.

This comment is still valid. rcl functions aren't thread safe (they aren't intended to be), and rclcpp should provide thread safety on top of that. It would be good to add a mutex (a recursive mutex sounds suitable) to protect access to timer_handle_ member of Timer class.

Can you open a PR proposing that?