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

kinetic-devel release & update #130

Merged
merged 4 commits into from
Sep 14, 2018
Merged

kinetic-devel release & update #130

merged 4 commits into from
Sep 14, 2018

Conversation

jproberge
Copy link
Contributor

@jproberge jproberge commented Jul 31, 2018

I know this is a rather big PR, but this is because it's been a long time since Robotiq's repo was last significantly updated. This is a summary of what this Pull Request contains:

  1. The .travis.yml file has been updated for ros-kinetic (reminder: in ros-kinetic, there's a debian package that solves the soem depency problem (i.e.: the ros-kinetic-soem package)

  2. Robotiq asked me to update their package names and files so that they reflect their real product names. Consequently, all C++ / Python identifiers have been updated to match Robotiq's real product names. This is where the largest number of changes come from.

  3. Minor package.xml file modifications to update the maintainer name and email address

  4. Finally: I'v added the "robotiq_gripper_upload.launch" file to address issue add launch/robotiq_gripper_upload.launch #99 .

This is meant to superseed PR #101 , and to create the first viable kinetic release for the Robotiq repo. This has been successfully tested on hardware and works fine.

@jproberge jproberge mentioned this pull request Jul 31, 2018
@jproberge
Copy link
Contributor Author

I'm not so sure about who could review this PR, but it would be great to eventually merge this (or a reviewed variant of this PR) in the not so far future :) Maybe @shaun-edwards or @AustinDeric could help? Any reviewer who wants to help out here is welcomed :) Thanks!

@jproberge jproberge requested review from shaun-edwards and removed request for shaun-edwards August 8, 2018 03:44
@gavanderhoorn
Copy link
Member

@ all robotiq users that watch this repository: @jproberge has put in quite some effort to get this kinetic-devel candidate in place.

I would personally appreciate it very much if any of you would be willing to test out his proposed changes and let us know your experiences.

With the refactor of these packages quite a few things are changing, and before merging something like this it would be good to have a few reviewers from the community confirm that everything is ok and in working order.

@gavanderhoorn
Copy link
Member

@jproberge: nice work with this PR.

Unfortunately I don't have any hw to test this against, so I can't be of any more help.

It would be nice if changes such as introduced in 2f12788 could be discussed first btw. Perhaps something for the future.

Again 💯 for the PR.

Copy link

@etienne-roberge etienne-roberge left a comment

Choose a reason for hiding this comment

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

This looks like cosmetic changes only. Everything seems good. I got maybe one comments on the class names, where some of them have underscore and some not...

Copy link

@CMouly CMouly left a comment

Choose a reason for hiding this comment

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

Everything looks fine except few extra indentations in .xacro and one .cpp files. GJ J-P Roberge

@jproberge
Copy link
Contributor Author

Hello all,

Three of my colleagues, me and even some of my students tested this PR on different grippers / FT sensors and they all reported that it worked right (and at least as well as the previous Jade version, which was also being used for Kinetic by many). It has been tested on all current Robotiq products. The fact that it is working properly is also not surprising since, as already pointed out by @etienne-roberge , this PR contains mostly cosmetic changes (mostly package name changes, as requested by Robotiq). Of course, it also passes all Travis-ci tests.

Now, a month and a half has already passed since this PR was first created. Only two reviewers commented and approved the PR, despite the "call to all" made by @gavanderhoorn a month ago. It is unfortunately a bottleneck to further improving the Kinetic branch of the Robotiq repo, since it was intended to be the first viable version for this branch. I'd like to start generating great and reliable documentation for this branch and also to address any potential issues that will arise in the future. I'd like to be able to maintain this branch properly, since a lot of users are using ros-kinetic, and I want to guarantee that they will have a high-quality repo at their disposal.

Since a lot of time has passed already, since two reviewers already approved the modifications (+many others have reported that it works properly), and since nobody will benefit in letting the current situation continue to last for a long time, I've decided to now merge this PR to the base branch. I hope that you'll all agree with this decision, as offering the best quality packages and improving Robotiq's Repo are really the only criteria on which I based that decision :).

Thanks to all!

@jproberge jproberge merged commit 8ca61a0 into ros-industrial-attic:kinetic-devel Sep 14, 2018
@wxmerkt
Copy link
Contributor

wxmerkt commented Sep 24, 2018

Hi, thank you very much for the update. Can we make the kinetic-devel branch the main branch of this repository? (Currently jade-devel).

@jproberge
Copy link
Contributor Author

Hi @wxmerkt ! Thanks for your comment and I also think this should become the main default branch. However, since I'm only a member, I cannot make this happen.

@gavanderhoorn : If I'm not mistaken, as an owner you have the permission to change the default branch, right? Assuming that is the case, what are your thoughts about that possibility?

@gavanderhoorn
Copy link
Member

@jproberge: I've made you a member of the admin team here, so you should be able to manage branches yourself now.

Remember: with great power .. ;)

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.

5 participants