Task request time and initiator in Booking #81

aaronchongth · 2023-04-17T01:52:37Z

New feature implementation

Implemented feature

Related to open-rmf/rmf_api_msgs#35,

Adds missing constructor implementation for rmf_task::Request
Adds new constructor for rmf_task::Task::Booking to include task request time and requester
Added new make(...) API for different types of requests
New constructors for request factories that accept a requester input, which will be propagated into each request they create
fixed some misc linting errors

Yadunund

Sorry for the overdue review. Left feedback on the API introduced to discuss further.

rmf_task/include/rmf_task/Task.hpp

Yadunund

Thanks for iterating! This is looking good. Last set of comments from me.

rmf_task/include/rmf_task/requests/ChargeBatteryFactory.hpp

rmf_task/src/rmf_task/requests/Clean.cpp

rmf_task/src/rmf_task/requests/Delivery.cpp

rmf_task/src/rmf_task/requests/Loop.cpp

rmf_task/src/rmf_task/requests/ParkRobotFactory.cpp

rmf_task/include/rmf_task/requests/ParkRobotFactory.hpp

rmf_task/src/rmf_task/Request.cpp

rmf_task/src/rmf_task/TaskPlanner.cpp

rmf_task/src/rmf_task/internal_task_planning.cpp

rmf_task/src/rmf_task/requests/ChargeBattery.cpp

aaronchongth · 2023-06-01T09:47:19Z

Most recent changes,

updated unit tests that use rmf_task::Request
overloaded RequestFactory::make_request to include requester and time, marked the older one as deprecated, we can remove it next time
overloaded TaskPlanner constructor to accept a name, otherwise it uses a default name from it's implementation
propagated changes to all the internal implementations creating automated requests

aaronchongth · 2023-06-13T01:21:35Z

@Yadunund the changes have been made

Yadunund

Thanks for iterating! Apart from the feedback to move task_planner_name into TaskPlanner::Configuration the rest of the suggestions are all nits.

rmf_task/include/rmf_task/TaskPlanner.hpp

rmf_task/include/rmf_task/Task.hpp

rmf_task/include/rmf_task/requests/ChargeBatteryFactory.hpp

rmf_task/include/rmf_task/requests/ChargeBattery.hpp

rmf_task/include/rmf_task/requests/ChargeBatteryFactory.hpp

rmf_task/include/rmf_task/Task.hpp

rmf_task/src/rmf_task/internal_task_planning.hpp

rmf_task/src/rmf_task/requests/ChargeBattery.cpp

Yadunund

LGTM. @mxgrey any feedback for these changes?

aaronchongth · 2023-06-15T05:31:57Z

I'll be holding off of merging till I finish end-to-end testing with rmf_ros2. I'll try to resolve the missing signed commit too

… time, new booking constructor Signed-off-by: Aaron Chong <[email protected]>

…attery and Loop Signed-off-by: Aaron Chong <[email protected]>

…ean and Deliver Signed-off-by: Aaron Chong <[email protected]>

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

…agated changes to all constructors and make functions Signed-off-by: Aaron Chong <[email protected]>

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

mxgrey

LGTM

Good job keeping the API stable with these additional fields!

aaronchongth · 2023-06-16T06:05:52Z

I've finished the changes required with open-rmf/rmf_ros2#267 and open-rmf/rmf_demos#180. Will be merging this

Yadunund · 2023-06-16T07:37:19Z

@aaronchongth could you help backport to iron and humble?

* Fixed missing constructor implementation, commented to deprecate next time, new booking constructor Signed-off-by: Aaron Chong <[email protected]> * Add requester to factories, add request time and initiator to ChargeBattery and Loop Signed-off-by: Aaron Chong <[email protected]> * Using requester instead of initiator, added new make API to legacy Clean and Deliver Signed-off-by: Aaron Chong <[email protected]> * lint Signed-off-by: Aaron Chong <[email protected]> * Revert to original constructor with optional arguments appended, propagated changes to all constructors and make functions Signed-off-by: Aaron Chong <[email protected]> * Move description into booking, lint Signed-off-by: Aaron Chong <[email protected]> * Pass a clock functor to ParkRobotFactory Signed-off-by: Aaron Chong <[email protected]> * Using non-deprecated Request constructor Signed-off-by: Aaron Chong <[email protected]> * new make_request API to include requester and request time Signed-off-by: Aaron Chong <[email protected]> * Overload TaskPlanner constructor with a name argument Signed-off-by: Aaron Chong <[email protected]> * Retained old API and marked as deprecated Signed-off-by: Aaron Chong <[email protected]> * Overloading booking instead of adding default arguments Signed-off-by: Aaron Chong <[email protected]> * Overloading all affected functions with non optional arguments Signed-off-by: Aaron Chong <[email protected]> * Lint Signed-off-by: Aaron Chong <[email protected]> * Revert deprecation tag on Request, to be done in a separate PR Signed-off-by: Aaron Chong <[email protected]> * Argument name changes, linting, documentation Signed-off-by: Aaron Chong <[email protected]> * TaskPlanner constructor to have planner id as first argument Signed-off-by: Aaron Chong <[email protected]> * request factories to accept a callback that returns the current time Signed-off-by: Aaron Chong <[email protected]> --------- Signed-off-by: Aaron Chong <[email protected]> (cherry picked from commit a7875a9) Signed-off-by: Aaron Chong <[email protected]>

* Task request time and initiator in Booking (#81) * Fixed missing constructor implementation, commented to deprecate next time, new booking constructor Signed-off-by: Aaron Chong <[email protected]> * Add requester to factories, add request time and initiator to ChargeBattery and Loop Signed-off-by: Aaron Chong <[email protected]> * Using requester instead of initiator, added new make API to legacy Clean and Deliver Signed-off-by: Aaron Chong <[email protected]> * lint Signed-off-by: Aaron Chong <[email protected]> * Revert to original constructor with optional arguments appended, propagated changes to all constructors and make functions Signed-off-by: Aaron Chong <[email protected]> * Move description into booking, lint Signed-off-by: Aaron Chong <[email protected]> * Pass a clock functor to ParkRobotFactory Signed-off-by: Aaron Chong <[email protected]> * Using non-deprecated Request constructor Signed-off-by: Aaron Chong <[email protected]> * new make_request API to include requester and request time Signed-off-by: Aaron Chong <[email protected]> * Overload TaskPlanner constructor with a name argument Signed-off-by: Aaron Chong <[email protected]> * Retained old API and marked as deprecated Signed-off-by: Aaron Chong <[email protected]> * Overloading booking instead of adding default arguments Signed-off-by: Aaron Chong <[email protected]> * Overloading all affected functions with non optional arguments Signed-off-by: Aaron Chong <[email protected]> * Lint Signed-off-by: Aaron Chong <[email protected]> * Revert deprecation tag on Request, to be done in a separate PR Signed-off-by: Aaron Chong <[email protected]> * Argument name changes, linting, documentation Signed-off-by: Aaron Chong <[email protected]> * TaskPlanner constructor to have planner id as first argument Signed-off-by: Aaron Chong <[email protected]> * request factories to accept a callback that returns the current time Signed-off-by: Aaron Chong <[email protected]> --------- Signed-off-by: Aaron Chong <[email protected]> (cherry picked from commit a7875a9) Signed-off-by: Aaron Chong <[email protected]> * Add rmf_task_sequence to workflows, lint (#91) Signed-off-by: Aaron Chong <[email protected]> (cherry picked from commit 40f3e92) Signed-off-by: Aaron Chong <[email protected]> * Only running iron jammy workflows Signed-off-by: Aaron Chong <[email protected]> --------- Signed-off-by: Aaron Chong <[email protected]>

* Task request time and initiator in Booking (#81) * Fixed missing constructor implementation, commented to deprecate next time, new booking constructor Signed-off-by: Aaron Chong <[email protected]> * Add requester to factories, add request time and initiator to ChargeBattery and Loop Signed-off-by: Aaron Chong <[email protected]> * Using requester instead of initiator, added new make API to legacy Clean and Deliver Signed-off-by: Aaron Chong <[email protected]> * lint Signed-off-by: Aaron Chong <[email protected]> * Revert to original constructor with optional arguments appended, propagated changes to all constructors and make functions Signed-off-by: Aaron Chong <[email protected]> * Move description into booking, lint Signed-off-by: Aaron Chong <[email protected]> * Pass a clock functor to ParkRobotFactory Signed-off-by: Aaron Chong <[email protected]> * Using non-deprecated Request constructor Signed-off-by: Aaron Chong <[email protected]> * new make_request API to include requester and request time Signed-off-by: Aaron Chong <[email protected]> * Overload TaskPlanner constructor with a name argument Signed-off-by: Aaron Chong <[email protected]> * Retained old API and marked as deprecated Signed-off-by: Aaron Chong <[email protected]> * Overloading booking instead of adding default arguments Signed-off-by: Aaron Chong <[email protected]> * Overloading all affected functions with non optional arguments Signed-off-by: Aaron Chong <[email protected]> * Lint Signed-off-by: Aaron Chong <[email protected]> * Revert deprecation tag on Request, to be done in a separate PR Signed-off-by: Aaron Chong <[email protected]> * Argument name changes, linting, documentation Signed-off-by: Aaron Chong <[email protected]> * TaskPlanner constructor to have planner id as first argument Signed-off-by: Aaron Chong <[email protected]> * request factories to accept a callback that returns the current time Signed-off-by: Aaron Chong <[email protected]> --------- Signed-off-by: Aaron Chong <[email protected]> (cherry picked from commit a7875a9) Signed-off-by: Aaron Chong <[email protected]> * Add rmf_task_sequence to workflows, lint (#91) Signed-off-by: Aaron Chong <[email protected]> (cherry picked from commit 40f3e92) Signed-off-by: Aaron Chong <[email protected]> * Running only humble jammy workflows Signed-off-by: Aaron Chong <[email protected]> --------- Signed-off-by: Aaron Chong <[email protected]>

aaronchongth requested review from mxgrey and Yadunund April 17, 2023 01:53

aaronchongth mentioned this pull request Apr 17, 2023

Adding initiator and request time to booking open-rmf/rmf_ros2#267

Merged

aaronchongth force-pushed the feat/book-with-time-and-user branch 2 times, most recently from 271183a to 6975638 Compare May 9, 2023 09:33

aaronchongth mentioned this pull request May 9, 2023

Port/date initiator columns open-rmf/rmf-web#702

Merged

5 tasks

aaronchongth force-pushed the feat/book-with-time-and-user branch from 6975638 to 89225c7 Compare May 9, 2023 10:02

Yadunund reviewed May 29, 2023

View reviewed changes

rmf_task/include/rmf_task/Task.hpp Outdated Show resolved Hide resolved

rmf_task/include/rmf_task/Task.hpp Outdated Show resolved Hide resolved

Yadunund requested changes May 31, 2023

View reviewed changes

aaronchongth requested a review from Yadunund June 1, 2023 09:47

Yadunund requested changes Jun 14, 2023

View reviewed changes

aaronchongth requested a review from Yadunund June 14, 2023 17:10

Yadunund approved these changes Jun 15, 2023

View reviewed changes

aaronchongth added 14 commits June 15, 2023 06:52

Fixed missing constructor implementation, commented to deprecate next…

04d30f2

… time, new booking constructor Signed-off-by: Aaron Chong <[email protected]>

Add requester to factories, add request time and initiator to ChargeB…

85b6d88

…attery and Loop Signed-off-by: Aaron Chong <[email protected]>

Using requester instead of initiator, added new make API to legacy Cl…

46c7520

…ean and Deliver Signed-off-by: Aaron Chong <[email protected]>

lint

d727898

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

Revert to original constructor with optional arguments appended, prop…

c702c9b

…agated changes to all constructors and make functions Signed-off-by: Aaron Chong <[email protected]>

Move description into booking, lint

80518e4

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

Pass a clock functor to ParkRobotFactory

3a87fc9

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

Using non-deprecated Request constructor

c4fe0a9

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

new make_request API to include requester and request time

9911e73

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

Overload TaskPlanner constructor with a name argument

4567762

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

Retained old API and marked as deprecated

14e1e1c

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

Overloading booking instead of adding default arguments

e6e0a7e

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

Overloading all affected functions with non optional arguments

cfd63bb

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

Lint

4df00c6

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

aaronchongth added 4 commits June 15, 2023 06:52

Revert deprecation tag on Request, to be done in a separate PR

2c29ba0

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

Argument name changes, linting, documentation

0370e5a

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

TaskPlanner constructor to have planner id as first argument

5029715

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

request factories to accept a callback that returns the current time

ff5f0ec

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

aaronchongth force-pushed the feat/book-with-time-and-user branch from 564e5a3 to ff5f0ec Compare June 15, 2023 06:53

mxgrey approved these changes Jun 15, 2023

View reviewed changes

aaronchongth merged commit a7875a9 into main Jun 16, 2023

aaronchongth deleted the feat/book-with-time-and-user branch June 16, 2023 06:06

This was referenced Jun 16, 2023

Task request time and initiator in Booking (#81) #89

Merged

Task request time and initiator in Booking (#81) #90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task request time and initiator in Booking #81

Task request time and initiator in Booking #81

aaronchongth commented Apr 17, 2023 •

edited

Loading

Yadunund left a comment

Yadunund left a comment

aaronchongth commented Jun 1, 2023

aaronchongth commented Jun 13, 2023

Yadunund left a comment

Yadunund left a comment

aaronchongth commented Jun 15, 2023

mxgrey left a comment

aaronchongth commented Jun 16, 2023

Yadunund commented Jun 16, 2023

Task request time and initiator in Booking #81

Task request time and initiator in Booking #81

Conversation

aaronchongth commented Apr 17, 2023 • edited Loading

New feature implementation

Implemented feature

Yadunund left a comment

Choose a reason for hiding this comment

Yadunund left a comment

Choose a reason for hiding this comment

aaronchongth commented Jun 1, 2023

aaronchongth commented Jun 13, 2023

Yadunund left a comment

Choose a reason for hiding this comment

Yadunund left a comment

Choose a reason for hiding this comment

aaronchongth commented Jun 15, 2023

mxgrey left a comment

Choose a reason for hiding this comment

aaronchongth commented Jun 16, 2023

Yadunund commented Jun 16, 2023

aaronchongth commented Apr 17, 2023 •

edited

Loading