doc/guides/device_mgmt: SMP protocol documentation #40894

de-nordic · 2021-12-03T16:52:11Z

The commit adds guide that describes format of SMP reqests/responses
as issued by Zephyr implementation within mcumgr.

Signed-off-by: Dominik Ermel [email protected]

de-nordic · 2021-12-03T16:53:36Z

@lairdjm @carlescufi FYI for preview.
Still in draft because I need to divide this into sections add more ref tagging and fix formatting.

simon-iversen

Adding myself as reviewer

doc/guides/device_mgmt/smp_protocol.rst

caspermeijn · 2021-12-06T10:32:03Z

doc/guides/device_mgmt/smp_protocol.rst

+Frame is encoded in "Big Endian" (Network endianess), where field is more than
+one byte lone, and takes the following form:


Suggested change

Frame is encoded in "Big Endian" (Network endianess), where field is more than

one byte lone, and takes the following form:

Frame is encoded in "Big Endian" (Network endianess) and takes the following form:

This sentence was incomplete. I think the explanation of endianess should be longer than a single line, if required at all.

So the sentence, with the exception of "lone" that should be "long" tried to clarify that you should not change bit order, only byte order for types longer than one byte.

doc/guides/device_mgmt/smp_protocol.rst

caspermeijn · 2021-12-06T13:44:07Z

doc/guides/device_mgmt/smp_protocol.rst

+    ```
+    {
+        (str)"argv"     : [
+            (str)<cmd>


Is it correct that the cmd in in the array?

The decoding in mcumgr says so: https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/mgmt/mcumgr/lib/cmd/shell_mgmt/src/shell_mgmt.c#L29-L54

de-nordic · 2021-12-06T14:38:04Z

@caspermeijn Thanks for starting the review. Mind that this is still draft, so depending on my further work and suggestions from reviewers it may change a lot.

nordicjm · 2021-12-09T08:32:48Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+    | "rc"                  | :ref:`mcumgr_smp_protocol_status_codes`           |
+    +-----------------------+---------------------------------------------------+
+    | "rsn"                 | :ref:`mcumgr_smp_protocol_status_codes`           |
+    +-----------------------+---------------------------------------------------+


Don't see the rsn entry here https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/mgmt/mcumgr/lib/cmd/os_mgmt/src/os_mgmt.c#L48 can only see rsn in the img mgmt module https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/mgmt/mcumgr/lib/cmd/img_mgmt/src/img_mgmt.c#L247 - is it planned to be expanded in a future commit?

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

nordicjm · 2021-12-09T08:40:10Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+        (str)"rc" : (int)
+        (str,opt)"rsn"  : (str)


rsn here also

nordicjm · 2021-12-09T09:06:27Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+==============
+
+The command lacks formal definition except for being assigned ID. It is not really
+know what the request should look like or what the response should contain.


nordicjm · 2021-12-09T09:06:39Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+
+The command lacks formal definition except for being assigned ID. It is not really
+know what the request should look like or what the response should contain.
+It is also not know whether this is ``write request`` or ``read reqest``


nordicjm · 2021-12-09T10:17:15Z

doc/guides/device_mgmt/smp_protocol.rst

+    +===============+===============================================+
+    | ``0``         | write request                                 |
+    +---------------+-----------------------------------------------+
+    | ``1``         | write response                                |
+    +---------------+-----------------------------------------------+
+    | ``2``         | read request                                  |
+    +---------------+-----------------------------------------------+
+    | ``3``         | read response                                 |
+    +---------------+-----------------------------------------------+


Wrong way around, read request is 0, read response is 1, write request is 2, write response is 3

doc/guides/device_mgmt/smp_protocol.rst

nordicjm · 2021-12-09T10:19:25Z

doc/guides/device_mgmt/smp_protocol.rst

+    | ``5``         | Designated for test crashing application      |
+    |               | (unused by Zephyr)                            |
+    +---------------+-----------------------------------------------+
+    | ``6``         | Designated for run tests                      |


6 is split image management and 7 is run https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/mgmt/mcumgr/lib/mgmt/include/mgmt/mgmt.h#L36

doc/guides/device_mgmt/smp_protocol.rst

nordicjm · 2021-12-09T10:21:10Z

doc/guides/device_mgmt/smp_protocol.rst

+    |               | when there is not enough memory to complete   |
+    |               | response.                                     |
+    +---------------+-----------------------------------------------+
+    | ``3``         | Invalid value; a request contains invalid     |


*contains an invalid

de-nordic · 2021-12-22T16:25:45Z

@caspermeijn @lairdjm @philips77 Somehow I can not re-request review for this PR, so I am mentioning you here to drag you back to review.

stale

nordicjm · 2021-12-23T10:21:10Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+    +========+==============+================+==================================+
+    | ``1``  | ``0``        |  ``0``         | When request ``OP`` was ``0``    |
+    +--------+--------------+----------------+----------------------------------+
+    | ``3``  | ``0``        |  ``0``         | When request ``OP`` was ``1``    |


nordicjm · 2021-12-23T10:23:50Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+                (str)"tid"          : (uint)
+                (str)"state"        : (uint)
+                (str)"stkuse"       : (uint)
+                (str)"stksize"      : (uint)


nordicjm · 2021-12-23T10:24:21Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+    +-----------------------+---------------------------------------------------+
+    | "stkuse"              | task's/thread's stack usage                       |
+    +-----------------------+---------------------------------------------------+
+    | "stksize"             | task's/thread's stack size                        |


nordicjm · 2021-12-23T10:25:16Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+    | "rc"                  | :ref:`mcumgr_smp_protocol_status_codes`           |
+    +-----------------------+---------------------------------------------------+
+
+Task statistics command


Worth adding the 'not known' text here about the terminal/echo command also?

nordicjm · 2021-12-23T10:30:09Z

doc/guides/device_mgmt/smp_groups/smp_group_1.rst

+            (str,opt)"len"      : (uint)
+            (str)"off"          : (uint)
+            (str,opt)"sha"      : (str)
+            (str,opt)"data"     : (str)


Re-querying this

nordicjm · 2021-12-23T10:40:03Z

doc/guides/device_mgmt/smp_groups/smp_group_8.rst

+
+Mcumgr server side re-opens a file for each subsequent request, and current
+specification does not provide means to identify subsequent requests as
+belonging to specified download session.  This means that the  file is not


nordicjm · 2021-12-23T10:40:08Z

doc/guides/device_mgmt/smp_groups/smp_group_8.rst

+    {
+        (str)"off"      : (uint)
+        (str)"data"     : (str)
+        (str)"rc"       : (int)0


nordicjm · 2021-12-23T10:40:16Z

doc/guides/device_mgmt/smp_groups/smp_group_8.rst

+*************
+
+Command allows to download contents of an existing file from specified path
+at a target device.  The command is stateless and mcumgr does not hold file


nordicjm · 2021-12-23T10:40:21Z

doc/guides/device_mgmt/smp_groups/smp_group_2.rst

+Statistics: group data request
+==============================
+
+Statistics group date request header


nordicjm · 2021-12-23T10:40:25Z

doc/guides/device_mgmt/smp_groups/smp_group_2.rst

+
+    +-----------------------+---------------------------------------------------+
+    | "stat_list"           | array of strings representing group names; this   |
+    |                       | array may be empty if there are not groups        |


doc/guides/device_mgmt/smp_protocol.rst

de-nordic · 2022-02-08T15:45:37Z

@carlescufi @caspermeijn @philips77 I have changed, in entire documentation, sentences:

CBOR data of request command:

to

CBOR data of request:

as the former was not really accurate.
I have also changed description of echo and reset commands where now the "rc" is not included in successful responses.

gmarull · 2022-02-08T16:45:01Z

doc/guides/device_mgmt/smp_groups/smp_group_0.rst

+.. code-block:: none
+    {


Suggested change

.. code-block:: none

{

.. code-block:: none

{

caspermeijn

I like how the tables are rendered. Overall, I think the text is clear and understandable. LGTM

caspermeijn · 2022-02-08T20:26:38Z

doc/guides/device_mgmt/smp_groups/smp_group_1.rst

+
+.. note::
+    Response from Zephyr running device may have "rc" value of 6, bad state,
+    (:ref:mcumgr_smp_protocol_status_codes) which means that the secondary


This reference is not rendered correctly

de-nordic · 2022-02-21T07:58:46Z

@dleach02 @caspermeijn @gmarull @carlescufi @philips77 The review is in some strange state: I have this "comment" icons on the right-side of reviewers nicks but there are not changes requested and the request review button works only for some reviewers.

nordicjm · 2022-02-21T08:23:52Z

doc/guides/device_mgmt/smp_protocol.rst

+    |               | an application specific management groups     |
+    +---------------+-----------------------------------------------+
+
+The payload for above groups, except for ``64`` which is not defined,


Is it not groups 64 and above? Would add a note here about that

The commit adds guide that describes format of SMP reqests/responses as issued by Zephyr implementation within mcumgr. Signed-off-by: Dominik Ermel <[email protected]>

nordicjm · 2022-03-02T08:05:14Z

doc/guides/device_mgmt/smp_groups/smp_group_2.rst

+        (str)"name"     : (str)
+        (str)"fields"   : {
+            (str)<entry_name> : (uint)
+            ...
+        }
+        (str)"rc"       : (int)


I was unaware of this restriction with zcbor/cddl-gen: NordicSemiconductor/zcbor#176
If the plan is to move to zcbor, should these entries show the current layout or the layout that things would go with? At present, the layout is rc before name

That only happens when you use decoder generated from CDDL. When you decode by hand it is up to you how the processing of CBOR is handled.
Anyway, this is comment for #43333 and/or #40591 rather than documentation for SMP.

github-actions bot added the area: Documentation label Dec 3, 2021

de-nordic requested review from carlescufi and nordicjm December 3, 2021 16:52

simon-iversen reviewed Dec 6, 2021

View reviewed changes

caspermeijn reviewed Dec 6, 2021

View reviewed changes

de-nordic force-pushed the mcumgr-smp-protocol-doc branch 10 times, most recently from 35f8eb4 to 2cbe3fe Compare December 8, 2021 14:33

nordicjm previously requested changes Dec 9, 2021

View reviewed changes

de-nordic force-pushed the mcumgr-smp-protocol-doc branch from 2cbe3fe to 4655339 Compare December 20, 2021 16:15

de-nordic requested a review from nordicjm December 20, 2021 16:15

de-nordic force-pushed the mcumgr-smp-protocol-doc branch 2 times, most recently from 9be2f9f to bfdf364 Compare December 20, 2021 16:42

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

de-nordic requested a review from nashif as a code owner December 20, 2021 16:42

de-nordic added the area: mcumgr label Dec 20, 2021

de-nordic self-assigned this Dec 20, 2021

de-nordic force-pushed the mcumgr-smp-protocol-doc branch from bfdf364 to ff7a165 Compare December 20, 2021 16:46

nordicjm reviewed Dec 23, 2021

View reviewed changes

gmarull reviewed Feb 8, 2022

View reviewed changes

doc/guides/device_mgmt/smp_protocol.rst Outdated Show resolved Hide resolved

de-nordic force-pushed the mcumgr-smp-protocol-doc branch 2 times, most recently from c08f1aa to 43540c3 Compare February 8, 2022 15:42

gmarull reviewed Feb 8, 2022

View reviewed changes

gmarull previously approved these changes Feb 8, 2022

View reviewed changes

de-nordic force-pushed the mcumgr-smp-protocol-doc branch from 43540c3 to 654ebdf Compare February 8, 2022 16:53

caspermeijn previously approved these changes Feb 8, 2022

View reviewed changes

de-nordic force-pushed the mcumgr-smp-protocol-doc branch 4 times, most recently from d69a538 to eca385a Compare February 9, 2022 14:22

dleach02 dismissed stale reviews from caspermeijn, gmarull, carlescufi, and philips77 via eca385a February 11, 2022 16:03

de-nordic requested review from carlescufi and gmarull February 21, 2022 07:54

nordicjm reviewed Feb 21, 2022

View reviewed changes

philips77 previously approved these changes Feb 21, 2022

View reviewed changes

doc/guides/device_mgmt: SMP protocol documentation

e1a229a

The commit adds guide that describes format of SMP reqests/responses as issued by Zephyr implementation within mcumgr. Signed-off-by: Dominik Ermel <[email protected]>

de-nordic dismissed philips77’s stale review via e1a229a February 28, 2022 14:27

de-nordic force-pushed the mcumgr-smp-protocol-doc branch from eca385a to e1a229a Compare February 28, 2022 14:27

de-nordic requested a review from nordicjm February 28, 2022 14:28

caspermeijn approved these changes Mar 1, 2022

View reviewed changes

nordicjm reviewed Mar 2, 2022

View reviewed changes

nordicjm approved these changes Mar 2, 2022

View reviewed changes

carlescufi approved these changes Mar 7, 2022

View reviewed changes

carlescufi merged commit 92109e1 into zephyrproject-rtos:main Mar 7, 2022

		Frame is encoded in "Big Endian" (Network endianess), where field is more than
		one byte lone, and takes the following form:

doc/guides/device_mgmt: SMP protocol documentation #40894

doc/guides/device_mgmt: SMP protocol documentation #40894

Conversation

de-nordic commented Dec 3, 2021

de-nordic commented Dec 3, 2021

simon-iversen left a 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 Dec 6, 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

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 Dec 22, 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

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 Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caspermeijn left a 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 Feb 21, 2022

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 Feb 8, 2022 •

edited

Loading