Skip to content

Topic/extend whole body state ros publisher#16

Merged
cmastalli merged 25 commits intoRobotMotorIntelligence:develfrom
Sergim96:topic/extend_WholeBodyStateRosPublisher
Apr 6, 2024
Merged

Topic/extend whole body state ros publisher#16
cmastalli merged 25 commits intoRobotMotorIntelligence:develfrom
Sergim96:topic/extend_WholeBodyStateRosPublisher

Conversation

@Sergim96
Copy link
Copy Markdown

Hi @cmastalli

This PR brings a new message type to be able to communicate dynamic parameters between different nodes.
It also implements the corresponding publisher/subscriber and unittests.

Additionally I have extended the WholeBodyStateRosPublisher in order to publish accelerations as well #5

Copy link
Copy Markdown
Collaborator

@cmastalli cmastalli left a comment

Choose a reason for hiding this comment

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

The main requested change is to support ROS 2 as well. This requires several changes, and I have written many of them. There are other minor changes.

Please let me know when you're done! Also check that the CI passes.

Comment thread CMakeLists.txt
Comment thread include/crocoddyl_msgs/inertial_parameters_publisher.h Outdated
Comment thread msg/InertialParameters.msg Outdated
Comment thread msg/InertialParameters.msg Outdated
Comment thread msg/InertialParameters.msg Outdated
Comment thread src/crocoddyl_ros.cpp Outdated
Comment thread unittest/test_inertial_parameters.py
Comment thread unittest/test_inertial_parameters.py Outdated
Comment thread unittest/test_inertial_parameters.py Outdated
Comment thread unittest/test_inertial_parameters.py Outdated
@Sergim96
Copy link
Copy Markdown
Author

@cmastalli IMO PR ready to merge

Copy link
Copy Markdown
Collaborator

@cmastalli cmastalli left a comment

Choose a reason for hiding this comment

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

The main change requested is to follow the Pinocchio convention when interfacing with inertial parameters. This needs to be done in both publisher and subscriber.

Additionally, there are minor changes needed for ROS2 and handling default arguments appropriately.

Finally, the PR misses update/get inertia parameters in the whole-body trajectory publisher and subscriber.

Please handle this ASAP. It is a good idea to finish earlier what was started.

Comment thread include/crocoddyl_msgs/conversions.h Outdated
Comment thread include/crocoddyl_msgs/conversions.h Outdated
Comment thread include/crocoddyl_msgs/conversions.h Outdated
Comment thread include/crocoddyl_msgs/inertial_parameters_publisher.h Outdated
Comment thread include/crocoddyl_msgs/inertial_parameters_subscriber.h Outdated
Comment thread include/crocoddyl_msgs/whole_body_state_publisher.h Outdated
Comment thread include/crocoddyl_msgs/whole_body_state_subscriber.h Outdated
Comment thread include/crocoddyl_msgs/whole_body_state_subscriber.h Outdated
Comment thread src/crocoddyl_ros.cpp Outdated
Comment thread unittest/test_whole_body_state.py Outdated
@cmastalli
Copy link
Copy Markdown
Collaborator

@Sergim96 Could you investigate why ROS 2 jobs are not passing? It is worth mentioning that Pinocchio fails to import in ROS 1 (see #14). This is known although I don't remember the reason now.

In short, after passing ROS 2 jobs, we could deactivate the inertial test. A better alternative would be to adapt the cmake to important inertial and whole-body tests for ROS 2 jobs only. It would be very useful if you could handle this in this PR. Please ping me when this is done. Let's try to finish this asap. Thanks!

@Sergim96
Copy link
Copy Markdown
Author

Sergim96 commented Apr 2, 2024

HI @cmastalli now the ROS2 humble is passing, but rolling is failing to setup the environment in this line:
'sudo apt-get -qq install -y --no-upgrade --no-install-recommends apt-utils build-essential gnupg2 dirmngr ca-certificates | grep -E 'Setting up' ' returned with 100

and as you said ROS1 fails because of pinocchio, I can disable it but the test for the whole body state also uses pinocchio, so I expect it to fail too

@Sergim96 Sergim96 force-pushed the topic/extend_WholeBodyStateRosPublisher branch 3 times, most recently from 33fc9c0 to 8842a02 Compare April 4, 2024 11:22
@cmastalli cmastalli force-pushed the topic/extend_WholeBodyStateRosPublisher branch from 8842a02 to e4c5046 Compare April 4, 2024 12:34
@cmastalli cmastalli merged commit f8d6a08 into RobotMotorIntelligence:devel Apr 6, 2024
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.

2 participants