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

Adds robot status publishing to Rapid driver #145

Conversation

yamokosk
Copy link
Contributor

@yamokosk yamokosk commented Mar 1, 2018

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
Copy link
Member

@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 👍

@gonzalocasas
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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

@gavanderhoorn
Copy link
Member

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

@gonzalocasas
Copy link
Contributor

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

@gonzalocasas
Copy link
Contributor

@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
Copy link
Contributor

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.


! Determine in_error
if (message.error_code >= 1) AND (message.error_code <= 90) THEN
Copy link
Member

Choose a reason for hiding this comment

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

It's been a while for me: are there no errors with ERRNO > 90?

Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm misunderstanding something, errors can have nrs > 90, so this line may not be sufficient for detecting whether the system is in error or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that comes out of the following description of errnum on RAPID reference:

image

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I had understood that as well. But that bit of doc doesn't seem to limit error nrs to [1, 90] specifically.

Copy link
Member

Choose a reason for hiding this comment

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

I'd forgotten about this: a knowledgable source has confirmed that error nrs can definitely be > 90, so this implementation is not sufficient to determine controller error status.

Most likely a combination of system outputs would be better.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't know all of them, but I would at least check those that have to do with e-stop status.

Perhaps @jontje can suggest some others?

Copy link

Choose a reason for hiding this comment

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

Hello,

errnum = [1, 90] are reserved for rasing custom errors, while predefined errors can be higher (e.g. ERR_ROBLIMIT = 1075).

Anyway, I would use the system output Execution Error, and I think it should be enough for determining the message.in_error field. This system output is set to HIGH when an error has occured that doesn't have any error handler.

For example:
system output

And then check the signal in an IF-statement with DOutput(signalExecutionError) = HIGH

Copy link
Contributor

Choose a reason for hiding this comment

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

@jontje this was well explained, thank you!
@gavanderhoorn I made a commit with this change. I updated the documentation too. Please review it.

Copy link
Member

Choose a reason for hiding this comment

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

@keerthanamanivannan: b97cbcb seems to remove the bit where the code retrieves the current error and initialises the message.error_code field with it. Was that intentional?

That field should be set to whatever the current error code is when the system is in an error mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gavanderhoorn ahh I see. Sorry. I've fixed it.

@gavanderhoorn
Copy link
Member

Thanks for the check @gonzalocasas 👍

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
Copy link
Member

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

@gonzalocasas
Copy link
Contributor

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

@gavanderhoorn
Copy link
Member

gavanderhoorn commented May 8, 2018

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

@yamokosk yep, screenshots would help

@gavanderhoorn
Copy link
Member

@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
Copy link
Member

@yamokosk: 🛎️

@gavanderhoorn
Copy link
Member

And another ping.

@keerthanamanivannan
Copy link
Contributor

keerthanamanivannan commented Oct 17, 2018

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
Copy link
Contributor

@gavanderhoorn @gonzalocasas ping.

@gavanderhoorn
Copy link
Member

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
Copy link
Contributor

keerthanamanivannan commented Oct 24, 2018

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

@gavanderhoorn
Copy link
Member

Do you have a wiki account?

@keerthanamanivannan
Copy link
Contributor

Yes, I do.

@gavanderhoorn
Copy link
Member

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
Copy link
Contributor

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

@gavanderhoorn
Copy link
Member

You can request that over in ros-infrastructure/roswiki#258.

@gavanderhoorn
Copy link
Member

@keerthanamanivannan ?

@keerthanamanivannan
Copy link
Contributor

@gavanderhoorn Please review. Thanks!

@jontje
Copy link

jontje commented Jan 10, 2019

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
Copy link
Member

gavanderhoorn commented Jan 10, 2019

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
Copy link
Contributor

keerthanamanivannan commented Jan 10, 2019

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
Copy link

jontje commented Jan 10, 2019

@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:

  • 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...

@keerthanamanivannan
Copy link
Contributor

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
Copy link
Contributor

@gavanderhoorn how are we moving forward with this?

@gavanderhoorn
Copy link
Member

@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
Copy link
Contributor

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

Better aligned with other simple_message server implementations.
So take that state into account. Only when motion task (not motion server!) is running can motions be executed.
@gavanderhoorn
Copy link
Member

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
Copy link
Member

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

@gonzalocasas
Copy link
Contributor

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 #161), but wiki seems to be the place for it right now.

@gavanderhoorn
Copy link
Member

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
Copy link
Member

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 👍.

Apologies for taking so long and all the iterations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants