Feature/ros publishers #103
Conversation
|
|
||
| // This class handles the callback creation for the different piksi message | ||
| // types. | ||
| class Callback { |
There was a problem hiding this comment.
Hm there's a few problems with this class. Callback is quite a non-ideal name, as it is not immediately clear what it does (breaks principle of least surprise).
Also I think it is a bit overkill to create a inherited Callback type for each different message. I would rather group them into one class, e.g.having a PositionCallbackHandler that contains all callbacks and publishers to output positions (I think there are quite a few of this). Then maybe a DebugCallbackHandler that contains all callbacks with system states, satelite count etc. and so on.
And then depending on the receiver type, it will contain more or less of these grouped handlers. I think that would make it a bit more readable
There was a problem hiding this comment.
I will address this issue in a separate PR as discussed. We will keep the sbp callback handler base class but
a) Try to allow multiple callbacks per message such that individual callback handlers can be defined for the same sbp callback
b) Create inherited superclasses that summarize behavior of similar callbacks, e.g., position callbacks.
c) Template the callback handlers such that they can automatically process
d) Create a RelayCallback class that handles the simple case of relaying a message and has a 1to1 mapping between sbp_msg_type and callback handler.
| reference receiver and broadcasting the moving baseline (also referred to as | ||
| heading). */ | ||
| enum Type { | ||
| kBaseStationReceiver = 0, |
There was a problem hiding this comment.
Here there is also quite some ambiguity - we have an enum for this, and a class hierarchy. I would do away with the enum and use the class hierarchy only. Any other class using these types doesn't need to know which it is, otherwise it breaks the some separation-of-concern principles
| const Type type) { | ||
| switch (type) { | ||
| case Type::kBaseStationReceiver: | ||
| return std::shared_ptr<Receiver>(new ReceiverBaseStation(nh, device)); |
There was a problem hiding this comment.
Hm, this creates a sort of circular dependency, as Receiver needs to know all its child types, and the child types need to know Receiver for inheritance
Towards cpp driver #102 .
Goal of this PR is to simplify creation of new callbacks and differentiate between GPS receivers.
This PR introduces
A receiver owns a hardware device and multiple sbp callbacks.
The ROS node now (auto) creates and processes all receivers.