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

Reactive easy full control #286

Merged
merged 22 commits into from
Aug 14, 2023

Conversation

mxgrey
Copy link
Contributor

@mxgrey mxgrey commented Jul 12, 2023

This PR significantly revises the implementation of the EasyFullControl API by making it reactive instead of running in a thread. It also tweaks the API itself in a way that I believe is much simpler for system integrators, especially for users of the Python bindings. I've also revised the API to be future-proof by using more PIMPL classes instead of basic data types.

In the process of testing this new API, I encountered pre-existing several bugs that had been reported before but were difficult to reproduce. This PR includes fixes for the following issues:

I'm creating this as a separate PR that targets #235 so that reviewers can have a more clear idea of what changes are being introduced and why.

To test out this PR, it should be used in conjunction with open-rmf/rmf_demos#188

mxgrey added 19 commits June 13, 2023 20:37
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
…trol API

Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@xiyuoh
Copy link
Member

xiyuoh commented Jul 17, 2023

Thanks for the revamp! Still testing the fleet adapter in various worlds and scenarios, so far it's looking really great.

A TODO that I found necessary in one of our use cases was making max_merge_waypoint_distance and max_merge_lane_distance configurable from the fleet's config.yaml, and then passing it to FleetConfiguration. Wondering if it's worth to add it at this time, happy to open a PR to this branch (or to the feature/easy_full_control branch after this PR is merged).

@mxgrey
Copy link
Contributor Author

mxgrey commented Jul 17, 2023

Sure, we can expose that in this PR. We might want to consider thinking of names that make their purpose more clear, but nothing comes to mind for me at the moment.

I'd suggest implementing it similarly to responsive_wait where there's a fleet wide default value and then a robot-specific override. If neither is specified then we fall back on the current hard-coded defaults.

For changing the values live we can add APIs to EasyRobotUpdateHandle. Currently the base RobotUpdateHandle doesn't use these parameters, so I don't think it would make sense to put them there.

Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Xiyu Oh <xiyu@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Co-authored-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey mxgrey merged commit 67079c2 into feature/easy_full_control Aug 14, 2023
2 of 4 checks passed
@mxgrey mxgrey deleted the reactive_easy_full_control branch August 14, 2023 06:53
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