MCUmgr and SMP Client support #56934

jheiskan81 · 2023-04-17T10:07:22Z

MCUmgr client

MCUmgr client basic implementation for support Image and OS group commands.

Image Group:
* Image state read/write
* Image Upload secondary slot
* Image Erase secondary slot

OS group:
* Reset
* Echo service, disabled by default

Operation's are blocked call and can't call inside worker queue.
IMG and OS needs SMP client object for transport.

SMP update

SMP client:

SMP buffer allocation
SMP object init for selected SMP transport
SMP client support for generate request and handling response message.

SMP transport:

Updated SMP transport for trigger client packets.
API for register SMP transport object.

Unit test

Unit test for MCUmgr & SMP client.

jheiskan81 · 2023-04-17T10:10:28Z

@SeppoTakalo Please review

include/zephyr/mgmt/mcumgr/smp/smp.h

include/zephyr/mgmt/mcumgr/smp/smp_client.h

include/zephyr/mgmt/mcumgr/transport/smp.h

SeppoTakalo

Left few remarks

include/zephyr/mgmt/mcumgr/smp/mcumgr_client.h

include/zephyr/mgmt/mcumgr/transport/smp.h

SeppoTakalo · 2023-04-17T13:05:50Z

Now MCUMGR client and its OS/Image operations are bundled under one module.

I wonder if it makes sense to have the generic McuMGR as a single file, then optionally enable OS, Image or some other functionality.

jheiskan81 · 2023-04-17T13:48:17Z

Now MCUMGR client and its OS/Image operations are bundled under one module.

I wonder if it makes sense to have the generic McuMGR as a single file, then optionally enable OS, Image or some other functionality.

OS Reset and Echo are used now. RESET is mandatory. I can replace echo command from Recovery mode by Read Image list command.

include/zephyr/mgmt/mcumgr/smp/smp.h

subsys/mgmt/mcumgr/grp/img_mgmt/src/img_mgmt_state.c

subsys/mgmt/mcumgr/smp/src/mcumgr_client.c

jheiskan81 · 2023-04-19T11:55:20Z

@SeppoTakalo and @nordicjm I removed Canonical fix commit. I have updated SMP and MCUMGR client commits missing headers doxygen comment's.

include/zephyr/mgmt/mcumgr/smp/mcumgr_client.h

subsys/mgmt/mcumgr/smp/src/mcumgr_client.c

include/zephyr/mgmt/mcumgr/smp/mcumgr_client.h

subsys/mgmt/mcumgr/smp/src/client.c

subsys/mgmt/mcumgr/smp/src/mcumgr_client.c

subsys/mgmt/mcumgr/smp/src/smp.c

subsys/mgmt/mcumgr/smp/src/mcumgr_client.c

jheiskan81 · 2023-04-20T11:57:34Z

I have been fixed now raised item's:
SMP:

SMP CLIENT retry logic is updated now more dynamic and retry schedule is using exact timestamp to next retry send
SMP client base retry is 500ms and send operation give a total timeout now in seconds

MCUmgr client:

Updated Image group state read response IMAGE list parser.
Hash and Version size used already defined definition from image group header
Removed rsn unused cased
Removed magic number usages and optimize cbor state count's

Thanks for good feedback @nordicjm and @de-nordic.

jheiskan81 · 2023-04-20T12:50:03Z

MCUmgr client file can be splitted to own seprated groups file but I want to review main functionality first.

jheiskan81 · 2023-04-21T09:14:08Z

rebased with upstream

jheiskan81 · 2023-07-21T07:21:43Z

Thanks @nordicjm for your comment's I have fix those and rebased PR with latest upstream.

nordicjm

Isssues found in test:

Client must not respond to packets with error responses, it needs to ignore corrupt packets and handle them i.e. with retransmissions
Client is not removing the data from the packet and it then tries to look at an invalid packet
Upload does not set the image status to 0, therefore it always fails because it always has the default value of EINVAL

nordicjm · 2023-07-25T08:29:54Z

Also logging does not work, instead of having:

#define LOG_MODULE_NAME smp_client LOG_MODULE_REGISTER(smp_client);

Use:

LOG_MODULE_REGISTER(mcumgr_client, <log_level_here_from_Kconfig>);

jheiskan81 · 2023-07-26T11:55:43Z

@nordicjm Thanks for testing and reporting issues. I have fixed issues and also fix logging.

jheiskan81 · 2023-07-26T14:27:09Z

Fixed Compliance checks

nordicjm

2 minor nits then ready for approval, thanks!

nordicjm · 2023-07-27T07:17:43Z

subsys/mgmt/mcumgr/grp/os_mgmt_client/Kconfig

No indenting in Kconfig files for if... blocks

nordicjm · 2023-07-27T07:19:52Z

subsys/mgmt/mcumgr/smp_client/src/client.c

Would flip this to error out if there is not at least sizeof(smp_header) to send, as that would not be valid, then the part below can be part of the main flow as it will only send packets of a minimum valid length

SMP client support for generate request and handling response message. Updated SMP transport for send request. Added API for register SMP transport. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

MCUmgr client basic implementation for support Image and OS grpup commands. Image Group: * Image state read/write * Image Upload secondary slot * Image Erase secondary slot OS group: * Reset * Echo service, disabled by default Opeartion's are blocked call and cant't call inside worker queue. IMG and OS need to be SMP client object for transport. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

Added unit test for testing IMG and OS group component's. Added Unit test for SMP Client. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

jheiskan81 · 2023-07-27T07:36:46Z

@nordicjm Thanks for review. I fixed those raised case.

de-nordic

Looks OK, thanks!

de-nordic · 2023-07-27T11:25:03Z

subsys/mgmt/mcumgr/transport/src/smp.c

+void smp_client_transport_register(struct smp_client_transport_entry *entry)
+{
+if (smp_client_transport_get(entry->smpt_type)) {
+/* Already in list */


We should probably have some warning here, as a LOG_DBG, as this would be strange behavior when certain transport type is registered twice; this also checks type only so it does not distinguish between different handler being registered for the same type.

I can add that no problem.

kartben · 2023-08-10T15:26:55Z

Is there a plan to add documentation and/or a sample for this? It would really help with making sure people are aware this now exists. Apologies if there is already an open issue or PR for this and I just missed it. @nordicjm @jheiskan81

jheiskan81 requested review from carlescufi, de-nordic and nordicjm as code owners April 17, 2023 10:07

zephyrbot added the area: mcumgr label Apr 17, 2023

zephyrbot assigned de-nordic Apr 17, 2023

jheiskan81 force-pushed the mcumgr_client branch 2 times, most recently from 0580e2d to acd829b Compare April 17, 2023 10:40

nordicjm requested changes Apr 17, 2023

View reviewed changes

include/zephyr/mgmt/mcumgr/smp/smp.h Outdated Show resolved Hide resolved

include/zephyr/mgmt/mcumgr/smp/smp_client.h Outdated Show resolved Hide resolved

include/zephyr/mgmt/mcumgr/transport/smp.h Outdated Show resolved Hide resolved

SeppoTakalo requested changes Apr 17, 2023

View reviewed changes

de-nordic reviewed Apr 17, 2023

View reviewed changes

jheiskan81 force-pushed the mcumgr_client branch from acd829b to f6ee3f5 Compare April 19, 2023 11:53

jheiskan81 force-pushed the mcumgr_client branch 4 times, most recently from 9c64380 to a547fcd Compare April 19, 2023 13:15

nordicjm reviewed Apr 19, 2023

View reviewed changes

subsys/mgmt/mcumgr/smp/src/client.c Outdated Show resolved Hide resolved

nordicjm reviewed Apr 19, 2023

View reviewed changes

subsys/mgmt/mcumgr/smp/src/mcumgr_client.c Outdated Show resolved Hide resolved

de-nordic reviewed Apr 19, 2023

View reviewed changes

jheiskan81 force-pushed the mcumgr_client branch 2 times, most recently from cc1f3fc to 66f6f93 Compare April 20, 2023 11:48

jheiskan81 changed the title ~~MCUMGR and SMP Client support~~ MCUmgr and SMP Client support Apr 20, 2023

jheiskan81 force-pushed the mcumgr_client branch from 66f6f93 to 830d1e7 Compare April 21, 2023 07:19

jheiskan81 force-pushed the mcumgr_client branch from 6c2dada to 26d7539 Compare July 21, 2023 07:19

nordicjm requested changes Jul 25, 2023

View reviewed changes

jheiskan81 force-pushed the mcumgr_client branch 4 times, most recently from f4ad187 to d6dcbad Compare July 26, 2023 09:03

jheiskan81 force-pushed the mcumgr_client branch 2 times, most recently from f84b6e8 to 31a28fa Compare July 26, 2023 14:08

nordicjm requested review from SeppoTakalo, aunsbjerg, de-nordic, jeppenodgaard and nordicjm July 26, 2023 14:09

jheiskan81 force-pushed the mcumgr_client branch 2 times, most recently from 5caffe2 to 7e3bf8e Compare July 26, 2023 14:21

nordicjm requested changes Jul 27, 2023

View reviewed changes

Juha Heiskanen added 3 commits July 27, 2023 10:35

mgmt: smp: SMP Client enabler

e8ab942

SMP client support for generate request and handling response message. Updated SMP transport for send request. Added API for register SMP transport. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

tests: mcumgr: MCUmgr and smp client unit test

2735fd5

Added unit test for testing IMG and OS group component's. Added Unit test for SMP Client. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

jheiskan81 force-pushed the mcumgr_client branch from 7e3bf8e to 2735fd5 Compare July 27, 2023 07:36

nordicjm approved these changes Jul 27, 2023

View reviewed changes

SeppoTakalo approved these changes Jul 27, 2023

View reviewed changes

de-nordic approved these changes Jul 27, 2023

View reviewed changes

carlescufi merged commit 9a06ce1 into zephyrproject-rtos:main Jul 27, 2023

MCUmgr and SMP Client support #56934

MCUmgr and SMP Client support #56934

Uh oh!

Conversation

jheiskan81 commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MCUmgr client

SMP update

Unit test

jheiskan81 commented Apr 17, 2023

Uh oh!

Uh oh!

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeppoTakalo commented Apr 17, 2023

jheiskan81 commented Apr 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jheiskan81 commented Apr 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jheiskan81 commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jheiskan81 commented Apr 20, 2023

jheiskan81 commented Apr 21, 2023

jheiskan81 commented Jul 21, 2023

nordicjm left a comment

Choose a reason for hiding this comment

nordicjm commented Jul 25, 2023

jheiskan81 commented Jul 26, 2023

jheiskan81 commented Jul 26, 2023

nordicjm left a comment

Choose a reason for hiding this comment

nordicjm Jul 27, 2023

Choose a reason for hiding this comment

nordicjm Jul 27, 2023

Choose a reason for hiding this comment

jheiskan81 commented Jul 27, 2023

de-nordic left a comment

Choose a reason for hiding this comment

de-nordic Jul 27, 2023

Choose a reason for hiding this comment

jheiskan81 Jul 27, 2023

Choose a reason for hiding this comment

kartben commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Labels

9 participants

jheiskan81 commented Apr 17, 2023 •

edited

Loading

jheiskan81 commented Apr 20, 2023 •

edited

Loading

kartben commented Aug 10, 2023 •

edited

Loading