Move mcumgr functionality as internal Zephyr service #36092

de-nordic · 2021-06-09T12:19:36Z

NOTE: Most of PRs can be squashed, are kept for easier re-editing of the PR contents.

The PR consists of commits:

affb8f9f66 mgmt/mcumgr: Add CONFIG_IMG_MGMT_FRUGAL_LIST
beba8296dc manifest: Remove mcumgr from manifest
fc2f666b29 mgmt/mcumgr/lib: Add Kconfig option MGMT_CBORATTR_FLOAT_SUPPORT
2581113690 mgmt/mcumgr/lib: Replace license block
053a701a84 mgmt/mcumgr/lib: Fix compliance
801ce9d859 mgmt/mcumgr/lib: Documentation fixes
35c4c220f2 mgmt/mcumgr/lib: Remove non-Zephyr code
49098c1d28 mgmt/mcumgr/lib: Remove log management commands
606d3aa mgmt/mcumgr/lib: Copy mcumgr library under mgmt subsystem

TODO:

Complete testing of MCUMGR.

~~Depends on: #39584~~

Resolves: #36017

github-actions · 2021-06-09T12:20:36Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
mcumgr	zephyrproject-rtos/mcumgr@`c854c85` (master)	N/A	N/A

Note: This message is automatically posted and updated by the Manifest GitHub Action.

carlescufi · 2021-06-09T16:00:24Z

@de-nordic thank you for the PR.
I assume that this is a first step, to later then start modifying the source accordingly in order to get rid of the shim layers that are no longer required now that the code is in-tree. I would also suggest not having a lib/ folder in the end, but rather the code directly in subsys/mgmt/mcumgr/ with folders for transports and other logically independent parts.

de-nordic · 2021-06-10T07:43:42Z

@de-nordic thank you for the PR.
I assume that this is a first step, to later then start modifying the source accordingly in order to get rid of the shim layers that are no longer required now that the code is in-tree. I would also suggest not having a lib/ folder in the end, but rather the code directly in subsys/mgmt/mcumgr/ with folders for transports and other logically independent parts.

Yes. This is first step. I want to bring code as it is, of course with filled in missing licenses, compliance and style fixes here and there, so that it would past tests.

Then plan is to change structure of subsys/mgmg/mcumgr' into something like: commands, protocol, transports` and move files around; then specialize and reorganize the code to work efficiently with Zephry and so on.

I want to keep the thing moving and I have decided to make changes in steps; as I have more priority task to complete and some of them collide, regarding the mcumgr changes, I need to structure work in a way that lets me keep the changes going in gradually while working on other items.

github-actions · 2021-08-17T00:28:37Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

rerickson1 · 2021-11-08T17:20:13Z

@ahedin25

subsys/mgmt/mcumgr/lib/README.md

rerickson1 · 2021-11-08T17:33:16Z

subsys/mgmt/mcumgr/lib/README-zephyr.md

@@ -11,5 +11,4 @@ are described below:
 | `CONFIG_MCUMGR` | Enable the mcumgr management library. | n |
 | `CONFIG_MCUMGR_CMD_FS_MGMT` | Enable mcumgr handlers for file management | n |
 | `CONFIG_MCUMGR_CMD_IMG_MGMT` | Enable mcumgr handlers for image management | n |
-| `CONFIG_MCUMGR_CMD_LOG_MGMT` | Enable mcumgr handlers for log management | n |


Need to squash this commit yet or change the commit description?
Leaving this commit may make the changes more readable.

Sorry, you need to be more verbose because I did not get that.

The Commit description for the commit containing this change says it will be squashed. You should squash this commit or remove that message in the commit description.
I would be in favor of leaving this commit to have better tracking of the changes.

Removed information on squashing from commit messages.

nordicjm · 2021-11-09T10:35:17Z

subsys/mgmt/mcumgr/lib/README-zephyr.md

+| `CONFIG_MCUMGR` | Enable the mcumgr management library. | n |
+| `CONFIG_MCUMGR_CMD_FS_MGMT` | Enable mcumgr handlers for file management | n |
+| `CONFIG_MCUMGR_CMD_IMG_MGMT` | Enable mcumgr handlers for image management | n |
+| `CONFIG_MCUMGR_CMD_OS_MGMT` | Enable mcumgr handlers for OS management | n |


Missing CONFIG_MCUMGR_GRP_ZEPHYR_BASIC and CONFIG_MCUMGR_CMD_STAT_MGMT

nordicjm · 2021-11-09T10:36:06Z

subsys/mgmt/mcumgr/lib/README.md

+
+To use mcumgr's image management support, your device must be running version
+1.1.0 or later of the [MCUboot boot
+loader](https://github.com/runtimeco/mcuboot).  The other mcumgr features do


Link is now https://github.com/mcu-tools/mcuboot

nordicjm · 2021-11-09T10:38:09Z

subsys/mgmt/mcumgr/lib/README.md

+* Command handlers:
+    * Image management (`img_mgmt`)
+    * File system management (`fs_mgmt`)
+    * Log management (`log_mgmt`)


Isn't log management empty and unimplemented?

nordicjm · 2021-11-09T10:43:04Z

subsys/mgmt/mcumgr/lib/README.md

+
+Developers welcome!
+
+* Our Slack channel: https://mynewt.slack.com/messages/C7Y3K0C2J


Can replace with zephyr discord link https://discord.com/invite/Ck7jw53nU2

nordicjm · 2021-11-09T10:45:37Z

subsys/mgmt/mcumgr/lib/cborattr/include/cborattr/cborattr.h

+struct cbor_out_attr_t {
+	const char *attribute;	  /** The attribute name (key). */
+	struct cbor_out_val_t val;  /** The attribute value. */
+	bool omit;				  /** Attribute ignored if true. */


Comment spacing here is a bit much

nordicjm · 2021-11-09T10:46:45Z

subsys/mgmt/mcumgr/lib/cborattr/src/cborattr.c

+		case CborAttrUnsignedIntegerType:
+			targetaddr = (char *)&cursor->addr.uinteger[offset];
+			break;
+#if FLOAT_SUPPORT


Would be good to use the zephyr float Kconfig name for this, or add a new Kconfig specifically for this

nordicjm · 2021-11-09T10:47:18Z

subsys/mgmt/mcumgr/lib/cborattr/src/cborattr.c

+			lptr = &arr->arr.uintegers.store[off];
+			err |= cbor_value_get_uint64(&elem, lptr);
+			break;
+#if FLOAT_SUPPORT


nordicjm · 2021-11-09T10:48:09Z

subsys/mgmt/mcumgr/lib/cborattr/test/src/test_cborattr.h

+TEST_CASE_DECL(test_cborattr_decode_string_array);
+TEST_CASE_DECL(test_cborattr_decode_object_array);
+TEST_CASE_DECL(test_cborattr_decode_unnamed_array);
+TEST_CASE_DECL(test_cborattr_decode_substring_key);


Would be good addition for a future commit to add support for testing float and half float here also

Yes we will have to do that.

nordicjm · 2021-11-09T10:51:36Z

subsys/mgmt/mcumgr/lib/cmd/img_mgmt/include/img_mgmt/img_mgmt.h

+#define IMG_MGMT_STATE_F_ACTIVE		0x04
+#define IMG_MGMT_STATE_F_PERMANENT	0x08
+
+#define IMG_MGMT_VER_MAX_STR_LEN	25  /* 255.255.65535.4294967295\0 */
+
+/*
+ * Swap Types for image management state machine
+ */
+#define IMG_MGMT_SWAP_TYPE_NONE		0
+#define IMG_MGMT_SWAP_TYPE_TEST		1
+#define IMG_MGMT_SWAP_TYPE_PERM		2
+#define IMG_MGMT_SWAP_TYPE_REVERT	3
+


Spacing seems different throughout file

nordicjm · 2021-11-09T10:58:07Z

subsys/mgmt/mcumgr/lib/mgmt/include/mgmt/mgmt.h

+#endif
+
+/* MTU for newtmgr responses */
+#define MGMT_MAX_MTU		1024


Would be good to have this as a Kconfig in future

Unfortunately right now this thing is not used anywhere.

nordicjm · 2021-11-09T11:00:33Z

subsys/mgmt/mcumgr/lib/protocol.md

+### Mcumgr/Newtmgr SMP Client Over Serial
+
+`mcumgr` or `newtmgr` can be used over TTY serial with the following parameters


Does newtmgr still work with zephyr?

Yes, mcumgr is just rename for newtmgr

The commit copies code of mcumgr, as a lib, under subsys/mgmt/mcumgr. Only source files relevant for Zephyr have been copied, and changes to source files have been limited only to path changes, where required. Samples have been left out as the only relevant sample, smp_svr, is already within Zephyr source tree. Signed-off-by: Dominik Ermel <[email protected]>

de-nordic · 2021-11-09T15:08:42Z

@lairdjm Can you ping me on discord so we could have brief talk on further plans for the mcumgr?

subsys/mgmt/mcumgr/lib/README.md

Not implemented. Signed-off-by: Dominik Ermel <[email protected]>

The commit removes MYNEWT sepcific code and code that has been conditionally compiled with "ifndef _ZEPHYR_". Signed-off-by: Dominik Ermel <[email protected]>

References to Mynewt have been removed and README for Zephyr has been merged directly to the README. Signed-off-by: Dominik Ermel <[email protected]>

Commit fixes compliance issues. Signed-off-by: Dominik Ermel <[email protected]>

The commit changes licensing to SPDX tags. Signed-off-by: Dominik Ermel <[email protected]>

The option enables float numbers processing within CBOR attributes. Signed-off-by: Dominik Ermel <[email protected]>

The commit removes mcumgr library, as module, from west.yml manifest, as the mcumgr library source is now kept within Zephyr source code tree. Signed-off-by: Dominik Ermel <[email protected]>

The commit adds Kconfig options that enables "frugal status list"; when enabled, the status list will be sent with zero, empty and false values omitted, slightly reducing number of bytes sent as response from device. Signed-off-by: Dominik Ermel <[email protected]>

Remove mcumgr library, as module, from manifest, as the mcumgr library source is now kept within Zephyr since v3.0, see: zephyrproject-rtos/zephyr#36092 issues: #105 Signed-off-by: Stephan Linz <[email protected]>

github-actions bot added the area: Documentation label Jun 9, 2021

de-nordic added area: mcumgr manifest-mcumgr and removed area: Documentation labels Jun 9, 2021

github-actions bot added manifest west labels Jun 9, 2021

mniestroj self-requested a review June 10, 2021 21:14

github-actions bot added the area: Documentation label Jun 11, 2021

de-nordic force-pushed the move-mcumgr-to-zephyr-as-lib branch 2 times, most recently from 838304c to ade2d28 Compare June 11, 2021 14:35

nvlsianpu self-requested a review June 15, 2021 06:05

de-nordic force-pushed the move-mcumgr-to-zephyr-as-lib branch 2 times, most recently from b9cc16e to 53d44d5 Compare June 17, 2021 11:49

github-actions bot added the Stale label Aug 17, 2021

de-nordic removed the Stale label Aug 17, 2021

de-nordic force-pushed the move-mcumgr-to-zephyr-as-lib branch 2 times, most recently from e808765 to 238e3af Compare August 20, 2021 11:22

de-nordic changed the title ~~RFC: Move mcugmr functionality as internal Zephyr service~~ RFC: Move mcumgr functionality as internal Zephyr service Oct 19, 2021

de-nordic force-pushed the move-mcumgr-to-zephyr-as-lib branch 2 times, most recently from 142e791 to 0db5184 Compare October 20, 2021 16:19

de-nordic marked this pull request as ready for review October 20, 2021 16:26

de-nordic requested a review from carlescufi as a code owner October 20, 2021 16:26

de-nordic force-pushed the move-mcumgr-to-zephyr-as-lib branch from ff197d3 to 8646913 Compare October 21, 2021 09:38

de-nordic changed the title ~~RFC: Move mcumgr functionality as internal Zephyr service~~ Move mcumgr functionality as internal Zephyr service Oct 21, 2021

rerickson1 requested review from nordicjm and rerickson1 October 22, 2021 13:51

rerickson1 requested changes Nov 8, 2021

View reviewed changes

nordicjm reviewed Nov 9, 2021

View reviewed changes

nordicjm approved these changes Nov 9, 2021

View reviewed changes

de-nordic force-pushed the move-mcumgr-to-zephyr-as-lib branch from cf0240b to 8fb0f8f Compare November 9, 2021 14:59

de-nordic requested review from rerickson1 and nordicjm November 9, 2021 15:07

rerickson1 reviewed Nov 9, 2021

View reviewed changes

subsys/mgmt/mcumgr/lib/README.md Outdated Show resolved Hide resolved

de-nordic force-pushed the move-mcumgr-to-zephyr-as-lib branch from 8fb0f8f to affb8f9 Compare November 9, 2021 15:37

de-nordic requested a review from rerickson1 November 9, 2021 15:41

rerickson1 requested changes Nov 9, 2021

View reviewed changes

subsys/mgmt/mcumgr/lib/README.md Outdated Show resolved Hide resolved

carlescufi requested a review from rerickson1 November 15, 2021 15:41

de-nordic added 8 commits November 15, 2021 15:46

mgmt/mcumgr/lib: Remove log management commands

a2614f4

Not implemented. Signed-off-by: Dominik Ermel <[email protected]>

mgmt/mcumgr/lib: Remove non-Zephyr code

ecb1471

The commit removes MYNEWT sepcific code and code that has been conditionally compiled with "ifndef _ZEPHYR_". Signed-off-by: Dominik Ermel <[email protected]>

mgmt/mcumgr/lib: Documentation fixes

138ab78

References to Mynewt have been removed and README for Zephyr has been merged directly to the README. Signed-off-by: Dominik Ermel <[email protected]>

mgmt/mcumgr/lib: Fix compliance

19391d3

Commit fixes compliance issues. Signed-off-by: Dominik Ermel <[email protected]>

mgmt/mcumgr/lib: Replace license block

a508746

The commit changes licensing to SPDX tags. Signed-off-by: Dominik Ermel <[email protected]>

mgmt/mcumgr/lib: Add Kconfig option MGMT_CBORATTR_FLOAT_SUPPORT

a263db9

The option enables float numbers processing within CBOR attributes. Signed-off-by: Dominik Ermel <[email protected]>

manifest: Remove mcumgr from manifest

55ffb8d

The commit removes mcumgr library, as module, from west.yml manifest, as the mcumgr library source is now kept within Zephyr source code tree. Signed-off-by: Dominik Ermel <[email protected]>

de-nordic force-pushed the move-mcumgr-to-zephyr-as-lib branch from affb8f9 to d8213cf Compare November 15, 2021 15:47

rerickson1 approved these changes Nov 15, 2021

View reviewed changes

nordicjm approved these changes Nov 15, 2021

View reviewed changes

carlescufi approved these changes Nov 16, 2021

View reviewed changes

carlescufi merged commit 1d17a4b into zephyrproject-rtos:main Nov 16, 2021

de-nordic deleted the move-mcumgr-to-zephyr-as-lib branch November 30, 2021 14:03

raiden00pl mentioned this pull request Sep 1, 2024

[FEATURE] MCUmgr support apache/nuttx-apps#2545

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move mcumgr functionality as internal Zephyr service #36092

Move mcumgr functionality as internal Zephyr service #36092

de-nordic commented Jun 9, 2021 •

edited

Loading

github-actions bot commented Jun 9, 2021 •

edited

Loading

carlescufi commented Jun 9, 2021

de-nordic commented Jun 10, 2021

github-actions bot commented Aug 17, 2021

rerickson1 commented Nov 8, 2021

rerickson1 Nov 8, 2021

de-nordic Nov 9, 2021

rerickson1 Nov 9, 2021

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021 •

edited

Loading

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021

nordicjm Nov 9, 2021

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021

de-nordic Nov 15, 2021

nordicjm Nov 9, 2021

de-nordic Nov 9, 2021

nordicjm Nov 9, 2021

de-nordic Nov 9, 2021

de-nordic commented Nov 9, 2021


		Developers welcome!

		* Our Slack channel: https://mynewt.slack.com/messages/C7Y3K0C2J

		### Mcumgr/Newtmgr SMP Client Over Serial

		`mcumgr` or `newtmgr` can be used over TTY serial with the following parameters

Move mcumgr functionality as internal Zephyr service #36092

Move mcumgr functionality as internal Zephyr service #36092

Conversation

de-nordic commented Jun 9, 2021 • edited Loading

github-actions bot commented Jun 9, 2021 • edited Loading

carlescufi commented Jun 9, 2021

de-nordic commented Jun 10, 2021

github-actions bot commented Aug 17, 2021

rerickson1 commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordicjm Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

de-nordic commented Nov 9, 2021

de-nordic commented Jun 9, 2021 •

edited

Loading

github-actions bot commented Jun 9, 2021 •

edited

Loading

nordicjm Nov 9, 2021 •

edited

Loading