search

ubi-agni / tactile_toolbox

software to handle tactile sensors in ROS
9 stars 5 forks source link

Improvements and fixes for various issues #13

Closed rhaschke closed 3 years ago

rhaschke commented 3 years ago
rhaschke commented 3 years ago

Looks like the "Hide Small Values" property of WrenchVisuals was introduced in Melodic only. This explains that #7 suddenly popped up: The property wasn't correctly initialized in rviz. I vote for dropping support for Kinetic.

guihomework commented 3 years ago

Looks like the "Hide Small Values" property of WrenchVisuals was introduced in Melodic only. This explains that #7 suddenly popped up: The property wasn't correctly initialized in rviz. I vote for dropping support for Kinetic.

I did not test the new proposed solution yet, but how can the "hide small values" come into play (whether it is on or off) when all values have the same amount (I am publishing a 1.0 on all) ?

rhaschke commented 3 years ago

How can the "hide small values" property come into play?

As I have written, the corresponding flag in rviz' WrenchVisual wasn't initialized and thus randomly true or false. If true, the arrows were hidden. What is considered small is hardcoded in rviz.

Looking at that code, I'm wondering why the vectors are hidden so early... Need to double-check.

guihomework commented 3 years ago

How can the "hide small values" property come into play?

As I have written, the corresponding flag in rviz' WrenchVisual wasn't initialized and thus randomly true or false. If true, the arrows were hidden. What is considered small is hardcoded in rviz.

So you mean that even with those "quite" large arrows they were hidden; Then the problem is indeed the decision of what is "small" that came into play.

regarding dropping support for kinetic. We have a melodic-branch, one can drop support of kinetic there I suppose.

guihomework commented 3 years ago

I confirm that changing the arrow width made the arrow visible even without this fix, so it is related to the way the hidesmallvalues is calculated. I will test this branch now

rhaschke commented 3 years ago

Travis is finally happy as well.

rhaschke commented 3 years ago

Thanks for the intense testing. Probably sim_time starts at zero and subtracting the timeout duration then would result in a negative deadline. Would be great if you could provide a rosbag file to test this scenario.

rhaschke commented 3 years ago

I fixed the offending commit, rewriting history. Could you give it another try?

guihomework commented 3 years ago

Thanks for the intense testing. Probably sim_time starts at zero and subtracting the timeout duration then would result in a negative deadline. Would be great if you could provide a rosbag file to test this scenario.

no rosbag needed, I thought this was clear. Just start a roscore, then set rosparam set /use_sim_time true then start rviz, load a tactile_contact_state plugin, crash.

guihomework commented 3 years ago

I am wondering now that this PR introduces changes on the messages and timeouts and stuff like that, could we have information, about newly incoming data, but that is immediately outdated ? I mean contact data that has a time inferior to (now - timeout) when it arrives ?

I ask because I fear I vaguely remember asking for such a feature in the past, and maybe the "no recent msg" was somehow there to indicate that. One should be sure before deleting it. More details follow.

what happens is that one thinks everything is ok in the contact_state plugin, but there is no display at all. I would not care about empty contact being received, or no new data when a certain contact was not refreshed for a while (which would be a "no recent msg" and I don't need that), but I care about the fact that a new data cannot be published because it is outdated before being even displayed and would like to be informed if possible. Such an information would mean one has forgotten to set use sim time to true, and publish valid contact state in the past, or if the processing of contact state is too slow for instance.

Another issues related to that, is in case the tf is also outdated, in this PR, the status messages stays at "not yet published...." when data is clearly published, but tf cannot be processed

Possible reasons are listed at http://wiki.ros.org/tf/Errors%20explained
[ WARN] [1602094688.843153787]: TF_OLD_DATA ignoring data from the past for frame thumb_proximal_link at time 1.60209e+09 according to authority unknown_publisher
guihomework commented 3 years ago

should I test now again with my more advanced examples ?

rhaschke commented 3 years ago

@guihomework, I performed a preliminary merge with melodic-devel to resolve the conflicts. If you give it the final approval, I will fast-forward melodic-devel to this PR branch.

rhaschke commented 3 years ago

@guihomework, you are asking a new feature (in https://github.com/ubi-agni/tactile_toolbox/pull/13#issuecomment-705113442), now that this PR is close to ready. My suggestion is that we personally discuss what you want and implement the new feature next week. I need to focus on other things for the rest of the week.

guihomework commented 3 years ago

@guihomework, you are asking a new feature (in #13 (comment)), now that this PR is close to ready. My suggestion is that we personally discuss what you want and implement the new feature next week. I need to focus on other things for the rest of the week.

I had to write a comment in this PR before you merge in case the feature I asked in the past WAS the "status" you removed due to Gereon's request. Maybe adapting the deletion would have been necessary. I was not certain.

Just tell that what I am talking about is not affecting this PR, that it is ok to delete the "no recent msg" and we are good. I was not really asking for more work, delaying the merge. On the other hand this PR solves various issues at the same time, so maybe my idea as close to the work done in this PR.

Forget what I say. I will test now.