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

Identifier name clashing #18

Closed
v-lopez opened this issue Nov 23, 2018 · 6 comments
Assignees
Labels

Comments

@v-lopez
Copy link
Contributor

@v-lopez v-lopez commented Nov 23, 2018

As initially commented here: https://discourse.ros.org/t/introducting-behaviortrees-cpp-who-needs-finite-state-machines-anymore/6899/7?u=v-lopez

Once you are reusing components (ideally even components you have not developed yourself), there will be some name clashing, which can be potentially dangerous (ie: mix the targetPose for foot placement with the targetPose for grasping).

We have developed a FSM implementation and faced this issues, and approached them in the same way ROS1 does (remapping and namespaces).

When a FSM includes another FSM, it can remap some of its identifiers (targetPose:=walkingTargetPose), or the sub state machine can be put inside a namespace, and the one including it can perform the required remappings.

@v-lopez v-lopez changed the title Name clashing Identifier name clashing Nov 23, 2018
@facontidavide facontidavide self-assigned this Nov 23, 2018
@facontidavide

This comment has been minimized.

Copy link
Collaborator

@facontidavide facontidavide commented Nov 23, 2018

It makes sense. I will probably work on this the first or second week of December. Thanks for the suggestion

@v-lopez

This comment has been minimized.

Copy link
Contributor Author

@v-lopez v-lopez commented Nov 23, 2018

No problem, we've got a bunch of use cases for this, if you ever drop by PAL I can show you in first person.

@facontidavide

This comment has been minimized.

Copy link
Collaborator

@facontidavide facontidavide commented Dec 10, 2018

The more I think about this, more I believe we do need it but it is also tricky to do it right.

@v-lopez Can we have a meeting/telco to brainstorm about this feature?

@v-lopez

This comment has been minimized.

Copy link
Contributor Author

@v-lopez v-lopez commented Dec 10, 2018

Sure, I just sent you an email so we can schedule it.

@facontidavide

This comment has been minimized.

Copy link
Collaborator

@facontidavide facontidavide commented Dec 19, 2018

I have been thinking a LOT about this, and I come to the conclusion that we need a major change in the library to address this in a way that is both future proof and elegant.

I will open a different issue with a long description of the use case to star a debate.

But summarizing:

  • The blackboard is like the "wild west" of the dataflow. In terms of models we don't know who writes and who reads. Dataflow is hard-coded as the key of the blackboard entry into the C++ code.

  • The solution is to add a slightly more strict concept of input/output ports similar to SMACH; under the hood, it is still a blackboard, with the exception that you can access only certain entries, not the entire blackboard.

  • I have been thinking a lot and I come to the conclusion that both in terms of semantic and implementation, NodeParameters are already a sound implementation of input ports.

  • As a consequence of the previous point, users should be encouraged to use TreeNode::getParam instead of TreeNode::blackboard()::get.

  • To avoid name clashing without modifying the C++ code, a simple solution is key remapping (similar to ROS one). Since user already specifies the name of the key as a NodeParameter, we need to explicitly allow remapping ONLY in the output port.

@facontidavide

This comment has been minimized.

Copy link
Collaborator

@facontidavide facontidavide commented Jan 3, 2019

Discussion moved to #41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.