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

Fixes for Windows #205

Merged
merged 7 commits into from
Jun 27, 2021
Merged

Fixes for Windows #205

merged 7 commits into from
Jun 27, 2021

Conversation

Ace314159
Copy link
Contributor

I changed a couple things to get this working on Windows.

  1. For the same reason as disable griper on OSX #192, the gripper controller doesn't work on Windows as well, so I disabled it.
  2. M_PI is not defined unless _USE_MATH_DEFINES is also defined
  3. I had to export some protected functions of the joint trajectory controller to fix missing symbol errors.
  4. Lastly, cxxabi.h doesn't exist on MSVC, so I didn't include it when MSVC was detected. It still still seems to compile fine without it.

@bmagyar
Copy link
Member

bmagyar commented Jun 24, 2021

Could we have _USE_MATH_DEFINES added in the cmake as a flag by any chance? It would move the file-based hack to a more hidden place and may avoid having to add it to more files in the future where we have a need for M_PI.

@bmagyar
Copy link
Member

bmagyar commented Jun 24, 2021

Also please fix uncrustify and cpplint

@Ace314159
Copy link
Contributor Author

I could add the define using add_definitions and that would fix this issue as well as future issues for only the diff_drive_controller. This would be fine in this case because it is used in a cpp file. However, if M_PI were to be used in a header file, then having it in the CMake would require anyone using the header file to also have that define, which I think is not ideal.

@Karsten1987
Copy link
Contributor

Karsten1987 commented Jun 24, 2021

thanks @Ace314159 for picking this up. Let's see what the official ROS2 buildfarm is saying:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

That CI run obviously also incorporates your changes from ros-controls/ros2_control#443

@Ace314159
Copy link
Contributor Author

They all seem to fail at a similar point involving logging. I don't think I modified that code, and I don't get that error. Do you have any idea what might be causing that?

@Karsten1987
Copy link
Contributor

I think I misconfigured the job, let's try again:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Ace314159
Copy link
Contributor Author

Looks like the only failing test is a linter one in ros2_control. I just put a fix for that.

Is the expected way to fix these linter errors to run ament_uncrustify. Is there a way to tell it to do it for all the packages in a workspace without having to manually specify all the packages.

@Karsten1987
Copy link
Contributor

This would be fine in this case because it is used in a cpp file. However, if M_PI were to be used in a header file, then having it in the CMake would require anyone using the header file to also have that define, which I think is not ideal.

I agree with this logic. I think the current solution pretty neat.

Co-authored-by: Denis Štogl <destogl@users.noreply.github.com>
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

It looks good now.

@Karsten1987 would it be possible to get Windows CI integrated?

@destogl destogl requested a review from bmagyar June 25, 2021 22:07
@bmagyar bmagyar merged commit 1c6621a into ros-controls:master Jun 27, 2021
gwalck pushed a commit to StoglRobotics-forks/ros2_controllers that referenced this pull request Jun 7, 2023
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.

4 participants