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

Catkinized pr2_common_actions #27

Closed
wants to merge 7 commits into from
Closed

Catkinized pr2_common_actions #27

wants to merge 7 commits into from

Conversation

airballking
Copy link

Everything compiles on our PR2 in Bremen under Hydro. However, some work remains to have everything work as it did some years ago:

  • pr2_move_arm_ik does not compile anything because I could not find pr2_kinematics
  • pr2_tilt_laser_interface needs to be refactored to comply with new practices under Hydro
  • the are no install rules for releasing

Still, I hope it's a first step enabling others to provide fixes in the future.

@airballking
Copy link
Author

relates to #2

@ahendrix
Copy link
Member

This looks reasonable. Care to review @ibaranov-cp ?

The ticket to catkinize or update pr2_kinematics is PR2/pr2_kinematics#2

@airballking
Copy link
Author

I've seen the ticket on pr2_kinematics. However, the repo itself is empty. I could not find the rosbuild-versions of the packages.

@ibaranov-cp
Copy link

I will look it over, more for my own knowledge than anything else.
I am still getting ramped up with PR2 related infrastructure and code.

Initial look through looks fine though.

On Tue, Apr 15, 2014 at 12:08 PM, Austin Hendrix
[email protected]:

This looks reasonable. Care to review @ibaranov-cphttps://github.com/ibaranov-cp?

The ticket to catkinize or update pr2_kinematics is PR2/pr2_kinematics#2PR2/pr2_kinematics#2


Reply to this email directly or view it on GitHubhttps://github.com//pull/27#issuecomment-40500911
.

Regards,

Ilia Baranov
Electrical Engineer
[email protected]

@ibaranov-cp
Copy link

Seems to compile fine for me, and changes seem ok.

However, catkin_lint finds some minor problems:

joint_trajectory_action_tools: warning: package 'trajectory_msgs' should be listed in catkin_package()
joint_trajectory_action_tools: warning: package 'pr2_controllers_msgs' should be listed in catkin_package()
joint_trajectory_action_tools: notice: meaningless package description 'joint_trajectory_action_tools'
joint_trajectory_generator: notice: catkin_package() exports package include path that is not installed
joint_trajectory_generator: notice: target 'joint_trajectory_generator' is not installed
joint_trajectory_generator: notice: exported library 'joint_trajectory_generation' is not installed
pr2_arm_move_ik: error: unconfigured build_depend on 'roscpp'
pr2_arm_move_ik: error: unconfigured build_depend on 'actionlib'
pr2_arm_move_ik: error: unconfigured build_depend on 'urdf'
pr2_arm_move_ik: error: unconfigured build_depend on 'pr2_common_action_msgs'
pr2_arm_move_ik: error: unconfigured build_depend on 'geometry_msgs'
pr2_arm_move_ik: error: unconfigured build_depend on 'tf'
pr2_arm_move_ik: error: unconfigured build_depend on 'actionlib_msgs'
pr2_arm_move_ik: error: unconfigured build_depend on 'pr2_controllers_msgs'
pr2_common_action_msgs: notice: meaningless package description 'The pr2_common_action_msgs package'
pr2_tilt_laser_interface: error: build include path 'include' does not exist
pr2_tilt_laser_interface: error: unconfigured build_depend on 'laser_geometry'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'pcl_conversions'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'actionlib'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'sensor_msgs'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'pr2_msgs'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'message_generation'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'pcl_ros'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'actionlib_msgs'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'roscpp'
pr2_tilt_laser_interface: error: unconfigured build_depend on 'message_generation'
pr2_tilt_laser_interface: error: unconfigured message dependency 'actionlib_msgs'
pr2_tilt_laser_interface: error: unconfigured message dependency 'sensor_msgs'
pr2_tilt_laser_interface: error: unconfigured message dependency 'pr2_msgs'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'roscpp'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'actionlib'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'actionlib_msgs'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'pr2_msgs'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'sensor_msgs'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'message_generation'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'pcl_ros'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'pcl_conversions'
pr2_tilt_laser_interface: CMakeLists.txt(4): error: missing COMPONENTS keyword before 'laser_geometry'
pr2_tilt_laser_interface: notice: package description starts with boilerplate 'Provides a set of tools'
pr2_tilt_laser_interface: notice: target 'grab_result' is not installed
pr2_tilt_laser_interface: notice: target 'grab_result' should contain package name
pr2_tuck_arms_action: warning: package 'trajectory_msgs' should be listed in catkin_package()
pr2_tuck_arms_action: warning: package 'pr2_common_action_msgs' should be listed in catkin_package()
pr2_tuck_arms_action: warning: package 'actionlib_msgs' should be listed in catkin_package()
pr2_tuck_arms_action: warning: package 'pr2_controllers_msgs' should be listed in catkin_package()
pr2_tuck_arms_action: notice: meaningless package description 'The pr2_tuck_arms_action package'

Some of these are expected, but others are mistakes of not following convention.

@airballking
Copy link
Author

Thanks for the fast review. I tried to address most of the issues noted by catkin_lint.

@airballking
Copy link
Author

@ahendrix Any outstanding chores for merging?

@ibaranov-cp
Copy link

Looks good to me, I would say we can merge.
Being the new kid on the block, I will wait until Austin agrees :)

@airballking
Copy link
Author

Thanks.

@trainman419
Copy link

I'm worried that this is half-done, but at a quick glance it will look done because it compiles now.

I'd rather see the kinematics fixed first, and then build this on top of the new kinematics.

@airballking
Copy link
Author

Fair enough. I'll have a look at pr2_kinematics, then.

@k-okada
Copy link
Contributor

k-okada commented May 22, 2014

I need to merge this PR for PR2/pr2_navigation#3. It seems pr2_navigation does not depends on pr2_kinematics.

@UltronDestroyer
Copy link
Contributor

Self note for review and catkinization. @thedash

@UltronDestroyer
Copy link
Contributor

@trainman419 Is there an update on

"I'd rather see the kinematics fixed first, and then build this on top of the new kinematics." What would be the repercussions of merging this right now?

@trainman419
Copy link

I haven't made any effort on catkinizing the pr2_kinematics package.

The trouble here is that there are a few packages in this repository that depend on pr2_kinematics, and the current catkinization converts them to catkin, but removes all of the compile commands, effectively rendering them empty and unusable. This would be extremely surprising for anyone else who tries to use the hydro-devel branch and expects all of the packages to be available, and would become worse if these packages were released, empty, to the build farm.

There are a couple of ways that we can proceed here:

  • Wait for someone to rewrite pr2_kinematics. Note that this should be a complete rewrite and not simply a catkinization of the existing package, since most of the current dependencies of pr2_kinematics are deprecated and unsupported.
  • Move the packages that depend on pr2_kinematics into a different repository and postpone converting them to catkin
  • Merge this into a feature branch such as hydro-devel-catkin or hydro-devel-unstable
  • Merge it anyway and hope that the resulting support of lost/confused users is low

@k-okada
Copy link
Contributor

k-okada commented May 30, 2014

+1 for

  • Merge it anyway and hope that the resulting support of lost/confused users is low

@UltronDestroyer
Copy link
Contributor

I've put this in the hydro-devel-unstable branch. It has been working for myself. Thank you airballking

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.

6 participants