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

Use panda_moveit_config or moveit_resources_panda_moveit_config? Unclear the direction and rationale #704

Open
130s opened this issue Jun 14, 2023 · 12 comments

Comments

@130s
Copy link
Contributor

130s commented Jun 14, 2023

Description

In #59, the reference point for moveit_config package for Panda robot moved from panda_moveit_config to moveit_resources_panda_moveit_config (under moveit_resources repo) but I do not see the rationale for that move, nor the instruction for future tutorial contributors.

Impact

That resulted in confusion, even to one of the maintainers at #61 (comment), and merging a PR is blocked panda_moveit_config!136 (the work was done based on an assumption that the switch to moveit_resources_panda_moveit_config was an agreed move, but apparently one maintainer is objecting now).

Expected result

Documented decision and rationale for the location of Panda's model and moveit_config that tutorials in moveit2_tutorials should reference to.

@130s
Copy link
Contributor Author

130s commented Jun 14, 2023

Pinging @DLu, @JafarAbdi, @sea-bass @rhaschke as I see your guys' names in the referenced pages.

@130s
Copy link
Contributor Author

130s commented Jun 14, 2023

Potential hint was found at #59 (comment), but not enough.

@rhaschke
Copy link
Contributor

I repeat my objection from #61 (comment) for clarity:

The tutorials should not depend on moveit_resources_panda_moveit_config (which is a minimal version of panda_moveit_config referring to a minimal version of the URDF model as well), but use the official panda_moveit_config package instead, which uses the official URDF from franka_description.

@130s 130s mentioned this issue Jun 28, 2023
4 tasks
@130s
Copy link
Contributor Author

130s commented Aug 24, 2023

@rhaschke Please review the following and see if you can agree.

Discussed in moveit maintainers' mtg. I heard @henningkayser or someone from maintainers will comment here, but my understanding was to stick to moveit_resources based on the following:

  • moveit2_tutorials should refer to moveit_resources as the first place to look up robot resource.
  • panda_moveit_config is a "vendor package", so changes that moveit2_tutorials need can't always be made there.
    • In hindsight, I'm not sure what that means, particularly when the repo is under ros-planning org. Is the repo maintained by vendor and there's no non-vendor maintainers on that repo?
  • moveit2 tutorials is migrating to kinova.

It's in the mtg minutes as well, citing here:

[Isaac] Robert says the tutorials shouldn’t depend on the moveit_resources versions
[Henning] He isn’t wrong. We have been using moveit_resources because we can easily update it and Franka is not actively maintaining their configs. We shouldn’t make moveit_resources depend on the franka repo. We are also transitioning to kinova arm in tutorials.
[Alex] Because Robert objected to Kinova configs being merged into moveit_resources we put in its own repo. It would be easier if all our configs were in one place but it does make sense that each robot should be in its own place.
[Tyler] You are welcome to use Kinova for your tutorial if that is easier.
[Isaac] Are you accepting PRs for non-Kinova arms?
[Tyler] Yes, we are happy to accept PRs with either. Thank you for working on this.


My personal comment as a community user: Obvious caveat is maintaining almost duplicate (if there's an upstream exists like the case with panda), but I assume communication overhead with vendors often kills the benefit of using upstream?

@henningkayser
Copy link
Member

The duplicate config packages are really not great for usability. moveit_resources only being a test dependency (and nothing else) is somewhat of an unwritten rule and indeed very confusing. The purpose of the panda_moveit_config (being an officially supported vendor package for the Panda robot) does not really exist anymore with the ROS 2 versions, due to the lack of sponsoring or collaboration with Franka Emika. There wasn't even a ROS 2 driver available for a long time. So, for most ported tutorials we switched to moveit_resources to eliminate redundancy and have now started updating the content to an actually supported robot.
In short: for simple migration, I would recommend using moveit_resources_* anyway, for consistency with the existing MoveIt 2 tutorials and for easier maintenance. For new content, we are debating defaulting to the Kinova robot.

@rhaschke
Copy link
Contributor

  • panda_moveit_config currently is not hosted in the @frankaemika organization but in @ros-planning. They were interested, though, to take ownership again (a year ago or so), but didn't take action along that direction.
  • However, panda_moveit_config depends on franka_description, which is hosted by @frankaemika.
  • Indeed we had issues in the past, @frankaemika changing their URDFs w/o notifying us or providing patches to panda_moveit_config directly. One major issue was the introduction of very coarse collision models (as used by the robot controller internally for self-collision checking). To handle this, I had to adapt srdf source code. The current URDF (Provide detailed and coarse collision models frankaemika/franka_ros#199) works and is stable since then.
  • panda_description on the other hand, is a very old state of franka_description used only within moveit_resources. It doesn't comprise the coarse collision models. These models should not be used on a real Panda.

For this reason, I still recommend using panda_moveit_config in the tutorials.
Adapting an existing ROS2 panda_moveit_config in moveit_resources to the latest version of franka_description shouldn't be a big deal, should it?

@130s
Copy link
Contributor Author

130s commented Sep 1, 2023

Thanks @henningkayser @rhaschke for maintainers' input. Here's an input from a Moveit2 tutorial user in the community.

2 orthogonal issues

  • One of the primary issues that is affected by ticket, from community's point of view, is some/many? moveit2 tutorials are not usable out of the box by ros2 users (ticketed a list of outdated tutorials Many tutorials are based on ROS1, while moveit2 is based on ros2  #760).
    • Internal design of moveit2 tutorials (like the one discussed in this ticket) may not affect/help majority of moveit2 tutorial users.
    • This ticket is blocking some of ROS2-conversion PRs e.g. ros2-ify perception pipeline #700
  • Sounds like there's a chance that a reference robot will be different robot (Kinova) but not decided yet.
  • Users want ros2 tutorials asap, but maintainers want a clear solution for the location of the moveit-config package. These are 2 separate issues.

Just a community user's suggestion

Although I'm not in the position to say which issue among the 2 above should be prioritized, I think we should unblock ros2 tutorials work (unable to use tutorials may result in losing potential moveit2 users, esp. commercial users who may have less luxury to be patient).

With a chance that reference robot might be switched, we may want to minimize effort for all the Panda packages relevant to moveit2. With that in mind, centralizing panda-related effort from moveit2 community to panda_moveit_config makes more sense to me in order to utilize upstream work e.g. franka_description.

@rhaschke
Copy link
Contributor

rhaschke commented Sep 1, 2023

Dear Isaac,

I wasn't aware that this issue is blocking #700 and other similar PRs and this was not my intention.
You are right, that tutorials should be migrated asap. As I am not involved in MoveIt2 (or ROS2) development yet, I am surprised that most of the tutorials are not yet ported, actually.
My suggestion is as follows:

  1. Merge port PRs using moveit_resources_panda_moveit_config as is - to make them usable asap.
  2. Migrate panda_moveit_config to ROS2 by merging work from moveit_resources_panda_moveit_config.
  3. Use this update-to-date panda_moveit_config for future ports and update existing ported tutorials to use that one as well.

(1) is in line with @henningkayser's comment. But it adds a roadmap for a migration to an up-to-date panda_moveit_config.

@130s
Copy link
Contributor Author

130s commented Sep 1, 2023

+1, that unblocks ros2 conversion, won't introduce any breaking change to moveit2 tutorials, yet doesn't interrupt the potential process of reference robot change. Thank you for suggestion @rhaschke. I'll wait for official consensus @henningkayser

130s added a commit to 130s/moveit2_tutorials that referenced this issue Sep 27, 2023
@tylerjw
Copy link
Member

tylerjw commented Dec 1, 2023

@130s do you know what the status of this is? I'm happy to help you get anything merged into the config packages to unblock work porting tutorials.

@130s
Copy link
Contributor Author

130s commented Dec 1, 2023

@tylerjw No updates I'm aware of since my post #704 (comment). I think Robert and I are on agreement on his post #704 (comment) and needs ratified by moveit2 maintainer team IMO.

@tylerjw
Copy link
Member

tylerjw commented Dec 1, 2023

Do you have a PR for some of this work I could look at?

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

No branches or pull requests

4 participants