search

ros-industrial / abb

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

driver: only check axes that are present #155

Closed gavanderhoorn closed 5 years ago

gavanderhoorn commented 5 years ago

While using the latest version of abb_driver I sometimes ran into #142, but even more unnecessarily than would normally be the case, as the code for external linear axes that was added in #150 appeared to also be checking axes that aren't present.

The proposed change updates is_near(..) to only check distance(tgt, curr) for axes that are actually configured/in use.

Tested in RobotStudio with both a IRB 120 stand-alone and an IRB 1600-10/1.45 on a 6m linear track.

gavanderhoorn commented 5 years ago

@gonzalocasas could you please review? You contributed #150, so probably know that code best.

gavanderhoorn commented 5 years ago

Tbh I'd rather improve the overall support for external axes (and determine and handle the presence/absence of external axes at a much earlier point in the control and dataflow) but I cannot commit to such extensive changes and didn't want to postpone submitting this fix earlier.

gavanderhoorn commented 5 years ago

This may also have been the cause of #156.

@rajmohan747 could you potentially test whether this fixes your issue?

gonzalocasas commented 5 years ago

@gavanderhoorn code looks good to me. 👍 I can test this next week on real hardware if you think it's necessary.

gavanderhoorn commented 5 years ago

It would be nice to get some confirmation from someone with an external axis configured.

I've only checked simulations of such setups.

krebsalad commented 5 years ago

I had an issue regarding this. I am currently working with the IRB1660 id. In the workspace there is an external axis available. Looks like this was the cause for very long trajectory executions times because I do not plan for the external axis.

At first, I tried decreasing the default 10-second duration for the first point in industrial_robot_client to 3 seconds to see if it changed anything (got this idea from issue #142). This did not do the job. The duration was still 10 seconds on the RAPID side. I do not understand why it did not change. This led me to the question: Why does the movement execution have a set duration and is not based on the move_speed? (works fine for me without the duration)

Anyways, this "fix" helped me out very much. I appreciate it. Thanks!

gavanderhoorn commented 5 years ago

@krebsalad: thanks for trying the PR. Your issue sounds like what I was running into as well.

gavanderhoorn commented 5 years ago

@gonzalocasas: have you already had a chance to test this?

gavanderhoorn commented 5 years ago

I'd like to merge this as we have at least one report of a successful test on actual hw.

@Levi-Armstrong: could you please review?

gavanderhoorn commented 5 years ago

Ping @gonzalocasas.

gonzalocasas commented 5 years ago

@gavanderhoorn sorry! haven't had time to check this on hardware so far, I might be able to do so in the next week or so if you still need it.

gavanderhoorn commented 5 years ago

Tested this on hardware myself now as well.

Merging.

gavanderhoorn commented 5 years ago

@rtonnaer this was the PR that needed to be merged.

gavanderhoorn commented 5 years ago

Thanks @Levi-Armstrong for the review.