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

Disco SenML resolves multiple issues (2, 18, 22, 25) and more. #24

Open
x448 opened this issue Nov 24, 2019 · 8 comments
Open

Disco SenML resolves multiple issues (2, 18, 22, 25) and more. #24

x448 opened this issue Nov 24, 2019 · 8 comments

Comments

@x448
Copy link

x448 commented Nov 24, 2019

I created a fork of cisco/senml named Disco SenML that resolves several issues.

Please let me know if you'd like a pull request. I can create one excluding the README.md.

For @fluffy @dusanb94

  • CBOR representation uses integers for labels, so it no longer violates RFC 8428.

For @drasko (please post a size comparison using your project if you have time)

  • Compiled programs are each 4 MB smaller (senmlCat and senmlServer).
  • missing Base Value and Base Sum are added to the model.

For everyone else:

  • all unit tests pass after correcting existing CBOR test data and adding new CBOR test using example from RFC 8428.
  • added go.mod, go.sum and I intend to tag at least one release soon to make it easier.

I needed to remove MessagePack for reasons mentioned at x448/senml. It wasn't mentioned in RFC 8428, it added to bloat and attack surface, and it prevented replacing codec library.

@dborovcanin
Copy link
Contributor

@x448 I see that there is still a lot of commented code in senml.go. Are you going to clean this up, as well?

@x448
Copy link
Author

x448 commented Nov 27, 2019

@dusanb94 If cisco/senml wants a pull request, it'll exclude my README.md and any code I commented out.

The first Disco SenML release note mentions:

I didn't perform any code review or refactoring beyond the bare minimum changes required to resolve cisco/senml issues 2, 18 and 22. Code review and refactoring is recommended.

I tagged a 2nd release to bump fxamacker/cbor to v1.3.2 and remove code I commented out.

Other changes like fixing spelling errors, removing other commented out code, or refactoring were left out to limit the scope of the initial pull request to cisco/senml.

I converted the fork into a standalone project so people can also submit pull requests to x448/senml (Disco SenML).

@dborovcanin
Copy link
Contributor

dborovcanin commented Dec 4, 2019

@x448 Thanks for the response. Since we need only a small subset of the basic SenML features, we've decided to write our lib. It's a simple implementation of the basic features described in the corresponding RFC. You can find it here. Thanks a lot, @fluffy @x448!

@fluffy
Copy link
Member

fluffy commented Dec 11, 2019

Love a PR on that. All seem very useful.

@x448
Copy link
Author

x448 commented Dec 11, 2019

@fluffy Great! I'll submit a PR this week.

@fluffy
Copy link
Member

fluffy commented Dec 11, 2019

Would it be possible to make the MessagePack stuff not included by default but still to include at compile time using an -X flag or something like that. A few people use it but as you point out, it is not part of the spec and adds bloat

@x448
Copy link
Author

x448 commented Dec 11, 2019

If I make the PR based on Disco v0.1 instead of v0.2, then the MessagePack stuff will be commented out rather than deleted. That should make it more convenient for someone to tackle a separate PR to add MessagePack if a user opens a ticket to add it back.

The current CBOR rep fails to comply with RFC 8428, so there might be problems with current MessagePack rep as well. A separate ticket to add back MessagePack could address this and other issues.

Should I submit a PR based on Disco v0.1 or v0.2? They both include an extra unit test using CBOR example data from RFC 8428, which could be useful when adding MessagePack.

@fluffy
Copy link
Member

fluffy commented Dec 12, 2019

I don't really know all the differences but give some people are still using MessagePack, I'd prefer to have a way to keep that in as a compile time option of some sort. We should also add a Contributors file. I really don't know enough about the differences of 0.1 or 0.2 to know which to do.

x448 added a commit to x448/senml that referenced this issue Dec 12, 2019
This project passes all unit tests and adds an extra unit test using CBOR example data from SenML RFC 8428.  Bump version of cisco/senml in README.md to Dec 11, 2019 commit.   See:
* cisco/senml#24
* cisco/senml#25
@x448 x448 changed the title Disco SenML resolves multiple issues (2, 18, 22) and more. Disco SenML resolves multiple issues (2, 18, 22, 25) and more. Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants