robotology / walking-teleoperation

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

Fix the transmission strategy of the haptic feedback #136

Closed S-Dafarra closed 2 months ago

S-Dafarra commented 10 months ago

Right now, both the force and the vibrotactile feedback are sent in a single command per finger using write strict. See https://github.com/robotology/walking-teleoperation/blob/a3b51def52b3c420d123331a03f924019e0e57f6/modules/HapticGlove_module/src/GloveWearable.cpp#L338-L354 and https://github.com/robotology/walking-teleoperation/blob/a3b51def52b3c420d123331a03f924019e0e57f6/modules/HapticGlove_module/src/GloveWearable.cpp#L368-L385

This is terribly inefficient, requires a huge bandwidth, and it is blocking the module in case of connection issues.

cc @EhsanRanjbari

EhsanRanjbari commented 10 months ago

Is there a way to measure and print the bandwidth? @S-Dafarra

S-Dafarra commented 10 months ago

We were using iftop. I am not sure if there is a way to get it programmatically.

EhsanRanjbari commented 10 months ago

I have some ideas in order to decrease the bandwidth usage by the haptic glove. First, let's start by seeing how I understood the issue so far:

hapticCommand

So, HapticGloveModule writes the haptic command to the buffer port, and on the other side, the HapticGloveDevice reads the haptic command from the buffer port.

HapticGloveModule, writes the haptic command using the write() method with the forceStrict = true option which is equivalent to the method writeStrict() (check here).

https://github.com/robotology/walking-teleoperation/blob/a3b51def52b3c420d123331a03f924019e0e57f6/modules/HapticGlove_module/src/GloveWearable.cpp#L352

So, from the figure above, the total bandwidth usage by the haptic glove is bandwidth_1 + bandwidth_2.

My idea is first,

  1. Decrease the number of commands sent in total by NOT sending the zero-value commands. (if this is helpful, we can also consider not sending small values when the network is weak.): _Decrease bandwidth1
  2. Change the buffering policy at the receiver side to FIFO by calling yarp::os::BufferedPort::setStrict(). The reason is in this case, the command will be stored in the buffer port until the next read call from the reader side. Simply meaning not use the bandwidth_2 until the reader calls it. However, there are drawbacks such as allocating memory in the background and the latency issue if the reading is slower than writing (which I think step 1 will help this): _Decrease bandwidth2

Moreover, I have some questions that I will try to answer:

  1. Do shorter messages use less bandwidth?

  2. Does removing string parts from messages and replacing them with an integer value result in using less bandwidth? or the string parts are the essential part and cannot be removed in any case?

S-Dafarra commented 10 months ago

Consider that sending many messages in a short amount of time is much more expensive than sending a single big message. This is because of the so-called overhead. Moreover, because of the write strict, if one message gets delayed, the sending is blocked, meaning that the whole hapticGloveModule gets stuck. Basically, if for some reason the haptic feedback to the pinky is delayed, the entire retargeting of both hands is blocked.

Hence, a first step could be simply to "merge" all the haptic commands in a single message, thus also allowing the removal of the writeStrict option.

mebbaid commented 7 months ago

@mebbaid check this issue!

EhsanRanjbari commented 7 months ago

with these commits:

I modified the haptic message. Previously, the message was like

"name" type value duration

as an example:

"HapticGlove::haptic::r_index_finger::ForceFeedback" 0 40 0.0
"HapticGlove::haptic::r_index_finger::VibroTactileFeedback" 0 30 0.0

Now, I merged two messages into one message which includes force and vibrotactile feedback as

"name" type forceValue vibroTactileValue duration

and changed the name to a general one as HapticFeedback For an example:

"HapticGlove::haptic::r_index_finger::HapticFeedback" 0 40 70 0.0

Note 1: To do this, I had to change the thrift message file which I still need to verify if changing it will not interfere with other applications which might use the same thrift.

Note 2: I noticed that another message is also being sent for the palm vibrotactile. I need to also deal with this.

Notre 3: I tried on my machine and it seems working.

Note 4: To see the effect on the bandwidth, tests will be performed on the robot.

Note 5: @S-Dafarra and @mebbaid, I am going to open two draft PRs for these changes. Please feel free to comment. Thanks!

S-Dafarra commented 7 months ago

Hi @EhsanRanjbari, that's ok, but consider that what I wrote in the first comment:

Right now, both the force and the vibrotactile feedback are sent in a single command per finger using write strict. See

is still not fixed, i.e., there is still a separate message per finger, and you still need to use force strict.

The idea would be to condense all the haptic commands for a given hand in a single message

EhsanRanjbari commented 7 months ago

Hi @EhsanRanjbari, that's ok, but consider that what I wrote in the first comment:

Right now, both the force and the vibrotactile feedback are sent in a single command per finger using write strict. See

is still not fixed, i.e., there is still a separate message per finger, and you still need to use force strict.

The idea would be to condense all the haptic commands for a given hand in a single message

Thanks for the suggestion, now with these commits

"name" type forceValueVector vibrotactileValueVector durationValueVector
"HapticGlove::haptic:HapticFeedback" 0 (0 0 0 0 0) (0 0 0 0 0) (0 0 0 0 0)

where the elements of forceValueVector, vibrotactileValueVector, and durationValueVector correspond to

"r/l_thumb_finger" "r/l_index_finger" "r/l_middle_finger" "r/l_ring_finger" "r/l_pinky_finger"

Note: There is a method called setMotorPosition that is used in Paexo.cpp, which also uses the same thrift message. I think this needs to be considered.

cc @S-Dafarra @mebbaid

S-Dafarra commented 7 months ago

There is a method called setMotorPosition that is used in Paexo.cpp, which also uses the same thrift message. I think this needs to be considered.

What about using two different messages, so that Paexo does not need to be touched?

mebbaid commented 7 months ago

With the above, we tried to replicate the teleoperation setup for testing even though the robot was not available. My labtop, connected to the robot server acted as the robot simulator, and on the other side the teleoperation setup. Both sides connected via vpn and the yarpserver running on the teleoperation side. Here are the output of iftop which we will analyze thanks to @EhsanRanjbari

After modificaiton Before modificaiton
after_msg before_msg
EhsanRanjbari commented 7 months ago

From the screenshots above it is visible that there is a significant reduction in the bandwidth usage. Almost %64. To be more clear I highlighted the photos below:

After modification Before modificaiton
After_Highlighted Before_Highlighted
S-Dafarra commented 7 months ago

From the screenshots above it is visible that there is a significant reduction in the bandwidth usage. Almost %64. To be more clear I highlighted the photos below:

After modificaiton Before modificaiton after_highlighted before_highlighted

To be honest, I think the two images are not helpful. If you look closely, the traffic you have highlighted is between different machines on the "before" and "after" images. You can also see some traffic toward google. Anyhow, it os worth to test on the robot, so it will be easier to check the traffic flowing between the two networks

mebbaid commented 7 months ago

the traffic you have highlighted is between different machines on the "before" and "after" images.

I thought the images always show the same traffic namely between iiticubws069 and 172.31.3.143 ?

EhsanRanjbari commented 7 months ago

To be honest, I think the two images are not helpful. If you look closely, the traffic you have highlighted is between different machines on the "before" and "after" images. You can also see some traffic toward google. Anyhow, it os worth to test on the robot, so it will be easier to check the traffic flowing between the two networks

I updated the highlight photos, it seems the traffic is between the same machines.

mebbaid commented 7 months ago

Perhaps @S-Dafarra means that there is traffic for which we don't know the cause since there is multiple machines in that network and also internet (even if the up/down/accum highlighted are between the same two machines). So on isolation in the check with the robot, we will see only the traffic resulting from the gloves @EhsanRanjbari

EhsanRanjbari commented 7 months ago

Perhaps @S-Dafarra means that there is traffic for which we don't know the cause since there is multiple machines in that network and also internet (even if the up/down/accum highlighted are between the same two machines). So on isolation in the check with the robot, we will see only the traffic resulting from the gloves @EhsanRanjbari

Yes, I understand now. In this case, let's see if we have similar results on the robot.

S-Dafarra commented 7 months ago

Perhaps @S-Dafarra means that there is traffic for which we don't know the cause since there is multiple machines in that network and also internet (even if the up/down/accum you highlighted are between the same two machines). So on isolation in the check with the robot, we will see only the traffic resulting from the gloves @EhsanRanjbari

Exactly!

Also, the traffic toward 172.31.3.143 is basically the same between the two figures. At the same time, I am not even sure what that machine is. It does not seem a machine in the IIT network.

In any case, even if the traffic remains the same, we can avoid using write strict (since we would not need every message to arrive in order), which is an advantage in a delayed network

EhsanRanjbari commented 7 months ago

In any case, even if the traffic remains the same, we can avoid using write strict (since we would not need every message to arrive in order), which is an advantage in a delayed network

I removed the argument true when calling the m_iWearActuatorPort.write(). According to the YARP documentation, this means write strict is avoided.

https://github.com/EhsanRanjbari/walking-teleoperation/blob/957f84545757f10b6a08da2fd1100c26e6dcc2cf/modules/HapticGlove_module/src/GloveWearable.cpp#L417

EhsanRanjbari commented 6 months ago

Testing the SenseGloves with the modified haptic message, @mebbaid and @S-Dafarra noticed a significant delay in the retargeting. After investigation and testing again on February 13th, it turned out that it was related to the failed calibration. More specifically, upon starting the Senseglove, a vibrotactile feedback with a value of 35 is sent to the glove that notifies the operator to start the calibration. Now with the modified haptic message, this is not being done and the user never knows when to start the calibration. Below is the video of the second test the operator started the calibration on his own and it is working fine.

https://github.com/robotology/walking-teleoperation/assets/75568997/d017481e-caeb-4608-9403-308d4c87b0cc

EhsanRanjbari commented 6 months ago

With this commit https://github.com/robotology/walking-teleoperation/pull/139/commits/c50fc6120639050fc509178ab4479c7c1c0f62eb I fixed the vibration feedback when starting the calibration. I tested it in simulation and it works as expected. However, I need to test it on the robot to fully confirm this.

Another point I would like to discuss is that the palm vibrotactile feedback seems to be turned off (https://github.com/EhsanRanjbari/wearables/blob/7094f84275c29e6e4c60312f4754574ab2db790e/devices/HapticGlove/src/HapticGlove.cpp#L177). If this is the case, at this level, this newly modified message does not include the palm vibrotactile value.

cc @mebbaid @S-Dafarra

S-Dafarra commented 6 months ago

Another point I would like to discuss is that the palm vibrotactile feedback seems to be turned off

I think that's just an initialization value

EhsanRanjbari commented 6 months ago

Another point I would like to discuss is that the palm vibrotactile feedback seems to be turned off

I think that's just an initialization value

yes, but it is not been updated (at least I could not find it) before being written to the port.

S-Dafarra commented 6 months ago

yes, but it is not been updated (at least I could not find it) before being written to the port.

It is set here: https://github.com/EhsanRanjbari/wearables/blob/7094f84275c29e6e4c60312f4754574ab2db790e/devices/HapticGlove/src/HapticGlove.cpp#L209-L210

EhsanRanjbari commented 6 months ago

With this commit c50fc61 I fixed the vibration feedback when starting the calibration. I tested it in simulation and it works as expected. However, I need to test it on the robot to fully confirm this.

Another point I would like to discuss is that the palm vibrotactile feedback seems to be turned off (https://github.com/EhsanRanjbari/wearables/blob/7094f84275c29e6e4c60312f4754574ab2db790e/devices/HapticGlove/src/HapticGlove.cpp#L177). If this is the case, at this level, this newly modified message does not include the palm vibrotactile value.

cc @mebbaid @S-Dafarra

Today, with @mebbaid we tested the code on the robot and the vibration upon starting the calibration works fine. I undrafted the related PRs:

S-Dafarra commented 6 months ago

Today, with @mebbaid we tested the code on the robot and the vibration upon starting the calibration works fine. I undrafted the related PRs:

Are they actually ready to be reviewed? I still see a lot of commented code

EhsanRanjbari commented 6 months ago

Are they actually ready to be reviewed? I still see a lot of commented code

Oh my bad, I had to remove them. I will do it now.

EhsanRanjbari commented 6 months ago

I will do it now.

Done! Thanks for the reminder.

S-Dafarra commented 2 months ago

The PRs have been merged. I think we can finally close this issue. Thanks a lot @EhsanRanjbari @mebbaid