Proposer: Josh Faust
Present at review:
- List reviewers
(proposed implementation) http://wiki/roscpp/NewMessageAPIProposal2
specifically see message_traits.h, serialization.h and service_traits.h in roslib
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.
I had a number of questions, but they all got answered by the time I got to the end. This looks really nice and well thought-out to me.
Thoughts on the questions:
SubscriptionMessageHelper doesn't seem like a uniquely good name. I would go ahead and implement the new ones with a different name, and deprecate the old versions, since that will let people play with the new system without breaking client libraries.
- Since we currently don't have any need for it, I'd push that off. Anyone who really needs it can write a custom serializer.
- default or standard serializer?
- For use in realtime that would definitely be nice to be able to do. Are boost shared pointers realtime safe?
- [Josh]: The reference count is done atomically, so copying them is realtime safe. Their creation can be made realtime safe as well, since they support allocators
- No opinion
Like Eric, I had questions along the way, but they were pretty much all answered. Some that weren't:
Is there value in implementing the << and >> operators for the IStream and OStream classes?
- [Josh]: Since it's trivial to do, I see no reason not to
How is LStream used?
- [Josh]: LStream really only exists because of the allinone serializer
I assume that the _foo_type typedefs (e.g., _frame_id_type) are intended to allow introspection; have you tried using them, to make sure you have everything you'll need?
- [Josh]: These have been around for a while (were requested to make actionlib require fewer template arguments)
On the questions:
- Agreed with Eric, if it's doable.
- Agreed with Eric.
twoway, symmetric, bidirectional?
If I template on an allocator type, I'd expect it to be used when I call or new or delete.
The implemented solution looks good to me, but the use of event isn't obvious. What about adding a connection header turns a message into an event?
I'd like to change the Allocator template name to ContainerAllocator, and not deal with overriding new/delete or providing an allocation function. STL allocators are really meant for containers and it's not possible to correctly implement delete using them.
- I'd prefer to use a concrete type as the allocator template parameter, and use rebind to switch to other types
To be filled out by proposer based on comments gathered during API review period
Package status change mark change manifest)
Action items that need to be taken.
Major issues that need to be resolved
On the question of SubscriptionMessageHelper: rename it SubscriptionCallbackHelper, and fix users of it with conditional compilation. Don't support backward compatibility in roscpp, because it's not worth it.
allinone -> allInOne
Stream operators will be defined for !IStream and !OStream
Hide details of !LStream and make serializedLength() return a uint32
Leave MessageEvent as is; include message receipt time to metadata
Go ahead with concrete Allocator type, and use rebind to switch between types