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

cisco / senml

Tool to convert senml between formats and act as gateway server to other services

BSD 2-Clause "Simplified" License

39 stars 15 forks source link

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

Open x448 opened 4 years ago

x448 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

Love a PR on that. All seem very useful.

x448 commented 4 years ago

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

fluffy commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.