Skip to content
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

Fix Windows Compilation Issues in joint_trajectory_controller and pid_controller (master branch) #1330

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

SENAI-GilmarCorreia
Copy link
Contributor

@SENAI-GilmarCorreia SENAI-GilmarCorreia commented Oct 30, 2024

Description:

This pull request addresses compilation issues encountered in the joint_trajectory_controller and pid_controller when building on Windows. The changes ensure compatibility with the Windows environment and improve overall stability.

Changes Made:

  • joint_trajectory_controller:

    • Modifies the lambda capture in is_erase_value from [ ] to [=], which captures all outer scope variables by value.
  • pid_controller:

    • Added compile definitions for Windows to minimize namespace collisions and define math constants:
      if(WIN32)
        add_compile_definitions(
          # For math constants
          _USE_MATH_DEFINES
          # Minimize Windows namespace collision
          NOMINMAX
          WIN32_LEAN_AND_MEAN
        )
      endif()

Testing:

  • Local tests have been run successfully, confirming that all modifications do not break existing functionality.

Please let me know if you have any questions or need further modifications. I appreciate your feedback!

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.57%. Comparing base (7c89c17) to head (f12076c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1330   +/-   ##
=======================================
  Coverage   80.57%   80.57%           
=======================================
  Files         109      109           
  Lines        9553     9552    -1     
  Branches      832      831    -1     
=======================================
  Hits         7697     7697           
  Misses       1578     1578           
+ Partials      278      277    -1     
Flag Coverage Δ
unittests 80.57% <ø> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
...include/joint_trajectory_controller/tolerances.hpp 79.79% <ø> (ø)

... and 3 files with indirect coverage changes

@christophfroehlich
Copy link
Contributor

I don't see the changes you listed in your description, are there commits missing?

@SENAI-GilmarCorreia
Copy link
Contributor Author

SENAI-GilmarCorreia commented Oct 30, 2024

It’s just the = sign and the CMakeLists changes in the commit: fixes for Windows compilation.

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.

LGTM and CI is also happy!

@christophfroehlich
Copy link
Contributor

It’s just the = sign and the CMakeLists changes in the commit: fixes for Windows compilation.

I was just wondering, you write

Modified conditional compilation flags to ensure compatibility with Windows builds.

but only the lambda capture was changed for JTC.

@SENAI-GilmarCorreia
Copy link
Contributor Author

What about: "Modifies the lambda capture in is_erase_value from [ ] to [=], which captures all outer scope variables by value."?

@christophfroehlich
Copy link
Contributor

I'm totally fine with the changes you did, just the description was not matching ;)

@christophfroehlich christophfroehlich added backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron. labels Oct 31, 2024
@christophfroehlich christophfroehlich merged commit fa42b5e into ros-controls:master Oct 31, 2024
18 of 20 checks passed
mergify bot pushed a commit that referenced this pull request Oct 31, 2024
Co-authored-by: SENAI-GilmarCorreia <[email protected]>
(cherry picked from commit fa42b5e)
mergify bot pushed a commit that referenced this pull request Oct 31, 2024
Co-authored-by: SENAI-GilmarCorreia <[email protected]>
(cherry picked from commit fa42b5e)
christophfroehlich pushed a commit that referenced this pull request Oct 31, 2024
Co-authored-by: SENAI-GilmarCorreia <[email protected]>
(cherry picked from commit fa42b5e)

Co-authored-by: Gilmar Correia <[email protected]>
christophfroehlich pushed a commit that referenced this pull request Oct 31, 2024
Co-authored-by: SENAI-GilmarCorreia <[email protected]>
(cherry picked from commit fa42b5e)

Co-authored-by: Gilmar Correia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants