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.

Sign up for GitHub

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

Jump to bottom

driver: add support for MotoROS read & write M registers #439

Merged
merged 16 commits into from
Feb 9, 2022
Merged

driver: add support for MotoROS read & write M registers #439

merged 16 commits into from
Feb 9, 2022

Conversation

gavanderhoorn
Copy link
Member

@gavanderhoorn gavanderhoorn commented Dec 12, 2021 •
edited
Loading

As per subject.

This is just the copy-paste boilerplate search-replace result.

Hasn't been tested on HW yet.

Edit: and just repeating #361 (comment) here for future readers:

While convenience is why we're adding them here, I'm going to repeat my advice (which I always give when talking about ROS, robot drivers and IO): consider using a proper fieldbus for industrial deployments. Having to add IO support to each and every robot driver doesn't scale very well, and will most likely use custom implementations of protocols which do not have the same level of support, integration nor testing as any of the available (software defined) fieldbuses in existence.

Even a simple wrapper around a modbus/tcp client would be a good option.

@gavanderhoorn
Copy link
Member Author

I expect conflicts with #435, but we'll resolve those when they happen.

@gavanderhoorn gavanderhoorn mentioned this pull request Dec 12, 2021
Closed
7 tasks
@akashjinandra
Copy link

@gavanderhoorn would you like some help testing on hardware?

@gavanderhoorn
Copy link
Member Author

gavanderhoorn commented Dec 14, 2021 •
edited
Loading

@akashjinandra: sure, the more testing this gets the better.

You should just have to build motoman_driver with this PR merged and make sure to have a recent-ish MotoROS installed on the controller (as in: anything >= 1.9.2).

Keep in mind that I haven't tested this at all yet, so it could not work, or it could work immediately, or anything in between.

@gavanderhoorn gavanderhoorn mentioned this pull request Dec 14, 2021
Merged
@gavanderhoorn gavanderhoorn marked this pull request as ready for review December 15, 2021 09:57
@gavanderhoorn gavanderhoorn changed the title WIP: add support for MotoROS read & write M registers driver: add support for MotoROS read & write M registers Dec 15, 2021
@gavanderhoorn gavanderhoorn mentioned this pull request Dec 15, 2021
Merged
akashjinandra
akashjinandra previously approved these changes Dec 15, 2021
View reviewed changes
Copy link

@akashjinandra akashjinandra left a comment

Choose a reason for hiding this comment

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

Hello @gavanderhoorn I tested this on a YRC1000 controller. Both the read and the write work well. I also tested reading out of range and writing out of range both threw errors, which was expected.

@gavanderhoorn
Copy link
Member Author

Thanks @akashjinandra 👍

@akashjinandra
Copy link

Hello, anything stopping this from getting merged?

@gavanderhoorn gavanderhoorn mentioned this pull request Dec 22, 2021
Merged
@gavanderhoorn
Copy link
Member Author

Tested on our YRC1000. Works. No regressions.

@akashjinandra
Copy link

Hello is there anything I can do to help get this merged?

@gavanderhoorn
Copy link
Member Author

I've switched the value fields to uint16, as that's what M-registers are/support.

@gavanderhoorn
Copy link
Member Author

@marip8: could you please also take a look at this one?

@gavanderhoorn
Copy link
Member Author

I'll fix the conflicts here after #435 gets merged, as that PR touches the same files.

@akashjinandra akashjinandra mentioned this pull request Feb 8, 2022
Closed
@gavanderhoorn
Copy link
Member Author

Friendly ping @marip8

@gavanderhoorn
driver: expose ReadMRegister as a ROS service
298f4fa 
Using the updated IoCtrl class to send the SimpleMessage service request for us.
@gavanderhoorn
driver: add write M register msg ID
ad1ce1e 
According to REP-I0004.
@gavanderhoorn
driver: add WriteMRegister and reply data structure
a082c02
@gavanderhoorn
driver: add WriteMRegister and reply msg
f58d406
@gavanderhoorn
driver: support WriteMRegister in IoCtrl
3eef371 
Encapsulates the SimpleMessage level of the WriteMRegister service invocation.
@gavanderhoorn
msgs: add new WriteMRegister service
a08d8df 
Intended to wrap the MotoROS WriteMRegister service.
@gavanderhoorn
driver: expose WriteMRegister as a ROS service
1beb0e4 
Using the updated IoCtrl class to send the SimpleMessage service request for us.
@gavanderhoorn
msgs: warn against adding 1e6 to M reg addresses
167b6c3 
It's not needed, even though the Yaskawa controller manuals will show the addresses as such.
@gavanderhoorn
msgs: warn against leading zeros in m-register addresses
bfce97d 
To prevent accidental use of octal notation.
@gavanderhoorn
msgs: switch M-register values to unsigned 16 bit
2571b66 
As that's what M-registers support.
@gavanderhoorn
msgs: even write_m_register returns something
5888013 
Namely: a boolean indicating success and a human readable error message in case of failure.

So remove confusing statement from the doc header.
@gavanderhoorn
Copy link
Member Author

Resolved the conflict.

We'll merge if/when CI turns green.

@gavanderhoorn gavanderhoorn merged commit 642dad8 into ros-industrial:kinetic-devel Feb 9, 2022
@gavanderhoorn gavanderhoorn deleted the driver_rw_mregs branch February 9, 2022 16:21
@gavanderhoorn
Copy link
Member Author

CI was green, so merging.

Thanks @marip8, @EricMarcil and @ted-miller 👍

And thank you @akashjinandra for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EricMarcil EricMarcil EricMarcil left review comments

@akashjinandra akashjinandra akashjinandra left review comments

@marip8 marip8 marip8 approved these changes

Assignees
No one assigned
Labels
awaiting-review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gavanderhoorn @akashjinandra @EricMarcil @marip8