Skip to content
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

EasyFullControl API #189

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 114 additions & 0 deletions rmf_fleet_adapter/include/rmf_fleet_adapter/agv/EasyFullControl.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
* Copyright (C) 2022 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 RMF_FLEET_ADAPTER__AGV__EASYFULLCONTROL_HPP
#define RMF_FLEET_ADAPTER__AGV__EASYFULLCONTROL_HPP

#include <Eigen/Geometry>
#include <rmf_fleet_adapter/agv/RobotUpdateHandle.hpp>

#include <rmf_traffic/agv/Planner.hpp>
#include <rmf_traffic/agv/VehicleTraits.hpp>
#include <rmf_traffic/agv/Graph.hpp>

namespace rmf_fleet_adapter {
namespace agv {

//==============================================================================
class EasyFullControl : public std::enable_shared_from_this<EasyFullControl>
{
public:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// The Configuration class contains parameters necessary to initialize an
/// Adapter instance and add fleets to the adapter.
class Configuration
{
public:

/// Constructor
///
/// \param[in] node_name
/// The name for the rclcpp::Node that will be produced for the Adapter.
///
/// \param[in] fleet_name
/// The name of the fleet that is being added.
///
/// \param[in] graph
/// The graph which is being used by the fleet.
///
/// \param[in] traits
/// The traits of the vehicle that is added to the fleet.
///
/// \param[in] server_uri
/// The URI for the websocket server that receives updates on tasks and
/// states. If nullopt, data will not be published.
Configuration(
const std::string& node_name,
const std::string& fleet_name,
rmf_traffic::agv::Graph graph,
rmf_traffic::agv::VehicleTraits traits,
std::optional<std::string> server_uri = std::nullopt);

class Implementation;
private:
rmf_utils::impl_ptr<Implementation> _pimpl;
};

/// Initialize and spin an Adapter instance in order to add fleets.
///
/// \param[in] config
/// The Configuration for the adapter that contains parameters used by the
/// fleet robots.
static std::shared_ptr<EasyFullControl> make(Configuration config);

using Start = std::variant<rmf_traffic::agv::Planner::Start, Eigen::Vector3d>;
using GetPosition = std::function<Eigen::Vector3d()>;
using ProcessCompleted = std::function<bool()>;

/// Initialize a robot in the fleet.
///
/// \param[in] name
/// The name of the robot.
///
/// \param[in] get_position
/// The position function that returns the robot's current location.
///
/// \param[in] navigate
/// The API function for navigating your robot to a pose.
/// Returns a ProcessCompleted callback to check status of navigation task.
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// \param[in] action_executor
/// The ActionExecutor callback to request the robot to perform an action.
bool add_robot(
const std::string& name,
Start pose,
GetPosition get_position,
std::function<ProcessCompleted(const Eigen::Vector3d pose)> navigate,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. the pose we want the robot to navigate to
  2. 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.

ActionExecutor action_executor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


class Implementation;
private:
EasyFullControl();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use PIMPL pattern

rmf_utils::unique_impl_ptr<Implementation> _pimpl;
};

using EasyFullControlPtr = std::shared_ptr<EasyFullControl>;

} // namespace agv
} // namespace rmf_fleet_adapter

#endif // RMF_FLEET_ADAPTER__AGV__EASYFULLCONTROL_HPP