search

robotology / wearables

Data collection framework for wearable sensors
BSD 3-Clause "New" or "Revised" License
19 stars 11 forks source link

Fix Haptic glove transmission - GloveDevice #204

Closed EhsanRanjbari closed 2 months ago

EhsanRanjbari commented 7 months ago

See https://github.com/robotology/walking-teleoperation/issues/136#issuecomment-1916788820

traversaro commented 5 months ago

@EhsanRanjbari what is the status of this PR?

EhsanRanjbari commented 5 months ago

@EhsanRanjbari what is the status of this PR?

Waiting to do some tests on the robot.

S-Dafarra commented 4 months ago

Hi @EhsanRanjbari, what is the status of this PR?

EhsanRanjbari commented 4 months ago

Hi @EhsanRanjbari, what is the status of this PR?

Hi @S-Dafarra, well, I am still waiting to test them on the robot. unfortunately, both robots were unavailable most of the last week. However, preliminary test has been done with success (https://github.com/robotology/walking-teleoperation/issues/136)

S-Dafarra commented 3 months ago

Hi @EhsanRanjbari, did you manage to test on the robot?

EhsanRanjbari commented 3 months ago

Hi @EhsanRanjbari, did you manage to test on the robot?

Hi. I have the robot today ftom 2. I will let you know how it went asap.

EhsanRanjbari commented 3 months ago

Hi @EhsanRanjbari, did you manage to test on the robot?

Hi. I have the robot today from 2. I will let you know how it went asap.

Hi again. I just finished testing and it went well. I also noticed that the vibration upon starting calibration was not working. I fixed that by just aligning the branches with the remote. Now, everything looks fine according to this test.

S-Dafarra commented 3 months ago

Thanks @EhsanRanjbari!

Nonetheless, I just realized that the changes to the WearableActuatorCommand can be problematic. First of all, it is weird that there is a single ActuatorInfo, but then a vector of values (This is related to https://github.com/robotology/wearables/pull/204#discussion_r1474875228). For example, why for a single actuator of type HEATER should I receive a list of forces? Secondly, it is not clear what I should use for the HEATER case. Thirdly, this is probably breaking the multihaptic code, cc @dariosortino @Gianlucamilani

At this point, I would suggest again not to edit the WearableActuatorCommand, and instead add new messages/methods to send and receive a list of commands. This would preserve backward compatibility

EhsanRanjbari commented 2 months ago

Thanks, @S-Dafarra for the reviews. I commit to the last change suggestions.

EhsanRanjbari commented 2 months ago

Looks good to me! Thanks a lot for keeping iterating on this. If you could do a final test on the robot, then this is good to go for me!

You're welcome. I am now waiting for @mebbaid reviews and I have the robot booked on Friday to perform the tests.

EhsanRanjbari commented 2 months ago

Today. I performed tests on the robot to verify this PR. The test was successful. Only, we noticed that not finding the parameter `` which is not used in the glove device, stops the haptic device. With this commit https://github.com/robotology/wearables/pull/204/commits/2bb61850b54d0ff3e7e27b52f2dd6bbc664e8e92 I tried to make it optional.

S-Dafarra commented 2 months ago

Thanks @EhsanRanjbari, looks good to me! @traversaro is there someone in particular to ask, or can I go ahead and merge this?

EhsanRanjbari commented 2 months ago

@traversaro is there someone in particular to ask, or can I go ahead and merge this?

Just need to mention that this PR is highly dependent on this PR https://github.com/robotology/walking-teleoperation/pull/143

S-Dafarra commented 2 months ago

@traversaro is there someone in particular to ask, or can I go ahead and merge this?

Just need to mention that this PR is highly dependent on this PR robotology/walking-teleoperation#143

Actually, it is the opposite. This PR blocks https://github.com/robotology/walking-teleoperation/pull/143

S-Dafarra commented 2 months ago

Thinking about it @EhsanRanjbari can you bump the version from 1.8.0 to 1.9.0 in https://github.com/robotology/wearables/blob/91722ca62b65b4a2919fabf4ee58b3f90fd14a76/CMakeLists.txt#L7

In this way, we can change the minimum version required of wearables in walking-teleoperation.

EhsanRanjbari commented 2 months ago

Thinking about it @EhsanRanjbari can you bump the version from 1.8.0 to 1.9.0 in

https://github.com/robotology/wearables/blob/91722ca62b65b4a2919fabf4ee58b3f90fd14a76/CMakeLists.txt#L7

In this way, we can change the minimum version required of wearables in walking-teleoperation.

Done! https://github.com/robotology/wearables/pull/204/commits/32ff17c018a0015e9859a3ee1496cfd4329bc6d9

traversaro commented 2 months ago

Thanks @EhsanRanjbari, looks good to me! @traversaro is there someone in particular to ask, or can I go ahead and merge this?

After Lorenzo Rapetti left, I think we do not have any obvious candidate for maintainer. Feel free to merge and release/tag, thanks!

lrapetti commented 2 months ago

Thanks @EhsanRanjbari, looks good to me! @traversaro is there someone in particular to ask, or can I go ahead and merge this?

After Lorenzo Rapetti left, I think we do not have any obvious candidate for maintainer. Feel free to merge and release/tag, thanks!

Probably before merging CHANGELOG could be updated as well :wink:

S-Dafarra commented 2 months ago

Probably before merging CHANGELOG could be updated as well 😉

I think it could be a good time to follow https://github.com/robotology/idyntree/pull/1162 and start using the automatic generation of CHANGELOG of GitHub releases 🤔

S-Dafarra commented 2 months ago

Probably before merging CHANGELOG could be updated as well 😉

I think it could be a good time to follow robotology/idyntree#1162 and start using the automatic generation of CHANGELOG of GitHub releases 🤔

I will deal with this in a separate PR. For the moment, I am going to merge this. Thanks a lot @EhsanRanjbari