-
Notifications
You must be signed in to change notification settings - Fork 8
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
Introduce a common TaskRemapper with wildcard support #55
base: main
Are you sure you want to change the base?
Changes from 2 commits
6eafff4
a8c5bf4
a7297a5
c61743e
b881d1e
8c62c69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
/* | ||
* Copyright (C) 2024 Open Source Robotics Foundation | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
|
||
#ifndef NEXUS_COMMON__TASK_REMAPPER_HPP | ||
#define NEXUS_COMMON__TASK_REMAPPER_HPP | ||
|
||
#include "nexus_common_export.hpp" | ||
|
||
#include <rclcpp/rclcpp.hpp> | ||
|
||
namespace nexus::common { | ||
|
||
/** | ||
* Provides task remapping capability | ||
*/ | ||
class NEXUS_COMMON_EXPORT TaskRemapper | ||
{ | ||
public: | ||
/* | ||
* Initialize the remapper with the value of a ROS parameter containing a YAML | ||
*/ | ||
TaskRemapper(const std::string& param); | ||
|
||
/* | ||
* Remaps, if necessary, the input task | ||
*/ | ||
std::string remap(const std::string& task) const; | ||
luca-della-vedova marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private: | ||
// If present, match every incoming task to the target task | ||
std::optional<std::string> _wildcard_match; | ||
std::unordered_map<std::string, std::string> _task_remaps; | ||
}; | ||
|
||
} | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
/* | ||
* Copyright (C) 2024 Open Source Robotics Foundation | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
|
||
#include "task_remapper.hpp" | ||
|
||
#include <yaml-cpp/yaml.h> | ||
|
||
namespace nexus::common { | ||
|
||
TaskRemapper::TaskRemapper(const std::string& param) | ||
{ | ||
const auto remaps = YAML::Load(param); | ||
for (const auto& n : remaps) | ||
{ | ||
const auto task_type = n.first.as<std::string>(); | ||
const auto& mappings = n.second; | ||
for (const auto& m : mappings) | ||
{ | ||
auto mapping = m.as<std::string>(); | ||
if (mapping == "*") | ||
{ | ||
this->_wildcard_match = task_type; | ||
this->_task_remaps.clear(); | ||
return; | ||
} | ||
// TODO(luca) check for duplicates, logging if found | ||
this->_task_remaps.emplace(mapping, task_type); | ||
} | ||
} | ||
} | ||
|
||
std::string TaskRemapper::remap(const std::string& task) const | ||
{ | ||
if (this->_wildcard_match.has_value()) | ||
{ | ||
return this->_wildcard_match.value(); | ||
} | ||
const auto it = this->_task_remaps.find(task); | ||
if (it != this->_task_remaps.end()) | ||
{ | ||
return it->second; | ||
} | ||
return task; | ||
} | ||
|
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Copyright (C) 2023 Johnson & Johnson | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*/ | ||
|
||
#define CATCH_CONFIG_MAIN | ||
#include <rmf_utils/catch.hpp> | ||
|
||
#include "task_remapper.hpp" | ||
#include "test_utils.hpp" | ||
|
||
namespace nexus::common::test { | ||
|
||
TEST_CASE("task_remapping") { | ||
std::string param = | ||
R"( | ||
pick_and_place: [pick, place] | ||
)"; | ||
auto remapper = TaskRemapper(param); | ||
CHECK(remapper.remap("pick") == "pick_and_place"); | ||
CHECK(remapper.remap("place") == "pick_and_place"); | ||
CHECK(remapper.remap("other") == "other"); | ||
} | ||
|
||
TEST_CASE("task_remapping_with_wildcard") { | ||
std::string param = | ||
R"( | ||
pick_and_place: [pick, place] | ||
main : ["*"] | ||
)"; | ||
auto remapper = TaskRemapper(param); | ||
CHECK(remapper.remap("pick") == "main"); | ||
CHECK(remapper.remap("place") == "main"); | ||
CHECK(remapper.remap("other") == "main"); | ||
} | ||
|
||
luca-della-vedova marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,18 +92,8 @@ WorkcellOrchestrator::WorkcellOrchestrator(const rclcpp::NodeOptions& options) | |
desc.read_only = true; | ||
desc.description = | ||
"A yaml containing a dictionary of task types and an array of remaps."; | ||
const auto yaml = this->declare_parameter("remap_task_types", std::string{}, | ||
desc); | ||
const auto remaps = YAML::Load(yaml); | ||
for (const auto& n : remaps) | ||
{ | ||
const auto task_type = n.first.as<std::string>(); | ||
const auto& mappings = n.second; | ||
for (const auto& m : mappings) | ||
{ | ||
this->_task_remaps.emplace(m.as<std::string>(), task_type); | ||
} | ||
} | ||
const auto param = this->declare_parameter("remap_task_types", "", desc); | ||
this->_task_remapper = std::make_shared<common::TaskRemapper>(param); | ||
} | ||
{ | ||
ParameterDescriptor desc; | ||
|
@@ -988,8 +978,18 @@ BT::Tree WorkcellOrchestrator::_create_bt(const std::shared_ptr<Context>& ctx) | |
{ | ||
// To keep things simple, the task type is used as the key for the behavior tree to use. | ||
this->_ctx_mgr->set_active_context(ctx); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note here we could either:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer what you did which preserves the original request. Users can always look for the remapping if needed from |
||
} | ||
|
||
void WorkcellOrchestrator::_handle_command_success( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, returnnullptr
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 amultimap
, then we read themultimap
, 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.