search

ros / joint_state_publisher

http://wiki.ros.org/joint_state_publisher
50 stars 81 forks source link

Races in GUI thread during joint state update #5

Closed disRecord closed 6 years ago

disRecord commented 7 years ago

This problem occurs when GUI is running and source_list topic is used to update joint state. Pose is updated only partially for long JointState messages.

source_cb() callback emits sliderUpdateTrigger signal. But it seems during sliders' values update in update_sliders() somehow onValueChange() is called. So pose is updated only partially: half of the pose comes from received source_list message and other half comes from the old sliders' state.

I added lock to onValueChange() and updateSliders() to prevent races.

clalancette commented 6 years ago

I'm sorry for the very long delay in answering this. I took a look at the code and the problem, and I'm not sure it is a locking problem as much as it is a design problem.

From what I can tell, the problem sequence goes something like this:

  1. JointStatePublisher.source_cb gets called when a new message comes in. It updates the free_joints member variable, then calls emit on sliderUpdateTrigger.
  2. The sliderUpdateTrigger signal causes JointStatePublisherGui.updateSliders to be called, which calls JointStatePublisherGui.update_sliders.
  3. update_sliders iterates over joint_map, which contains a reference back to free_joints from JointStatePublisher, plus references to the objects dealing with the Qt slider. During the loop, it updates the values in the map, then calls setValue on the Qt slider.
  4. The call to setValue is what ends up calling onValueChanged, which then iterates over all the values in joint_map. At this point, though, only some of them had their values updated, so you get the partial update that you originally saw.

I kind of feel like having the data in the joint updated in two different places like this is asking for trouble. It seems kind of like we want to update all of the data in update_sliders, and then only touch the Qt slider objects once all of that has been updated. But right at the moment, I'm not entirely sure how to do that; it will take some additional investigation.

Renha commented 6 years ago

hi! How's the progress with investigation?

clalancette commented 6 years ago

hi! How's the progress with investigation?

I finally had some time to get back to this and try some things out. I think I've fixed this on the slider-update branch of this repository. It works in my testing, but it would be better if someone else could confirm. Could someone who is having this problem try pulling that branch and giving some feedback on whether it works for them? Thanks!

disRecord commented 6 years ago

Thank you! It fixed the issue.

clalancette commented 6 years ago

Thank you! It fixed the issue.

Fantastic, thanks. I'm going to close this PR out and then open a new one from the slider-update branch.