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.

Sign up for GitHub

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

Jump to bottom

Add Event Descriptions #46

Open
wants to merge 90 commits into
base: main
Choose a base branch
from
Open

Add Event Descriptions #46

wants to merge 90 commits into from

Conversation

Yadunund
Copy link
Member

This PR adds Descriptions for items listed in #43

@mxgrey
Remove agv namespace and introduce CompositeData
29f3b8c 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Redesign State to be composable
2ea66b5 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Defining task execution interfaces
b181017 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Adding a task logging interface
096ae8a 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Drafting the interfaces for phase sequence tasks
86c3c13 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Satisfy uncrustify
e00b02c 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Draft interface for Task Factory
7a7d7d3 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Rework the design, improve consistency
15aac52 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Satisfy uncrustify
873693a 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Beginning to implement GoToPlace::Description
67f9398 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Redesigning travel estimation cache
68f2fda 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Redesigning travel estimation
1707f4f 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Prototyping PickUp phase
45d62a6 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Finish implementation of PickUp and begin API of DropOff
81380c8 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Implementing DropOff phase
21054b9 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Reorganizing
3b83b5d 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
More reorganization
7c31994 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Gradually ironing out the transition to the reorganization
9103c90 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Continuing implementation of phase sequence task
66777a8 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Thinking about how to implement backups for phase sequence tasks
820cc33 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Work reactive backup callbacks into the system
5e14e82 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Creating minimal example of task activation
e330d36 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Create minimal example of task restoring
d2911aa 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Use derived class implementation of backup
3505ac0 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Finish comment
724eb0a 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Creating an integration test to guide the implementation of backups
ecbe258 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Move backups test into integration folder
37830d5 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Continuing to implement backup and restore for phase sequence tasks
56c1f40 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Finished backup/restore implementation -- needs testing
ed651ae 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Implementing phase activator
e1c2d51 
Signed-off-by: Michael X. Grey <[email protected]>
@Yadunund Yadunund marked this pull request as ready for review November 17, 2021 10:31
@Yadunund
Copy link
Member Author

@mxgrey this is ready for review. There were some linker errors when compiling and hence I had to implement rmf_task_sequence::detail::Backup and some inherited functions in rmf_task_sequence::Task. For the latter I have left most of it as TODOs.

@mxgrey mxgrey self-requested a review November 18, 2021 03:11
@mxgrey mxgrey self-assigned this Nov 25, 2021
@mxgrey
tweaks
3610da5 
Signed-off-by: Michael X. Grey <[email protected]>
mxgrey
mxgrey reviewed Dec 2, 2021
View reviewed changes
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.

This is only a partial review with some comments of things that I think could be tweaked.

Overall this PR is looking good.

rmf_task_sequence/include/rmf_task_sequence/detail/ContactCard.hpp Outdated

//==============================================================================
/// Store contact details for telecommunication
class ContactCard
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why this class would be in a detail namespace. If it gets touched by a user of the library then it should be in a normal public API namespace. If it doesn't get touched by a user of the library then we should keep it inside of src/ and we don't need to pimpl it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to events namespace fb34755

rmf_task_sequence/include/rmf_task_sequence/detail/ContactCard.hpp Outdated
{
public:

struct PhoneNumber
Copy link
Contributor

Choose a reason for hiding this comment

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

We should pimpl this class if it's in the public API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done fb34755

rmf_task_sequence/include/rmf_task_sequence/detail/ContactCard.hpp Outdated
const std::string& name,
const std::string& address,
const std::string& email,
PhoneNumber number);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if some of these fields should be optional. I suppose an empty string could imply that it's unknown, but I always prefer having the "null type" as an entirely separate object type for explicitness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Made address and email optional fb34755

rmf_task_sequence/include/rmf_task_sequence/events/Call.hpp Outdated
using DescriptionPtr = std::shared_ptr<Description>;
using ConstDescriptionPtr = std::shared_ptr<const Description>;

class Model;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a custom Call::Model we can just use WaitFor::Description::make_model and return its result for Call::Description::make_model.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using WaitFor::Model for Call and SMS fb34755

rmf_task_sequence/include/rmf_task_sequence/events/PerformAction.hpp Outdated

/// Make a PerformAction description.
///
/// \param[in] action_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of an action name, I wonder if we should just accept an entire JSON payload to pass along to the command handle, and allow either side of the pipeline to do whatever they want with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done fb34755

mxgrey and others added 17 commits December 7, 2021 13:41
@mxgrey
Refactor Sequence into Bundle
10fd78a 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Filling in implementation gaps and beginning task sequence test
2314e14 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Fix implementation bugs and begin unit tests for task sequences
1dd1659 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Allow event bundles to be activated from a vector of Standbys
d11032e 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Allow users to directly create Bundle Standbys, not Actives
cc8bf3d 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Implement unfolding for tasks and events
7420e52 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Remember to include implementation header
9842bd9 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Make activate and restore const
0112e25 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Add sequence numbers to logs and fix log iteration
070e864 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Add plain text info for task descriptions
3aef75f 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Handle cases with no ambient drain
416503d 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Create a Placeholder event to help eliminate boilerplate
f05dda0 
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Fix circular reference
9fbd9d4 
Signed-off-by: Michael X. Grey <[email protected]>
@Yadunund
Pimplify PhoneNumber. Reuse WaitFor model
fb34755 
Signed-off-by: Yadunund <[email protected]>
@mxgrey
Return more specific Clean Description
113b671 
Signed-off-by: Michael X. Grey <[email protected]>
@Yadunund
Merge branch 'redesign_v2' into new_descriptions
d37c5cc
@Yadunund
Add missing files
11bf281 
Signed-off-by: Yadunund <[email protected]>
@Yadunund Yadunund mentioned this pull request Jan 15, 2022
Merged
Base automatically changed from redesign_v2 to main February 13, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxgrey mxgrey mxgrey left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@mxgrey mxgrey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Yadunund @mxgrey @cnboonhan