search

open-rmf / rmf_task

RMF library for managing task allocations
Apache License 2.0
22 stars 19 forks source link

update ParkRobotFactory to read and use robot's dedicated parking waypoint; load parking waypoint to robot state #116

Open CarlyyyChen opened 2 months ago

CarlyyyChen commented 2 months ago

Bug fix

Fixed bug

Currently if we set the parking waypoint of a robot to a waypoint that is not a charger, and configure the finishing request as "park", the robot will still go back to its charger, instead of its parking spot, after finishing a task. This bug is raised in issue https://github.com/open-rmf/rmf_demos/issues/219 and https://github.com/open-rmf/rmf_task/issues/115.

Fix applied

This issue happens because when we generate a parking request for the robot, we did not pass in a parking spot waypoint, and therefore the robot will head to its charger as default. My approach is the similar as suggested by Yadunund in https://github.com/open-rmf/rmf_task/issues/115 (the only difference is because I am on the humble branch instead of the main branch).

Basically, the Fleet adapter will read the dedicated parking waypoint for each robot from the configuration file, pass that waypoint to RobotCommandHandle and RobotUpdateHandle so that this waypoint will be set as the dedicated parking waypoint for each robot. When sending requests to robots, fleet adapter needs to know each robot's State, which can be retrieved through RobotContext. The robots' current state with a dedicated parking waypoint will be sent back to the fleet adapter. When creating finishing request, if the finishing request is configured as "park", fleet adapter will call the ParkRobotFactory class to use that parking waypoint as a finishing waypoint.

To implement the fix, I need to make changes to three repos: rmf_demos, rmf_task, and rmf_ros2. And therefore I have three pull requests. Please review them together.

Kindly noted that there is a version discrepancy in rmf_task/Task/Booking on humble branch as it is not consistent with the version being used in other packages. This is causing the issue I mentioned in https://github.com/open-rmf/rmf_task/issues/115. And therefore I have revert back the version of Booking class here as well.

mxgrey commented 2 months ago

Thanks for the set of PRs for adding this new capability.

I'm in a bit of a pickle about this, because the ROS release policy is to not add new features to already-released packages. Any new updates are supposed to be bug fixes only.

I understand that you're using the humble branches for your own work, but if you would be willing to target your changes at the main branches then there would be less friction for accepting the changes.

For what it's worth, the main branches are all still compatible with the Humble release of ROS if you're willing to build them from source on top of an installation of humble.

I'll look more into the linking error that you're experiencing with the Booking argument. I think that argument was added after the original release of the humble branches, which does risk breaking ABI, but I wasn't expecting downstream users to be using that function, and I thought the ABI would be okay if all the branches were updated on the buildfarm simultaneously.

CarlyyyChen commented 2 months ago

Hi Grey, thanks for the feedback. However, this set of PRs are more of bug fix instead of adding new features, as it fixes the bug mentioned in https://github.com/open-rmf/rmf_task/issues/115. Currently if user configures the finishing request as "park" it would not work as the robot will always go back to charger.

mxgrey commented 2 months ago

I'm open to considering this a bug fix since there shouldn't need to be any break to the API or ABI for this, but we'll need to figure out why the Booking argument is causing linking problems for you. I'll test some things on my end, and after that I may need to ask you to run some tests on your end to figure out why you've been getting those linking errors.

CarlyyyChen commented 2 months ago

I think this issue is caused by the version discrepancy. If I ignore the latest commit pushed to the Task::Booking class (which is about adding labels to it), then the issue is gone. I guess it is because other packages/classes on humble who are using this Booking class is still referencing to its old version.

mxgrey commented 2 months ago

Right I understand which commit is causing the problem but it shouldn't matter if

  1. You're using binaries for humble which are up to date, and/or
  2. You're using the latest humble branches across all the repos.

If both of those are okay but you're still getting this linking error then we need to fix the released humble binaries.