Skip to content
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

subsys/mgmt: Add UDP transport for SMP #20983

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

aunsbjerg
Copy link
Collaborator

This makes it possible to perform mcumgr operation like DFU over ethernet instead of bluetooth or serial. The mcumgr tool already has support for UDP transport.

@carlescufi
Copy link
Member

@aunsbjerg thanks for the PR!
I will let @de-nordic and @nvlsianpu decide over this, but would it make sense to target the upstream mcumgr repository for (parts of) this PR?

@zephyrbot zephyrbot added area: API Changes to public APIs area: Samples Samples labels Nov 25, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Nov 25, 2019

All checks are passing now.

checkpatch (informational only, not a failure)

-:1141: WARNING:LINE_SPACING: Missing a blank line after declarations
#1141: FILE: subsys/mgmt/smp_udp.c:31:
+	struct k_thread thread;
+	K_THREAD_STACK_MEMBER(stack, CONFIG_MCUMGR_SMP_UDP_STACK_SIZE);

- total: 0 errors, 1 warnings, 1203 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@aunsbjerg
Copy link
Collaborator Author

@carlescufi I don't think it's necessary to add anything to upstream mcumgr, as we're basically just adding a transport that's already supported by the mcumgr-cli. I might be wrong though.

@de-nordic
Copy link
Collaborator

@aunsbjerg thanks for the PR!
I will let @de-nordic and @nvlsianpu decide over this, but would it make sense to target the upstream mcumgr repository for (parts of) this PR?

The PR only adds transport, similarly to smp_uart, smp_bt sources. The management part is still performed by mcumgr lib. I think that this is the the right place for the code.

@nvlsianpu
Copy link
Collaborator

I have been looking at other transport solution as well. This PR is in right place.

@nvlsianpu
Copy link
Collaborator

@aunsbjerg Can you become one of code-owner of files you added or modified?

samples/subsys/mgmt/mcumgr/smp_svr/prj_udp.conf Outdated Show resolved Hide resolved
samples/subsys/mgmt/mcumgr/smp_svr/src/main.c Outdated Show resolved Hide resolved
subsys/mgmt/Kconfig Outdated Show resolved Hide resolved
subsys/mgmt/Kconfig Show resolved Hide resolved
subsys/mgmt/Kconfig Outdated Show resolved Hide resolved
samples/subsys/mgmt/mcumgr/smp_svr/prj_udp.conf Outdated Show resolved Hide resolved
samples/subsys/mgmt/mcumgr/smp_svr/prj_udp.conf Outdated Show resolved Hide resolved
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for doc change :)

subsys/mgmt/smp_udp.c Outdated Show resolved Hide resolved
subsys/mgmt/smp_udp.c Outdated Show resolved Hide resolved
@aunsbjerg
Copy link
Collaborator Author

Thanks for the feedback. I will try and get the changes made during the weekend. I will also update the documentation for the sample and for the subsys in general where applicable.

@carlescufi
Copy link
Member

Thanks for the feedback. I will try and get the changes made during the weekend. I will also update the documentation for the sample and for the subsys in general where applicable.

Thanks @aunsbjerg for the PR and the feedback. I think you are right and nothing is required upstream, but I'd like @nvlsianpu and @de-nordic to approve.

@aunsbjerg
Copy link
Collaborator Author

Alright, finally got around to actually reworking this according to feedback.

Sample application is refactored. Common configuration stuff is put into prj.conf and transport specific is put into overlays. Code is separated into main, bt and udp specific stuff. I do not have any nordic boards so I have not been able to test that the bluetooth samples still work. Can someone verify this for me?

I have added myself as codeowner for the smp_udp.c in subsys and for the sample application. It would be nice to have a co-owner for the sample for the bluetooth stuff.

subsys/mgmt/Kconfig Show resolved Hide resolved
subsys/mgmt/smp_udp.c Outdated Show resolved Hide resolved
subsys/mgmt/smp_udp.c Outdated Show resolved Hide resolved
subsys/mgmt/smp_udp.c Outdated Show resolved Hide resolved
subsys/mgmt/smp_udp.c Show resolved Hide resolved
Mikkel Jakobsen added 3 commits April 16, 2020 10:02
Adds a UDP driver dedicated to transporting mcumgr SMP requests and
responses.

Signed-off-by: Mikkel Jakobsen <mikkel.aunsbjerg@prevas.dk>
To prepare this sample supporting future SMP transports, the sample
code is now split into a main and bluetooth file. A common config
has been identified and application specific config is put into
overlay config files.

Signed-off-by: Mikkel Jakobsen <mikkel.aunsbjerg@prevas.dk>
This sample now supports SMP UDP transport.
Two config overlays have been added for ipv4 and ipv6, respectively.

The sample documentation has been completely revamped to be less
bluetooth focused and more general.

Signed-off-by: Mikkel Jakobsen <mikkel.aunsbjerg@prevas.dk>
@aunsbjerg
Copy link
Collaborator Author

Updated commits according to feedback from @jukkar
Notably, both ipv4 and ipv6 support can be enabled at the same time or independently of each other.
UDP ipv4 and ipv6 samples have been merged into a single sample.

Enable SMP UDP using IPv4 addressing.
Can be enabled alongside IPv6 addressing.

config MCUMGR_SMP_UDP_IPV6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this extra level of configuration? What I mean is that now it is possible to have the device to have IPv6 enabled, but SMP UDP to not support it. Perhaps this is ok, it really depends on the use cases.
For example in networking libs and apps, it is usually enough to just enable IPv6 which then enables IPv6 usage in corresponding lib/app. So usually it is not needed to turn off IPv6 in app if the system globally supports IPv6.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary, but I figured one might want to disable ipv4/ipv6 for SMP independently of the network stack configuration. I can remove these config items and use the network config values instead if that's preferable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more or less up to you and the use cases. If you have a good use case for it, that is fine. It was just not obvious by looking the patch so wanted to clarify it.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jukkar jukkar merged commit a518516 into zephyrproject-rtos:master Apr 17, 2020
@aunsbjerg aunsbjerg deleted the mcumgr-udp branch April 17, 2020 07:36
CONFIG_FILE_SYSTEM_LITTLEFS=y

# Enable file system commands
CONFIG_MCUMGR_CMD_FS_MGMT=n
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONFIG_MCUMGR_CMD_FS_MGMT=y

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #24444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants