Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
dpgeorge merged 2 commits into micropython:master from iabdalkader:add_kpn_senml
Feb 28, 2023

Conversation

@iabdalkader
Copy link
Contributor

@iabdalkader iabdalkader commented Sep 27, 2022

No description provided.

Copy link
Contributor Author

iabdalkader commented Oct 17, 2022
edited
Loading

@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 ?

Copy link
Member

dpgeorge commented Nov 8, 2022

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.)

Copy link
Contributor Author

iabdalkader commented Nov 8, 2022
edited
Loading

, 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

Copy link
Member

dpgeorge commented Nov 9, 2022

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.

Copy link
Contributor Author

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.

@iabdalkader iabdalkader force-pushed the add_kpn_senml branch 2 times, most recently from 3bc265c to fa9837e Compare November 29, 2022 12:06
Copy link
Member

dpgeorge commented Dec 7, 2022
edited
Loading

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

Copy link
Member

jimmo commented Dec 8, 2022

Agree to both -- cbor2 and rename to senml, remove all kpn references.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Hi, what other changes are needed here to move forward with this ?

Copy link
Contributor Author

@dpgeorge @jimmo Hi! We need this hosted somewhere so we can link to it and update the imports (kpn_senml to senml etc..) before publishing our library which needs to be out very soon, what's blocking this PR ?

Copy link
Member

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).

Copy link
Contributor Author

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 ?

@iabdalkader iabdalkader force-pushed the add_kpn_senml branch 3 times, most recently from 96e54bf to 864c95b Compare February 27, 2023 17:37
Copy link
Member

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 ?

Yes they need to be nested, because it's a package. So you have it correct.

Copy link
Member

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.

@iabdalkader iabdalkader force-pushed the add_kpn_senml branch 2 times, most recently from ca31744 to a57282a Compare February 28, 2023 09:02
Copy link
Contributor Author

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.
@dpgeorge dpgeorge merged commit 9ee0257 into micropython:master Feb 28, 2023
Copy link
Member

Merged!!

iabdalkader reacted with hooray emoji

@iabdalkader iabdalkader deleted the add_kpn_senml branch February 28, 2023 14:26
Copy link
Contributor Author

@dpgeorge Those libraries run on CPython without any changes, except for removing all of the "u" prefix (CC @jimmo ) Can I send another PR to remove it ? It would be nice to reuse the same libraries on CPython.

Copy link
Member

dpgeorge commented Mar 1, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /