ros-industrial / abb

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

Adds robot status publishing to Rapid driver #145

Closed yamokosk closed 5 years ago

yamokosk commented 6 years ago

Continuation of #144. Fully tested on hardware and a packet capture is included to verify. In capture robot was commanded to move to a few points and at the end of the capture, the E-stop was pressed.

The PR also standardizes whitespace indentation. Spaces, as opposed to tabs, were used more often for indentation so I changed everything to spaces. They are in separate commits to better see the changes.

robot_status_message.pcapng.zip

gavanderhoorn commented 6 years ago

@Levi-Armstrong: is this something you'd pick up?

@yamokosk: my apologies for our late response, this is definitely something that is of great interest :+1:

gonzalocasas commented 6 years ago

It looks good to me.

Is there anything I could do to speed up the merge of this PR? I can offer to test it on some of our hardware if that helps. (or if there's another way to contribute to make the process smoother/faster, please let me know).

gavanderhoorn commented 6 years ago

Getting a second user to verify this on their hw would be good, yes.

If you would be willing to do that, it would be appreciated.

gonzalocasas commented 6 years ago

Yes, sure, I will test this next week in our hardware and post here! Thx!

gavanderhoorn commented 6 years ago

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

gonzalocasas commented 6 years ago

@gavanderhoorn: preparations too a bit longer than expected, but I've scheduled the test for this Tuesday.

gonzalocasas commented 6 years ago

@yamokosk is this how the signals should be configured?

image

(In general, if a PR requires config changes, I'd suggest to include them as part of the description, so testing/review and transfer to the wiki is unambiguous).

gonzalocasas commented 6 years ago

Ok, this seems to work. I am still not sure if the signal configuration that I used is correct, but the robot_status looks like properly reflecting it, so lgtm.

gavanderhoorn commented 6 years ago

Thanks for the check @gonzalocasas :+1:

We'll have to make sure to document the signal configuration, as it's unclear right now, as you remark.

Ideally with both a comment at the top of the file as well as in the installation tutorial.

gavanderhoorn commented 6 years ago

I actually believe signalMotorOn should be mapped to Motors On State, not Motors On.

gonzalocasas commented 6 years ago

@gavanderhoorn it could be; but then it's unclear to me what's the correct mapping for signalMotionPossible.

gavanderhoorn commented 6 years ago

Not sure either right now. Not near a robot/robotstudio/manual.

In any case: drives_powered (in the ROS msg) should reflect whether the drives are powered now. IIRC, Motors On is a pulsing signal, it does not stay high when motors are already enabled/on.

gonzalocasas commented 6 years ago

hi @yamokosk! could you pls give me a hand in describing how you configured the signals in your setup? I can then help with the documentation part so that this PR gets merged ;)

yamokosk commented 6 years ago

Hi guys, sorry for the delayed response. Sure. Do you want me to document how I created the signals in RobotStudio? I can send screenshots of the relevant screens if that works.

gonzalocasas commented 6 years ago

@yamokosk yep, screenshots would help

gavanderhoorn commented 6 years ago

@yamokosk: it would be great if we could include some documentation on how to setup everything needed wrt signals needed for this extension.

Could you provide the screenshot(s) that you offered?

gavanderhoorn commented 6 years ago

@yamokosk: :bellhop_bell:

gavanderhoorn commented 6 years ago

And another ping.

keerthanamanivannan commented 5 years ago

Alright, I work with @yamokosk and I've been working on our robots with these changes. I'm attaching the screenshots that @yamokosk mentioned above.

pic1 The above screenshot shows the place where we add these signals.

pic2 And this one shows what System Outputs we tied the Signals to. Let me know if anything else is required!

keerthanamanivannan commented 5 years ago

@gavanderhoorn @gonzalocasas ping.

gavanderhoorn commented 5 years ago

Let me know if anything else is required!

thanks for adding this @keerthanamanivannan.

It would be great if we could add this to either the readme of abb_driver and/or the installation tutorial.

keerthanamanivannan commented 5 years ago

@gavanderhoorn Sure, I can do that! How would I edit this link: installation tutorial? It says that its an immutable page.

gavanderhoorn commented 5 years ago

Do you have a wiki account?

keerthanamanivannan commented 5 years ago

Yes, I do.

gavanderhoorn commented 5 years ago

And have you requested write access? That's a separate step.

If the answer is also "yes", then you should be able to open this link and start editing.

keerthanamanivannan commented 5 years ago

Oh. Well, no. I have never requested write access. How would I do that?

gavanderhoorn commented 5 years ago

You can request that over in https://github.com/ros-infrastructure/roswiki/issues/258.

gavanderhoorn commented 5 years ago

@keerthanamanivannan ?

keerthanamanivannan commented 5 years ago

@gavanderhoorn I edited the abb InstallServer tutorial and clicked the save button and it got published! Thought it might go through some approval process of some sort, but yeah. Review that and change/modify it if necessary. Merge this PR. Let me know if anything else is required.

gavanderhoorn commented 5 years ago

I edited the abb InstallServer tutorial and clicked the save button and it got published!

great :) thanks.

Thought it might go through some approval process of some sort, but yeah.

No, the wiki has no review process :)

Review that and change/modify it if necessary. Merge this PR. Let me know if anything else is required.

Thanks for updating the docs.

I'll take a look at the PR, review your wiki changes and then merge.

keerthanamanivannan commented 5 years ago

@gavanderhoorn alright, so I made some commits and those should take care of points 2, 3 and 4 of the requested changes. Updated the documentation too!

I had a question about the first one though: this line says that the broadcast rate is in fact 10 Hz. and we're waiting for this update_rate time to send both joint_states and robot_status messages here. Correct me if I'm wrong.

And the fifth one, I'm waiting to hear from @jontje for suggestions.

gavanderhoorn commented 5 years ago

@gavanderhoorn alright, so I made some commits and those should take care of points 2, 3 and 4 of the requested changes.

Great. Thanks for that.

re: pt 3: does signalRobotActive in 17c1a5de check for the status of T_ROB1?

I had a question about the first one though: this line says that the broadcast rate is in fact 10 Hz. and we're waiting for this update_rate time to send both joint_states and robot_status messages here. Correct me if I'm wrong.

You're correct. My comment was meant to say "right now status and joint state msgs are published at the same frequency, which is not necessarily what we want". But they're both at 10 Hz right now. It would be good to decouple those publication rates, but that would be something for another PR.

And the fifth one, I'm waiting to hear from @jontje for suggestions.

Yes. I would have to check documentation and my notes again to answer that one, so if @jontje could comment on that one it would save me some time.


Thanks for updating the PR again.

keerthanamanivannan commented 5 years ago

re: pt 3: does signalRobotActive in 17c1a5d check for the status of T_ROB1?

I saw this doc, pg 253 in the manual itself titled Mechanical Unit Active and I believe this System Output would check if T_ROB1 is active or not. Would be great if someone else can confirm this.

You're correct. My comment was meant to say "right now status and joint state msgs are published at the same frequency, which is not necessarily what we want". But they're both at 10 Hz right now. It would be good to decouple those publication rates, but that would be something for another PR.

Alright got it, yes, maybe a different PR.

Thanks for updating the PR again.

No problem!

gonzalocasas commented 5 years ago

Minor question: shouldn't we "namespace" the signals that are linked to a specific unit? i.e. signalRobotActive and signalRobotNotMoving? Otherwise, setting up for a multi-robot system means more departing from standard setup instructions.

keerthanamanivannan commented 5 years ago

Minor question: shouldn't we "namespace" the signals that are linked to a specific unit? i.e. signalRobotActive and signalRobotNotMoving? Otherwise, setting up for a multi-robot system means more departing from standard setup instructions.

Good point, and if I had to do this, do you know how I would go about implementing this? @gavanderhoorn @jontje any ideas?

@jontje I see a "Assigned to Device" option when I'm creating a Signal in the Robot Controller. But it's showing up empty. Would that be of any help to this task at all? capture 3

gavanderhoorn commented 5 years ago

@keerthanamanivannan: could you please rebase your PR on-top of current kinetic-devel? Travis refuses to run CI for this PR. I'm not sure whether that is because of the conflicts, but they'll need to be resolved in any case.

keerthanamanivannan commented 5 years ago

@gavanderhoorn Please review. Thanks!

jontje commented 5 years ago

Good point, and if I had to do this, do you know how I would go about implementing this? @gavanderhoorn @jontje any ideas?

When I have multiple mechanical units in a system then I usually append the unit names as a prefix. For example using "ROB_X" like this: ROB_1_signal_name, ROB_2_signal_name, etc...

I would also choose to create a RobotWare Add-In that inspects the robot system during installation, and automatically loads the necessary configurations and RAPID modules. However, this would probably require quite a bit of effort if you are not familiar with creating such Add-Ins.

@jontje I see a "Assigned to Device" option when I'm creating a Signal in the Robot Controller. But it's showing up empty. Would that be of any help to this task at all?

The "Assigned to Device" field is only required when connecting the signal to a physical IO-device. When the field is left empty, then the signal will only be virtual. So it will not be of any help here.

gavanderhoorn commented 5 years ago

Good point, and if I had to do this, do you know how I would go about implementing this? @gavanderhoorn @jontje any ideas?

When I have multiple mechanical units in a system then I usually append the unit names as a prefix. For example using "ROB_X" like this: ROB_1_signal_name, ROB_2_signal_name, etc...

hm. That would need changes to the code when deploying on systems with multiple mechanical units.

@jontje: there is no way to "scope" these signal declarations that would make it less intrusive? Something similar to assigning to a mech unit, as you do with motion tasks?

keerthanamanivannan commented 5 years ago

hm. That would need changes to the code when deploying on systems with multiple mechanicle units.

Right. I actually work on a multi-robot system. I set each robot separately as a single mechanical unit in ABB Settings. Like, all the robots are named ROB_1 in the ABB settings, but I launch all the nodes for each robot in its own namespace. Like @gavanderhoorn mentioned in #106.

So I essentially get the info on all the robots like robot1/robot_status and robot2/robot_status, and so on.

jontje commented 5 years ago

@jontje: there is no way to "scope" these signal declarations that would make it less intrusive? Something similar to assigning to a mech unit, as you do with motion tasks?

Hm, no direct way that I can think of at the moment. Some of the signals are related to the robot controller (e.g. signalRobotEStop) and others are related to mechanical units (e.g. signalRobotNotMoving). Maybe an idea could be to use cross connections to apply some logic to the signals related to multiple mechanical units.

Right. I actually work on a multi-robot system. I set each robot separately as a single mechanical unit in ABB Settings. Like, all the robots are named ROB_1 in the ABB settings, but I launch all the nodes for each robot in its own namespace. Like @gavanderhoorn mentioned in #106.

So I essentially get the info on all the robots like robot1/robot_status and robot2/robot_status, and so on.

If I understand correctly, then you work with a multi robot controller system (with one robot each), instead of one robot controller with multiple robots? If so, to make things as generalized as possible then it might be reasonable to set up some namespaces like this:

keerthanamanivannan commented 5 years ago

If I understand correctly, then you work with a multi robot controller system (with one robot each), instead of one robot controller with multiple robots? If so, to make things as generalized as possible then it might be reasonable to set up some namespaces like this:

  • robot_controller1/robot1/robot_status
  • robot_controller1/robot2/robot_status
  • robot_controller1/robot3/robot_status
  • robot_controller2/robot1/robot_status
  • robot_controller2/robot2/robot_status
  • Etc...

Yes you are correct, I do not have the setup of one robot controller with multiple robots. I have four robots and four controllers, with one to one mapping. So my namespacing setup works for me. But I hear you, yeah.

keerthanamanivannan commented 5 years ago

@gavanderhoorn how are we moving forward with this?

gavanderhoorn commented 5 years ago

@keerthanamanivannan: all your commits appear to not be attributed to any email address Github knows about. Is that ok for you? Otherwise I'd recommend you either register the email address with your account, or update the commits.

keerthanamanivannan commented 5 years ago

@gavanderhoorn thank you for bringing that to my attention. I added the email address to my account. :+1:

gavanderhoorn commented 5 years ago

Ok.

So I would like to get this merged in.

Even without explicit "support" for multi-mech-unit setups (or: a nice way to scale/scope the signals in those cases).

I've just added ee970d4 which fixes the in_motion field (it didn't take execution status of the motion execution task into account, leading to in_motion being 1 while in reality trajectories could not be executed).

I'll wait for Travis to give us a green checkmark and then I'll merge.

gavanderhoorn commented 5 years ago

I've also updated the install server tutorial on the wiki to mention the new signal.

gonzalocasas commented 5 years ago

I've also updated the install server tutorial on the wiki to mention the new signal.

👍

I actually would prefer to move all those instructions to github, so that they evolve together with the codebase (hence https://github.com/ros-industrial/abb/pull/161), but wiki seems to be the place for it right now.

gavanderhoorn commented 5 years ago

I actually would prefer to move all those instructions to github, so that they evolve together with the codebase (hence #161), but wiki seems to be the place for it right now.

Yes, that's the still ongoing debate about where ROS pkg documentation should live.

The wiki is still the go-to place for many users. If something is not on the wiki, it doesn't exist for many.

gavanderhoorn commented 5 years ago

I'm going to close this in favour of #168.

As I wrote in the PR there: all the hard work has been done by @yamokosk and @keerthanamanivannan in this PR. I've merely cleaned up and reordered some things.

Thanks @yamokosk and @keerthanamanivannan :+1:.

Apologies for taking so long and all the iterations.