protoboard-rcll: Translate between RCLL ProtoBuf messages and the blackboard #328
Conversation
This plugin still resides in the domain repository, but all domain-independent code is separated. Domain-specific protobuf-blackboard mappings are specified as template specializations. So in effect, this is not a plugin, but a plugin template wich can be moved to the core repo after the source layout has been refactored a bit.
Required if a repeated protobuf field is mapped to multiple blackboard interfaces.
So far, only a direct mapping from one or more incoming protobuf messages to one or more blackboard interfaces could be specified. This commit adds the ability to extract a repeated field from a message and map each element to a specific blackboard interface. Blackboard interfaces are opened dynamically for each repeated field, and closed again for each field that no longer exists (in relation to the previous message).
In the case of RCLL Orders, each repeated Order field in the OrderInfo message has a persisting identity specified by its order ID. Each order ID is either new, or it needs to be mapped to the specific blackboard interface holding that order. The appropriate blackboard interface should then only be changed if some of the other fields have changed. This commit changes the pb->bb conversion templates to require a method corresponds_to() that must return true if a certain pb message has the same identity as the respective blackboard interface.
protoboard-rcll is effectively a new plugin, but it's based on the protoboard plugin. Only this time, it contains ONLY the domain-specific template specializations & instantiations. All of the generic (i.e. domain-agnostic) functionality has been moved to a plugin template in the core repo. This compiles, but is COMPLETELY UNTESTED yet.
Well, the result isn't exactly beautiful, but it's acceptable for the sake of consistency.
creates a ProtoBuf peer using protoboard-rcll
src/plugins/protoboard-rcll/interfaces/MachineInfoInterface.xml
Outdated
Show resolved
Hide resolved
vmatare
force-pushed
the
vmatare/protoboard-rcll
branch
from
cc51152
to
9b41c1d
Compare
| using namespace std; | ||
|
|
||
| std::unordered_map<std::string, std::shared_ptr<pb_convert>> | ||
| make_receiving_interfaces_map() |
There was a problem hiding this comment.
It should be called by the core library. Bear in mind that this plugin is basically just a template instantiation that does not implement any control flow.
There was a problem hiding this comment.
Hm so in the core library you implicitly rely on the user to define this global function. Imo the function should be either defined as part of the template or as a method of some class.
There was a problem hiding this comment.
What do you mean by "implicitly"? It's explicitly declared, but not defined: https://github.com/fawkesrobotics/fawkes/blob/master/src/libs/protoboard/protobuf_to_bb.h#L42-L46
If a consumer plugin forgets to implement it, there will be a very explicit linker error.
There was a problem hiding this comment.
It's also not logically part of any existing class, and it doesn't depend on any template. The only way to make it more self-documenting would be to make it a pure virtual method, but that would complicate the entire use case since then the user would need to derive an entire class and register it somehow with the core library.
There was a problem hiding this comment.
This will probably also break if you need more than one plugin that defines that function. It's logically tied to the interface, it's not a generic function.
There was a problem hiding this comment.
Sure, having the linker error at runtime is not ideal, but it's still the same error message. And strictly speaking it's load-time, not runtime, so it's a usability hassle, but not a malfunction risk.
There was a problem hiding this comment.
I agree that it can and should be improved, but that's a core issue which can't be addressed in this PR.
There was a problem hiding this comment.
There you go: fawkesrobotics/fawkes#187
There was a problem hiding this comment.
Sorry, didn't get those two other comments in time. Sure, with multiple plugin instances it might become an issue. I can certainly fix it, but I can't put it at the top of my agenda right now, so it will delay this PR quite a bit I guess.
|
I agree with the peer ID problem, but should we address that in this PR? Sounds to me like it's worth a bit of discussion and two separate PRs. |
|
Issue with the peer IDs: fawkesrobotics/fawkes#188 |
vmatare
force-pushed
the
vmatare/protoboard-rcll
branch
from
d5561a4
to
c0a411f
Compare
There was a problem hiding this comment.
All remaining remarks are tracked in separate issues (fawkesrobotics/fawkes#187 and fawkesrobotics/fawkes#188), so this looks good to me!
This an instantiation of the
protoboardplugin template for the RCLL. Merge should wait until the core PR is finished.