Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to handle spinning on the client node in BT nodes #1698

Closed
maxlein opened this issue May 6, 2020 · 11 comments
Closed

How to handle spinning on the client node in BT nodes #1698

maxlein opened this issue May 6, 2020 · 11 comments

Comments

@maxlein
Copy link
Contributor

maxlein commented May 6, 2020

I am wondering if there is a potential bug and how to handle topic subscriptions in behavior tree nodes.

I was working on my own nodes and cross checking with nav2 how it's done here.
And basically there is one client node created and written to the blackboard. This node gets spinned only when an action client is present with an active request.

For example the is_stuck_condition's odom subscription is updated only when the spin of one of the action client's gets called. Do I see this right?

And my solution would be calling spin_some on the node which I think is missing in is_stuck_condition. But then again, this could lead to problems if we have nodes in parallel which both spin.

Is there some design pattern / rule on how such situations should be handled?

@SteveMacenski
Copy link
Member

SteveMacenski commented May 6, 2020

You're also welcome to spin_some in other nodes as well. Spin some will spin until the queue is empty so you know you'll process your information if its available. However, you're also going to process any other nodes' stuff. If you are doing something that is extra sensitive, it may be best to create your own rclcpp node in your BT node so you have complete control.

Also remember that the nodes are ticked in order, so there arent going to be issues with multiple nodes spinning at once. The BT is single threaded and the node is single threaded. When we spin for futures in the action or service nodes, those are happening one at a time. Since none of the callbacks in the BT do any "work" (just setting flags and returning) I think its fine to use the BT provided blackboard node for spinning for similarly light weight callbacks. If you want to add a subscriber callback or other work in the background of the BT that is substantive, I would agree with you that a new node for that specific BT node would be good so you don't bog down the rest of the system. I think this all works out because of A) single threaded and node executes in order and B) all of the uses of the node are lightweight.

I think the rule of thumb would be to use the node in the blackboard for lightweight tasks but if you're going to do anything intense that would cause undue latency to the rest of the system, create a separate node.

Does that make sense / answer the question? I agree that could be valuable to document. Maybe we should consider adding a section to the website for notes / information / docs / tutorials for behavior tree designers like yourself. What do you think? I'd also appreciate your input as to what would be valuable to include there (and maybe willing to help write it with me and others?). You're one of the more active of the users that have actually explored the behavior trees so I think coming up with a list and knocking them out when we have free time to add to https://navigation.ros.org/ would be fabulous.

@maxlein
Copy link
Contributor Author

maxlein commented May 7, 2020

The BT is single threaded and the node is single threaded.

Ok, I somehow thought there was an AsyncActionNode used somewhere.

I think there are some good quotes in your comment to put in the docs.
Like nav stack is single threaded all over, so don‘t use async action nodes but use coro action nodes instead if you need „parallelism“.

But most of this stuff has not that much to do with the nav stack but more with ROS itself and how to use ROS nodes in BT nodes.

For example when to read directly from the blackboard and when to use In/Output port mapping. If you have a single tree its not that of a problem but if you have subtrees also, you need to map between blackboards in the xml. And if some blackboard elements are hidden, you forget about them when changing the xml or use a gui. So I think it‘s a bit against the modularity of BT‘s.

Also I am not sure if I understood the ideas behind the architecture completely..

Hope this is understandable as you may have noticed it‘s not my mother tongue 😉

@SteveMacenski
Copy link
Member

So keep in mind that I did not write the BT code. From another paper I was writing about this work, I had to learn a bunch of the theory behind BTs and then looked over our implementation, I myself wasn't involved in the majority of the BT work while it was happening.

But from my view of the theory:

when to read directly from the blackboard

When you have some information that each node wants to share with another. If the planner updates a path, the controller gets the path from the blackboard because its updating dynamically over time. Its used to transfer knowledge through the tree

when to use In/Output port mapping

When you want to have some static parameters like a timeout, a rate to run something at, a topic name, etc. Things that need to be defined per specific use-cases and do not change through out execution.

If you have a single tree its not that of a problem but if you have subtrees also, you need to map between blackboards in the xml. And if some blackboard elements are hidden, you forget about them when changing the xml or use a gui. So I think it‘s a bit against the modularity of BT‘s.

Can you give me a tangible example for this? Also is any of your BT work open-source? I suppose I never asked to see if this stuff was anything I could see and we could learn from. Also feel 100% welcome to propose new BT nodes to add to the stack if they're useful. I would love to have a good library of BT primitive nodes for designers to use.

@maxlein
Copy link
Contributor Author

maxlein commented May 7, 2020

I can give some abstract examples the next days with a little explanation.

Btw. another thing I am not clear about:

When to pass a ROS node via BT node constructor and when to pass via blackboard. Maybe it‘s just faster to adopt an existing node by getting it via blackboard...

But if it doesn‘t really matter, maybe have a rule on which way you want.

@SteveMacenski
Copy link
Member

When to pass a ROS node via BT node constructor and when to pass via blackboard. Maybe it‘s just faster to adopt an existing node by getting it via blackboard...

I need a couple specific examples, I'm not sure I understand

@maxlein
Copy link
Contributor Author

maxlein commented May 11, 2020

Nothing dramatic, just these two options:

Pasing the ros node to the constructor of a BT node:

template <typename ActionNodeT>
  void registerBTNodeAndPassROSNode(const std::string& actionName)
  {
    BT::NodeBuilder nodeBuilder = [&](const std::string& name, const BT::NodeConfiguration& config) {
      auto node = std::make_unique<ActionNodeT>(name, config, rosNode);
      return node;
    };
    factory.registerBuilder<ActionNodeT>(actionName, nodeBuilder);
  }

Or adding to the blackboard:

blackboard->set<rclcpp::Node::SharedPtr>(BT::PORT::ROS_NODE, rosNode);

As a little abstract example on our usage of BT's:

Basically we have an upper level node with a BT running which handles more abstract tasks like Navigation, Transfer handling, etc..
And the navigation task for example has Subtrees like Dock and Undocking.
So for each robot type you may have a custom Dock tree to fulfill some "special" needs of a robot.
Also the subtree is easily run standalone for repeated docking tests, where you don't want to waste time with navigating to the goal and other tasks of the main behavior.

This also reminds me of a missing feature in ros2, where it's not possible to include other xml's like it was in ROS1.

And nav2 then has it's own BT for all the more atomic "movement tasks".

But to get back to my question with sharing data between trees/subtrees:
In practice it is not that big of a problem, as it's only the ros node which is shared mostly.

Screenshot from 2020-05-11 22-55-45

@SteveMacenski
Copy link
Member

Where do you see registerBTNodeAndPassROSNode in the codebase?

This also reminds me of a missing feature in ros2, where it's not possible to include other xml's like it was in ROS1.

Not sure what you mean by this. There is no BT.CPP in ROS1 nav.

But to get back to my question with sharing data between trees/subtrees:

That doesn't seem to be a problem with Nav2, that seems to be a problem you reference with BT.CPP. Have you filed a ticket to have the data sharing between trees /subtrees?

@SteveMacenski
Copy link
Member

Oh whoops, hit the wrong button

@SteveMacenski SteveMacenski reopened this May 11, 2020
@maxlein
Copy link
Contributor Author

maxlein commented May 11, 2020

registerBTNodeAndPassROSNode is just our version.
I remembered seeing it somewhere but not sure anymore if it was in nav2. But to answer, the recommended way in nav2 is to pass a ros node via blackboard.

That doesn't seem to be a problem with Nav2, that seems to be a problem you reference with BT.CPP. Have you filed a ticket to have the data sharing between trees /subtrees?

I was not talking of a bug or similar.

If you have a single tree its not that of a problem but if you have subtrees also, you need to map between blackboards in the xml. And if some blackboard elements are hidden, you forget about them when changing the xml or use a gui. So I think it‘s a bit against the modularity of BT‘s.

I think it's fine to just read from the blackboard and write the remapping to every subtree.
Maybe a custom ROS subtree where the node is automatically remapped would be an idea.

Not sure what you mean by this. There is no BT.CPP in ROS1 nav.

Not in the nav stack, but there were some examples.

<include ros_pkg="name_package" path="path_relative_to_pkg/grasp.xml"/>

Described here.
I can't find the ros1 example trees anymore though.

@SteveMacenski
Copy link
Member

At this point, I don't know what we're talking about anymore. You're referencing a different internal version of something and asking about the pros and cons of it. I can't know, its something you wrote and haven't pushed upstream.

The other topics are BT.CPP related and not navigation related. You should take it up with them.

@maxlein
Copy link
Contributor Author

maxlein commented May 11, 2020

Yeah, you are right.
I think you can close this now..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants