moveit / moveit2_tutorials

A sphinx-based centralized documentation repo for MoveIt 2
https://moveit.picknik.ai
BSD 3-Clause "New" or "Revised" License
150 stars 191 forks source link

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

Open 130s opened 1 year ago

130s commented 1 year ago

Description

In https://github.com/ros-planning/moveit2_tutorials/issues/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 https://github.com/ros-planning/moveit2_tutorials/pull/61#issuecomment-1589701991, 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 commented 1 year ago

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

130s commented 1 year ago

Potential hint was found at https://github.com/ros-planning/moveit2_tutorials/issues/59#issuecomment-1584843073, but not enough.

rhaschke commented 1 year ago

I repeat my objection from https://github.com/ros-planning/moveit2_tutorials/pull/61#issuecomment-1589701991 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 commented 1 year ago

@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:

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 commented 1 year ago

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 moveitresources* 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 commented 1 year ago

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 commented 1 year ago

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

2 orthogonal 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 commented 1 year ago

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 commented 1 year ago

+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

tylerjw commented 9 months ago

@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 commented 9 months ago

@tylerjw No updates I'm aware of since my post https://github.com/ros-planning/moveit2_tutorials/issues/704#issuecomment-1703137286. I think Robert and I are on agreement on his post https://github.com/ros-planning/moveit2_tutorials/issues/704#issuecomment-1702553484 and needs ratified by moveit2 maintainer team IMO.

tylerjw commented 9 months ago

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