Allow robot-specific finishing request and specify parking spots #379
Conversation
Signed-off-by: Xiyu Oh <[email protected]>
Signed-off-by: Xiyu Oh <[email protected]>
|
One edge case not resolved is if the fleet-wide finishing request is either |
|
The edge case mentioned above is fixed in 337b79f. |
Signed-off-by: Xiyu Oh <[email protected]>
| @@ -264,7 +264,8 @@ class EasyFullControl::RobotConfiguration | |||
| std::optional<bool> responsive_wait = std::nullopt, | |||
| std::optional<double> max_merge_waypoint_distance = 1e-3, | |||
| std::optional<double> max_merge_lane_distance = 0.3, | |||
| std::optional<double> min_lane_length = 1e-8); | |||
| std::optional<double> min_lane_length = 1e-8, | |||
| std::optional<rmf_task::ConstRequestFactoryPtr> finishing_request = std::nullopt); | |||
There was a problem hiding this comment.
We can't add a new argument to this constructor without breaking ABI stability, even if we give it a default value.
Unfortunately we'll need to remove this and rely on the set_finishing_request method while parsing the config file.
| std::nullopt; | ||
| const YAML::Node& finishing_request_yaml = | ||
| robot_config_yaml["finishing_request"]; | ||
| if (finishing_request_yaml) |
There was a problem hiding this comment.
I'm a bit concerned about how much parsing logic we're duplicating between here and the fleet-level parsing for finishing requests. If we find a bug or make a change in one, we'll need to remember to update the other, which is an easy thing to forget after months or years from now. I have some suggestions which could help eliminate the duplication:
- Define one new separate function that parses finishing requests. The signature should be something like:
std::optional<rmf_task::ConstRequestFactoryPtr> parse_finishing_request(
const YAML::Node& finishing_request_yaml,
const rmf_traffic::agv::Graph& graph,
/* ... any other variables needed during parsing ... */
bool robot_specific,
bool& error)
- If
robot_specificis true then we allow the new format, i.e.{ type: park, parking_spot: 0 }. Otherwise the presence of that format should produce an error. - If a parsing error happens, the
errorargument should be set to true, and we should check that immediately after the function is called so we know to immediately return anullptr - The error messages should take advantage of the
YAML::Node::Mark()function (example here) to tell the user precisely where the error happened. Then there is no need for us to explicitly mention the robot or fleet information in the error messages.
There was a problem hiding this comment.
Thanks! I've implemented this in 6b655d8 but changed bool robot_specific to std::optional<std::string> robot_name to specify which robot's finishing request we are parsing.
| // We will need to know the configured parking spot for this | ||
| // robot. | ||
| const YAML::Node& parking_spot_yaml = | ||
| finishing_request_yaml["parking_spot"]; |
There was a problem hiding this comment.
Minor nitpick, I think we should call this waypoint_name instead to make the meaning of the value more explicit.
|
One last issue I just realized after submitting the previous review: When users call To fix that we should add some kind of flag to |
Signed-off-by: Xiyu Oh <[email protected]>
Signed-off-by: Xiyu Oh <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
There was a problem hiding this comment.
Thanks for iterating on this. I've made two sets of changes to bring this to a state that I think is ready to merge. Take a look over them and if they seem good then go ahead and merge:
- f61c220: This tweaks the parsing behavior to be stricter in some ways (some of the fallback behaviors that you programmed are now errors that will cause the parsing to fail) but also more consistent (the
{ type: _, .. }format can be used to express all finishing requests in all situations). Hopefully we never have to manually program a parser ever again after this since we're transitioning to Rust. - 92f1f17: The public API functions of
RobotUpdateHandlewill schedule their actions on the worker to avoid potential data races with other activities that may be happening in the fleet adapter.
|
@mxgrey The changes look good, thanks for helping to add them in! |
* Allow robot-specific finishing request Signed-off-by: Xiyu Oh <[email protected]> * Add printout for configured finishing request without parking spot Signed-off-by: Xiyu Oh <[email protected]> * Cancel existing waiting behavior if new idle task is nothing Signed-off-by: Xiyu Oh <[email protected]> * Add brackets Signed-off-by: Xiyu Oh <[email protected]> * Refactor based on review comments Signed-off-by: Xiyu Oh <[email protected]> * Cleanup Signed-off-by: Xiyu Oh <[email protected]> * Tweak finishing_request parsing Signed-off-by: Michael X. Grey <[email protected]> * Schedule all API activity on the worker Signed-off-by: Michael X. Grey <[email protected]> * Small fixes Signed-off-by: Michael X. Grey <[email protected]> --------- Signed-off-by: Xiyu Oh <[email protected]> Signed-off-by: Michael X. Grey <[email protected]> Co-authored-by: Michael X. Grey <[email protected]>
This PR provides a way for users to specify different finishing requests for individual robots. Currently all robots in a fleet follow the fleet-wide finishing request configured. If the finishing request is either
parkorcharge, the robots will head to their assigned charger waypoint to perform their respective ParkRobotIndefinitely or ChargeBattery tasks.With this PR, users can:
Relevant issue ticket: open-rmf/rmf_task#115