Skip to content

Conversation

@enrico391
Copy link
Collaborator

@enrico391 enrico391 commented Jun 5, 2025

Closes #3

Comment on lines 18 to 30
if urdf_path is None and robot_description is None:
raise ValueError("URDF path or robot description is required but was not provided")

if urdf_path is not None and robot_description is not None:
raise ValueError("Only one of URDF path or robot description should be provided, not both")

# Load the robot model from URDF or XML
if urdf_path is not None:
self.model = pin.buildModelFromUrdf(urdf_path)

if robot_description is not None:
self.model = pin.buildModelFromXML(robot_description)

Copy link
Member

Choose a reason for hiding this comment

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

Do we really have a need to support both urdf paths and the robot description?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made it because if we want to use the library in a non ROS environment we can parse the URDF file. Maybe there is a method to convert the xml or the urdf in the same format , I'll check it

Copy link
Member

Choose a reason for hiding this comment

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

ok

Copy link
Member

Choose a reason for hiding this comment

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

Check it and let me know. What will be the type of both the types? both strings, one is XML or etc?. Let me know

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

robot_description is a string contains all the urdf description, instead urdf_path is the URL of the file .urdf
Maybe I could get the same string provided by the robot_description in the case of urdf path using read file and save the string inside the file. Like this:

with open("my_robot.urdf", "r") as file:
robot_description = file.read()

but I don't like so much this solution

Or in a easier way I could leave only the robot_description as a input ( string provided by robot_description) and if we will use the library with urdf path we will convert it in a string as the method above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought about another method and I saw that you can specify multiply type hints in the input, so I thougth that I can specify the type hint as " str | Path " and check when the input is str and I'll use buildFromXML, instead if the input is a path I'll use the other method.

Could it be a good approach ?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, check if you can have multiple constructors with different single argument types. This way it is better for you

enrico391 and others added 2 commits June 5, 2025 16:52
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Comment on lines 18 to 30
if urdf_path is None and robot_description is None:
raise ValueError("URDF path or robot description is required but was not provided")

if urdf_path is not None and robot_description is not None:
raise ValueError("Only one of URDF path or robot description should be provided, not both")

# Load the robot model from URDF or XML
if urdf_path is not None:
self.model = pin.buildModelFromUrdf(urdf_path)

if robot_description is not None:
self.model = pin.buildModelFromXML(robot_description)

Copy link
Member

Choose a reason for hiding this comment

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

Check it and let me know. What will be the type of both the types? both strings, one is XML or etc?. Let me know

Comment on lines 48 to 52
if extForce is None:
tau = pin.rnea(self.model, self.data, q, qdot, qddot)

if extForce is not None:
tau = pin.rnea(self.model, self.data, q, qdot, qddot, extForce)
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/stack-of-tasks/pinocchio/blob/master/bindings/python/algorithm/expose-rnea.cpp#L36-L47

I see it is a list of extForces right?, this would be easier with the type hinting

Suggested change
if extForce is None:
tau = pin.rnea(self.model, self.data, q, qdot, qddot)
if extForce is not None:
tau = pin.rnea(self.model, self.data, q, qdot, qddot, extForce)
if extForce :
tau = pin.rnea(self.model, self.data, q, qdot, qddot, extForce)
else:
tau = pin.rnea(self.model, self.data, q, qdot, qddot)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it is the vector provided by create_ext_force

joint_id = self.model.frames[frame_id].parentJoint

# force in the world frame
ext_force_world = pin.Force(np.array([0.0, 0.0, mass * -9.81]), np.array([0.0, 0.0, 0.0])) # force in z axis of the world frame
Copy link
Member

Choose a reason for hiding this comment

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

Can you add link to the force convention that it is in the global frame not the local frame? Missing this information in the documentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I change the convention of the force after when I use:
fext[joint_id] = self.data.oMi[joint_id].actInv(ext_force_world)

I watched this example : https://github.com/stack-of-tasks/pinocchio/blob/master/examples/static-contact-dynamics.py

Copy link
Member

Choose a reason for hiding this comment

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

Ok then just add that to the documentation that it is a local reference

def get_random_configuration(self):
"""
Get a random configuration for configuration and velocity vectors.
:return: Random configuration vector.
Copy link
Member

@saikishor saikishor Jun 5, 2025

Choose a reason for hiding this comment

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

Suggested change
:return: Random configuration vector.
:return: Random configuration vector.
:todo: Generate configuration within the joint limits

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:return: Random configuration vector.
:return: Random configuration vector.
:todo: Generate configuration within the joint limits

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Just few things. Rest looks good to me

def get_random_configuration(self):
"""
Get a random configuration for configuration and velocity vectors.
:return: Random configuration vector.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:return: Random configuration vector.
:return: Random configuration vector.
:todo: Generate configuration within the joint limits

commit small changes in mass matrix name and function method

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Hello!

I don't see the package creation as in : https://docs.ros.org/en/foxy/Tutorials/Beginner-Client-Libraries/Creating-Your-First-ROS2-Package.html

You will need to do that so, I can merge it

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Not perfect but will do for the first version

@saikishor saikishor merged commit 73bc4c0 into master Jun 9, 2025
@saikishor saikishor deleted the enrico_calculator branch June 9, 2025 16:08
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.

Change the package naming to dynamic_payload_analysis_core

3 participants