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.

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

Rewrite the I/O logic and define a protocol #89

Merged
merged 45 commits into from Jan 18, 2016

Conversation

Snaipe
Copy link
Owner

@Snaipe Snaipe commented Dec 9, 2015

This is mostly here to discuss and test anything related to the new protocol definition and the rewrite of the I/O logic.

The scope of this PR is to implement a client/server model, to start implementing the feature request #69.

The current technologies under test are:

  • Protocol definition using the protobuf DSL.
  • Protobuf implementation provided by nanopb.
  • I/O abstraction provided by nanomsg.

nanopb should be more adequate than libprotobuf for serialization, as there seems to be a lot of effort done to make in run on embedded systems.

@Snaipe
Copy link
Owner Author

Snaipe commented Dec 9, 2015

The current protocol definition can be found here

Breaking changes to this protocol will be possible until this rewrite hits a full release.

@codecov-io
Copy link

Current coverage is 90.01%

Merging #89 into bleeding will not affect coverage as of 4f43b0b

@@            bleeding     #89   diff @@
========================================
  Files             32      32       
  Stmts           1713    1713       
  Branches           0       0       
  Methods            0       0       
========================================
  Hit             1542    1542       
  Partial            0       0       
  Missed           171     171       

Review entire Coverage Diff as of 4f43b0b

Powered by Codecov. Updated on successful CI builds.

@ckaran
Copy link

ckaran commented Dec 9, 2015

Looks good! Although if you're going with protocol buffers, do you need the version field? Protocol buffers support a method of updating messages: https://developers.google.com/protocol-buffers/docs/proto3#updating. That said, I don't know what nanopb supports in terms of updated messages, so it may still be necessary.

@Snaipe
Copy link
Owner Author

Snaipe commented Dec 10, 2015

The version field is mostly here to allow updated versions of the protocol that would be otherwise breaking backward compatibility -- although it might never be used at all, its just a safeguard. Message updating will still be used within a protocol version.

@ckaran
Copy link

ckaran commented Dec 10, 2015

OK, but isn't that the point of protocol buffers? So that you can detect if you have backwards-incompatible changes and deal with them in a sane manner? Or maybe I'm wrong... Eh, you're probably right, even if protocol buffers are able to deal with it, as human beings we like to see what version we're dealing with. I withdraw the comment! :-)

@Snaipe
Copy link
Owner Author

Snaipe commented Dec 10, 2015

If you have backward-incompatible changes with protobuf, you're kinda screwed because the implementation deserialize the message as the new protocol regardless. This is why they tell you to never do that by never reusing field numbers (or making them reserved if you remove some), and not adding new required fields.

@Snaipe Snaipe force-pushed the bleeding branch 9 times, most recently from 2e07ebe to 55187c7 Compare January 16, 2016 20:31
@Snaipe
Copy link
Owner Author

Snaipe commented Jan 18, 2016

I'm going to merge this into bleeding -- the only bugs left come from the WIP on fork support for nanomsg and will be fixed in time. However, as there is a maintainer change undergoing over nanomsg and hence slower processing of PRs and reviews, and because the bleeding branch is slowly diverging from the changes introduced by the rewrite, it becomes necessary to merge the changes and iterate further on the new bleeding branch.

@Snaipe Snaipe merged commit 0019864 into bleeding Jan 18, 2016
@am11
Copy link
Contributor

am11 commented Jan 18, 2016

👍

@ckaran
Copy link

ckaran commented Feb 2, 2016

I'm coming to the party late, but 👍 :)

@Snaipe Snaipe deleted the features/io-rewrite-nanopb branch January 3, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants