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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Oct 31, 2024
Merged

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

merged 1 commit into from
Oct 31, 2024

Conversation

SENAI-GilmarCorreia
Copy link
Contributor

@SENAI-GilmarCorreia SENAI-GilmarCorreia commented Oct 30, 2024 •
edited
Loading

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!

@christophfroehlich christophfroehlich mentioned this pull request Oct 30, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Oct 30, 2024 •
edited
Loading

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 •
edited
Loading

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

saikishor
saikishor approved these changes Oct 30, 2024
View reviewed changes
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 maintainers only! Label triggers PR backport to ROS2 humble. backport-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
@SENAI-GilmarCorreia @mergify
fixes for windows compilation (#1330)
a52ad83 
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
@SENAI-GilmarCorreia @mergify
fixes for windows compilation (#1330)
95895df 
Co-authored-by: SENAI-GilmarCorreia <[email protected]>
(cherry picked from commit fa42b5e)
This was referenced Oct 31, 2024
Merged
Merged
christophfroehlich pushed a commit that referenced this pull request Oct 31, 2024
@mergify @SENAI-GilmarCorreia
fixes for windows compilation (#1330) (#1333)
54e72a2 
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
@mergify @SENAI-GilmarCorreia
fixes for windows compilation (#1330) (#1332)
a9645b0 
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
Reviewers

@saikishor saikishor saikishor approved these changes

@christophfroehlich christophfroehlich christophfroehlich approved these changes

@arne48 arne48 Awaiting requested review from arne48

@DasRoteSkelett DasRoteSkelett Awaiting requested review from DasRoteSkelett

@destogl destogl Awaiting requested review from destogl

@mamueluth mamueluth Awaiting requested review from mamueluth

@mcbed mcbed Awaiting requested review from mcbed

@peterdavidfagan peterdavidfagan Awaiting requested review from peterdavidfagan

Assignees
No one assigned
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SENAI-GilmarCorreia @christophfroehlich @saikishor