[Documentation] [TitleIndex] [WordIndex] 

API review

Proposer: Tully Foote

Present at review:

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.

Meeting agenda

One more pass before clearing the API

Stu

After using the filter library I have a number of thoughts and suggestions:

XML Parsing

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:

  1. It clutters up the FilterBase class

  2. If the helpers are truly useful, they should be available to other packages (controllers, mechanism model, etc...).

  3. 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)

Other results:

FilterBase

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.

Channels

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: 

   1   KDL::Twist twist_meas_filtered;
   2   std::vector<double> tmp_twist(6);
   3   for (size_t i = 0; i < 6; ++i)
   4     tmp_twist[i] = twist_meas_[i];
   5   twist_filter_.update(tmp_twist, tmp_twist);
   6   for (size_t i = 0; i < 6; ++i)
   7     twist_meas_filtered[i] = tmp_twist[i];

FilterChain

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.

Misc

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.

Josh

Conclusion

Package status change mark change manifest)

2024-11-23 14:38