Skip to content

RFC: Move mcumgr functionality as internal Zephyr service #36017

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

Closed
de-nordic opened this issue Jun 7, 2021 · 8 comments · Fixed by #36092
Closed

RFC: Move mcumgr functionality as internal Zephyr service #36017

de-nordic opened this issue Jun 7, 2021 · 8 comments · Fixed by #36092
Assignees
Labels
area: mcumgr RFC Request For Comments: want input from the community

Comments

@de-nordic
Copy link
Collaborator

de-nordic commented Jun 7, 2021

Origin

Zephyr port of mcumgr: https://github.com/zephyrproject-rtos/mcumgr

Purpose

Currently used fork is, snapshot updated, copy of https://github.com/apache/mynewt-mcumgr that relies on modified fork of TinyCBOR (https://github.com/zephyrproject-rtos/tinycbor) , taken from https://github.com/apache/mynewt-core/tree/master/encoding/tinycbor.

Mcumgr is missing some features, for example "image" number parsing that allows to select image which will be uploaded, has some logic that does not suit Zephyr hardcoded in common code, or written in a way that does not allow Zephyr to easily extend it's functionalities.

Ownership of mcumgr will allow Zephyr to provide required, or Zephyr specific, features in more system optimized and development process efficient way; additionally an implementation of client-side library may be considered that may benefit many projects, by making it easier to update one device from another.

Additionally to above the mcumgr uses "non-standard" (https://github.com/intel/tinycbor) version of TinyCBOR, which means that there are possible two, incompatible, sources of changes to TinyCBOR that may collide, and may force us to basically implement any changes by hand, making the Zephyr the third possible source of changes; although there have not been may updates lately to both forks, the risk is there.

Moving the mcumgr into the Zephyr code base could enable switch to the CDDL-gen (https://github.com/NordicSemiconductor/cddl-gen) CBOR API and codec generator, a move similar to done by the mcuboot (mcu-tools/mcuboot#997); the API of the project is quite similar to TinyCBOR, but it also allows to auto-generate coding-decoding functions for structures; the project is actively maintained.

Mode of integration

The mcumgr is currently a module of zephyr (modules/lib/mcumgr) and its integration would require moving it to zephyr/subsys/mgmt/mcumgr/.
Then the work on optimizing the code for Zephyr may start.
Afterwards a switch to CDDL and implementation of client side library may be performed.

Dependencies

Currently the mcumgr depends on TinyCBOR fork within Zephyr modules (https://github.com/zephyrproject-rtos/tinycbor),
but it is already there anyway.

License

Current license of mcumgr source code in modules/lib/mcumgr/, and in https://github.com/apache/mynewt-mcumgr, is http://www.apache.org/licenses/LICENSE-2.0

@de-nordic de-nordic added TSC Topics that need TSC discussion RFC Request For Comments: want input from the community labels Jun 7, 2021
@de-nordic de-nordic self-assigned this Jun 7, 2021
@de-nordic
Copy link
Collaborator Author

@galak
Copy link
Collaborator

galak commented Jun 16, 2021

+1 for this change.

@nashif nashif removed the TSC Topics that need TSC discussion label Jun 16, 2021
@nvlsianpu
Copy link
Collaborator

+1.

@nandojve
Copy link
Member

+1

@0Grit
Copy link

0Grit commented Jul 23, 2021

Has there been any thought of just forking mcumgr completely and maintaining it under zephyr? Such that it can still potentially be used by other projects outside of Zephyr and imported into Zephyr by west?

@kurddt
Copy link
Contributor

kurddt commented Sep 7, 2021

Is there any chances the changes you want to make (remove some hard-coded logic from common code, change to CDDL-gen, etc ) would be accepted upstream ?

I do see the advantage to move mcumgr to an internal Zephyr service but that also means than any features added to upstream mcumgr would be quite hard to merge back.

I'm also wondering about the compatibility with mcumgr-cli do you plan to have an internal fork of it as well ?

@de-nordic
Copy link
Collaborator Author

Is there any chances the changes you want to make (remove some hard-coded logic from common code, change to CDDL-gen, etc ) would be accepted upstream ?

I don't know. We may try to push them there, but lack of acceptance upstream will not block us here.

I do see the advantage to move mcumgr to an internal Zephyr service but that also means than any features added to upstream mcumgr would be quite hard to merge back.

Yes, they will have to be ported. Note that it may be easier for us to bring things here or reimplement as we will have full control.

I'm also wondering about the compatibility with mcumgr-cli do you plan to have an internal fork of it as well ?

No, rather not. The current feature set of mcumgr-cli will keep working; bringing new features, added to Zephyr, to the mcumgr-cli will depend on the person that implements them.
We will probably rewrite the mcumgr to Python, as it is more known and easier to maintain than go; it should also allow us higher level of protocol instrumentation and black-box testing of specific features. But the work has not yet been started, so for now we rely on mcumgr-cli.

@nordicjm
Copy link
Collaborator

Would also be good to get the float support define to actually use the zephyr float Kconfig options, right now it needs to be manually passed a define outside of any Kconfig options which is not very user friendly

@de-nordic de-nordic changed the title RFC: Move mcugmr functionality as internal Zephyr service RFC: Move mcumgr functionality as internal Zephyr service Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: mcumgr RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants