Look into ramifications of increasing server_timeout in BT

[SteveMacenski]

CC ros2/ros2#1074

See if we can increase this using async BT nodes without slowing the tick rate, or if we need to lower the tick rate proportionally to the time we can allot to this.

18ms is the high number we've seen

[SteveMacenski]

@KavenYau this is somewhat related to your other ticket. If you wanted to look into this, increasing this timeout will fix the send_goal failure mode. We just don't yet understand the ramifications of increasing this. The BT ticks at 100hz (so the 10ms number we have set) so if this takes longer, we need to profile that this is OK, make it OK, or slow down the BT ticking rate so that it is OK.

I'd be more than happy to seed this to you if you had immediate need to fix it. It might take me a couple of weeks to get to this.

[SteveMacenski]

This timeout should also be a parameter, right now its hardcoded at 10ms. This can be done if we also do the RUNNING state work as discussed in the working group meeting @naiveHobo if that is going to be part of your fix 😄

[SteveMacenski]

@naiveHobo did you make any progress on this?

[KavenYau]

I increased server_timeout to 3000ms for temporarily fixing the issue. KavenYau@87e43c4

With using some low performance mechine or high CPU load cases, I guess the number would be higher than 18ms, it's stochastic. This is probably a communication performance issue.

In my opinions, although the timeout is a parameter, it's difficult for me to determine a perfect timeout to set, maybe it's not crucial. I mean slowing down the BT ticking rate could only reduce the number of timeout occurring,

How about trying to reduce the timeout impact on BT running? or adding threshold of timeout times to BT running?

[SteveMacenski]

This is probably a communication performance issue.

Yeah, we are aware of some regression in rclcpp causing this suddenly to be an issue. I don't think its necessarily crucial to know the exact right number, but the main question of this ticket is the ramifications that increasing that timeout has on the behavior tree system. The tree is being ticked at 100hz, so if this timeout is > 10ms, how does the BT action node respond? Should we be having 10ms-increments spinning until future is complete and then yielding the BT status as RUNNING to then spin again next tick? Can we just naively increase that timeout without any other action, does that slow the BT tick rate in those iterations or have other consequences? Should we also lower than BT tick rate? Is there another better solution?

I think we all agree this number needs to get pushed back up -- or the rclcpp regression needs to be identified and patched. I'd really like a solution to this before the galactic release so that this isn't a problem anymore since this is relatively serious for autonomous systems to miss these goals from networking timeouts.

[naiveHobo]

Can we just naively increase that timeout without any other action, does that slow the BT tick rate in those iterations or have other consequences?

Currently, increasing the server timeout only slows down the BT tick rate in those particular iterations when a BT action node sends a goal to an action server and waits for the response. This does not have any consequences down the line. For the iteration which took longer than the specified BT execution period, rclcpp::WallRate (that is used to maintain the tick rate for BT execution) does not sleep and returns false while also resetting its internal state to ensure that the succeeding iterations run at the specified rate.

[SteveMacenski]

But what happens if the duration its waiting for the goal request is larger than the rate duration? (e.g. we iterate the tree at 100hz, what if it takes 100ms to return a goal) In that case, the tick rate of the tree would be lowered and miss a few regular 10ms ticks. Within the current implementation, that isn't a huge deal, but there may be other BT nodes in the future that rely on more regular timing.

I also think that this is kind of the purpose of the RUNNING state to be able to have longer-running things be revisited in future ticks. I think if an easy mechanism to resolve the timing issue (RUNNING) didn't exist, I'd agree with you just overflowing the timing for 1 tick out of 10,000 wouldn't be a big deal. Wouldn't it be more "general" if we had it yield and spin again in a future tick up to the timeout duration rather than blocking the entire tree?

[SteveMacenski]

Merging imminent, closing, thanks @naiveHobo !!

[SteveMacenski]

And... that's it! I think that's the full V1 ticket list! I'll need to go around and see if anything else popped up we should add before releasing a V1, but this seems like it !!