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

Add lookahead behavior for GoToPlace and fix various sources of seg faults #205

Merged
merged 25 commits into from
May 17, 2022

Conversation

mxgrey
Copy link
Contributor

@mxgrey mxgrey commented May 5, 2022

This PR came from some rapid development cycles while getting a demo to work, so it's a mixed bag of changes.

  • Adds a traffic schedule lookahead ability for GoToPlace, which helps to smooth out traffic negotiations for tasks that involve passing through a sequence of waypoints (e.g. patrolling)
  • Removes stdout/stderr redirect for the python bindings, since that was found to cause segfaults (this is a known issue which should be fixed in newer versions of pybind11: pythonbuf fix pybind/pybind11#2675)
  • Fixes a logic error for the cancel + kill API which prevented those APIs from working unless the user provides a callback
  • Provides the most general possible update_position overload to give system integrators maximum flexibility in how they report their robot's position

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>
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>
@mxgrey mxgrey requested a review from aaronchongth May 5, 2022 06:04
Base automatically changed from wait_until to main May 6, 2022 07:17
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #205 (46d1cac) into main (212225e) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #205      +/-   ##
==========================================
- Coverage   17.91%   17.81%   -0.10%     
==========================================
  Files         420      844     +424     
  Lines       38606    77812   +39206     
  Branches    18500    37372   +18872     
==========================================
+ Hits         6915    13860    +6945     
- Misses      24283    49054   +24771     
- Partials     7408    14898    +7490     
Flag Coverage Δ
tests 17.81% <ø> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...os2/rmf_fleet_adapter/src/lift_supervisor/main.cpp
...f_task_ros2/src/rmf_task_ros2/bidding/Response.cpp
...RxCpp-4.1.0/Rx/v2/src/rxcpp/sources/rx-iterate.hpp
...RxCpp-4.1.0/Rx/v2/src/rxcpp/sources/rx-iterate.hpp
...adapter/src/rmf_fleet_adapter/phases/DockRobot.hpp
...ter/src/rmf_fleet_adapter/events/PerformAction.cpp
...s2/src/rmf_traffic_ros2/schedule/convert_Patch.cpp
.../rmf_fleet_adapter/test/phases/test_IngestItem.cpp
...xCpp-4.1.0/Rx/v2/src/rxcpp/sources/rx-interval.hpp
...ter/src/rmf_fleet_adapter/phases/WaitForCharge.cpp
... and 1254 more

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

These features look great! The refactoring in Task manager also makes it easier to go through, thanks!

I just have 2 pretty trivial comments, but otherwise LGTM! Should we be updating the change logs for these packages as well? You can also mention the newly added API for cancelling or killing tasks from RobotUpdateHandle, those were extremely useful ✨

pending_json["unix_millis_start_time"] =
to_millis(pending.deployment_time().time_since_epoch()).count();
copy_assignment(pending_json["assigned_to"], *_context);
pending_json["status"] = "canceled";
Copy link
Member

Choose a reason for hiding this comment

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

Got confused by this for a while and thought it was a typo, we might need to converge to single or double L at some point, I found multiple locations where we use double Ls, https://github.com/open-rmf/rmf_api_msgs/blob/main/rmf_api_msgs/schemas/task_state.json#L51.

This should not block merging this PR though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the inconsistency between the number of ls in canceled versus cancellation probably creates a high risk of typos, but I chose those spellings as they're the more common spellings in American English, which is usually preferred over British spellings in the global software industry. I'm open to revising the API if people anticipate that it will cause problems.

// std::cout << "No deps for " << description->name()
// << " " << plan_id << "|" << r << " vs "
// << vc->description.name() << " " << vc->plan_id << "|" << vc->route_id
// << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Are these comments still useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that, comments are removed in 9433066

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

On second thought, since these are using features in 2.1.0 of rmf_task and rmf_task_sequence, I'd say we should probably consider bumping a minor release to match as well. I'm not super well versed in our versioning workflow, let me know what you think.

@youliangtan
Copy link
Member

youliangtan commented May 9, 2022

I tested a few scenarios, with queue task cancellation and the new generic update_position(startset) API for some demo use cases. All works perfectly 👍

Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey
Copy link
Contributor Author

mxgrey commented May 17, 2022

@aaronchongth great idea, CHANGELOG is updated in 46d1cac. Let me know if there's anything else to block this PR.

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for the changes fixes! 🙇LGTM!

@mxgrey mxgrey merged commit b1676a0 into main May 17, 2022
@mxgrey mxgrey deleted the demo_experiments branch May 17, 2022 08:15
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.

None yet

3 participants