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

Robot library remove old robots #407

Merged
merged 37 commits into from
Apr 12, 2024
Merged

Conversation

yck011522
Copy link
Contributor

@yck011522 yck011522 commented Feb 20, 2024

This depends on #406, we should review and merge #406 first before this one.

Removed compas_fab.robots.ur5 because it is now part of compas_fab.robots.RobotLibrary.
Removed data files of ur5 and ur10e from src/compas_fab/data/universal_robots because they are now in of src/compas_fab/data/robot_library.

Future constructs should use:

from compas_fab.robots import RobotLibrary
robot = RobotLibrary.ur5(load_geometry=True)

Or from URDF and SRDF from scratch

urdf_filename = compas_fab.get("robot_library/ur5_robot/urdf/robot_description.urdf")
robot_model = RobotModel.from_urdf_file(urdf_filename )

mesh_loader = LocalPackageMeshLoader(compas_fab.get("robot_library/ur5_robot"), "")
robot_model.load_geometry(mesh_loader)

srdf_filename = compas_fab.get("robot_library/ur5_robot/robot_description_semantic.srdf")
semantics = RobotSemantics.from_srdf_file(srdf_filename, robot_model)
robot = Robot(robot_model, semantics=semantics)

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas_fab.robots.CollisionMesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@yck011522
Copy link
Contributor Author

yck011522 commented Feb 22, 2024

@gonzalocasas (whenever you are free, not urgent). I cannot seem to get pass two integration doctests that connects to ROS in the Docker. They both threw the "Cannot Connect to ROS" error.

I see that there are only two files with docs that connects to ROS for testing.

  • src\compas_fab\backends\ros\client.py
  • src\compas_fab\robots\robot.py

Somehow there was no problem connecting to ROS from the client.py file.

I have tried to change the doc example to use a context manager (with RosClient() as ros:) but it doesn't help. I have tried adding timeout=30 to the connection but it doesn't help. I cannot reproduce the error in my local environment with python 3.8 (Windows), so maybe it is a Ubuntu thing?

@yck011522
Copy link
Contributor Author

yck011522 commented Feb 22, 2024

@gonzalocasas (whenever you are free, not urgent). I cannot seem to get pass two integration doctests that connects to ROS in the Docker. They both threw the "Cannot Connect to ROS" error.

I see that there are only two files with docs that connects to ROS for testing.

  • src\compas_fab\backends\ros\client.py
  • src\compas_fab\robots\robot.py

Somehow there was no problem connecting to ROS from the client.py file.

I have tried to change the doc example to use a context manager (with RosClient() as ros:) but it doesn't help. I have tried adding timeout=30 to the connection but it doesn't help. I cannot reproduce the error in my local environment with python 3.8 (Windows), so maybe it is a Ubuntu thing?

Phew, Finally!! I just tried to change the Integration Test to run on py3.11 instead of 3.8 and the connection problem goes away. Also tried 3.10 and it does not work. Can you see if you are okay with py3.11 for Integration. @gonzalocasas

Edit: Okay, it is driving me crazy because it appears that the Ci just went through by sheer luck. The commit test in 0e25492 was failing for no reason. Damn. I officially declare that I'm going crazy and we should merge now because the test just happened to go Green at the current commit.

@yck011522 yck011522 marked this pull request as ready for review March 8, 2024 13:49
@yck011522
Copy link
Contributor Author

@gonzalocasas How do you feel about the rather frequent testdocs failing. Or if you have any insight why it is failing.

@yck011522 yck011522 mentioned this pull request Mar 8, 2024
10 tasks
@yck011522 yck011522 self-assigned this Apr 12, 2024
Comment on lines 150 to 168
def run(self, timeout=2.0):
"""Kick-starts a non-blocking event loop.

Args:
timeout: Timeout to wait until connection is ready.
"""
import threading
import time
from roslibpy.core import RosTimeoutError

t1 = time.time()
wait_connect = threading.Event()
self.factory.on_ready(lambda _: wait_connect.set())

self.factory.manager.run()
if not wait_connect.wait(timeout):
t2 = time.time()
raise RosTimeoutError("Failed to connect to ROS. Start Time: {}, Time elapsed: {}".format(t1, t2 - t1))

Copy link
Member

Choose a reason for hiding this comment

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

Please delete this one! ;)

Comment on lines -33 to -49


@pytest.fixture(scope="function", autouse=True)
def connect_to_ros(request, doctest_namespace):
if request.module.__name__ == "compas_fab.robots.robot":
doctest_namespace["robot"] = Robot()
yield
elif request.module.__name__ == "compas_fab.robots.planning_scene":
with RosClient() as client:
robot = Robot(client)

doctest_namespace["client"] = client
doctest_namespace["robot"] = robot

yield
else:
yield
Copy link
Member

Choose a reason for hiding this comment

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

Yay! Out with the trash!

Copy link
Member

@gonzalocasas gonzalocasas left a comment

Choose a reason for hiding this comment

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

🚀 LGTM!

@yck011522 yck011522 merged commit 08773ba into main Apr 12, 2024
15 checks passed
@yck011522 yck011522 deleted the robot_library_remove_old_robots branch April 12, 2024 16:28
@jf---
Copy link

jf--- commented Apr 12, 2024

Amazing work @yck011522 !

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