-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add override_status API #191
Conversation
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Signed-off-by: Yadunund <yadunund@openrobotics.org>
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.
Thanks for addressing this need. I'm just requesting a couple small tweaks. Otherwise looks good 👍
if (status.has_value()) | ||
{ | ||
const auto loader = | ||
[n = context->node(), s = _pimpl->schema_dictionary]( |
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.
It's not a huge issue since the schema dictionary only contains one value, but theoretically if it ever increased then this would be a needlessly costly capture, because it makes a complete copy of all the contents of schema_dictionary
.
This is a very specific case where it's okay to capture [this]
, because the lambda will only be used within the scope of this function. I recommend just capturing [this]
here to avoid the unnecessary copy. We should also leave a comment explaining why [this]
is okay.
Signed-off-by: Yadunund <yadunund@openrobotics.org>
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.
Thanks for the quick turnaround!
Signed-off-by: Yadunund <yadunund@openrobotics.org>
Sorry @mxgrey I just pushed a commit to update the bindings. Need your review again. |
Signed-off-by: Yadunund <yadunund@gmail.com>
@@ -126,6 +126,8 @@ PYBIND11_MODULE(rmf_adapter, m) { | |||
py::arg("battery_soc"), | |||
py::call_guard<py::scoped_ostream_redirect, | |||
py::scoped_estream_redirect>()) | |||
.def("override_status", &agv::RobotUpdateHandle::override_status, | |||
py::arg("battery_soc")) |
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.
Is py::arg("battery_soc"))
correct?
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.
Nope, that's definitely a copy/paste error. I'll correct that in an upcoming PR.
This PR introducing
RobotUpdateHandle::override_status(std::optional<std::string > status)
API.Users can use this API to override the string status for a given robot. By default the
TaskManger
assigns eitheridle
orworking
depending on whether the robot is performing a task or not. But in some cases, users may want to explicitly set the status of the robot. In these scenarios, this API may be used. The API internally checks against therobot_state.json
schema before accepting the new status. When users want RMF to take back control of the status, they may call this API withstd::nullopt
.Signed-off-by: Yadunund yadunund@openrobotics.org