Support PerformAction event in composed tasks #155

Yadunund · 2022-01-15T23:15:41Z

This PR aims to introduce support for PerformAction event. Changes

JSON schema for PerformAction event description
FleetUpdateHandle::add_performable_action(const std::string& category) API: Set the category of actions that robots in this fleet can perform.
RobotUpdateHandle::set_action_executor(ActionExecutor action_executor) API: Set the executor which can be used by the fleet adapter to request a robot to perform an action and RobotUpdateHandle::update_action_remaining_time(rmf_traffic::Duration duration) to update the estimate of the time remaining for the action.
Standby and Active definitions for PerformAction

Signed-off-by: Yadunund <yadunund@openrobotics.org>

mxgrey · 2022-01-17T04:51:39Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/RobotUpdateHandle.hpp

+  using ActionExecutor =
+    std::function<void(
+      const nlohmann::json& action,
+      const ActionCompleted& completed)


Let's send this argument by value instead of by reference as a kind of encouragement for the user to save a copy of it. There's nothing wrong with passing by reference, but it usually carries the implication that the object is only meant to be viewed rather than used.

mxgrey · 2022-01-17T05:03:18Z

rmf_fleet_adapter/src/rmf_fleet_adapter/agv/FleetUpdateHandle.cpp

+      try
+      {
+        const nlohmann::json action =
+          nlohmann::json::parse(msg["action"].get<std::string>());


I don't think this is a good way to use the nlohmann library or JSON more generally. What this is literally saying is that the action field will refer to a string, and that string will contain an entire JSON message inside of it. So that might look something like this:

"action": "{ \"clean\": { \"zone\": \"terminal_3\" } }"

It would become a nightmare of escaping quotes and a very confusing structure.

I recommend the following schema instead:

"perform_action": { "category": "some_simple_string", "description": { "any": 0 "details": { }, "about": [ ], "this": true "action": " " }, "expected_finish_location": <optional>, "unix_millis_action_duration_estimate": <optional>, ... any other optional task planner details ... }

The add_performable_action function will have this signature instead:

FleetUpdateHandle& FleetUpdateHandle::add_performable_action( const std::string& category, ConsiderRequest consider);

We will maintain a map from category string keys to consider callbacks. When a perform_action event comes in, we will do a look up on that map. If the lookup fails, we reject the task. If the look up succeeds, we trigger the consider callback and then accept or reject based on its response.

The ActionExecutor signature would look like this instead:

using ActionExecutor = std::function<void( const std::string& category, const nlohmann::json& description, ActionCompleted completed)

This should make it very easy for users to put in their own arbitrary JSON details for the action while still allowing the fleet adapter to do the heavy lifting in weeding out irrelevant task requests.

Thanks for the feedback. I can see how this is much easier for the user. I have a couple of questions on the schema

What is the purpose of the any, detail and about fields?

I assume the intention is for the entire description JSON block to be passed into PerformAction::Description::make() as the action. Is this correct?

The any, details, about, this, action are just examples of how a user can fill the description JSON with any kind of data they want. You can totally disregard them, they're not actually part of the schema, just placeholders.

I assume the intention is for the entire description JSON block to be passed into PerformAction::Description::make() as the action. Is this correct?

That's right, the PerformAction::Description would need to have some kind of action_message field (or something similarly named) to store the message that gets passed to the robot command handle.

Signed-off-by: Yadunund <yadunund@openrobotics.org>

mxgrey

This is generally looking really good! I just have some recommendations for tightening up the API.

mxgrey · 2022-01-17T18:11:35Z

rmf_fleet_adapter/src/rmf_fleet_adapter/agv/FleetUpdateHandle.cpp

+      }
+    };
+
+  _pimpl->deserialization.event->add(


We shouldn't need to re-add "perform_action" each time the user adds a new category. We can add this once at startup and then just update the action category map when the user wants to add a new action category.

Moved it into FleetUpdateHandle::Implementation::make(~)

Sorry to be nitpicky about this, but could we imitate the pattern that's being used in FleetUpdateHandle::Implementation::add_standard_tasks? Basically put this implementation into a events::add_perform_action(~) function and define that function in src/rmf_fleet_adapter/events/PerformAction.cpp?

I think that would organize the code a bit better and reduce the already considerable amount of noise in this make function.

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/RobotUpdateHandle.hpp

rmf_fleet_adapter/schemas/event_description_PerformAction.json

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/RobotUpdateHandle.hpp

rmf_fleet_adapter/src/rmf_fleet_adapter/events/PerformAction.cpp

Signed-off-by: Yadunund <yadunund@openrobotics.org>

mxgrey · 2022-01-24T10:42:13Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/RobotUpdateHandle.hpp

+    // TODO: Consider giving access to the participant schedule and
+    // traffic negotiation
+
+    ~ActionExecution()


We should put the definition of this function into the proper translation unit, i.e. RobotUpdateHandle.cpp. I understand you were probably copying the example code that I provided, but I defined it inline for brevity. It's not actually good to leave this function definition exposed in a public API, because that can lead to poorly defined behavior if we ever change the implementation of the function.

mxgrey · 2022-01-24T10:44:48Z

rmf_fleet_adapter/schemas/event_description_PerformAction.json

+    },
+    "description": {
+      "description": "A dictionary describing the action to be performed.",
+      "type": "object"


Let's not force this to be a dictionary. Maybe a system integrator would like this to be an array. We can just remove this "type": "object" specification and leave it ambiguous.

I'd also recommend changing description to something like `"Additional information that will be passed along to the action executor."

mxgrey · 2022-01-24T10:50:06Z

rmf_fleet_adapter/src/rmf_fleet_adapter/agv/RobotContext.cpp

+void RobotContext::action_executor(
+  RobotUpdateHandle::ActionExecutor action_executor)
+{
+  if (action_executor != nullptr)


I'm not actually sure that we should block this. What if the user is trying to erase its action executor for some reason?

I recommend instead of blocking, we simply emit a warning that the action executor is being nullified which will lead to an automatic critical task error if a task requires an action to be performed while this is still set to null.

mxgrey · 2022-01-24T10:52:31Z

rmf_fleet_adapter/src/rmf_fleet_adapter/agv/internal_RobotUpdateHandle.hpp

  {
    return *handle._pimpl;
  }

-  static const Implementation& get(const RobotUpdateHandle& handle)
+  static const Implementation &get(const RobotUpdateHandle &handle)


Looks like some automatic styling has messed things up here.

mxgrey · 2022-01-24T10:56:07Z

rmf_fleet_adapter/src/rmf_fleet_adapter/events/PerformAction.cpp

+    _state->update_status(Status::Error);
+    const std::string msg = "ActionExecutor not set via RobotUpdateHandle. "
+      "Unable to perform the requested action.";
+    _state->update_log().info(msg);


This should be an error-level log, not info.

mxgrey · 2022-01-24T10:56:34Z

rmf_fleet_adapter/src/rmf_fleet_adapter/events/PerformAction.cpp

+    const std::string msg = "ActionExecutor not set via RobotUpdateHandle. "
+      "Unable to perform the requested action.";
+    _state->update_log().info(msg);
+    _finished();


Let's update the status to an error before finishing.

mxgrey · 2022-01-24T10:58:53Z

This is shaping up very nicely. I just left a few more small remarks to address and then I'll be happy to approve 👌

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Yadunund · 2022-01-24T15:45:03Z

Thanks for all the great feedback!

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/RobotUpdateHandle.hpp

rmf_fleet_adapter/src/rmf_fleet_adapter/agv/RobotUpdateHandle.cpp

Signed-off-by: Yadunund <yadunund@openrobotics.org>

mxgrey

Looks great! 🎉

Yadunund added 2 commits January 16, 2022 03:25

Implemented PerformAction event and schema

b6026b1

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Added ActionExecutor to RobotUpdateHandle

28c5a30

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Yadunund requested a review from mxgrey January 15, 2022 23:16

Merge branch 'redesign_v2' into perform_action

e85d85f

mxgrey requested changes Jan 17, 2022

View reviewed changes

Yadunund added 4 commits January 17, 2022 17:42

Add category to PerformAction schema

c2c63b3

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Support teleop in full_control

13abab5

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Print errors when auctioneer does not receive any bids

4cf4da4

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Fix bug in ActiveTask::publish_task_state()

4e659ad

Signed-off-by: Yadunund <yadunund@openrobotics.org>

mxgrey requested changes Jan 20, 2022

View reviewed changes

Yadunund added 4 commits January 24, 2022 11:27

Add PerformAction event inside make()

39f9909

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Implement ActionExecution

bb94cee

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Update ActionExecution implementation

22ac842

Signed-off-by: Yadunund <yadunund@openrobotics.org>

Update ActionExecution API

9a8ad64

Signed-off-by: Yadunund <yadunund@openrobotics.org>

mxgrey reviewed Jan 24, 2022

View reviewed changes

Schema and other fixes

4fbf6cb

Signed-off-by: Yadunund <yadunund@openrobotics.org>

mxgrey reviewed Jan 24, 2022

View reviewed changes

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/RobotUpdateHandle.hpp Show resolved Hide resolved

mxgrey reviewed Jan 24, 2022

View reviewed changes

rmf_fleet_adapter/src/rmf_fleet_adapter/agv/RobotUpdateHandle.cpp Show resolved Hide resolved

Call finished only once

22a4fba

Signed-off-by: Yadunund <yadunund@openrobotics.org>

mxgrey approved these changes Jan 24, 2022

View reviewed changes

Yadunund merged commit 10c9a51 into redesign_v2 Jan 24, 2022

Yadunund deleted the perform_action branch January 24, 2022 16:29

xiyuoh mentioned this pull request Feb 7, 2022

Feature/teleop xiyuoh/rmf_demos#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PerformAction event in composed tasks #155

Support PerformAction event in composed tasks #155

Yadunund commented Jan 15, 2022 •

edited

Loading

mxgrey Jan 17, 2022

mxgrey Jan 17, 2022 •

edited

Loading

Yadunund Jan 17, 2022

mxgrey Jan 17, 2022

mxgrey left a comment

mxgrey Jan 17, 2022

Yadunund Jan 24, 2022

Yadunund Jan 24, 2022

mxgrey Jan 24, 2022 •

edited

Loading

mxgrey Jan 24, 2022

mxgrey Jan 24, 2022

mxgrey Jan 24, 2022

mxgrey Jan 24, 2022

mxgrey Jan 24, 2022

mxgrey Jan 24, 2022

mxgrey commented Jan 24, 2022

Yadunund commented Jan 24, 2022

mxgrey left a comment

Support PerformAction event in composed tasks #155

Support PerformAction event in composed tasks #155

Conversation

Yadunund commented Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

mxgrey Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxgrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxgrey Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxgrey commented Jan 24, 2022

Yadunund commented Jan 24, 2022

mxgrey left a comment

Choose a reason for hiding this comment

Yadunund commented Jan 15, 2022 •

edited

Loading

mxgrey Jan 17, 2022 •

edited

Loading

mxgrey Jan 24, 2022 •

edited

Loading