search

robotology / walking-teleoperation

Software related to walking and teleoperation.
BSD 3-Clause "New" or "Revised" License
30 stars 14 forks source link

Add Haptic Glove Module #72

Closed kouroshD closed 2 years ago

kouroshD commented 3 years ago
kouroshD commented 2 years ago

The PR is ready to be reviewed, except the points mentioned in the first comment.

kouroshD commented 2 years ago

I have tested many times during these days the code, and I applied bug fixes as well as updates on the configuration file. All the features are implemented. The only remaining point is the ReadMe documentation, that I may do it later. In any case, the PR is ready to be reviewed. I have also updated the CI, so that has dependency into wearables as well. Now, in fact, it compiles haptic glove module in all OS.

traversaro commented 2 years ago

Given the size of the PR, if @S-Dafarra is already reviewing it I would remove myself from the reviewers. If there is any specific point on which you need my input @kouroshD feel free to point it out, thanks!

kouroshD commented 2 years ago

With this https://github.com/robotology/walking-teleoperation/pull/72/commits/fc89789c4fabb663a023cae91fd3c22e512bb5b0, I have modified the writeStrict option of the remotecontrolboard from true to false.

kouroshD commented 2 years ago

another thing is that in the RemoteControlBoard the default carrier is set to udp as in https://github.com/robotology/yarp/blob/ce6f008c86ba58a652b04b6bffdc2b46e9b157e2/src/devices/RemoteControlBoard/RemoteControlBoard.cpp#L247-L250 and https://github.com/robotology/yarp/blob/ce6f008c86ba58a652b04b6bffdc2b46e9b157e2/src/devices/RemoteControlBoard/RemoteControlBoard.cpp#L307. And I remember, I could run the HapticGloveModule some months ago from the operator side as well, however with some difficulties. In any case, now we are running it the robot head.

kouroshD commented 2 years ago

@S-Dafarra let me know if the responses are OK, if it needs further declarations/modifications.

S-Dafarra commented 2 years ago

@S-Dafarra let me know if the responses are OK, if it needs further declarations/modifications.

I think it is OK to me! I would just suggest opening an issue to remove hardcoded numbers when setting a haptic feedback

kouroshD commented 2 years ago

Before merging, I ran once the code in order to be sure it is running after changing the requested code changes, and I found a bug indeed related to those changes: remoteControlBoardsOpts.put("writeStrict", "off"); in https://github.com/robotology/walking-teleoperation/pull/72/commits/e24625747437e7b35f805fc43318c484cb05ccdd . Here is the video:

https://user-images.githubusercontent.com/17707730/149796456-f13ed2ff-6107-4cfc-aee3-9a12119071c1.mp4

S-Dafarra commented 2 years ago

I guess you can also remove that line. By default it should be off

kouroshD commented 2 years ago

I guess you can also remove that line. By default it should be off

It is not. Look at here:

https://github.com/robotology/yarp/blob/165a46c6e034fc27b90cc0bd80b35419679b22c3/src/devices/RemoteControlBoard/RemoteControlBoard.h#L96-L97

S-Dafarra commented 2 years ago

I guess you can also remove that line. By default it should be off

It is not. Look at here:

https://github.com/robotology/yarp/blob/165a46c6e034fc27b90cc0bd80b35419679b22c3/src/devices/RemoteControlBoard/RemoteControlBoard.h#L96-L97

Ah nice catch. It seems that it enables it when writing to multiple joints. I guess this is to make sure that the commands sent to all the joints is coherent in time :thinking: .

traversaro commented 2 years ago

I guess you can also remove that line. By default it should be off

It is not. Look at here: https://github.com/robotology/yarp/blob/165a46c6e034fc27b90cc0bd80b35419679b22c3/src/devices/RemoteControlBoard/RemoteControlBoard.h#L96-L97

Ah nice catch. It seems that it enables it when writing to multiple joints. I guess this is to make sure that the commands sent to all the joints is coherent in time 🤔 .

Yes, writeStrict is necessary for all the commands that require multiple messages for different joints, otherwise tipically the last one would be the only one considered.

S-Dafarra commented 2 years ago

So then, maybe it is worth leaving it with the default behavior :thinking:

kouroshD commented 2 years ago

Yes, writeStrict is necessary for all the commands that require multiple messages for different joints, otherwise tipically the last one would be the only one considered.

@traversaro What do you mean by requiring multiple messages for different joints? Can you give an example?

traversaro commented 2 years ago

Yes, writeStrict is necessary for all the commands that require multiple messages for different joints, otherwise tipically the last one would be the only one considered.

@traversaro What do you mean by requiring multiple messages for different joints? Can you give an example?

Let's say that for some reason you are sending velocity reference joint-by-joint with: https://github.com/robotology/yarp/blob/165a46c6e034fc27b90cc0bd80b35419679b22c3/src/devices/RemoteControlBoard/RemoteControlBoard.cpp#L1914 . In that case, if you do not have writeStrict, only the last command will be considered.

kouroshD commented 2 years ago

Yes, writeStrict is necessary for all the commands that require multiple messages for different joints, otherwise tipically the last one would be the only one considered.

@traversaro What do you mean by requiring multiple messages for different joints? Can you give an example?

Let's say that for some reason you are sending velocity reference joint-by-joint with: https://github.com/robotology/yarp/blob/165a46c6e034fc27b90cc0bd80b35419679b22c3/src/devices/RemoteControlBoard/RemoteControlBoard.cpp#L1914 . In that case, if you do not have writeStrict, only the last command will be considered.

As I see in that snippet, it sends the joint i reference value with false write strict option. I am not seeing any part that is discarding other commands! booh! I can put it back in our code in default option, what is your suggestion? When I tried yesterday with off option, I did not see any strange behavior. In any case, in our code, we are always send vector of joint values not a single joint!

kouroshD commented 2 years ago

@S-Dafarra @traversaro If you do not have other comments or suggestion, I will merge the PR. Thanks. I will keep the history of PR as it is.

traversaro commented 2 years ago

@S-Dafarra @traversaro If you do not have other comments or suggestion, I will merge the PR. Thanks. I will keep the history of PR as it is.

Sure on my side, see https://github.com/robotology/walking-teleoperation/pull/72#issuecomment-1008733201 !

S-Dafarra commented 2 years ago

@S-Dafarra @traversaro If you do not have other comments or suggestion, I will merge the PR. Thanks. I will keep the history of PR as it is.

GO also for me!