search

ros-industrial / abb

ROS-Industrial ABB support (http://wiki.ros.org/abb)
146 stars 154 forks source link

Refactor repository to comply better with layout guidelines #43

Closed gavanderhoorn closed 10 years ago

gavanderhoorn commented 10 years ago

As per subject.

This should only introduce additional packages, and doesn't touch any of the existing ones, apart from adding deprecation notices to them. roslaunch testing has been added to packages wherever possible, to prevent changes from introducing any issues with backwards compatibility.

Deprecated packages have been described as slated for removal in Indigo (tick/tock).

Summary of changes:

This should also take care of #38 and #40.

gavanderhoorn commented 10 years ago

Ping? :) I'll leave it open here, anyway.

shaun-edwards commented 10 years ago

@gavanderhoorn, I've failed you ;). I appologize for the delay. I've asked @JeremyZoss to review this since I'm a little overwhelmed. Should be merged in soon. Thanks for the contribution.

JeremyZoss commented 10 years ago

Mostly looks good. Thanks for your effort in standardizing this layout. A few comments:

1) I disagree with making J23_coupled a required launch parameter in robot_interface.launch.

I think required parameters should only be used when there is no reasonable default value (e.g. for IP addresses). If we go down the path of requiring an argument for any option that might "critically" impact robot motion, then things might get messy if we add more options in the future. In reviewing ABB's website, most (20/28) robots do not seem to use any Joint 2/3 coupling, so it seems that "false" is a sensible default here.

2) Similarly, robot-specific launch files should not require the user to specify J23_coupled at runtime. These launch files should explicitly specify the correct default value for their respective robot configuration:

3) Your commit states that the irb2400 ikfast moveit plugin was re-generated, but it does not appear to match the current MoveIt_IKFast template. In particular, the irb2400 plugin generates deprecated warnings for tf::PoseMsgToKDL. I will log this as a separate issue.

4) I think theabb_irb6600_support package may be too broad. I'm not too familiar with ABB's product line, but it doesn't seem like there is much commonality between robot models in the 6600 series. Should this be abb_irb6640_support instead, or was the current naming deliberate? From the datasheets, it appears as though there are several variants of the 6640 (and 2400). I will log variant-support as a separate issue.

gavanderhoorn commented 10 years ago

Thanks for the review.

1) I disagree with making J23_coupled a required launch parameter in robot_interface.launch.

I think required parameters should only be used when there is no reasonable default value (e.g. for IP addresses). [..]

I disagree, but if you feel strongly about this, then I'll change it. Imho, something like a joint coupling factor is a critical parameter, as it directly influences the correctness of both the reported robot state, as well as the execution of motion. The idea was to not give the parameter a default at the robot_interface.launch level, and then provide the correct -- and verified -- default value in the support package launch files. That way users using 'unsupported' models would be forced to check the value of the parameter for themselves, while users using released pkgs can be sure we took the time to figure things out for them (but I think that is what you refer to in your next point).

2) Similarly, robot-specific launch files should not require the user to specify J23_coupled at runtime. These launch files should explicitly specify the correct default value for their respective robot configuration:

IRB2400 = true (correct as-is)
IRB5400 = false? (currently undefined)
IRB6640 = false (correct as-is)

I could be misunderstanding you, but is this not what d9b8adf4 does? That commit introduces per variant specific launch files that supply defaults for the required parameters wherever possible. The only reasons the files in the 5400 support pkg don't provide a default is because I couldn't find the correct value anywhere quickly (I was in a car, without internet :)). I noted this in the commit msg: "TODO: provide default for coupling factor of IRB 5400 (no existing value available)."

3) Your commit states that the irb2400 ikfast moveit plugin was re-generated, but it does not appear to match the current MoveIt_IKFast template. In particular, the irb2400 plugin generates deprecated warnings for tf::PoseMsgToKDL. I will log this as a separate issue.

I think that is actually the old abb_moveit_plugins pkg that is causing those errors. 29c3831f introduces a new pkg: abb_irb2400_moveit_plugins. As far as I can tell, that actually compiles fine here. In fact, if you CATKIN_IGNORE all of the old pkgs (except the meta-pkg), you shouldn't get any errors or warnings (catkin_lint should also be pretty happy).

4) I think the abb_irb6600_support package may be too broad. I'm not too familiar with ABB's product line, but it doesn't seem like there is much commonality between robot models in the 6600 series. Should this be abb_irb6640_support instead, or was the current naming deliberate? From the datasheets, it appears as though there are several variants of the 6640 (and 2400). I will log variant-support as a separate issue.

Well, we can discuss this, but the idea behind the naming scheme was exactly to put all variants of a specific model in a single support package. The fact that there isn't much commonality doesn't really matter, imo. End-users with any 6600 series robot should just have to install abb_irb6600_support and be done.

Additionally, I just ported what was there. I may be even less familiar with ABB's product line, so I just took the model nrs as I found them. We should be as specific as possible when decribing the supported models / variants, so your remark is certainly valid.

shaun-edwards commented 10 years ago

@dpsolomon, do you know if there is any joint coupling on the 5400? If not, we can load up the ABB simulator and try it out.

dpsolomon commented 10 years ago

Not that I am aware of. I will try to load it and see.

gavanderhoorn commented 10 years ago

Merging this would also take care of ros-industrial/ros_industrial_issues#11 for this repository.

gavanderhoorn commented 10 years ago

@dpsolomon wrote:

Not that I am aware of. I will try to load it and see.

If you provide me with the proper value, I'll add the default to the 5400 launch file.

JeremyZoss commented 10 years ago

More discussion:

1) I agree that joint-coupling is critical. However, this particular option only activates a fixed joint-coupling ratio for certain known configurations. Other models might have other joint-coupling needs, or different (non-joint-coupling) options that could be added to the driver nodes as needed. My point is that the driver should provide nominal operation in its default state, and use the option flags to trigger off-normal behavior, when required. We don't need these to be required options, forcing all robot packages to explicitly specify "I want the default behavior" for each option.

This is mostly a philosophical objection, since we only have this one option, currently. I'm just trying to anticipate what might happen if the driver were expanded to include more optional behavior in the future.

2) Yes, I agree that your current launch files cover most cases. I was just pointing out that we should provide the correct value for the 5400 as part of this commit, or log it as a new issue. If we remove this as a "required parameter" (as per 1), than this is obviously a non-issue.

3) You are absolutely correct. I missed that the warnings were coming from the old/deprecated abb_moveit_plugins package. You are right that the new abb_irb2400_moveit_plugin package compiles without errors.

4) I agree with the general philosophy. I'm just not sure that the 66xx models are all considered "6600 variants":

I'm not very familiar with these models... I'm just going by their website.

gavanderhoorn commented 10 years ago

@JeremyZoss wrote:

1) I agree that joint-coupling is critical. However, this particular option only activates a fixed joint-coupling ratio for certain known configurations. [..] We don't need these to be required options, forcing all robot packages to explicitly specify "I want the default behavior" for each option.

I see your point, but I still favour explicitness over the minor extra verboseness of all pkgs having to specify the "I want the default behavior" (which isn't default if we don't make it a default). It's just one configuration aspect of the driver that the support pkgs are best suited for to configure ánd document.

I'll revert 3765cd6a though: I don't feel strongly enough about it to make this a blocker.

This is mostly a philosophical objection, since we only have this one option, currently. I'm just trying to anticipate what might happen if the driver were expanded to include more optional behavior in the future.

Obviously this depends on the type of behaviour that is parametrised by those options. I'd say that something like joint-coupling deserves special attention, which is why I didn't like the default. The fanuc_driver also has no default, but perhaps it is different as you say, as that is a three-valued coupling factor with no sensible default.

Yes, I agree that your current launch files cover most cases. I was just pointing out that we should provide the correct value for the 5400 as part of this commit, or log it as a new issue. If we remove this as a "required parameter" (as per 1), than this is obviously a non-issue.

As soon as I have the correct value, I'll amend the commit (even after reverting 3765cd6a, just to document it).

I agree with the general philosophy. I'm just not sure that the 66xx models are all considered "6600 variants":

I'm not very familiar with these models... I'm just going by their website.

Looking at those pages I think you're right: it doesn't seem to make sense to put all those models in the same package. The 6620LX is even on a rail.

Also: seeing as there are 7 variants of the 6640, the current model really needs some more specific naming and documentation.

JeremyZoss commented 10 years ago

Issue #47 was already added to track variant support. I think we can handle that independently of this commit.

I'm also comfortable with merging this pull request as-is, if you'd like to update the 5400 launch file separately. If so, we should list it as an open issue, so it doesn't get lost.

gavanderhoorn commented 10 years ago

Ok, let's merge as it is now. The idea of this PR was to only refactor the layout of existing files anyway, and it seems that has been achieved. We'll solve any other issues with following PRs.