robotology / human-dynamics-estimation

Software repository for estimating human dynamics
BSD 3-Clause "New" or "Revised" License
81 stars 28 forks source link

Feature: add vector wise publish in human state wrapper to be read by simulink #218

Closed CarlottaSartore closed 3 years ago

CarlottaSartore commented 3 years ago

Added the possibility to publish the output of the IK vector wise, in order to read such information from simulink.

the following information can be published:

The information will be published only if the related port name is set as a parameter in the configuration file, i.e. the following new configuration parameter can be instantiated:

    <param name="jointPositionPortName">$JOINTS_POSITION_PORT_NAME</param>
        <param name="jointVelocityPortName">$JOINTS_VELOCITY_PORT_NAME</param>
    <param name="basePositionPortName">$BASE_POSITION_PORT_NAME</param>
        <param name="baseVelocityPortName">$BASE_VELOCIY_PORT_NAME</param>
    <param name="CoMPositionPortName">$COM_POSITION_PORT_NAME</param>
        <param name="CoMVelocityPortName">$COM_VELOCITYPORT_NAME</param>

C.C. @lrapetti @Yeshasvitvs @S-Dafarra @nunoguedelha @gabrielenava

yeshasvitirupachuri commented 3 years ago

Thanks for the PR @CarlottaSartore

We implemented a new device called HumanDataCollector https://github.com/robotology/human-dynamics-estimation/pull/181 that adds the same functionality but not limited to only HumanStateProvider device.

This feature is currently in feature/visualize-berdy-estimated-wrench branch on which we froze the development for a while, and planned to merge it soon to devel following the tests in https://github.com/robotology/human-dynamics-estimation/issues/187. So, in view of that, the features added by this PR may become redundant.

CC @lrapetti

CarlottaSartore commented 3 years ago

Thanks, @Yeshasvitvs for the hints! For a project I am currently working on, we need this feature really soon (the very next week) for this reason if it does not interfere with other work, it would be important for us to merge also this feature in devel. Please note that it is possible to not add any new parameter, only if the port is instantiated the vector are published, hence you would not need to change any configuration file already created !

yeshasvitirupachuri commented 3 years ago

Please note that it is possible to not add any new parameter, only if the port is instantiated the vector are published, hence you would not need to change any configuration file already created !

This is really good.

For a project I am currently working on, we need this feature really soon (the very next week) for this reason if it does not interfere with other work, it would be important for us to merge also this feature in devel.

Did you consider to have a fork of this repo and use it for your project purpose? As the features added by this PR may become redundant, I would request you to consider maintaining a separate fork.

lrapetti commented 3 years ago

So, in view of that, the features added by this PR may become redundant.

I agree the feature seems to be redundant with the implementation of HumanDataCollector

This feature is currently in feature/visualize-berdy-estimated-wrench branch on which we froze the development for a while, and planned to merge it soon to devel following the tests in #187.

However, at this stage this branch is based on a development fork of iDynTree (https://github.com/dic-iit/idyntree-hde-fork/tree/feature/stack-of-tasks-berdy) that most likely might take some time before being merged.

Since the HumanDataCollector is somehow an independent device, @Yeshasvitvs do you think it could be merged separately from the whole branch? In case I can help @CarlottaSartore to cherry-pick the commits related to HumanDataCollector and open the PR for them only.

traversaro commented 3 years ago

For the specific need of deployment of @CarlottaSartore, the quickest solution is just to create wrappers/addVectorWisePublish branch on this repo, so that @CarlottaSartore can refer to specific commit of this repo without the need to get this merged in master or devel. However, it is quite important that, once deadline are passed, that we make sure that all this technical debt is properly addressed. Keeping a fork in development for six months without thinking how of how that could impact the work of other teams is simply not maintainable.

DanielePucci commented 3 years ago

@S-Dafarra

However, it is quite important that, once deadline are passed, that we make sure that all this technical debt is properly addressed. Keeping a fork in development for six months without thinking how of how that could impact the work of other teams is simply not maintainable.

Indeed agreed.

CC @Yeshasvitvs @claudia-lat

yeshasvitirupachuri commented 3 years ago

For the specific need of deployment of @CarlottaSartore, the quickest solution is just to create wrappers/addVectorWisePublish branch on this repo, so that @CarlottaSartore can refer to specific commit of this repo without the need to get this merged in master or devel.

@traversaro This is a good choice for the moment. I agree with this.

However, at this stage this branch is based on a development fork of iDynTree (https://github.com/dic-iit/idyntree-hde-fork/tree/feature/stack-of-tasks-berdy) that most likely might take some time before being merged.

@lrapetti Good point, given the priority of the other tasks may be this will take a while

traversaro commented 3 years ago

@traversaro This is a good choice for the moment. I agree with this.

@CarlottaSartore has now the write access to the repo.

CarlottaSartore commented 3 years ago

Created branch and commit the modification needed. You can find it at the wrappers/addVectorWisePublish branch (on top of devel).

I think we can close this PR then? @Yeshasvitvs @lrapetti

lrapetti commented 3 years ago

I think we can close this PR then? @Yeshasvitvs @lrapetti

yes, let's close it.