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

Added check to ensure that the targets are reachable within the robot… #184

Merged
merged 21 commits into from
Sep 10, 2024

Conversation

urmahp
Copy link
Collaborator

@urmahp urmahp commented Sep 26, 2023

…s limits

This will break the test_joint_trajectory_position_interface test in the ROS Driver as part of the test is designed to activate the speed scaling by sending unreachable targets. These changes therefore require that we update the test in the ROS Driver.

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.02%. Comparing base (d21a402) to head (2c2ed96).
Report is 19 commits behind head on master.

Current head 2c2ed96 differs from pull request most recent head 92ee816

Please upload reports for the commit 92ee816 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
- Coverage   72.05%   72.02%   -0.04%     
==========================================
  Files          71       71              
  Lines        2652     2652              
  Branches      337      337              
==========================================
- Hits         1911     1910       -1     
- Misses        555      557       +2     
+ Partials      186      185       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

examples/full_driver.cpp Show resolved Hide resolved
resources/external_control.urscript Outdated Show resolved Hide resolved
resources/external_control.urscript Outdated Show resolved Hide resolved
resources/external_control.urscript Outdated Show resolved Hide resolved
resources/external_control.urscript Outdated Show resolved Hide resolved
@fmauch
Copy link
Collaborator

fmauch commented Jun 10, 2024

While this is in a state where everything seems to do its job as intended, it's actually not as simple as that. Since we use servoj for executing trajectories in the position-based case, there is some delay between the moment where we call servoj and servoj actually starting the motion. Thus, simply looking at the current positions and the ones coming in as new targets will not necessarily reflect the actual velocities. I'll try to add a test showing this.

@fmauch fmauch self-assigned this Jun 19, 2024
@fmauch
Copy link
Collaborator

fmauch commented Jul 10, 2024

I've updated this so that it doesn't terminate the program but instead ignores the commands as long as they are infeasible. With this, when I start this with a driver that always commands [0,0,0,0,0,0], the robot doesn't move until we send a target that is close to its current position.

I used a non-blocking popup as we discussed, but thinking about this, a blocking popup might actually be more feasible, don't you agree @urrsk ?

@fmauch fmauch requested review from urrsk and VinDp July 10, 2024 14:16
resources/external_control.urscript Outdated Show resolved Hide resolved
resources/external_control.urscript Outdated Show resolved Hide resolved
resources/external_control.urscript Outdated Show resolved Hide resolved
resources/external_control.urscript Outdated Show resolved Hide resolved
@fmauch fmauch requested a review from urrsk July 15, 2024 14:12
@fmauch
Copy link
Collaborator

fmauch commented Jul 16, 2024

@urrsk Now everything is where I think I would like it to be and the tests are in line, as well. If you could take another look, that would be awesome. Or @VinDp.

resources/external_control.urscript Show resolved Hide resolved
socket_send_int(TRAJECTORY_RESULT_SUCCESS, "trajectory_socket")
textmsg("Trajectory finished")
socket_send_int(trajectory_result, "trajectory_socket")
textmsg("Trajectory finished with result ", trajectory_result)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe convert the trajectory_result to a readable string before sending it to the log

@fmauch
Copy link
Collaborator

fmauch commented Jul 18, 2024

@urrsk I implemented the things discussed. I was a bit puzzled as I found out that when ignoring the first trajectory point, apparently the read-from-socket call times out. That's why I increased the timeout in this PR. However, that doesn't seem right to me. I checked that sending out the trajectory points only took a few microseconds in the test program, so the timeout could either result from

  • The network delaying the package
  • Delay between socket send command and package actually entering the network layer
  • Socket read in the thread being at lower priority?

I quickly modified the trajectoryThread() in order to just print the points to a textmsg instead of executing them and that showed the same behavior. The first trajectory point was printed fine, the others timed out.

@urrsk
Copy link
Member

urrsk commented Jul 18, 2024

@urrsk I implemented the things discussed. I was a bit puzzled as I found out that when ignoring the first trajectory point, apparently the read-from-socket call times out. That's why I increased the timeout in this PR. However, that doesn't seem right to me. I checked that sending out the trajectory points only took a few microseconds in the test program, so the timeout could either result from

  • The network delaying the package
  • Delay between socket send command and package actually entering the network layer
  • Socket read in the thread being at lower priority?

I quickly modified the trajectoryThread() in order to just print the points to a textmsg instead of executing them and that showed the same behavior. The first trajectory point was printed fine, the others timed out.

Interesting, it seems we need to do a bit more investigation on this

@fmauch
Copy link
Collaborator

fmauch commented Sep 6, 2024

@urrsk as discussed I use the higher timeout when the robot isn't moving, yet only. Could you please have another look?

Accessing a list of constant strings would require updating the minimum required version. This change doesn't really justify that in my opinion.
@fmauch
Copy link
Collaborator

fmauch commented Sep 6, 2024

I'll investigate why the noetic tests fail. A quick look didn't make it clear to me.

.github/workflows/industrial-ci.yml Outdated Show resolved Hide resolved
@fmauch
Copy link
Collaborator

fmauch commented Sep 9, 2024

I'll investigate why the noetic tests fail. A quick look didn't make it clear to me.

Found it. The actual test wasn't working properly and this got uncovered by this PR. Necessary changes are being implemented in UniversalRobots/Universal_Robots_ROS_Driver#721

@fmauch fmauch self-requested a review September 10, 2024 07:47
Copy link
Collaborator

@fmauch fmauch left a comment

Choose a reason for hiding this comment

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

Approving so my change-request block goes away.

Copy link
Member

@urrsk urrsk left a comment

Choose a reason for hiding this comment

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

I have mainly focused on the script code in my review

@fmauch fmauch merged commit f6cd468 into UniversalRobots:master Sep 10, 2024
18 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants