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

Implement RT priority assignment #257

Open
wants to merge 4 commits into
base: melodic-devel
Choose a base branch
from

Conversation

balandbal
Copy link

The README for the (KRC4) RSI hardware interface recommends setting the PC real-time priority for it if the connection to RSI is sporadically dropped. The hardware interface for Universal Robots arms sets this priority when it can; I thought it sensible to adopt that part of the code into the KUKA interface.

As far as I could determine at a glance, the implementation should not conflict with anything already in the KUKA interface, though my experience with C++ and the source code of the hardware interface is limited.

Once our cell is built completely in the following weeks, I will be using the patch more extensively. If there are any specific tests that would be great to run, I am willing to try and set them up.

The implementation is as used in the Universal Robots ROS driver.
@simonschmeisser
Copy link
Collaborator

Thanks for your PR! This looks fine now from the formal side. I have no experience with RSI (yet) so I would like to wait for either you or me to test this patch before merging and ideally for someone with real time experience to also review it.

Brought over from the UR driver

Co-authored-by: Simon Schmeisser <s.schmeisser@gmx.net>
@gavanderhoorn
Copy link
Member

I believe, as this was copied from the/a UR driver, that the provenance of this code should be clearly stated, including the license it is covered by, and the original author(s).

IIRC, the code actually comes from the Franka Emika driver, so perhaps the UR driver authors already didn't abide by the license(s) there, but that doesn't mean we can do the same here.

@balandbal
Copy link
Author

I believe, as this was copied from the/a UR driver, that the provenance of this code should be clearly stated, including the license it is covered by, and the original author(s).

IIRC, the code actually comes from the Franka Emika driver, so perhaps the UR driver authors already didn't abide by the license(s) there, but that doesn't mean we can do the same here.

I added the license, copyright and author information in c51e1b4.

  • To make the distinction cleaner between the BSD and Apache-2.0 code (the UR driver comes subject to that), I factored out the priority assigning into the separate header kuka_rsi_hw_interface/assign_rt_priority.h.
  • Apache-2.0 requires retaining a copy of the license in the project root; kuka_experimental already has one, so I thought it was enough not to add that again.
  • To notify those reading the source about the mixed licensing, I modified the Software License Agreements in kuka_hardware_interface_node.cpp and kuka_hardware_interface.h.
  • To credit the original author, I added them to the package.xml and as authors to the new header.

Adding a notice about the mixed licensing seemed excessive for kuka_hardware_interface.cpp, though it also includes the header with the priority assignment through kuka_hardware_interface.h.

@gavanderhoorn
Copy link
Member

thanks for the update.

However, calling a function does not change the copyright on the file that calls it. Neither does including a .h. So the changes to the files that just call the new function or include the header would not be needed (kuka_rsi_hw_interface/include/kuka_rsi_hw_interface/kuka_hardware_interface.h fi).

I'm also not sure whether Felix would have to be added to the manifest: many people have contributed to these packages, and we don't list all of them. Only the main contributors are listed.

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.

3 participants