-
Notifications
You must be signed in to change notification settings - Fork 542
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
Python Bindings - moveit_py #1546
Conversation
This pull request is in conflict. Could you fix it @peterdavidfagan? |
88110be
to
6674d4c
Compare
This pull request is in conflict. Could you fix it @peterdavidfagan? |
You marked this as WIP. Is it ready to review/merge? Do you mind rebasing it so we can run CI? |
Hi @tylerjw
It is not ready in its current form, I spoke with @henningkayser in our last check in and the goal is to have it prepared for review by this Sunday.
I will perform a rebase as requested when working on this later today. |
840f5ae
to
253e4f0
Compare
Hi @tylerjw I wish to push some changes today to the overall structure of the project. I will ping you here once it is ready for review. |
f604382
to
5060475
Compare
I am starting to revise testing of the current functionalities of the library (there is still a bit to cover). For integration test cases, I am now encountering an issue with the simple_controller_manager timing out.
I have seen that there were some recent changes to the I am also seeing some messages relating to kinematics solvers being initialized:
The above appears to be tracked in the following issue. I need to work on a presentation this evening so it will be tomorrow before I can return to this but I plan to dedicate ~2 hours tomorrow to continue adding tests and cleaning up this pr. |
Hi @tylerjw, I made some updates to move some of the binding code out of the files I don't think the library is ready (lots of TODO comments and improvements that need to be made) but if people wish to start reviewing the code I will work on any suggestions that are made during the hours I have dedicated in my current schedule to MoveIt. The strict deadline from GSoC for my code to be finalized is November 26th. |
The only change I'm aware of recently is a renaming. This is actually an optional renaming with a proper deprecation warning, so it shouldn't fail if you don't make the change. But you should go ahead and make the text replacement anyway:
I don't know think that would have anything to do with "waitForExecution timed out". The timeout sounds like an action response is getting lost in the middleware somehow. Are you using CycloneDDS? |
Thanks for the quick response on this @AndyZe, I haven't altered the
I did test changing the The README instructions for the
|
moveit_py/src/moveit_py/moveit_ros/planning_scene_monitor/planning_scene_monitor.cpp
Outdated
Show resolved
Hide resolved
moveit_py/src/moveit_py/moveit_ros/planning_scene_monitor/planning_scene_monitor.cpp
Outdated
Show resolved
Hide resolved
void PlanningSceneMonitor::applyPlanningSceneServiceCallback(std::shared_ptr<moveit_msgs::srv::ApplyPlanningScene::Request> req, | ||
std::shared_ptr<moveit_msgs::srv::ApplyPlanningScene::Response> res) | ||
{ | ||
updateFrameTransforms(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be necessary to update the transforms. Also, it's important to notice that newPlanningSceneMessage()
only slots a new update event, the actual change will be applied after this service call has returned successfully (and might still fail). If you want to apply the planning scene immediately, you'd have to call PlanningScene::usePlanningSceneMsg()
while locking the scene.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@henningkayser should I remove the apply planning scene service from the services provided by the planning scene monitor? I think this service was only leveraged by the planning_scene_interface package which we no longer include in the bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and removed the apply planning scene service functionality, please let me know if you agree with this 790223c.
Is there any good way to test this, or should I just looks at the tests for usage? (Edit: I found the moveit2_tutorials PR. I'll add a link to the PR description) |
Do you mean the entire set of functionalities of the library? I am looking to increase test coverage by adding unit tests here. The following has examples of usage of the library: https://github.com/peterdavidfagan/moveit_py_example. I will open up a pr for moveit2_tutorials with similar content to the above linked repository. |
Planning to a pose with |
This pull request is in conflict. Could you fix it @peterdavidfagan? |
1 similar comment
This pull request is in conflict. Could you fix it @peterdavidfagan? |
790223c
to
01d5dc8
Compare
ad3654a
to
de4092b
Compare
Hi, I have a separate workspace to build moveit2 from source, and I was able to build it on branch humble. Is there another repository where I should change branches? Thanks, |
You need to update your moveit_task_constructor's ros2 branch |
Also be sure to uninstall preexisting debians as these may also cause this error (if you installed the debian for moveit_task_constructor by mistake):
|
Thanks for the fast answer. Indeed I had 2 repositories that needed to be updated since last thursday. I deleted the build folders and made a full build, I have different yet similar errors:
Any idea? My workspace has: Best, Edit: I changed by hand the few instances of this problem (trajectory_ and error_code_) and now it compiles. |
@@ -308,6 +308,7 @@ bool PlanningComponent::setStartState(const std::string& start_state_name) | |||
return false; | |||
} | |||
moveit::core::RobotState start_state(moveit_cpp_->getRobotModel()); | |||
start_state.setToDefaultValues(); // required to ensure all joints are initialized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this's needed, what if the current state of the gripper is open, wouldn't this cause the planner to plan with that state rather than the actual one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was encountering a segfault without initializing all joints to default values. There's a related discussion on Discord for this here and a related issue here. Without this line of code some joints remain uninitialised if they are not part of the planning_group you set the start state for.
Yes this may be an issue, my understanding is that we assume that the user only cares about the given planning group? They can always define another planning group to set values for all joints they care about. Maybe this requires an update to the documentation to highlight this point? If you have another suggestion I would be happy to work on it.
For Reference:
Default value definition from API docs: "Set all joints to their default positions. The default position is 0, or if that is not within bounds then half way between min and max bound."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JafarAbdi do you think it is reasonable to resolve this item with better documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to use this in multiple places makes me think that we have a deeper bug, I'm more than happy to debug it if you have a small reproducible example.
I think we should merge this PR and handle that issue in a separate one!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually you would want to copy and modify some sort of 'current' RobotState
instead of creating a new one.
(I haven't looked at this code, just read the discussion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should merge this PR and handle that issue in a separate one!
Happy to open up a separate issue for this and address it there.
Usually you would want to copy and modify some sort of 'current' RobotState instead of creating a new one.
(I haven't looked at this code, just read the discussion)
This makes sense, currently we call the getRobotState on the moveit_cpp instance to retrieve the robot state before setting the state of joints in a given planning_group. Joints not within the planning_group we are setting sometimes have uninitialised values which results in the errors we saw in the past :(.
@v4hn suggested the following in the past:
but it's really bad that we do not have proper error messages for these cases :/
So if these initializations/updates fix your problem consider writing a small patch that yields a proper error message (or just works) when someone tries to do it again
I think having a proper error messages is important, it is likely bad to assume that the user only cares about the state of the planning_group when setting the start state. We should probably check if joints outside the planning_group are uninitialised and print some sort of message to notify the user that we set them to default values as they were uninitialised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened the following issue.
It might also be worth making @henningkayser aware of this as it relates to moveit_cpp
.
I don't have capacity to work on this today as I have to focus on writing today, I would be happy to pick this up next week though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this solution is perfectly fine. The values are not initialized to default values because of performance reasons. In Python this doesn't exist, afaik, so always using default values is the way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve this as things work for me, but I have a few minor comments that would be nice to get in. Hopefully they don't take long.
There are also still some lingering comments in moveit_py/src/moveit/moveit_core/planning_scene/planning_scene.cpp
, moveit_py/src/moveit/moveit_ros/moveit_cpp/moveit_cpp.cpp
, and `moveit_py/src/moveit/moveit_ros/moveit_cpp/planning_component.cpp, from my last reviews. They are all related to docstrings, but still would be good to get in.
moveit_py/src/moveit/moveit_core/kinematic_constraints/utils.cpp
Outdated
Show resolved
Hide resolved
Check if a transform to the frame id is known. | ||
This will be known if id is a link name, an attached body id or a collision object. | ||
Args: | ||
robot_state (:py:class:`moveit_py.core.RobotState`): The robot state to check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump. This docstring should still be fixed, right? Looking again is it just that the robot_state
arg should be removed in this one?
moveit_py/src/moveit/moveit_core/planning_scene/planning_scene.cpp
Outdated
Show resolved
Hide resolved
CI for rolling is currently failing during the build stage for the |
e496a97
to
8b6299c
Compare
@sea-bass @JafarAbdi do we want to keep the commit history as is in its current form. I don't want to squash anything that has a co-contributor as I believe they should get credit for these commits. Happy to squash and add multiple co-contributors to the one |
I would say definitely squash. If you can give others credit in that squash commit, that's good! |
Co-authored-by: Henning Kayser <[email protected]> Co-authored-by: Michael Gorner <[email protected]> Co-authored-by: Robert Haschke <[email protected]> Co-authored-by: AndyZe <[email protected]> Co-authored-by: Peter Mitrano <[email protected]> Co-authored-by: Sebastian Castro <[email protected]> Co-authored-by: Jafar <[email protected]> Co-authored-by: Shahwas Khan <[email protected]>
8b6299c
to
3c31c8b
Compare
@sea-bass I have squashed the commits as requested. CI seems to be failing due to a dependency issue but this issue is unrelated to the changes in this pr. |
Ok, seems like no one wants to click the merge button. I'm doing it :D |
Description
This pull request adds a new Python library
moveit
which can be used to interface with MoveIt 2. Below is a basic summary of the initial feature set of the library:moveit.core: a python module containing implementations of moveit_core objects.
moveit.planning: a python module that exposes the planning functionalities of moveit_cpp
moveit.servo_client: a python module that defines a client for interacting with moveit_servo.
Note: this is an initial release, in future releases further functionalities will be added (e.g. moveit task constructor; task planning tools; interfaces for machine learning experiments).
Related Pull Requests
As this is a new library we also have associated API documentation and tutorials that should be merged around the same time as the initial library release. Below is a list of the currently open pull requests:
API Documentation: moveit/moveit2_tutorials#504
Robot State and Model Tutorial: TBC
Motion Planning Tutorial: moveit/moveit2_tutorials#565
Jupyter Notebook Tutorial: moveit/moveit2_tutorials#564
Robot Learning Tutorial (Simplified application of QT-OPT or similar ): TBC (likely to be moved until after the initial release)