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

Update coupling handlers of ergoCub Hand MK5 #650

Merged
merged 3 commits into from
May 23, 2023

Conversation

xEnVrE
Copy link
Contributor

@xEnVrE xEnVrE commented May 17, 2023

As per the title.

Notes:

As regards the last point, I am happy to discuss different solutions - also given the recent discussion in #649. The fact that decoupleRefVel is working properly has been tested by enabling, just for testing purposes, the direct_velocity_pid mode for the YARP velocity control.

This PR is tightly coupled with icub-tech-iit/ergocub-software#115.

Fixes #647

cc @mfussi66 @Nicogene @traversaro @pattacini

@traversaro
Copy link
Member

Thanks! I will go in depth on this tomorrow, but in the meanwhile CI fails with:

2023-05-17T21:40:48.9126043Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(268,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:48.9128132Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(280,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:48.9129544Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(292,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:49.0488142Z   ControlBoardDriverCoupling.cpp
2023-05-17T21:40:53.9123252Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(268,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:53.9125533Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(280,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:53.9127590Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(292,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:53.9650444Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\src\ControlBoardDriverCoupling.cpp(68,51): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:54.0199272Z   ControlBoardLog.cpp
2023-05-17T21:40:54.5328321Z   ControlBoardDriverVirtualAnalogSensor.cpp
2023-05-17T21:40:59.3990476Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(268,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:59.3992516Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(280,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:59.3995922Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\include\yarp\dev\ControlBoardDriverCoupling.h(292,9): error C7555: use of designated initializers requires at least '/std:c++20' [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:59.4490700Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\src\ControlBoardDriverVirtualAnalogSensor.cpp(30,36): warning C4267: 'return': conversion from 'size_t' to 'int', possible loss of data [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]
2023-05-17T21:40:59.4495932Z D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\plugins\controlboard\src\ControlBoardDriverVirtualAnalogSensor.cpp(45,73): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [D:\a\gazebo-yarp-plugins\gazebo-yarp-plugins\build\plugins\controlboard\gazebo_yarp_controlboard.vcxproj]

Probably we just need to target_compile_features(<lib> PUBLIC cxx_std_20) for the library that contains ControlBoardDriverCoupling.h, see also the related PR ami-iit/meshcat-cpp#3 (comment) .

@xEnVrE
Copy link
Contributor Author

xEnVrE commented May 17, 2023

It depends on the fact that I use designated initializers for the members of the FingerParameters struct I guess:

https://github.com/xEnVrE/gazebo-yarp-plugins/blob/4dbe0ffe25afd6d9e6f2184b0a0a77b40ac47c09/plugins/controlboard/include/yarp/dev/ControlBoardDriverCoupling.h#L266

I could find a different solution if we don't want to enable cxx_std_20, although it seemed more clear as it is now where the field names are clearly visible.

@traversaro
Copy link
Member

I could find a different solution if we don't want to enable cxx_std_20, although it seemed more clear as it is now where the field names are clearly visible.

I think C++20 is fine, as long as the library compiles fine on Ubuntu 20.04 with apt dependencied, we are fine. Please ignore the Ubuntu 18.04 failure, I need to disable that job.

@xEnVrE
Copy link
Contributor Author

xEnVrE commented May 18, 2023

Added support for C++ 20 compiler features for the gazebo_yarp_controlboard plugin.

Not sure if these lines

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED 11)

are to be changed as well.

@xEnVrE
Copy link
Contributor Author

xEnVrE commented May 19, 2023

As regards the last point, I am happy to discuss different solutions

After f2f with @traversaro, it was decided that it would be better to add second argument to decoupleVelRef, something like

virtual yarp::sig::Vector decoupleRefVel (yarp::sig::Vector& vel_ref, const yarp::sig::Vector& pos_feedback) = 0; 

so that any velocity reference decoupling law that requires the access to the joint positions can be implemented. I will update the PR to match the above idea.

@xEnVrE xEnVrE force-pushed the fix/hand_coupling branch from 7894826 to 27de5f4 Compare May 22, 2023 09:31
@xEnVrE
Copy link
Contributor Author

xEnVrE commented May 22, 2023

The PR has been updated as discussed.

cc @traversaro @Nicogene

@traversaro
Copy link
Member

traversaro commented May 22, 2023

Ok for me, thanks @xEnVrE ! @mebbaid you may want to check this as well, as it is related to #649 (note that this is not the cleanup suggested in that issue, it is just a change required for iCub Hand MK5 to work.

@traversaro traversaro merged commit 830ab76 into robotology:master May 23, 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.

Coupling handlers for ergoCub hand MK3, 4 and 5 possibly incorrect
2 participants