-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add HDE profile to superbuild #231
Add HDE profile to superbuild #231
Conversation
README.md
Outdated
@@ -636,6 +654,27 @@ To check and install the Cyberith SDK, please follow the steps for Cyberith SDK | |||
### Configuration | |||
To configure the Cyberith SDK please follow the steps for Cyberith SDK mentioned in [here](https://github.com/robotology/walking-teleoperation/blob/master/docs/Dependencies.md). | |||
|
|||
## Xsens | |||
Support for this dependency is enabled by the `ROBOTOLOGY_ENABLE_HUMAN_DYNAMICS` CMake option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would invert the causality, I would document that the option ROBOTOLOGY_USES_XSENS_MVN_SDK
option is only enabled when the ROBOTOLOGY_ENABLE_HUMAN_DYNAMICS
CMake option is set to ON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I wrote and what you said to me is equal! If you prefer, I can rephrase it as you mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed I see what you mean. As long as we have the ROBOTOLOGY_USES_XSENS_MVN_SDK
I am ok with what you wrote.
Co-Authored-By: Silvio Traversaro <pegua1@gmail.com>
Co-Authored-By: Silvio Traversaro <pegua1@gmail.com>
Co-Authored-By: Silvio Traversaro <pegua1@gmail.com>
Co-Authored-By: Silvio Traversaro <pegua1@gmail.com>
Co-Authored-By: Silvio Traversaro <pegua1@gmail.com>
Co-Authored-By: Silvio Traversaro <pegua1@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beside minor comments the logic is now ok, thanks. I guess we need to wait for the (at least partial merge) of robotology/wearables#25 . @Yeshasvitvs @lrapetti any update?
Co-Authored-By: Silvio Traversaro <pegua1@gmail.com>
I will update the |
OK, then we only will mention it in documentations.
It throws here in this line, https://github.com/robotology/human-dynamics-estimation/blob/0a7b4be41ba379ed0ff391d62db93b4018512133/devices/HumanStateProvider/IKWorkerPool.h#L28, complaining about
Since, you and @fjandrad said, ft sensors in linux handled by |
So you enabled the |
Indeed, I am not sure how that is related to |
Indeed, you are right, in the error was mentioning #include <linux/can.h> is missing. |
yes, I think that is the case you are mentioning. |
Thanks for the documentation link, it was interesting. |
@traversaro I updated the README. |
maybe this option can be added after this line instead: https://github.com/robotology/human-dynamics-estimation/blob/0a7b4be41ba379ed0ff391d62db93b4018512133/devices/HumanStateProvider/CMakeLists.txt#L6 |
since it is a very small change, can we add it without PR to HDE repo? @traversaro @lrapetti @Yeshasvitvs |
@kouroshD, I think you can open a PR and ping me so that I can approve it as soon as the PR is there. In this way, we can track the changes properly. |
That is up to the HDE mantainers. : ) |
CMakeLists.txt
Outdated
@@ -150,6 +157,17 @@ if(ROBOTOLOGY_ENABLE_TELEOPERATION) | |||
find_or_build_package(walking-teleoperation) | |||
endif() | |||
|
|||
# Human Dynamics Estimation | |||
if(ROBOTOLOGY_ENABLE_HUMAN_DYNAMICS) | |||
set(ROBOTOLOGY_ENABLE_ICUB_HEAD TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we already discussed during the review, but the idea behind profiles is that they are completly independent, and if a user need to set ROBOTOLOGY_ENABLE_ICUB_HEAD
to TRUE
on a given machine (for example because he need to connect to iCub CAN devices) he/she can do it manualy. If instead any project in ROBOTOLOGY_ENABLE_HUMAN_DYNAMICS
depends for compilation on a given project/option, this should be added as all the other dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not the compile-time dependency. What if we set ROBOTOLOGY_ENABLE_ICUB_HEAD
to TRUE
and find_or_build_package(forcetorque-yarp-devices)
, if ROBOTOLOGY_USES_ESDCAN
is TRUE
? Since with this option the user is indicating, he/she wants to use FT sensors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
README.md
Outdated
@@ -62,6 +65,7 @@ Note that any dependencies of the included packages that is not available in the | |||
| `ROBOTOLOGY_ENABLE_DYNAMICS` | The robotology software packages related to balancing, walking and force control. | [`iDynTree`](https://github.com/robotology/idyntree), [`blockfactory`](https://github.com/robotology/blockfactory), [`wb-Toolbox`](https://github.com/robotology/wb-Toolbox), [`whole-body-controllers`](https://github.com/robotology/whole-body-controllers), [`walking-controllers`](https://github.com/robotology/walking-controllers). [`icub-gazebo-wholebody`](https://github.com/robotology-playground/icub-gazebo-wholebody) if the `ROBOTOLOGY_USES_GAZEBO` option is enabled. | `OFF` | [Documentation on Dynamics profile.](#dynamics) | | |||
| `ROBOTOLOGY_ENABLE_ICUB_HEAD` | The robotology software packages needed on the system that is running on the head of the iCub robot, or in general to communicate directly with iCub low-level devices. | [`icub-firmware`](https://github.com/robotology/icub-firmware), [`icub-firmware-shared`](https://github.com/robotology/icub-firmware-shared). Furthermore, several additional devices are compiled in `YARP` and `ICUB` if this option is enabled. | `OFF` | [Documentation on iCub Head profile.](#icub-head) | | |||
| `ROBOTOLOGY_ENABLE_TELEOPERATION` | The robotology software packages related to teleoperation. | [`walking-teleoperation`](https://github.com/robotology/walking-teleoperation). To use Oculus or Cyberith Omnidirectional Treadmill enable `ROBOTOLOGY_USES_OCULUS_SDK` and `ROBOTOLOGY_USES_CYBERITH_SDK` options. | `OFF` | [Documentation on teleoperation profile.](#teleoperation) | | |||
| `ROBOTOLOGY_ENABLE_HUMAN_DYNAMICS` | The robotology software packages related to human dynamics estimation. | [`human-dynamics-estimation`](https://github.com/robotology/human-dynamics-estimation), [`wearables`](https://github.com/robotology/wearables). `ROBOTOLOGY_ENABLE_ICUB_HEAD`, `ROBOTOLOGY_USES_XSENS_MVN_SDK`, and `ROBOTOLOGY_USES_ESDCAN`, [`forcetorque-yarp-devices`](https://github.com/robotology/forcetorque-yarp-devices) if using a Windows OS. | `OFF` | [Documentation on human dynamics profile.](#human-dynamics) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove references of the optional dependencies from this table, as this column just lists the main repositories included in this component, and I am afraid that mentioning options here would be confusing for users:
| `ROBOTOLOGY_ENABLE_HUMAN_DYNAMICS` | The robotology software packages related to human dynamics estimation. | [`human-dynamics-estimation`](https://github.com/robotology/human-dynamics-estimation), [`wearables`](https://github.com/robotology/wearables). `ROBOTOLOGY_ENABLE_ICUB_HEAD`, `ROBOTOLOGY_USES_XSENS_MVN_SDK`, and `ROBOTOLOGY_USES_ESDCAN`, [`forcetorque-yarp-devices`](https://github.com/robotology/forcetorque-yarp-devices) if using a Windows OS. | `OFF` | [Documentation on human dynamics profile.](#human-dynamics) | | |
| `ROBOTOLOGY_ENABLE_HUMAN_DYNAMICS` | The robotology software packages related to human dynamics estimation. | [`human-dynamics-estimation`](https://github.com/robotology/human-dynamics-estimation), [`wearables`](https://github.com/robotology/wearables). [`forcetorque-yarp-devices`](https://github.com/robotology/forcetorque-yarp-devices) if using a Windows OS. | `OFF` | [Documentation on human dynamics profile.](#human-dynamics) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I accept that ROBOTOLOGY_ENABLE_ICUB_HEAD
should be removed. But similar to other profiles, the options enabled to the user by ROBOTOLOGY_ENABLE_HUMAN_DYNAMICS
should be stated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this PR is still blocked by (at least a partial merge) of robotology/wearables#25 . @Yeshasvitvs @lrapetti Do you have any update? |
I am checking if everyone agrees I would proceed with merging. |
The PR has been merged |
PR is made here: |
Merged. |
I will proceed with "Squash and merge" if for you it is ok. |
Thanks, please proceed with that. |
With this PR, We add the HDE Profile to superbuild.
Relevant issue:
robotology/human-dynamics-estimation#120