-
Notifications
You must be signed in to change notification settings - Fork 317
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
feat(franka_gazebo): implement set_franka_model_configuration
service
#226
feat(franka_gazebo): implement set_franka_model_configuration
service
#226
Conversation
23e230a
to
eeaa513
Compare
Good idea @rickstaa! Makes total sense to have something like this in franka_gazebo. |
eeaa513
to
88b3eff
Compare
@gollth I just forced pushed a new version onto this pull request that implements the Some parts can be improved like:
However, I left these decisions up to you since they depend on the code style you want to use. |
@gollth I just saw that I use clamp which was included in C17. I will add a replacement for this. |
88b3eff
to
b28a8a7
Compare
@gollth Clang seems to fail but does not throw a warning about what goes wrong https://github.com/frankaemika/franka_ros/runs/5246382311?check_suite_focus=true. |
0fa2a0f
to
a1f7a11
Compare
This commit migrates from the `reset_model_states` service to the `set_franka_model_configuration` service. For more information see frankaemika/franka_ros#226.
a1f7a11
to
99d6a63
Compare
set_franka_model_configuration
service
370aecb
to
2e2813f
Compare
I've been working on resolving the CI errors we've encountered, but I could use some assistance in identifying what's going wrong. Here's what I've tried so far:
For context, I've configured my "C_Cpp.codeAnalysis.clangTidy.headerFilter": "/opt/ros/noetic/include/*:../devel/include/*",
"C_Cpp.codeAnalysis.clangTidy.path": "/usr/bin/clang-tidy-7", Since the CI tests appear to fail despite these local checks showing no issues, I'd appreciate your insights and suggestions on pinpointing the problem. Your assistance would be invaluable in resolving this matter. |
@rickstaa seems like the formatting is off, try running: catkin config --cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
catkin build --no-deps --make-args format -- franka_gazebo and see if that fixes the error by running: catkin build --no-deps --make-args check-format -- franka_gazebo |
This comment was marked as outdated.
This comment was marked as outdated.
Ah, I found out that something went wrong during building. It was fixed after I reset/cleaned the catkin workspace 👍🏻. |
This commit implements a `set_franka_model_configuration` service. This service can be used to set the Franka configuration in the Gazebo simulation. Under the hood, this service calls Gazebo's 'set_model_configuration' service while ensuring that the reported joint positions stay within joint limits (see frankaemika#225 for more information).
2e2813f
to
61d9210
Compare
Hey @gollth, it looks like the tests are now passing 😄. Adding a simple CONTRIBUTING.md file to provide clear guidance for contributors is helpful. In my recent experience, I had to dive into the This file doesn't need to be overly detailed; just a few key points could suffice:
Such a contribution guide could streamline the onboarding process for new contributors. |
6c8206c
to
6608e14
Compare
@gollth I've recently observed an issue stemming from the changes introduced in commit 89d2571. Specifically, after invoking either the original Gazebo
To address this, I've made some adjustments in commit 6608e14, rectifying this issue and simplifying the code and the related service message. Considering that the current Gazebo service is incompatible with your package, it might be the right moment to consider merging this pull request. This update could significantly improve the functionality and reliability of the system 🤔. I am looking forward to your thoughts on this. |
Hi @rickstaa, I'm not working for Franka Emika anymore. Maybe @FE-EnricoSartori can take this over? |
Hello @gollth, congratulations on your new position 🚀! It's great that you've tagged the appropriate person for this review—just a tiny reminder: your GitHub profile still indicates that you are employed at Franka. You might want to update that information. Best of luck in your new role! |
@rickstaa I can support you from Franka side. We are mostly focused on the ROS 2 support for FR3 robot, that's why ROS1 was left unsupported. Do you need me to review and merge the PR? |
This commit ensures the `set_franka_model_configuration` works with the changes made in 89d2571. It also simplifies the `SetJointConfiguration.srv` message.
6608e14
to
bae0507
Compare
@BarisYazici, I appreciate your prompt response and the assistance you provided in getting this pull request processed. Given that ROS Noetic will reach its end-of-life in May 2025, I fully understand your team's focus on ROS2 development 👍🏻. However, I believe that approving this pull request will be advantageous. It addresses an issue where your package is incompatible with the Gazebo |
Thank you for your contribution!! 🎆 👍 |
This commit fixes a problem that was introduced when https://github.com/rickstaa/franka_ros/tree/fix_gazebo_set_model_config_problem was rebased onto the `develop` branch in frankaemika#226. The force push resulting from this rebase unfortunatelly removed the code that setsup the service.
Hey @BarisYazici, thanks for your patience. I've just revisited the pull request and noticed that the rebase and force push inadvertently removed essential code from #226 related to the |
This PR implements areset_joint_states
service. This service can reset the joint states when pushed outside the joint limits. This can, for example, happen when Gazebo'sset_model_configuration
is used (see #225 for more information).Update
This PR implements a
set_franka_model_configuration
, which can be used to set call Gazebo'sset_model_configuration
service while ensuring that the reported joint positions stay within joint limits (see #225 for more information)