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.

Sign up for GitHub

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

Jump to bottom

Fix Haptic glove transmission - GloveDevice #204

Merged
merged 15 commits into from
Jun 25, 2024
Merged

Fix Haptic glove transmission - GloveDevice #204

merged 15 commits into from
Jun 25, 2024

Conversation

EhsanRanjbari
Copy link
Contributor

@EhsanRanjbari EhsanRanjbari marked this pull request as ready for review January 30, 2024 13:02
@EhsanRanjbari EhsanRanjbari marked this pull request as draft January 30, 2024 13:02
@mebbaid mebbaid requested review from S-Dafarra and mebbaid February 1, 2024 10:44
@EhsanRanjbari EhsanRanjbari marked this pull request as ready for review March 1, 2024 16:37
@EhsanRanjbari EhsanRanjbari mentioned this pull request Mar 1, 2024
Closed
S-Dafarra
S-Dafarra reviewed Mar 4, 2024
View reviewed changes
Copy link
Contributor

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

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

There a few things I did not manage to understand

devices/HapticGlove/src/HapticGlove.cpp Outdated Show resolved Hide resolved
devices/HapticGlove/src/HapticGlove.cpp Show resolved Hide resolved
devices/HapticGlove/src/HapticGlove.cpp Show resolved Hide resolved
wrappers/IWearActuators/src/IWearActuatorsWrapper.cpp Outdated Show resolved Hide resolved
@traversaro
Copy link
Member

@EhsanRanjbari what is the status of this PR?

@EhsanRanjbari
Copy link
Contributor Author

@EhsanRanjbari what is the status of this PR?

Waiting to do some tests on the robot.

@S-Dafarra
Copy link
Contributor

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

@EhsanRanjbari
Copy link
Contributor Author

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 (robotology/walking-teleoperation#136)

@S-Dafarra
Copy link
Contributor

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

@EhsanRanjbari
Copy link
Contributor Author

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
Copy link
Contributor Author

EhsanRanjbari commented May 31, 2024 •
edited
Loading

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
Copy link
Contributor

S-Dafarra commented May 31, 2024 •
edited
Loading

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 #204 (comment)). 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

S-Dafarra
S-Dafarra reviewed Jun 17, 2024
View reviewed changes
Copy link
Contributor

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

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

Good move in adding a new message! Thanks for dealing with my comments @EhsanRanjbari. I have given it another round of review

devices/HapticGlove/src/HapticGlove.cpp Show resolved Hide resolved
devices/HapticGlove/src/HapticGlove.cpp Show resolved Hide resolved
interfaces/IWear/include/Wearable/IWear/Actuators/IHaptic.h Outdated Show resolved Hide resolved
wrappers/IWearActuators/src/IWearActuatorsWrapper.cpp Outdated Show resolved Hide resolved
wrappers/IWearActuators/src/IWearActuatorsWrapper.cpp Outdated Show resolved Hide resolved
@S-Dafarra S-Dafarra mentioned this pull request Jun 19, 2024
Merged
@EhsanRanjbari
Copy link
Contributor Author

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

S-Dafarra
S-Dafarra approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

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

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!

@EhsanRanjbari
Copy link
Contributor Author

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
Copy link
Contributor Author

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 2bb6185 I tried to make it optional.

@S-Dafarra
Copy link
Contributor

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

@EhsanRanjbari
Copy link
Contributor Author

@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

@S-Dafarra
Copy link
Contributor

@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 robotology/walking-teleoperation#143

@S-Dafarra
Copy link
Contributor

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

wearables/CMakeLists.txt

Line 7 in 91722ca

set(PROJECT_VERSION "1.8.0")

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

@EhsanRanjbari
Copy link
Contributor Author

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

wearables/CMakeLists.txt

Line 7 in 91722ca

set(PROJECT_VERSION "1.8.0")

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

Done! 32ff17c

@traversaro
Copy link
Member

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
Copy link
Member

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 😉

@S-Dafarra
Copy link
Contributor

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 🤔

@S-Dafarra
Copy link
Contributor

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

@S-Dafarra S-Dafarra merged commit 6e646b9 into robotology:master Jun 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@S-Dafarra S-Dafarra S-Dafarra approved these changes

@mebbaid mebbaid mebbaid approved these changes

Assignees

@EhsanRanjbari EhsanRanjbari

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EhsanRanjbari @traversaro @S-Dafarra @lrapetti @mebbaid