ros-perception / laser_filters

Assorted filters designed to operate on 2D planar laser scanners, which use the sensor_msgs/LaserScan type.
BSD 3-Clause "New" or "Revised" License
166 stars 204 forks source link

footprint_filter: print less tf warnings #13

Closed v4hn closed 10 years ago

v4hn commented 10 years ago

On startup this filter produces about two pages of console output (ROS_ERRORs) on ExtrapolationExceptions because the listener is not yet setup. This commit reduces this to one warning.

v4hn commented 10 years ago

In my opinion, the proper way to fix this is to wait for the requested transformation before update is called for the first time. However, it seems to me that neither the configure function nor the constructor is a good place to do this...

vrabaud commented 10 years ago

hi, thx for trying to fix that inconvenience. I think it is dangerous to do it the way you did: what if the tf is lost later but for different reasons ? You then do not get any information. How about the simpler:

ROS_ERROR_THROTTLE(1, "Transform unavailable %s", ex.what());

v4hn commented 10 years ago

Hey Vincent,

my pull-request works in the situation you describe. For every successful transform I reset the transformavailable flag, so the warning is printed once each time the transform is lost.

I didn't know about *_THROTTLE. I still believe it's enough to print the warning once every time a problem is first encountered, but if you insist, _THROTTLE would work as well. A rate of .2 should suffice for this message though; 1 would likely still produce more than one message on startup (over here).

More importantly though, I do not consider missing tfs an error. It happens quite often in ROS that tfs are not available unless you wait for them explicitly. Imho waiting does not really make sense in this case because it's called so often that it's probably better to drop a scan than to wait for the tf. That's why I changed the macro to ROS_WARN.

Even one error on startup is too much in my opinion. It takes much more time to look through the startup log output to see whether everything works as expected, because "oh a red message" is no indicator for serious problems anymore.

Thanks for your time!

vrabaud commented 10 years ago

right, your fix works sorry. Now, there are two ways to behave I believe:

I believe 2 is usually what's done: you get a warning whenever there's a problem and nothing otherwise (it's weird to get a warning when things are back to normal).

So please either add something in the log to say you are back to normal, or go with a throttle (if you go with it, a frequency of 1Hz does not seem crazy: you will end up have less than 10 warnings overall right ? if you get a real problem, it seems to me that 0.2 would make you notice it too late or it would get lost in the logs).

Opinions ? Thx ! I'm learning things :)

v4hn commented 10 years ago

I don't see why you would need a warning when things work (again), because then you have a working pipeline and no need to look into the log files anymore. But I see your point.

Concerning the "few" warnings on startup: I aim at a clean startup without warnings. Even one warning is annoying, although much better than two pages of errors..

I completely reworked the pull-request, please comment.

vrabaud commented 10 years ago

ok, I think your PR works as it is, thx for ironing out those details !