API review
Proposer: Josh F
Present at review:
- List reviewers
This is a review of the following additions to message_filters
API Additions
Chain
doc/api/message_filters/html/c++/classmessage__filters_1_1Chain.html
message_filters::Chain allows you to store a dynamic chain of filters. For example:
It is also possible to pass bare pointers in, which will not be automatically deleted when Chain is destructed:
It also includes a base class, ChainBase (doc/api/message_filters/html/c++/classmessage__filters_1_1ChainBase.html), which allows you to store the chain itself in a type-independent manner.
Chain only works with single-input/single-output (simple) filters.
In addition to adding a filter you can also retrieve one added previously:
Policy-based Synchronizer and two policies (ExactTime, ApproximateTime)
This is a replacement for the current TimeSynchronizer class. It is currently in the message_filters::synchro namespace (sync is reserved by the sync() system call).
Synchronizer and friends are in a nested namespace because I can see other message filters (like the sequencer) moving to follow this pattern, and I don't want their policy classes colliding. If people have suggestions for other names for the namespace I'd like to hear them, since I'm not a big fan of synchro
The driving force behind changing TimeSynchronizer to be policy based is the creation of Romain's ApproximateTimeSynchronizer that duplicated TimeSynchronizer's code until we could integrate them together.
The equivalent of:
is:
Using the new approximate time policy:
Approximate Time Policy
ApproximateTime policy algorithm description
Drop callback
This is in a separate category because it adds functionality to what used to be the TimeSynchronizer.
You can register a drop callback in the same way you register a normal one, but with registerDropCallback. This callback is called whenever a set of messages are dropped.
Use of traits instead of directly accessing message.header.*
All filters now use the ros::message_traits::TimeStamp trait to retrieve the timestamp from a message. This means you can now specialize that trait if you like.
Question / concerns / comments
Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.
- (Romain) One issue is that the definition is a little more verbose, and the users has to know what a "Policy" is. Would it make sense and is it possible to typedef
message_filters::synchro::Synchronizer<message_filters::synchro::ExactTime<...> >
- (Romain) Given that you managed to make a larger part of the code non-repetitive (i.e. it does not depend on the number of template parameters), what would it take to remove the restriction of 9 messages. Sure, I can't think of anything that would require it...
(Romain) This is probably not the place for this, but would it be possible to synchronize on the availability of tf as well? I.e. a mix between Synchronizer and TransformListener. I had a case with the pan-tilt table where that might have been useful (I since found a better way that does not need this).
to message_filters::synchro::ExactSynchronizer<...>?
Meeting agenda
To be filled out by proposer based on comments gathered during API review period
Conclusion
Package status change mark change manifest)
Action items that need to be taken.
Major issues that need to be resolved
Check sync:: namespace again
Possibly pull Synchronizer up into message_filters namespace
Fix TimeSynchronizer constructors to call connectInput() and check the unit tests
Look into convenience constructor for policy creation
Move Synchronizer drop callback into the ExactTime policy
Implement queue size in ApproximateTime policy