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

Adding initiator and request time to booking #267

Merged
merged 22 commits into from
Jul 3, 2023

Conversation

aaronchongth
Copy link
Member

@aaronchongth aaronchongth commented Apr 17, 2023

New feature implementation

Implemented feature

Related to open-rmf/rmf-web#702, and requires open-rmf/rmf_task#81 and open-rmf/rmf_api_msgs#35.

  • To use new rmf_task::Request and rmf_task::Task::Booking constructors
  • Propagated request time and requester information to Dispatcher, TaskManager and FleetUpdateHandle
  • Added default argument to set_task_planner_parameter for python bindings of FleetUpdateHandle
  • Updated full control fleet adapter's finishing task factories

@aaronchongth aaronchongth marked this pull request as ready for review May 9, 2023 09:00
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

I left some recommendations to not use steady_clock for timestamps, but once that's changed I'll be happy to approve.

auto get_time =
[]()
{
return std::chrono::steady_clock::now();
Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely do not want steady_clock here. That won't be a meaningful time stamp; it would just be the time since this node began running.

I would recommend using ROS2 time given by the ROS2 node's clock, or using std::chrono::system_clock::now() instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I've made the change to use node time when it's available, otherwise system_clock, 6815ea7

const auto node = n.lock();
if (!node)
{
return std::chrono::steady_clock::now();
Copy link
Contributor

Choose a reason for hiding this comment

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

If the ROS2 node isn't available, system_clock is more likely to give a time that makes sense, so I'd suggest using that instead.

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, I did a tiny workaround to lump system_clock to rmf_traffic::Time which is based on steady_clock. Was there any specific reason we were using steady_clock for rmf_traffic::Time in the past instead of system?
6815ea7

@aaronchongth aaronchongth force-pushed the feat/booking-initiator-request-time branch from c2f20ad to 6815ea7 Compare June 19, 2023 05:36
@aaronchongth aaronchongth requested a review from mxgrey June 19, 2023 07:20
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Hold on, I just noticed something that I'd like to tweak before we merge this.

Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

I've removed the need to pass a node into set_task_planner_params. I believe open-rmf/rmf_demos#180 should be unnecessary now.

@aaronchongth
Copy link
Member Author

Thanks for the changes! The node API is definitely the cleanest way to do it

@aaronchongth aaronchongth removed the request for review from Yadunund July 3, 2023 05:13
Yadunund and others added 9 commits July 3, 2023 05:36
* Fix style for rmf_fleet_adapter

Signed-off-by: Yadunund <[email protected]>

* Make colcon test pass for rmf_traffic_ros2

Signed-off-by: Yadunund <[email protected]>

* Add rmf_fleet_adapter_python to build ci

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Yadunund <[email protected]>
…immediately (#273)

* Put the action finished callback in a schedule instead of triggering immediately

Signed-off-by: Michael X. Grey <[email protected]>

* Style

Signed-off-by: Yadunund <[email protected]>

* Update CHANGELOG

Signed-off-by: Michael X. Grey <[email protected]>

* Move changelog entry into forthcoming

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Co-authored-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@aaronchongth aaronchongth force-pushed the feat/booking-initiator-request-time branch from a99b37e to 6956c0e Compare July 3, 2023 05:36
@aaronchongth aaronchongth merged commit dc740df into main Jul 3, 2023
4 checks passed
@aaronchongth aaronchongth deleted the feat/booking-initiator-request-time branch July 3, 2023 06:14
aaronchongth added a commit that referenced this pull request Jul 3, 2023
* Adding initiator and request time to booking

Signed-off-by: Aaron Chong <[email protected]>

* Use new booking and request API for EmergencyPullover and ResponsiveWait

Signed-off-by: Aaron Chong <[email protected]>

* Using new booking and request API for FleetUpdateHandle

Signed-off-by: Aaron Chong <[email protected]>

* Use requester instead of initiator, use new API

Signed-off-by: Aaron Chong <[email protected]>

* requester name for finishing task factories

Signed-off-by: Aaron Chong <[email protected]>

* Using reverted constructors with nullopt default parameters

Signed-off-by: Aaron Chong <[email protected]>

* Fix build failures on build farm (#274)

* Fix style for rmf_fleet_adapter

Signed-off-by: Yadunund <[email protected]>

* Make colcon test pass for rmf_traffic_ros2

Signed-off-by: Yadunund <[email protected]>

* Add rmf_fleet_adapter_python to build ci

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Yadunund <[email protected]>

* Update changelogs and bump patch (#275)

Signed-off-by: Yadunund <[email protected]>

* Switch to rst changelogs (#276)

Signed-off-by: Yadunund <[email protected]>

* Put the action finished callback in a schedule instead of triggering immediately (#273)

* Put the action finished callback in a schedule instead of triggering immediately

Signed-off-by: Michael X. Grey <[email protected]>

* Style

Signed-off-by: Yadunund <[email protected]>

* Update CHANGELOG

Signed-off-by: Michael X. Grey <[email protected]>

* Move changelog entry into forthcoming

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Co-authored-by: Yadunund <[email protected]>

* Revert changes to constructing finish request factories

Signed-off-by: Aaron Chong <[email protected]>

* Using overloaded TaskPlanner constructor to pass in name of fleet update handle

Signed-off-by: Aaron Chong <[email protected]>

* Using overloaded rmf_task make functions

Signed-off-by: Aaron Chong <[email protected]>

* Update changelogs

Signed-off-by: Yadunund <[email protected]>

* 2.2.0

* Bump 2.3.0 (#282)

Signed-off-by: Yadunund <[email protected]>

* Use new booking and request API, updated legacy FullControl fleet adapter

Signed-off-by: Aaron Chong <[email protected]>

* Added node as parameter to pybinded set_task_planner_params, to pass planner_id and time functor to finishing task factory

Signed-off-by: Aaron Chong <[email protected]>

* Using system_clock instead of steady_clock

Signed-off-by: Aaron Chong <[email protected]>

* Remove the need to pass a node into the set_task_planner_params python binding

Signed-off-by: Michael X. Grey <[email protected]>

---------

Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Co-authored-by: Yadu <[email protected]>
Co-authored-by: Grey <[email protected]>
Co-authored-by: Yadunund <[email protected]>
(cherry picked from commit dc740df)
Signed-off-by: Aaron Chong <[email protected]>
aaronchongth added a commit that referenced this pull request Jul 3, 2023
* Adding initiator and request time to booking

Signed-off-by: Aaron Chong <[email protected]>

* Use new booking and request API for EmergencyPullover and ResponsiveWait

Signed-off-by: Aaron Chong <[email protected]>

* Using new booking and request API for FleetUpdateHandle

Signed-off-by: Aaron Chong <[email protected]>

* Use requester instead of initiator, use new API

Signed-off-by: Aaron Chong <[email protected]>

* requester name for finishing task factories

Signed-off-by: Aaron Chong <[email protected]>

* Using reverted constructors with nullopt default parameters

Signed-off-by: Aaron Chong <[email protected]>

* Fix build failures on build farm (#274)

* Fix style for rmf_fleet_adapter

Signed-off-by: Yadunund <[email protected]>

* Make colcon test pass for rmf_traffic_ros2

Signed-off-by: Yadunund <[email protected]>

* Add rmf_fleet_adapter_python to build ci

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Yadunund <[email protected]>

* Update changelogs and bump patch (#275)

Signed-off-by: Yadunund <[email protected]>

* Switch to rst changelogs (#276)

Signed-off-by: Yadunund <[email protected]>

* Put the action finished callback in a schedule instead of triggering immediately (#273)

* Put the action finished callback in a schedule instead of triggering immediately

Signed-off-by: Michael X. Grey <[email protected]>

* Style

Signed-off-by: Yadunund <[email protected]>

* Update CHANGELOG

Signed-off-by: Michael X. Grey <[email protected]>

* Move changelog entry into forthcoming

Signed-off-by: Yadunund <[email protected]>

---------

Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Co-authored-by: Yadunund <[email protected]>

* Revert changes to constructing finish request factories

Signed-off-by: Aaron Chong <[email protected]>

* Using overloaded TaskPlanner constructor to pass in name of fleet update handle

Signed-off-by: Aaron Chong <[email protected]>

* Using overloaded rmf_task make functions

Signed-off-by: Aaron Chong <[email protected]>

* Update changelogs

Signed-off-by: Yadunund <[email protected]>

* 2.2.0

* Bump 2.3.0 (#282)

Signed-off-by: Yadunund <[email protected]>

* Use new booking and request API, updated legacy FullControl fleet adapter

Signed-off-by: Aaron Chong <[email protected]>

* Added node as parameter to pybinded set_task_planner_params, to pass planner_id and time functor to finishing task factory

Signed-off-by: Aaron Chong <[email protected]>

* Using system_clock instead of steady_clock

Signed-off-by: Aaron Chong <[email protected]>

* Remove the need to pass a node into the set_task_planner_params python binding

Signed-off-by: Michael X. Grey <[email protected]>

---------

Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Co-authored-by: Yadu <[email protected]>
Co-authored-by: Grey <[email protected]>
Co-authored-by: Yadunund <[email protected]>
(cherry picked from commit dc740df)
Signed-off-by: Aaron Chong <[email protected]>
This was referenced Jul 3, 2023
shu-qing added a commit to shu-qing/rmf_ros2 that referenced this pull request Jul 6, 2023
Adding initiator and request time to booking (open-rmf#267)
Yadunund added a commit that referenced this pull request Jul 10, 2023
* Adding initiator and request time to booking



* Use new booking and request API for EmergencyPullover and ResponsiveWait



* Using new booking and request API for FleetUpdateHandle



* Use requester instead of initiator, use new API



* requester name for finishing task factories



* Using reverted constructors with nullopt default parameters



* Fix build failures on build farm (#274)

* Fix style for rmf_fleet_adapter



* Make colcon test pass for rmf_traffic_ros2



* Add rmf_fleet_adapter_python to build ci



---------



* Update changelogs and bump patch (#275)



* Switch to rst changelogs (#276)



* Put the action finished callback in a schedule instead of triggering immediately (#273)

* Put the action finished callback in a schedule instead of triggering immediately



* Style



* Update CHANGELOG



* Move changelog entry into forthcoming



---------





* Revert changes to constructing finish request factories



* Using overloaded TaskPlanner constructor to pass in name of fleet update handle



* Using overloaded rmf_task make functions



* Update changelogs



* 2.2.0

* Bump 2.3.0 (#282)



* Use new booking and request API, updated legacy FullControl fleet adapter



* Added node as parameter to pybinded set_task_planner_params, to pass planner_id and time functor to finishing task factory



* Using system_clock instead of steady_clock



* Remove the need to pass a node into the set_task_planner_params python binding



---------









(cherry picked from commit dc740df)

Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Co-authored-by: Yadu <[email protected]>
Co-authored-by: Grey <[email protected]>
Co-authored-by: Yadunund <[email protected]>
Yadunund added a commit that referenced this pull request Jul 10, 2023
* Adding initiator and request time to booking



* Use new booking and request API for EmergencyPullover and ResponsiveWait



* Using new booking and request API for FleetUpdateHandle



* Use requester instead of initiator, use new API



* requester name for finishing task factories



* Using reverted constructors with nullopt default parameters



* Fix build failures on build farm (#274)

* Fix style for rmf_fleet_adapter



* Make colcon test pass for rmf_traffic_ros2



* Add rmf_fleet_adapter_python to build ci



---------



* Update changelogs and bump patch (#275)



* Switch to rst changelogs (#276)



* Put the action finished callback in a schedule instead of triggering immediately (#273)

* Put the action finished callback in a schedule instead of triggering immediately



* Style



* Update CHANGELOG



* Move changelog entry into forthcoming



---------





* Revert changes to constructing finish request factories



* Using overloaded TaskPlanner constructor to pass in name of fleet update handle



* Using overloaded rmf_task make functions



* Update changelogs



* 2.2.0

* Bump 2.3.0 (#282)



* Use new booking and request API, updated legacy FullControl fleet adapter



* Added node as parameter to pybinded set_task_planner_params, to pass planner_id and time functor to finishing task factory



* Using system_clock instead of steady_clock



* Remove the need to pass a node into the set_task_planner_params python binding



---------









(cherry picked from commit dc740df)

Signed-off-by: Aaron Chong <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Yadunund <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Co-authored-by: Yadu <[email protected]>
Co-authored-by: Grey <[email protected]>
Co-authored-by: Yadunund <[email protected]>
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.

3 participants