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

Use own implementation of stod() #428

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

christophfroehlich
Copy link
Contributor

Use own implementation of stod(), see ros-controls/ros2_control#1257

@christophfroehlich
Copy link
Contributor Author

@Mergifyio backport humble iron

Copy link
Contributor

mergify bot commented Jan 8, 2024

backport humble iron

✅ Backports have been created

Copy link
Collaborator

@olivier-stasse olivier-stasse left a comment

Choose a reason for hiding this comment

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

While compiling the demos I got the following problem:

--- stderr: ros2_control_demo_example_14
/usr/bin/ld: CMakeFiles/ros2_control_demo_example_14.dir/hardware/rrbot_sensor_for_position_feedback.cpp.o: in function `hardware_interface::stod(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)':
rrbot_sensor_for_position_feedback.cpp:(.text+0x3a): multiple definition of `hardware_interface::stod(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'; CMakeFiles/ros2_control_demo_example_14.dir/hardware/rrbot_actuator_without_feedback.cpp.o:rrbot_actuator_without_feedback.cpp:(.text+0xb): first defined here
/usr/bin/ld: CMakeFiles/ros2_control_demo_example_14.dir/hardware/rrbot_sensor_for_position_feedback.cpp.o: in function `hardware_interface::parse_bool(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)':
rrbot_sensor_for_position_feedback.cpp:(.text+0x1cb): multiple definition of `hardware_interface::parse_bool(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'; CMakeFiles/ros2_control_demo_example_14.dir/hardware/rrbot_actuator_without_feedback.cpp.o:rrbot_actuator_without_feedback.cpp:(.text+0x19c): first defined here
collect2: error: ld returned 1 exit status
gmake[2]: *** [CMakeFiles/ros2_control_demo_example_14.dir/build.make:257: libros2_control_demo_example_14.so] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:137: CMakeFiles/ros2_control_demo_example_14.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2
---

@olivier-stasse
Copy link
Collaborator

I guess that the best is to make lexical_casts a cpp file like Moveit https://github.com/ros-planning/moveit/blob/3d2105584f414f2494eebff17122b398721d9c2a/moveit_core/utils/src/lexical_casts.cpp

@christophfroehlich
Copy link
Contributor Author

I saw that also in the CI and was not sure if this is an artifact of old binary packages. Not sure where this comes from, when I understand it I'm happy to change this to a cpp file.

@saikishor
Copy link
Member

I saw that also in the CI and was not sure if this is an artifact of old binary packages. Not sure where this comes from, when I understand it I'm happy to change this to a cpp file.

@christophfroehlich it seems like we need to separate them into header and cpp files, if not we will have this linking issue with multiple definitions. Shall I do it for you? or do you want to do it?

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.

Needs minor changes in the parent repository of the hardware_interfaces to avoid these linking errors.
The current changes LGTM

Copy link
Collaborator

@olivier-stasse olivier-stasse left a comment

Choose a reason for hiding this comment

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

LGTM with PR #1281

@christophfroehlich
Copy link
Contributor Author

thanks @saikishor, semi-binary builds now. Should we wait for the merge until the next release and sync of ros2_control?

@saikishor
Copy link
Member

thanks @saikishor, semi-binary builds now. Should we wait for the merge until the next release and sync of ros2_control?

If this or the prior changes are never released, it is better to wait. However, 90% of people who use ros2_control in rolling probably compile from source.

@christophfroehlich
Copy link
Contributor Author

However, 90% of people who use ros2_control in rolling probably compile from source.

not sure about that, I'd just wait

@saikishor
Copy link
Member

not sure about that, I'd just wait

Yes, let's wait for the release and the sync. I think we should also have waiting_for_sync tags, they are useful in these cases.

@saikishor saikishor merged commit c9742eb into ros-controls:master Feb 15, 2024
12 checks passed
@christophfroehlich christophfroehlich deleted the lexical_casts branch February 15, 2024 12:51
mergify bot pushed a commit that referenced this pull request Feb 15, 2024
mergify bot pushed a commit that referenced this pull request Feb 15, 2024
saikishor pushed a commit that referenced this pull request Feb 15, 2024
(cherry picked from commit c9742eb)

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
saikishor pushed a commit that referenced this pull request Feb 15, 2024
(cherry picked from commit c9742eb)

Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants