Introduce a common TaskRemapper with wildcard support #55
Conversation
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
| const auto new_task = this->_task_remapper->remap(ctx->task.type); | ||
| if (new_task != ctx->task.type) | ||
| { | ||
| RCLCPP_DEBUG( | ||
| this->get_logger(), | ||
| "Loading remapped BT [%s] for original task type [%s]", | ||
| new_task.c_str(), | ||
| ctx->task.type.c_str() | ||
| ); | ||
| } | ||
| return this->_bt_factory->createTreeFromFile(this->_bt_store.get_bt( | ||
| ctx->task.type)); | ||
| new_task)); |
There was a problem hiding this comment.
Note here we could either:
- Use the new name to only load the behavior tree (what I did)
- Override the task type (what I didn't do)
There was a problem hiding this comment.
I prefer what you did which preserves the original request. Users can always look for the remapping if needed from ctx-> task_remapper
| /* | ||
| * Initialize the remapper with the value of a ROS parameter containing a YAML | ||
| */ | ||
| TaskRemapper(const std::string& param); |
There was a problem hiding this comment.
This API is error prone especially if users do not pass a string that can be deserialized into a YAML. ie YAML::Load() can throw an exception in the implementation.
I suggest we accept a concrete type here instead like an std::multimap where the key is the remapped process and the values for each key are the process steps in the work order.
This way if the param definition changes in the orchestrators, this API would not have to change.
There was a problem hiding this comment.
This PR doesn't introduce any new exception since all uses of this class were doing YAML::Load anyway, so they would have thrown on an invalid YAML.
The "proper" way I considered going for was either passing a YAML node as an input (so we won't throw) or keep the string and make the constructor fallible (i.e. a make function, return nullptr if the YAML failed deserializing). I didn't do it only because I saw that the codebase doesn't seem to use this paradigm but I'm happy to do it.
I feel that if we add a multimap and users of the API need to manually parse the YAML, handle exceptions, add its content to a multimap, then we read the multimap, iterate over it, check for wildcards, populate internal data structures accordingly, it becomes much more complex and clunky. Instead of being a refactor to reduce duplicated code we actually end up with a fair bit more code and complexity.
| const auto new_task = this->_task_remapper->remap(ctx->task.type); | ||
| if (new_task != ctx->task.type) | ||
| { | ||
| RCLCPP_DEBUG( | ||
| this->get_logger(), | ||
| "Loading remapped BT [%s] for original task type [%s]", | ||
| new_task.c_str(), | ||
| ctx->task.type.c_str() | ||
| ); | ||
| } | ||
| return this->_bt_factory->createTreeFromFile(this->_bt_store.get_bt( | ||
| ctx->task.type)); | ||
| new_task)); |
There was a problem hiding this comment.
I prefer what you did which preserves the original request. Users can always look for the remapping if needed from ctx-> task_remapper
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
This PR refactors the task remapping capability in
nexus_system_orchestrator(which was fully fledged) and the one innexus_workcell_orchestrator(that was actually unimplemented) into a commonTaskRemapperclass.This class has basic wildcard support, if it finds such a remapping:
It will override every other defined remap and just remap every single task to the requested
pick_and_place.I added a set of unit tests to show how the functionality works and now both system and workcell orchestrator share the same codebase for task remapping.
In the future we can consider more complex behaviors (i.e. proper wildcard support)