Dev angular interval filter#107
Dev angular interval filter#107ulassbin wants to merge 2 commits intoros-perception:kinetic-develfrom
Conversation
|
|
||
| gen = ParameterGenerator() | ||
|
|
||
| gen.add("interval_sets", str_t, 0, "A string parameter", "[[-1,1],[-1.2,-1.1],[0.5,0.8]]") |
There was a problem hiding this comment.
interval_sets could use a better help string. Also what do you think about shortening the name to just intervals? sets sounds discrete.
| gen = ParameterGenerator() | ||
|
|
||
| gen.add("interval_sets", str_t, 0, "A string parameter", "[[-1,1],[-1.2,-1.1],[0.5,0.8]]") | ||
| gen.add("enable", bool_t, 0, "Whether the filter is enabled or not", False) |
There was a problem hiding this comment.
Maybe remove enable? I can see how it saves a bit of yaml editing during testing, but it might confuse users since none of the other filter types have this option.
There was a problem hiding this comment.
I think the most reasonable default here should be probably be an empty list.
| # | ||
| # \author Yannick de Hoop | ||
|
|
||
| from dynamic_reconfigure.parameter_generator_catkin import * |
There was a problem hiding this comment.
Thanks for including dynamic_reconfigure for this!
| dyn_server_->setCallback(f); | ||
|
|
||
|
|
||
| if (!filters::FilterBase<sensor_msgs::LaserScan>::getParam(std::string("interval_sets"), interval_sets_)) |
There was a problem hiding this comment.
Doesn't dynamic_reconfigure::Server call your callback with default values when you instantiate it? I think it will read the parameter here so you could skip this part. I guess the one downside is that if the user doesn't set a ROS param to override the default values from the .cfg file, you won't know to warn them. But that's usually how dynamically reconfigureable params work in ros, so probably fine?
|
|
||
| void reconfigureCB(AngularIntervalFilterConfig& config, uint32_t level) | ||
| { | ||
| interval_sets_ = config.interval_sets; // th11,th12;th21,th22;th31,th32 |
There was a problem hiding this comment.
Does the interval_sets_ string need to be a member variable? It seems like you could parse it here and only store the intervals_ as doubles.
There was a problem hiding this comment.
Also it looks like the format in the comment doesn't match what is in the .cfg file. Shouldn't this be [[th11 th12],[th3, th21]...?
| else | ||
| { | ||
| got_intervals_ = false; | ||
| ROS_WARN("Failed to calculate intervals: %s", parse_error.c_str()); |
There was a problem hiding this comment.
I think it would be cleaner here to throw an exception on an error immediately, rather than printing out a warning. (This is what polygon filter does on a bad reconfigure parameter:
laser_filters/src/polygon_filter.cpp
Line 94 in 353392e
| filtered_scan = input_scan; | ||
| double replacement_value = std::numeric_limits<double>::quiet_NaN(); | ||
| double current_angle = input_scan.angle_min; | ||
| if(got_intervals_ && enable_) |
There was a problem hiding this comment.
Consider making the name of this variable a bit more detailed. Maybe have_valid_intervals?
| } | ||
|
|
||
|
|
||
| bool parseIntervalString(const std::string& input, std::string& error_return, std::vector<std::vector<double>>& interval) |
There was a problem hiding this comment.
This function is a bit verbose, but it looks since it has worked in polygon filter, we can leave it alone.
| <class name="laser_filters/LaserScanAngularIntervalFilter" type="laser_filters::LaserScanAngularIntervalFilter" | ||
| base_class_type="filters::FilterBase<sensor_msgs::LaserScan>"> | ||
| <description> | ||
| This is a filter which filters sensor_msgs::LaserScan messages based on angular intervals. |
There was a problem hiding this comment.
Could you add a couple more sentences to the description? Maybe something like For example, if the intervals parameter is ...., then the filter will only allow laser beams whose angle is between .... and ....
2 participants
Implemented laser angular interval filter and added yaml description in test/test_scan_filter_chain.yaml.
Angular interval filter takes a parameter interval sets in the form of:
interval_sets: "[[0,0.5],[2,3.2],[5,6.5]]"
which will crop the laser scan between the ranges
0 to 0.5
2 to 3.2
5 to 6.5
Also this parameter can be changed from dynamic parameter server.
It is disabled by default. Can be enabled via dynamic parameter server.