Split the urscript into a header and a control loop. #14

urmahp · 2021-02-18T10:06:25Z

The header is only appended to the UR program once, while the control loop is appended for each urcap node. This enables the possibility to have multiple urcap nodes in one program.

This implements #5
It is related to pr #7 and UniversalRobots/Universal_Robots_ROS_Driver#207.
But it is a slightly different approach, with this approach the urscript doesn't have to be changed, it works with how the urscript looks at moment.

A more robust solution might be to add a line to the urscript which makes it possible to distinguish between header and control loop. The current solution is searching for the "socket_open" line, which is the first line of code in the control loop.

fmauch

In general, I think the idea is really good, but we should definitely tackle the generateScript problem mentioned.

src/main/java/com/fzi/externalcontrol/impl/ExternalControlInstallationNodeContribution.java

fmauch · 2021-02-20T20:51:43Z

src/main/java/com/fzi/externalcontrol/impl/ExternalControlInstallationNodeContribution.java

+
+    while (it < splitData.length) {
+      // Find the first line of the control loop
+      if (splitData[it].startsWith(" socket_open(")) {


This seems absolutely brittle. As you mentioned in the OP, it might be better to define some anchors.

Once the script has been moved to the client library issue I will define some anchors to search for.

src/main/java/com/fzi/externalcontrol/impl/ExternalControlInstallationNodeContribution.java

urmahp · 2021-03-10T13:33:03Z

I have added a PR in the ROS driver, where I define the header and control loop part in the control script. These changes are needed in order to be able to have multiple URCap nodes in one program.

urrsk · 2021-03-17T09:56:56Z

@urmahp If the HEADER section is not defined, will it result in the similar behavior as before?
If, I will suggest that we suggest we merge it when ready.

fmauch · 2021-03-17T10:12:51Z

@urmahp If the HEADER section is not defined, will it result in the similar behavior as before?
If, I will suggest that we suggest we merge it when ready.

I think this comment has not been addressed, right?

urmahp · 2021-03-17T10:19:37Z

@urmahp If the HEADER section is not defined, will it result in the similar behavior as before?
If, I will suggest that we suggest we merge it when ready.

Yes we should see same behavior as before the changes if the header is not defined.

I think this comment has not been addressed, right?

It has been addressed in commit efbdfb8. I moved the request part, so that the script is only requested if the URCap is part of the robot program.

fmauch · 2021-03-17T10:40:56Z

@urmahp If the HEADER section is not defined, will it result in the similar behavior as before?
If, I will suggest that we suggest we merge it when ready.

Yes we should see same behavior as before the changes if the header is not defined.

I think this comment has not been addressed, right?

It has been addressed in commit efbdfb8. I moved the request part, so that the script is only requested if the URCap is part of the robot program.

I'll give it another review then, later today / tonight

fmauch

I just have minor suggestions for the documentation, otherwise it looks good. However, I didn't have time to properly test it, yet. Will do that tonight.

README.md

urmahp · 2021-03-17T15:02:07Z

I just have minor suggestions for the documentation, otherwise it looks good. However, I didn't have time to properly test it, yet. Will do that tonight.

Thanks for the review @fmauch.

I will update the documentation first thing tomorrow.

fmauch · 2021-03-17T15:05:47Z

I just have minor suggestions for the documentation, otherwise it looks good. However, I didn't have time to properly test it, yet. Will do that tonight.

Thanks for the review @fmauch.

I will update the documentation first thing tomorrow.

I would be fine with simply accepting my suggestions 😉

urmahp · 2021-03-17T15:07:43Z

Could I just click commit suggestion to accept them?

fmauch · 2021-03-17T15:19:46Z

Could I just click commit suggestion to accept them?

Yes, indeed.

fmauch · 2021-03-17T20:10:12Z

src/main/java/com/fzi/externalcontrol/impl/RequestProgram.java

+    String[] programArray = program.split("\n");
+
+    for (int it = 0; it < programArray.length; ++it) {
+      if (programArray[it].matches("(.*)# HEADER_BEGIN(.*)")) {


Allowing only whitespaces before and after might be safer. If people write comments like

# This does not go into the "# HEADER_BEGIN" section

Suggested change

if (programArray[it].matches("(.*)# HEADER_BEGIN(.*)")) {

if (programArray[it].matches("(\s*)# HEADER_BEGIN(\s*)")) {

This seems to brake the URCap when building, to make it build following if (programArray[it].matches("(\\s*)# HEADER_BEGIN(\\s*)")) will be needed.

When testing it with # This does not go into the "# HEADER_BEGIN" section the only time it seems to be able to find the # HEADER_BEGIN is with the original implementation.

According to this post \\p{Zs} might be a working solution.

I tested \\p{Zs} and it recognizes # HEADER_BEGIN but not # This is a comment about # HEADER_BEGIN

@urmahp If you would be fine with this, I will push that to your branch and then we're ready to merge.

Yes I have also tested that it works and it seems like a good solution, thanks for you help.

src/main/java/com/fzi/externalcontrol/impl/RequestProgram.java

fmauch · 2021-03-17T20:17:52Z

I've tested it and it seems to do its job perfectly!

ipa-lth · 2021-03-18T07:25:40Z

Thx, for your approach!
I was desperatly trying to uncorrelate PRs on the urcap and the driver. This led to a too hacky proposal (which worked for my application, however), but never left WIP status.
Anyways, I am looking forward to rebase to an official release.
In general, I still feel like this is a (elegant) workaround for an urcap-API mishap.

urrsk · 2021-03-18T08:03:29Z

@ipa-lth Thanks for your suggestion and sharing it, we are really grateful for that. It help pushing us in the right direction.

I agree that the control loop could be placed into a script function as well. Thus global variables have the disadvantage/feature that they will show up in the teach pendant's variable window.

The "share codes between the URCap nodes" is a general problem and the "workaround" should also work with other contexts than the ROS driver. See e.g. https://sdurobotics.gitlab.io/ur_rtde/examples/examples.html#use-with-externalcontrol-ur-cap

ipa-lth · 2021-03-18T08:10:28Z

See e.g. https://sdurobotics.gitlab.io/ur_rtde/examples/examples.html#use-with-externalcontrol-ur-cap

Nice lib! Thanks for sharing.
This will be very interesting for those colleques trying to minimize ROS dependencies.

The header is only appended to the UR program once, while the control loop is appended for each urcap node. This enables the possibility to have multiple urcap nodes in one program.

Changed behavior so the program is only requested once the urcap is part of the program.

urrsk mentioned this pull request Feb 18, 2021

Multiple instances of node in ur program not possible #5

Closed

fmauch requested changes Feb 20, 2021

View reviewed changes

fmauch mentioned this pull request Mar 1, 2021

Passing control back and forth between local pendant program and ROS UniversalRobots/Universal_Robots_ROS_Driver#323

Closed

urmahp mentioned this pull request Mar 10, 2021

Added header and control loop definitions. UniversalRobots/Universal_Robots_ROS_Driver#333

Closed

fmauch reviewed Mar 17, 2021

View reviewed changes

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

fmauch approved these changes Mar 17, 2021

View reviewed changes

fmauch reviewed Mar 17, 2021

View reviewed changes

src/main/java/com/fzi/externalcontrol/impl/RequestProgram.java Outdated Show resolved Hide resolved

This was referenced Mar 17, 2021

WIP: make variables global UniversalRobots/Universal_Robots_ROS_Driver#207

Closed

WIP: Encapsulate urscript by driver into unique function #7

Closed

mahp and others added 3 commits March 21, 2021 23:14

Split the urscript into header and controlLoop.

174fc79

The header is only appended to the UR program once, while the control loop is appended for each urcap node. This enables the possibility to have multiple urcap nodes in one program.

Splitting the program based on anchers defining the header

3b5590c

Changed behavior so the program is only requested once the urcap is part of the program.

Only allow whitespaces around anchors

d97f7a2

fmauch force-pushed the multiple_urcap_nodes branch from 7d9dd9b to d97f7a2 Compare March 21, 2021 22:16

fmauch mentioned this pull request Mar 22, 2021

Controller not started on ROS-side although external_control is running. UniversalRobots/Universal_Robots_ROS_Driver#340

Closed

fmauch merged commit dce9e2e into UniversalRobots:master Mar 22, 2021

urmahp deleted the multiple_urcap_nodes branch March 24, 2021 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the urscript into a header and a control loop. #14

Split the urscript into a header and a control loop. #14

urmahp commented Feb 18, 2021

fmauch left a comment

fmauch Feb 20, 2021

urmahp Mar 4, 2021

urmahp commented Mar 10, 2021

urrsk commented Mar 17, 2021

fmauch commented Mar 17, 2021

urmahp commented Mar 17, 2021

fmauch commented Mar 17, 2021

fmauch left a comment

urmahp commented Mar 17, 2021

fmauch commented Mar 17, 2021

urmahp commented Mar 17, 2021

fmauch commented Mar 17, 2021

fmauch Mar 17, 2021

urmahp Mar 18, 2021

fmauch Mar 18, 2021

fmauch Mar 21, 2021

urmahp Mar 22, 2021

fmauch commented Mar 17, 2021

ipa-lth commented Mar 18, 2021

urrsk commented Mar 18, 2021

ipa-lth commented Mar 18, 2021

	if (programArray[it].matches("(.)# HEADER_BEGIN(.)")) {
	if (programArray[it].matches("(\s)# HEADER_BEGIN(\s)")) {

Split the urscript into a header and a control loop. #14

Split the urscript into a header and a control loop. #14

Conversation

urmahp commented Feb 18, 2021

fmauch left a comment

Choose a reason for hiding this comment

fmauch Feb 20, 2021

Choose a reason for hiding this comment

urmahp Mar 4, 2021

Choose a reason for hiding this comment

urmahp commented Mar 10, 2021

urrsk commented Mar 17, 2021

fmauch commented Mar 17, 2021

urmahp commented Mar 17, 2021

fmauch commented Mar 17, 2021

fmauch left a comment

Choose a reason for hiding this comment

urmahp commented Mar 17, 2021

fmauch commented Mar 17, 2021

urmahp commented Mar 17, 2021

fmauch commented Mar 17, 2021

fmauch Mar 17, 2021

Choose a reason for hiding this comment

urmahp Mar 18, 2021

Choose a reason for hiding this comment

fmauch Mar 18, 2021

Choose a reason for hiding this comment

fmauch Mar 21, 2021

Choose a reason for hiding this comment

urmahp Mar 22, 2021

Choose a reason for hiding this comment

fmauch commented Mar 17, 2021

ipa-lth commented Mar 18, 2021

urrsk commented Mar 18, 2021

ipa-lth commented Mar 18, 2021