haltTree doesn't effectively halt tickWhileRunning

[tony-p]

haltTree resets the tree status to IDLE which then in the tickRoot only breaks out of the inner loop, which is then restarted by the outer loop

NodeStatus Tree::tickRoot(TickOption opt, std::chrono::milliseconds sleep_time)
{
  ...
  // Always run if IDLE
  while (status == NodeStatus::IDLE ||
         (opt == TickOption::WHILE_RUNNING && status == NodeStatus::RUNNING))
  {
   ...

    // Inner loop. The previous tick might have triggered the wake-up
    // in this case, unless TickOption::EXACTLY_ONCE, we tick again
    while( opt != TickOption::EXACTLY_ONCE &&
           status == NodeStatus::RUNNING &&
           wake_up_->waitFor(std::chrono::milliseconds(0)) )
    {
      status = rootNode()->executeTick();
      // Due to halt tree status is now IDLE, normal exiting operation would be SUCCESS or FAIL
    }
    ...
  }
  return status;
}

[tony-p]

It may make more sense to make haltTree set the tree status to FAILED as it has not ended successfully

[facontidavide]

I agree and, funny coincidence, I was thinking about that this morning. But of course, since tickWhileRunning is blocking, the only way you can call haltTree is in another thread.

But I should cover this case!

About returning FAILED... that is another problem, since FAILING and HALTING are two different concepts. I will address this in the next release. Fell free to suggest a PR if you have an idea

[tony-p]

Yes a new HALTED/HALTING status would be more appropriate

[facontidavide]

I see your point but... not gonna happen. A new status would mean rewriting ALL the control nodes and decorators

[tony-p]

Yeah I understand is a pretty major API change, and that is why I originally suggested FAILED instead, but I also get your point that it is not each node that failed, (and can imagine could induce nasty side effects), so that status is only really relevant for the root node