Proposer: Tully Foote
Present at review:
- List reviewers
Question / concerns / comments
This is a follow up of the previous API review. filters/2009-01-23_API_Review
I have resolved most issues.
The outstanding issue from the last review is that I have not added callbacks for intermediate value debugging.
Please take a look at generic_laser_scan_filter.cpp in laser_scan package as an example usage.
And there are three filters in that package as well as the ones in the filters package.
One more pass before clearing the API
After using the filter library I have a number of thoughts and suggestions:
FilterBase contains a number of functions for parsing XML. I do agree that XML parsing is currently clumsy, but the helper functions do not belong in FilterBase for several reasons:
It clutters up the FilterBase class
- If the helpers are truly useful, they should be available to other packages (controllers, mechanism model, etc...).
It adds a new data type for storing configuration info map<string,string>. Now I have to understand how the XML converts into the string map, and then extract values from the string map.
We should either continue using XML to store configuration or switch to the parameter server. If we do continue using XML, then we should use a standard, tested way of pulling individual values out, either by using the TinyXML interface, by using XPath, or by writing our an interface and putting it through a careful API review.
(My preference is to switch over to the parameter server, or if we keep using XML to use XPath)
- loadXml is no longer needed
- The configure(...) method (with arguments) is declared pure virtual and the configure() method is deleted.
The filter_name_ and filter_type_ fields should be set externally from the filter and not within the filter itself.
The configured_ member should be deleted. When configure(...) returns false, the result of every operation is undefined.
I found the concept of channels to be difficult to understand and clumsy to use. I think the filter library becomes much cleaner and easier to use if the concept of channels is not deeply embedded within the filter itself.
I would much rather see an array of filters rather than a filter that sometimes operates on arrays. Similarly, an array of FilterChain's makes more sense than a FilterChain that contains (some number of) filters with some number of channels each. I much prefer the structure used with Pid objects inside the CartesianPoseController. It contains an array of Pid elements, each of which is responsible for a single control loop. Can you justify turning it into a Pid controller which has multiple channels, one for each control loop?
Having the filters take in vectors of data has a few other annoying effects:
Perhaps update should return false once the first filter fails?
If you want to be really clever, make FilterChain inherit from FilterBase.
Laser Scan Filters
Many of these filters only operate on a single channel.
I definitely do not understand what the median filter is doing here.
Please try to use scoped_ptr throughout (TransferFunctionFilter).
Besides my criticism, the filter library was quite easy to use and let me do mean filtering on velocities quickly and easily. Thanks.
- ROS_REGISTER_FILTER should probably be FILTERS_REGISTER_FILTER
(Parameter server loading now) Need a more unified parser/dictionary loading
- Don't need XML's extra structure.
- Easy import from param_server or yaml file
- stl based storage
- seperate loaders from param, YAML, xmlrpc
- introspection of presence ( fail on wrong type)
(reconfiguring supported but must be triggered) Reconfiguring would be nice to allow slider tuning
- some parameters are reconfigurable
- could have a model of makring some parameters as configurable
- it could be stated that reconfiguring might break realtime when reconfiguring
document name and type loading in base class better
Make a basic filter and a multi channel filter which inherits
Note where median is taking place in laserScan filter
- loook at making chain inherit from base
break after false update
scoped_prt in transferfunction filter
Package status change mark change manifest)
Action items that need to be taken.
Major issues that need to be resolved