-
Notifications
You must be signed in to change notification settings - Fork 1.1k
python-ecosys/kpn-senml: Add KPN SenML library. #541
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
Conversation
fbe7e6a to
261dbca
Compare
@jimmo If this is merged, and then require'd in a manifest.py, can I import kpn_senml or need to move it up a level ?
How does this relate to https://github.com/kpn-iot/senml-python-library and https://github.com/kpn-iot/senml-micropython-library, is it just a copy of the latter? If so, then I don't think adding it verbatim here is the right way to go. Instead one can just use the github URL to reference this library.
(All the examples have a boot.py and pymakr.conf which are really not necessary.)
, is it just a copy of the latter?
It's a copy of https://github.com/kpn-iot/senml-micropython-library with some patches on top (fixes for broken types, broken iterator etc..). Both libraries seem unmaintained and they don't respond to PRs/issues, and as of now we don't have a way to freeze remote modules in manifest.py. If that's implemented soon, we can fork that library instead and freeze it.
(All the examples have a boot.py and pymakr.conf which are really not necessary.)
I can remove whatever you don't want, I just didn't want to remove something that might be useful
Out of interest, what do you need this library for? Are there other libraries that could be used instead?
Both libraries seem unmaintained and they don't respond to PRs/issues, and as of now we don't have a way to freeze remote modules in
manifest.py. If that's implemented soon, we can fork that library instead and freeze it.
I think that's the best solution at this point in time: to fork that repo, apply your patches as new commits, then implement freezing of remote URLs in manifest.py.
Out of interest, what do you need this library for? Are there other libraries that could be used instead?
It's used by cloud clients to parse/create CBOR/JSON. And not aware of another library, but this one works really well.
I think that's the best solution at this point in time: to fork that repo, apply your patches as new commits, then implement freezing of remote URLs in manifest.py.
I don't mind that. My end goal is just to make devices capable of connecting to the cloud out of the box, and for that I need this library, #508 for time and improved logging in #507 and the cloud client library which won't be in micropython-lib anyway, but we don't have remote freezing yet. Any ETA on that @jimmo ?
I think #508 and #507 are generic enough to be in micropython-lib though.
3bc265c to
fa9837e
Compare
We need to consider this as two separate components: cbor and senml. And for both of them we need to consider whether the library is trying to be compatible with an existing library, or if it's a custom MicroPython library.
For cbor: there is an existing cbor library on PyPI but it looks abandoned. Instead there is cbor2 which had a commit 8 days ago (see https://github.com/agronholm/cbor2). It looks like the cbor code here (from KPN) is trying to be compatible with cbor2. So if we decide to keep that compatibility then our version should be python-ecosys/cbor2. And I think that's what we should do.
For senml: aside from kpn_senml, there doesn't seem to be any other maintained senml library. If we want to maintain compatibility with kpn_senml then our library should be python-ecosys/kpn_senml. Otherwise it should be micropython/senml and we are free to change the API. I would suggest the latter (call it micropython/senml) because kpn_senml has not been updated in a year, and the PyPI package has not been updated in about 4 years. Also I think we can remove the special KPN names, we don't need them and they take up space.
Edit: link to kpn_senml: https://github.com/kpn-iot/senml-python-library
Agree to both -- cbor2 and rename to senml, remove all kpn references.
Also I think we can remove the special KPN names, we don't need them and they take up space.
remove all kpn references
@dpgeorge @jimmo By remove them, you mean rename KPN_SENML_* to maybe SENML_NAME* (to match SENML_UNIT_* ? Because they're used in the examples, if I remove them will have to also remove all the examples that reference them.
fa9837e to
82eb971
Compare
Hi, what other changes are needed here to move forward with this ?
By remove them, you mean rename KPN_SENML_* to maybe SENML_NAME* (to match SENML_UNIT_* ? Because they're used in the examples, if I remove them will have to also remove all the examples that reference them.
I suggest to just remove all KPN_SENML_* names. The examples can then simply use strings with the appropriate string value. I don't think there's anything special about the string (the units on the other hand seem to be "official", so can stay as an enum).
82eb971 to
522b560
Compare
I suggest to just remove all KPN_SENML_* names. The examples can then simply use strings with the appropriate string value.
Removed the file and and replaced all references with strings, I also updated the license headers, how does it look now ?
I just have one concern, do we need the py files nested like cbor2/cbor2/decoder.py or can we move them up one level ?
96e54bf to
864c95b
Compare
I just have one concern, do we need the
pyfiles nested likecbor2/cbor2/decoder.pyor can we move them up one level ?
Yes they need to be nested, because it's a package. So you have it correct.
For cbor2/__init__.py, I think it should contain imports of the decoder and encoder files, so it matches how other packages (eg senml) work.
ca31744 to
a57282a
Compare
For cbor2/init.py, I think it should contain imports of the decoder and encoder files, so it matches how other packages (eg senml) work.
Done.
This aims to follow the API of the cbor2 library found at https://github.com/agronholm/cbor2 (also on PyPI as cbor2). The original source for this MicroPython version of cbor2 is from https://github.com/kpn-iot/senml-micropython-library.
This is a new library that doesn't follow any existing API. The library is originally from https://github.com/kpn-iot/senml-micropython-library.
a57282a to
9ee0257
Compare
Merged!!
Can I send another PR to remove it ? It would be nice to reuse the same libraries on CPython.
Yes! Please do remove the u-prefixes on the imports.
No description provided.