Skip to content

Conversation

@suchetanrs
Copy link

The tf prefix is added to the odom_frame_id and the base_frame_id. However, it is not done to the left_wheel_names and right_wheel_names.

The differential drive controller is often used as a gazebo plugin like below.

    <gazebo>
      <plugin filename="libgazebo_ros2_control.so" name="gazebo_ros2_control">
        <namespace>${robot_namespace}</namespace>
        <robot_param>robot_description</robot_param>
        <robot_param_node>robot_state_publisher</robot_param_node>
        <parameters>package://robot_description/config/robot_control.yaml</parameters>
      </plugin>
    </gazebo>

Since this is defined inside a xacro, the Rewritten yaml cannot be used to add namespaces to joints. Adding the namespace in the cpp file bypasses this issue. In the PR, the convention is followed. That is, "robot_namespace/left_wheel_name". Please let me know if I need to change it in any other place. The modified changes solves my issue and seems to run correctly on the Gazebo simulator.

The tf prefix is added to the odom_frame_id and the base_frame_id. However, it is not done to the left_wheel_names and right_wheel_names.

The differential drive controller is often used as a gazebo plugin like below. 
```
    <gazebo>
      <plugin filename="libgazebo_ros2_control.so" name="gazebo_ros2_control">
        <namespace>${robot_namespace}</namespace>
        <robot_param>robot_description</robot_param>
        <robot_param_node>robot_state_publisher</robot_param_node>
        <parameters>package://robot_description/config/robot_control.yaml</parameters>
      </plugin>
    </gazebo>
```

Since this is defined inside a xacro, the Rewritten yaml cannot be used to add namespaces to joints. Adding the namespace in the cpp file bypasses this issue.  In the PR, the convention is followed. That is, "robot_namespace/left_wheel_name". Please let me know if I need to change it in any other place. The modified changes solves my issue and seems to run correctly on the Gazebo simulator.
Copy link
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM

@bmagyar
Copy link
Member

bmagyar commented Nov 30, 2023

Please fix the issues reported by the "Format" stage by running pre-commit run --all locally and adding the changes to the PR

@codecov
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.78%. Comparing base (9f7e9e9) to head (9078dd0).
Report is 29 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #831       +/-   ##
===========================================
+ Coverage   47.71%   71.78%   +24.06%     
===========================================
  Files          41       41               
  Lines        3871     3643      -228     
  Branches     1833     1786       -47     
===========================================
+ Hits         1847     2615      +768     
+ Misses        751      715       -36     
+ Partials     1273      313      -960     
Flag Coverage Δ
unittests 71.78% <100.00%> (+24.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...iff_drive_controller/src/diff_drive_controller.cpp 69.09% <100.00%> (+21.03%) ⬆️

... and 34 files with indirect coverage changes

@suchetanrs
Copy link
Author

Hi @bmagyar !
After fixing the format issues, I synced my fork with the master since its been a while. I now have a build error in rolling binary build / binary (main). All the other checks seem to pass. is it ok?

@christophfroehlich
Copy link
Member

Hi @bmagyar ! After fixing the format issues, I synced my fork with the master since its been a while. I now have a build error in rolling binary build / binary (main). All the other checks seem to pass. is it ok?

Rolling binary build on main fails because of an API break, which hasn't been synced yet. That's fine and unrelated to this PR.

@suchetanrs
Copy link
Author

Ah I see! Thanks @christophfroehlich :D

@bmagyar
Copy link
Member

bmagyar commented Aug 14, 2024

Closing in favour of #1145

@bmagyar bmagyar closed this Aug 14, 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.

3 participants