Better error handling when loading chains #90
Conversation
|
Is this always an error though? With laser filters for example, It's not uncommon to modify the set of filters used on a robot, and sometimes I try running with no filters, but leave the filter chain node because I'll continue changing the set of filters in the future. |
|
Hello @jonbinney, Thanks for your comments! I don't fully understand what you mean by the "chain_filter" node. Do you mean you have a dedicated node for filtering and you change the number of filters at runtime? For example, starting the node with an empty filter chain and adding new filters afterwards? IMHO, the configure function should fail/return false if there's nothing to configure, but maybe I'm not seeing the full picture. If you think the changes I'm proposing could break a lot of code, I could indeed add a function to check the number of filters in the chain, which would allow for better handling of the chain in controllers and so on. Please let me know what you think and I will quickly adapt the PR accordingly. Thanks a lot! |
|
Sorry I wasn't clear on which "node" I was talking about. I use the filters mainly through the ros2 node that laser_filters provides, which internally uses the filters package to load and configure the chain of filters. There are other ros2 nodes that other people use, which filter various different things. Yeah, I can see an argument either way for whether an empty chain is a valid chain. I've had use cases where it was, particularly because it makes it easy to try some filters, then try having the raw data, but not change the topology of the graph (the chain still runs but does nothing, since it isn't "filtering"). That makes it easy to add filters back in as part of an iterative process to figure out what works best for the application (slam/obstacle avoidance/object detection/etc). Since this has been the behavior for a while, it'd be a breaking change for a lot of third party code. If you change your PR to instead add a function to check the filter length, that should be pretty painless to review and get merged. Thanks! |
OscarMrZ
force-pushed
the
load_chain_better_errors
branch
from
170fd49 to
cf3668a
Compare
OscarMrZ
force-pushed
the
load_chain_better_errors
branch
from
cf3668a to
67890ab
Compare
|
Thanks a lot for the clarification! I get what you mean now, I see how an empty chain could be a valid use case. I updated the PR accordingly, adding a length getter and the appropriate tests. Thanks a lot for your time! |
| @@ -190,6 +191,9 @@ class FilterChain | |||
| */ | |||
| bool update(const T & data_in, T & data_out) | |||
| { | |||
| if (!configured_) { | |||
There was a problem hiding this comment.
Thank you for adding these checks for configured_!
|
@OscarMrZ I'm going to let this sit for 48 hours after merging before I do a release, just in case someone notices a problem. Thank you for your contribution, and thank you for a very well written PR! |
|
Perfect, sounds good! You're welcome, thank you for your time! |
|
This is included in version 2.2.2 on humble, jazzy, kilted, and rolling. I've done the releases but it'll take some time to build and usually a couple weeks to sync to the main apt repos. |
As described in #89, loading a sensor chain without configuration would succeed, making it impossible to know from the calling code if there's actually a working filter chain.
The fix is trivial: just consider a chain configured if the resulting
found_filtersvector after loading the configuration is not empty.I also added a check in the
updatefunction to throw if someone tries to call update without configuring the chain first.