EasyFullControl API #189

xiyuoh · 2022-03-25T10:23:20Z

This PR introduces an EasyFullControl API for users to initialize robots and set important API functions as callbacks. This provides an easy way of creating their own adapters and also prevents them from having to update the internals of their modified code manually when new changes are pushed.

Signed-off-by: Xi Yu Oh <xiyu@openrobotics.org>

codecov · 2022-03-25T11:26:57Z

Codecov Report

Merging #189 (64baf02) into main (3be4edf) will increase coverage by 0.26%.
The diff coverage is n/a.

❗ Current head 64baf02 differs from pull request most recent head a463312. Consider uploading reports for the commit a463312 to get more accurate results

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   17.61%   17.87%   +0.26%     
==========================================
  Files         209      418     +209     
  Lines       19351    38138   +18787     
  Branches     9297    18248    +8951     
==========================================
+ Hits         3409     6819    +3410     
- Misses      12274    24000   +11726     
- Partials     3668     7319    +3651

Flag	Coverage Δ
tests	`17.87% <ø> (+0.26%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...-4.1.0/Rx/v2/src/rxcpp/operators/rx-observe_on.hpp
...os2/rmf_traffic_ros2/examples/participant_node.cpp
...Cpp-4.1.0/Rx/v2/src/rxcpp/subjects/rx-behavior.hpp
...src/rmf_fleet_adapter/events/EmergencyPullover.cpp
...rmf_traffic_ros2/schedule/internal_MonitorNode.hpp
...os2/rmf_fleet_adapter/test/tasks/test_Delivery.cpp
...f_fleet_adapter/src/read_only/FleetAdapterNode.cpp
..._ros2/src/rmf_traffic_ros2/schedule/YamlLogger.cpp
..._adapter/src/rmf_fleet_adapter/agv/parse_graph.cpp
...er/src/rmf_fleet_adapter/phases/EndLiftSession.hpp
... and 617 more

Yadunund

Thanks for getting started on this. Couple of comments for your consideration.

Yadunund · 2022-04-12T05:04:00Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp

+  bool add_robot(
+    const std::string& name,
+    const Eigen::Vector3d& pose,
+    std::function<Eigen::Vector3d()> get_position,


Let's alias the type of some of these parameters similar to what you have defined to ProcessCompleted.
Eg.

using GetPosition = std::function<Eigen::Vector3d()>; bool add_robot( ... GetPosition get_position, ...);

Yadunund · 2022-04-12T05:07:07Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp

+
+  bool add_robot(
+    const std::string& name,
+    const Eigen::Vector3d& pose,


Maybe instead of Eigen::Vector3d, this could be a

using Start = std::variant<rmf_traffic::agv::Planner::Start, Eigen::Vector3d>; ... bool add_robot( ... Start start ...);

This would allow users to either submit a known Panner:Start or a pose. In the latter case, we will use compute_plan_starts to estimate the start.

Yadunund · 2022-04-12T05:07:38Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp

+    ActionExecutor action_executor);
+
+private:
+  EasyFullControl();


Use PIMPL pattern

Yadunund · 2022-04-12T05:08:08Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp

+class EasyFullControl : public std::enable_shared_from_this<EasyFullControl>
+{
+public:
+


Add a static make() function which will initialize and spin an Adapter.

Yadunund · 2022-04-12T05:09:57Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp

+{
+public:
+
+  using ProcessCompleted = std::function<bool(const std::string& id)>;


Since a ProcessCompleted is returned for a specific request, we do not need to give it a string id to get feedback. The internal implementation will store a map.

Yadunund · 2022-04-12T05:13:57Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp

+    const Eigen::Vector3d& pose,
+    std::function<Eigen::Vector3d()> get_position,
+    std::function<ProcessCompleted(const Eigen::Vector3d pose)> navigate,
+    ActionExecutor action_executor);


We would also need to accept an object that allows us to perform coordinate transformation between RMF and robot map frames. We could create a class Transformation subclass that stores Eigen Transform2D/Affine objects to convert between RMF <> Robot coordinates

On second thought, ignore this. If the documentation makes it explicit that all coordinates are in RMF coordinate frame, then we do not have to worry about this.

Signed-off-by: Xi Yu Oh <xiyu@openrobotics.org>

xiyuoh · 2022-04-21T04:14:43Z

@Yadunund Thanks for the comments, addressed them in the latest commit. Also added a Configuration class as suggested to store the parameters needed for initializing an adapter and fleet.

mxgrey · 2022-09-01T15:57:32Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp

+    const std::string& name,
+    Start pose,
+    GetPosition get_position,
+    std::function<ProcessCompleted(const Eigen::Vector3d pose)> navigate,


I'm not super sure about this navigate callback. I understand how it would work: We issue a navigation command by triggering this callback, then we get back a new callback that we can poll to figure out if the robot has finished navigating.

What I would've expected is something more like this:

std::function<void(const Eigen::Vector3d pose, std::function<void()> notify_finished)> navigate

We would trigger this navigate callback and provide the user with:

the pose we want the robot to navigate to

a callback for the user to trigger when the robot has reached its goal

I do understand the appeal of the current design: The user is less fallible because by polling we can demand to know whether the robot is finished yet, whereas giving them a callback to trigger means we're relying on them to remember to trigger it eventually.

However, I think from the user's point of view it will probably be easier for them to just store a callback that they will trigger when their navigating is finished, rather than write a callback that can correctly report whether the destination has been reached whenever it is called. It would also fit better in a reactive programming paradigm.

I don't feel super strongly about this since the current API could absolutely work. Any preference between the two approaches is pretty subjective, so I'd be interested in hearing other people's opinions.

mxgrey · 2022-09-01T15:59:50Z

rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp

+  /// \param[in] navigate
+  ///   The API function for navigating your robot to a pose.
+  ///   Returns a ProcessCompleted callback to check status of navigation task.
+  ///


It seems to me like the add_robot function is missing a callback that would let us command the robot to stop immediately. That feature in the RobotCommandHandle does get used sometimes.

I think we're also missing something for dock commands.

Sorry i didn't mention in this PR that i'm continuing the work on open-rmf's feature/easy_full_control branch. I added the stop callback there, but let me push some updates from today there as well

xiyuoh · 2022-09-13T17:23:56Z

Closed, continuing on #235

Initial draft

882919f

Signed-off-by: Xi Yu Oh <xiyu@openrobotics.org>

xiyuoh changed the title ~~Initial draft~~ EasyFullControl API Mar 26, 2022

Yadunund reviewed Apr 12, 2022

View reviewed changes

Added Configuration and make()

64baf02

Signed-off-by: Xi Yu Oh <xiyu@openrobotics.org>

xiyuoh and others added 2 commits August 23, 2022 18:20

Merge branch 'open-rmf:main' into feature/easyfullcontrol

4518d3c

Merge branch 'main' into feature/easyfullcontrol

a463312

mxgrey reviewed Sep 1, 2022

View reviewed changes

xiyuoh mentioned this pull request Sep 13, 2022

EasyFullControl #235

Merged

3 tasks

xiyuoh closed this Sep 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EasyFullControl API #189

EasyFullControl API #189

xiyuoh commented Mar 25, 2022

codecov bot commented Mar 25, 2022 •

edited

Loading

Yadunund left a comment

Yadunund Apr 12, 2022

Yadunund Apr 12, 2022

Yadunund Apr 12, 2022

Yadunund Apr 12, 2022

Yadunund Apr 12, 2022

Yadunund Apr 12, 2022

Yadunund Apr 12, 2022

xiyuoh commented Apr 21, 2022

mxgrey Sep 1, 2022

mxgrey Sep 1, 2022

mxgrey Sep 1, 2022

xiyuoh Sep 1, 2022

xiyuoh commented Sep 13, 2022

EasyFullControl API #189

EasyFullControl API #189

Conversation

xiyuoh commented Mar 25, 2022

codecov bot commented Mar 25, 2022 • edited Loading

Codecov Report

Yadunund 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

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

xiyuoh commented Apr 21, 2022

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

xiyuoh commented Sep 13, 2022

codecov bot commented Mar 25, 2022 •

edited

Loading