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

Fleet adapter and PerformAction tutorial #105

Merged
merged 10 commits into from
Nov 9, 2023
Merged

Conversation

xiyuoh
Copy link
Member

@xiyuoh xiyuoh commented Oct 3, 2023

This PR updates the existing fleet adapter tutorial based on the Easy API introduced in open-rmf/rmf_ros2#235. It also adds a tutorial for integrating custom performable actions using the Easy API.

This should be merged after open-rmf/fleet_adapter_template#22 as the template should be provided as a base for readers to work off from, and the links will direct readers to the main branch.

@arjo129 arjo129 self-requested a review October 5, 2023 23:57
Copy link
Member

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

I went through the tutorials. The perform action stuff is fine. The fleet adapter tutorial has few minor typos/issues with it.

## 1. Pre-requisites

### Fetch dependencies

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 note about installing RMF. Link it to some section.

- Fleet `config.yaml` file
- Navigation graph

### a. Install ROS 2 and sRMF
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### a. Install ROS 2 and sRMF
### a. Install ROS 2 and RMF

Move this to the top with pre-requisites. I don't think there is a need to repeat ourselves


Since the `rmf_demos_fleet_adapter` uses REST API as an interface between the fleet adapter and robot fleet manager, we will need to install the required dependencies to use FastAPI.
```bash
pip3 install fastapi uvicorn
Copy link
Member

Choose a reason for hiding this comment

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

Reading the whole tutorial, I'm a bit confused why we need this. Sure FastAPI and Uvicorn are useful for creating fleet managers. But do we use it in the fleet adapter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, added an explanation to explicitly mention that this step is only required to get the demos working.


### Fetch dependencies

Since the `rmf_demos_fleet_adapter` uses REST API as an interface between the fleet adapter and robot fleet manager, we will need to install the required dependencies to use FastAPI.
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth explaining the difference between a robot fleet manager and a fleet adapter.
i imagine the fastapi dependency is for the fleet manager and not fleet adapter.

Copy link
Member Author

Choose a reason for hiding this comment

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

The explanation is available in the previous section of the ROS 2 Book.

)
```

Then, in our `main` function, we at the computed transforms to our fleet configuration. The EasyFullControl fleet adapter will process these transforms and send out navigation commands in the robot's coordinates accordingly.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Then, in our `main` function, we at the computed transforms to our fleet configuration. The EasyFullControl fleet adapter will process these transforms and send out navigation commands in the robot's coordinates accordingly.
Then, in our `main` function, we add the computed transforms from our fleet configuration. The EasyFullControl fleet adapter will process these transforms and send out navigation commands in the robot's coordinates accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected the first typo, but the second edit is not a typo, fleet configuration is an object and we are adding the transforms to it. Modified it to FleetConfiguration for clarity.

We have defined a helper function to compute the transforms between RMF and the robot's coordinates. In the event your robot operates in the same coordinates as RMF (e.g. in simulation), you won't need this portion of the code.

```python
def compute_transforms(level, coords, node=None):
Copy link
Member

Choose a reason for hiding this comment

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

explain the role of level and why we are not using it. Also explain the role of node

Copy link
Member Author

Choose a reason for hiding this comment

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


### c. Initialize the robot API and set up RobotAdapter

The `config.yaml` should include any connection credentials we'd need to connect to our robot or robot fleet manager. We parse this to the `RobotAPI` to easily interact between RMF and the robot's API.
Copy link
Member

@arjo129 arjo129 Oct 9, 2023

Choose a reason for hiding this comment

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

I'm kind of surprised we are doing this. I always thought credentials should be seperate from configs so that they don't get checked in to a public git version control repository. I guess on private networks its fine. However, in a tutorial I would advise against recommending storing credentials in a config file. Or put in a warning asking a user to do a threat assessment first.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only for this example, added a note on that in aadb22d


Notice that `execute_action(~)` does not have any implemented code in the fleet adapter template. This callback is designed to be flexible and caters to custom performable actions that may not be availble under the tasks offered in RMF. You can learn how to design and compose your own actions and execute them from the fleet adapter in the [PerformAction tutorial](./integration_fleets_action_tutorial.md) section.

```python
Copy link
Member

Choose a reason for hiding this comment

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

What is this code block about?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is just to add all the callbacks at one go, added in aadb22d

Signed-off-by: Xiyu Oh <[email protected]>
@arjo129 arjo129 merged commit 0810513 into master Nov 9, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants