Icub dev #241

omareldardear · 2024-12-09T15:55:43Z

Dear all,

I create here the main pull request for the major updates of the integration of the iCub robot into pycram.
In the next 2 days, I will replace some prints with exception handling and add some documentation.

I create this pull request to initiate the process to speed up the integration.
Please have a look and let me know if there are any reviews

There are some requirements to have the code running. I will add all the details in README soon.

Thanks

added test move joints.

…s - upating few tests

…simulation

Tigul · 2024-12-09T19:11:46Z

src/pycram/process_module.py

@@ -419,3 +419,9 @@ def close(self) -> Type[ProcessModule]:
        """
        raise NotImplementedError(
            f"There are no Process Modules for '{inspect.currentframe().f_code.co_name}' for robot '{self.robot_name}'")
+
+    def exit(self)->None:


The exit method for Process modules is a nice idea.
But the actual code to exit the connection to a robot should be directly in the Process module.
Please also add an exit method to the ProcessModule parent class and then call the currently loaded ProcessModule.exit method from this method

isn't this what i did ?
Would you explain more this part

Yes what you did is correct, I was just confused about the structure of the ProcessModuleManager

Tigul · 2024-12-09T19:14:27Z

src/pycram/process_modules/icub_process_modules.py

+from ..datastructures.enums import JointType, ObjectType, Arms, ExecutionType, GripperState
+from ..external_interfaces import giskard
+from ..external_interfaces.robokudo import *
+import yarp


This add yarp as a hard dependency. Wrap this into a try, catch and add a log function in case the imports fail. Look here for reference

Tigul · 2024-12-09T19:14:57Z

src/pycram/process_modules/icub_process_modules.py

+    for i in hand_values:
+        target_loc.addFloat32(i)
+
+    print("command Ready to send to iCub  part tcp")


Use pycram.ros.logging instead of print statements, throughout the whole file

Tigul · 2024-12-09T19:17:08Z

src/pycram/process_modules/icub_process_modules.py

+            return False
+
+HAND_CLOSED = [60.0,60.0,0,85.0,20,85,20,85,85]
+HAND_OPENED = [0   ,0   ,0,0   ,0 ,0 ,0 ,0 ,0]


I guess these are joint values for the hand, they are already in the robot_description use these insetad and remove the global variable

not all the joint are directly actuated 1:1.
In the robot description I have the values for every joint for close and open. However the control joints are different and also different values.
So this list is the actuated control values. which is only used for the real robot
What do you think ?

Ah ok, then you cloud put them in the move_gripper process module of the real robot. This way it should be clear where they are used.

Tigul · 2024-12-09T19:20:10Z

src/pycram/process_modules/icub_process_modules.py

+from ..external_interfaces.robokudo import *
+import yarp
+
+from pycram.yarp_utils.yarp_networking import NO_ACK_VOCAB, ACK_VOCAB, open_rpc_client_port, open_buffered_bottle_port, \


I think it is better to move the calls to yarp into its own file and put this in external_interfaces instead of yarp_utils. So people know what purpose yarp has.

Tigul · 2024-12-09T19:41:33Z

test/test_icub.py

+        ProcessModuleManager.execution_type = ExecutionType.SIMULATED
+        cls.viz_marker_publisher = VizMarkerPublisher()
+
+    def test_move_joint_direct(self):


There is no assert here

I deleted the whole file, this was just for me to test the implementation.
I think I will try to make jupyter notebook on virtual labs for this instead of here

Tigul · 2024-12-09T19:42:15Z

test/test_icub.py

+    def test_move_joints_simulated(self):
+        with simulated_robot:
+            MoveJointsMotion(["torso_roll"], [math.radians(-20.0)]).perform()
+            sleep(3)


Are these sleeps necessary? Are the calls in yarp asynchronous?

no, not necessary, it was just too fast to see it in the simulation. Anyways, the whole file os removed :)

Tigul · 2024-12-09T19:42:58Z

test/test_icub.py

+            MoveGripperMotion(GripperState.CLOSE, Arms.LEFT).perform()
+            sleep(10)
+            MoveGripperMotion(GripperState.OPEN, Arms.RIGHT).perform()
+            MoveGripperMotion(GripperState.OPEN, Arms.LEFT).perform()


Also no asserts here. So you are only executing the motions and checking if nothing crashes

Tigul · 2024-12-09T19:44:32Z

test/test_icub.py

+
+
+    def test_try_world(self):
+        sleep(100)


What? Why?
I guess this was for testing, in this case please delete it

Tigul · 2024-12-09T19:46:19Z

test/test_icub.py

+
+
+        with real_robot:
+            yarpModule = run_icub_state_updater(['/', '--robot', 'icubSim'])


There is unittest.skip for this case, look here for reference

sunava · 2024-12-11T13:13:52Z

src/pycram/process_modules/icub_process_modules.py

Please checkout the new default_process_modules and how specific process modules are handled

I didn't change the default processing modules for the simulated robot.
I used them as it is.

For the real robot, I implemented the specific functionalities to control icub in the right way.
Note: the tcp/looking/ etc poses are expected to be w.r.t. robot frame. At the moment as we don't have a simulated world for the real robot. maybe lateron, we can easily do the transformation.

If there is any other notes, please clarify what you mean.
Thanks

Tigul · 2024-12-11T15:13:47Z

src/pycram/external_interfaces/yarp_networking.py

+    """
+        Initializes the YARP network.
+
+        **Returns:**


Please stick to the rst standard

Tigul · 2024-12-11T15:14:28Z

src/pycram/external_interfaces/yarp_networking.py

@@ -0,0 +1,66 @@
+from typing_extensions import Tuple,Optional
+
+import yarp


I think this needs a try, catch block

Tigul · 2024-12-11T15:18:42Z

src/pycram/process_modules/icub_process_modules.py

+        loginfo("Received: another reply")
+        return False
+
+def update_part(state_port,ctp_port, joint_to_change_idx, joints_to_change_pos):


Please add documentation here

Tigul · 2024-12-11T15:20:44Z

src/pycram/process_modules/icub_process_modules.py

+        This point can either be a position or an object.
+        """
+
+    def __init__(self,lock,cmd_port:yarp.RpcClient):


The type annotation yarp.RpcClient will not work if yarp is not there. You can put it in quotation marks to prevent the interpreter evaluating it

omareldardear · 2024-12-11T15:30:31Z

I noticed that the CI/CD checks failed because of the imports of yarp.
I did a fix and pushed the changes.

omareldardear · 2024-12-11T15:39:37Z

let me check again your last reviews. I just noticed it

omareldardear · 2024-12-11T16:21:57Z

I went through the code again and all your comments.
You can check again now @Tigul

omareldardear added 15 commits December 4, 2024 15:04

[TestMultiverse] Fix when multiverse is not found.

5dca4e7

[adding iCub] first test for robot description

6a15414

[development] first valid test on icub (looking)

c083502

[feature] icub processing module add move joints motion action

0632c6c

[feature] first test icub on pybullet

be79bf2

[iCub] (WIP) added update part to robot description.

3a0f739

added test move joints.

[iCub] updating robot description to include actuated joints

0a788fc

[iCub] updating robot description to include fingers and left camera

316e7a1

[iCub] radian angles - meshes scale - using default simulation method…

b0a980e

…s - upating few tests

[iCub] updating real robot states using yarp updater

6083ad7

[iCub] updating real robot states using yarp updater with fingers

087d86d

[iCub] adding gripper close and open fuctionalities in both real and …

05d791c

…simulation

Merge branch 'dev' of github.com:cram2/pycram into icub_dev

8b305fb

[iCub] rename test

be8500b

clean up

9b62853

Tigul requested changes Dec 9, 2024

View reviewed changes

omareldardear added 3 commits December 10, 2024 15:16

[iCub] adding failure and logging correctly

7ba39af

[iCub] minor refactoring

13e8c5b

[iCub] minor refactoring and documentation

9a3b7e3

omareldardear requested a review from Tigul December 11, 2024 09:39

omareldardear added 3 commits December 11, 2024 10:39

Merge branch 'dev' into icub_dev

8124de0

[iCub] fix

ec1fdb5

resolve conflicts

837e4aa

sunava reviewed Dec 11, 2024

View reviewed changes

sunava self-requested a review December 11, 2024 13:14

Merge remote-tracking branch 'original_pycram/dev' into icub_dev

abd246c

Tigul reviewed Dec 11, 2024

View reviewed changes

[iCub] by pass yarp

2a07761

[iCub] minor structure and documentation changes

3da3b33

omareldardear added 2 commits December 11, 2024 17:42

[iCub] minor fix

949efc0

add icub demo

c78f026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Icub dev #241

Icub dev #241

omareldardear commented Dec 9, 2024 •

edited

Loading

Tigul Dec 9, 2024

omareldardear Dec 10, 2024

Tigul Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 10, 2024

Tigul Dec 11, 2024

omareldardear Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 11, 2024

Tigul Dec 9, 2024

omareldardear Dec 11, 2024

sunava Dec 11, 2024 •

edited

Loading

omareldardear Dec 11, 2024

Tigul Dec 11, 2024

omareldardear Dec 11, 2024

Tigul Dec 11, 2024

Tigul Dec 11, 2024

omareldardear Dec 11, 2024

Tigul Dec 11, 2024

omareldardear commented Dec 11, 2024

omareldardear commented Dec 11, 2024

omareldardear commented Dec 11, 2024 •

edited

Loading



		with real_robot:
		yarpModule = run_icub_state_updater(['/', '--robot', 'icubSim'])

		@@ -0,0 +1,66 @@
		from typing_extensions import Tuple,Optional

		import yarp

Icub dev #241

Are you sure you want to change the base?

Icub dev #241

Conversation

omareldardear commented Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunava Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omareldardear commented Dec 11, 2024

omareldardear commented Dec 11, 2024

omareldardear commented Dec 11, 2024 • edited Loading

omareldardear commented Dec 9, 2024 •

edited

Loading

sunava Dec 11, 2024 •

edited

Loading

omareldardear commented Dec 11, 2024 •

edited

Loading