Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

protoboard-rcll: Translate between RCLL ProtoBuf messages and the blackboard #328

Merged
merged 20 commits into from
Dec 11, 2019
Merged

protoboard-rcll: Translate between RCLL ProtoBuf messages and the blackboard #328

merged 20 commits into from
Dec 11, 2019

Conversation

vmatare
Copy link
Contributor

@vmatare vmatare commented Oct 16, 2019

This an instantiation of the protoboard plugin template for the RCLL. Merge should wait until the core PR is finished.

@vmatare
protoboard: Templated plugin to convert protobuf to blackboard
0c5fae4 
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.
@vmatare
protoboard: add dummy handler for RobotInfo & VersionInfo
d862d88
@vmatare
protoboard: auto. set time on BeaconSignal
04e57b1
@vmatare
protoboard: catch & log exceptions in handlers
cf07e25
@vmatare
protoboard: add pb_nesting_converter
583dfea 
Required if a repeated protobuf field is mapped to multiple blackboard
interfaces.
@vmatare
protoboard: add quantity fields to OrderInterface
55ce29c
@vmatare
protoboard: fix pb->bb conversion for repeated fields
f48ca2d 
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).
@vmatare
protoboard: implement matching for repeated fields
d93f1e5 
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.
@vmatare
protoboard: default operators for converter
e956e3c
@vmatare
protoboard-rcll: factor out domain definition
dddf79f 
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.
@vmatare
protoboard-rcll: allow clang-format
afc0106 
Well, the result isn't exactly beautiful, but it's acceptable for the
sake of consistency.
@vmatare
protoboard-rcll: add interfaces for RCLL
5786569
@vmatare
protoboard-rcll: simplify iface type -> id mapping
5527f34
@vmatare
skills: add create_peer skill
d80e164 
creates a ProtoBuf peer using protoboard-rcll
@vmatare
protoboard-rcll: adapt to API changes in core
795c7cd
@vmatare
protoboard-rcll: handle MachineInfo & RingInfo
6a86879
@vmatare
protoboard-rcll: handle RS & DS prepare
c39bfc9
@vmatare
protobuf-rcll: add doxygen documentation
36c7829
@vmatare vmatare mentioned this pull request Oct 16, 2019
Merged
@vmatare vmatare requested a review from a team December 5, 2019 17:47
morxa
morxa reviewed Dec 9, 2019
View reviewed changes
Copy link
Member

@morxa morxa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, the only thing I do not like is that we need to guess the ID of the created peer. Can't we use a different identifier that's passed into the skill, e.g., a name? This would also require some changes in core.

src/plugins/protoboard-rcll/rcll_receiving.cpp
using namespace std;

std::unordered_map<std::string, std::shared_ptr<pb_convert>>
make_receiving_interfaces_map()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it can and should be improved, but that's a core issue which can't be addressed in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There you go: fawkesrobotics/fawkes#187

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vmatare
Copy link
Contributor Author

vmatare commented Dec 9, 2019

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.

@carologistics carologistics deleted a comment from todo bot Dec 9, 2019
@carologistics carologistics deleted a comment from todo bot Dec 9, 2019
@carologistics carologistics deleted a comment from todo bot Dec 9, 2019
@vmatare
Copy link
Contributor Author

vmatare commented Dec 9, 2019 •
edited
Loading

Issue with the peer IDs: fawkesrobotics/fawkes#188

@vmatare vmatare requested a review from morxa December 11, 2019 15:00
morxa
morxa approved these changes Dec 11, 2019
View reviewed changes
Copy link
Member

@morxa morxa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All remaining remarks are tracked in separate issues (fawkesrobotics/fawkes#187 and fawkesrobotics/fawkes#188), so this looks good to me!

@morxa morxa merged commit f423af3 into master Dec 11, 2019
@morxa morxa deleted the vmatare/protoboard-rcll branch December 11, 2019 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morxa morxa morxa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vmatare @morxa