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
MCUmgr and SMP Client support #56934
Conversation
@SeppoTakalo Please review |
0580e2d
to
acd829b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left few remarks
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. |
acd829b
to
f6ee3f5
Compare
@SeppoTakalo and @nordicjm I removed Canonical fix commit. I have updated SMP and MCUMGR client commits missing headers doxygen comment's. |
9c64380
to
a547fcd
Compare
cc1f3fc
to
66f6f93
Compare
I have been fixed now raised item's:
MCUmgr client:
Thanks for good feedback @nordicjm and @de-nordic. |
MCUmgr client file can be splitted to own seprated groups file but I want to review main functionality first. |
66f6f93
to
830d1e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Also logging does not work, instead of having:
Use:
|
f4ad187
to
d6dcbad
Compare
@nordicjm Thanks for testing and reporting issues. I have fixed issues and also fix logging. |
f84b6e8
to
31a28fa
Compare
5caffe2
to
7e3bf8e
Compare
Fixed Compliance checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 minor nits then ready for approval, thanks!
if MCUMGR_GRP_OS_CLIENT | ||
|
||
config MCUMGR_GRP_OS_CLIENT_ECHO | ||
default y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No indenting in Kconfig files for if... blocks
} | ||
|
||
smp_read_hdr(nb, &smp_header); | ||
if (nb->len > sizeof(smp_header)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
7e3bf8e
to
2735fd5
Compare
@nordicjm Thanks for review. I fixed those raised case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK, thanks!
void smp_client_transport_register(struct smp_client_transport_entry *entry) | ||
{ | ||
if (smp_client_transport_get(entry->smpt_type)) { | ||
/* Already in list */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add that no problem.
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 |
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 transport:
Updated SMP transport for trigger client packets.
API for register SMP transport object.
Unit test
Unit test for MCUmgr & SMP client.