-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Introduce LWM2M subsystem for Zephyr #712
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
Conversation
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.
legacy code is sometime quite hard to read due to numerous if/for/while/else/... imbrication. I don't know if the plan is to backport future patches from legacy code to this port, if not maybe it will be nice to rearrange the code for better readability.
Could be done later though.
subsys/net/lib/zoap/zoap.c
Outdated
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.
move it one line down, below token
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.
Done
include/net/zoap.h
Outdated
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.
move it below token, to ensure only 1byte is lost due to struct alignment.
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.
Done. Looks a bit odd to split token / tkl though. :/
include/net/lwm2m.h
Outdated
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.
not sure if it's github's viewer fault, but could you indent and align parameters under last ( + 1 space?
Apply that everywhere.
include/net/lwm2m.h
Outdated
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.
why such space between #define and the actual term?
I guess it's legacy stuff from contiki, it's worth changing this however.
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.
This is a file I created, and I don't mind fixing spacing / readability issues like this. Done.
subsys/net/lib/lwm2m/Kconfig
Outdated
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 think you could stick with LWM2M without _LIB
Like HTTP does etc...
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.
Heh, I used the MQTT_LIB model. Removing the _LIB throughout.
subsys/net/lib/lwm2m/lwm2m_engine.c
Outdated
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.
having a lot of for/if/else imbrication makes things hard to read. Though here it's contiki legacy I guess, could be worth to change it so if () { continue; }
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.
This is my code and I'll fix for readability.
subsys/net/lib/lwm2m/lwm2m_engine.c
Outdated
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.
ouch, full of if/while/else/... imbrication below, hard to read. (legacy thing again)
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.
Same. This is my code and I'll fix for readability.
subsys/net/lib/lwm2m/lwm2m_rw_json.c
Outdated
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 there be a way for using part of zephyr's json library?
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.
Yes, I have a TODO marked in the header:
"Research using Zephyr JSON lib for json_next_token()"
subsys/net/lib/lwm2m/Kconfig.ipso
Outdated
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.
LWM2M_ prefix is needed on all sub-config options in this menu.
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.
Done.
include/misc/byteorder.h
Outdated
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.
ok but why is this patch part of lwm2m patchset? You probably want to send this patch in another PR.
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 added it here so testers could pull this as a part of the PR to test LWM2M. I will definitely submit this separately.
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.
as long as it is in its commit, I do not think it make sense to create another PR, the commit is needed here, so it should be part of this PR, lets not go into this please, it is enough to have commits separated and have PRs be more inclusive and allow coverage of major features that touch different parts of the system.
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.
Machine-to-Machine
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.
Done.
samples/net/lwm2m_client/README.rst
Outdated
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.
Header underline needs to be at least as long as the title (needs one more #)
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.
Done.
|
Thank you for the initial feedback / reviews! I've pushed a V2. CHANGES:
DISCUSSION POINT:
|
subsys/net/lib/zoap/zoap.c
Outdated
| int age; | ||
|
|
||
| if (r->tkl != tkl) { | ||
| if ((r->id == 0) && |
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.
Apparently there's some more testing to do with this patch. It might cause a regression where we're matching erroneously. This patch fixes a RST return from Leshan on an observer notification where the user as stopped the observer. The RST message has an ID which matches the newly stored reply->id, but there's no referencing token. I should probably add this information to the patch description.
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 should probably add this information to the patch description.
Yes please.
|
FYI: I'm going to be travelling this next week so I won't be as responsive to questions on this PR. But, I will check in from time to time. |
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.
Hi Mike, lot of code to review and I probably missed something. Anyway, looks good but had some comments.
subsys/net/lib/lwm2m/Kconfig
Outdated
| bool "support for RD LWM2M client state machine" | ||
| default y | ||
| help | ||
| Client will use RD client state machine to stay connected to LWM2M |
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.
It might be good idea to open the RD here in the help text.
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.
Ack. Providing a better description in general.
subsys/net/lib/lwm2m/Kconfig
Outdated
| This value sets the maximum number of error codes that the device | ||
| object will store before ignoring new values. | ||
|
|
||
| menu "IPSO Alliance Object Support" |
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.
Perhaps "IPSO Alliance Smart Object Support" here?
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.
Done.
subsys/net/lib/lwm2m/Kconfig.ipso
Outdated
| # | ||
|
|
||
| menuconfig LWM2M_IPSO_SUPPORT | ||
| bool "IPSO Alliance Object Support" |
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.
s/Object/Smart Object/
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.
Done.
subsys/net/lib/lwm2m/lwm2m_engine.c
Outdated
| static struct k_thread engine_thread_data; | ||
|
|
||
| /* for debugging: to print IP addresses */ | ||
| char *sprint_ip_addr(const struct sockaddr *addr) |
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.
Perhaps static here, or add a prefix?
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.
added a lwm2m_* prefix here as this is used in other files
subsys/net/lib/lwm2m/lwm2m_engine.c
Outdated
| return NULL; | ||
| } | ||
|
|
||
| char *sprint_token(const u8_t *token, u8_t tkl) |
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.
Same here
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.
Making this a static function as it's not used in other files
|
|
||
| /* initialize instance resource data */ | ||
| INIT_OBJ_RES_DATA(res[index], i, SECURITY_SERVER_URI_ID, | ||
| security_uri[index], SECURITY_URI_LEN); |
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.
Indentation looks wrong here and in other init macros few lines below.
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.
Done. Adjusted all of the smart object INIT_OBJ_* indentations. I had been lazy here during development as this data was changing fairly often.
|
|
||
| /* initialize instance resource data */ | ||
| INIT_OBJ_RES_DATA(res[index], i, SERVER_SHORT_SERVER_ID, | ||
| &server_id[index], sizeof(*server_id)); |
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.
Indentation
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.
Done.
|
|
||
| s64_t last_update; | ||
|
|
||
| char ep_name[32]; |
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.
Perhaps we could have a define instead of magic constant here?
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.
Done. Added a #define for CLIENT_EP_LEN
|
|
||
| static int lwm2m_rd_client_init(struct device *dev) | ||
| { | ||
| k_thread_create(&lwm2m_rd_client_thread_data, |
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.
There are lot of threads created by lwm2m system, is it possible to combine them (or some of them)? These many threads use quite a lot of precious ram for stack space.
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'd like to address this in a follow up patch if possible. I can add a JIRA if needed.
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.
Created JIRA to track memory reduction effort: https://jira.zephyrproject.org/browse/ZEP-2418
subsys/net/lib/zoap/zoap.c
Outdated
| int age; | ||
|
|
||
| if (r->tkl != tkl) { | ||
| if ((r->id == 0) && |
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 should probably add this information to the patch description.
Yes please.
| ret = zoap_packet_init(&request, pkt); | ||
| if (ret < 0) { | ||
| SYS_LOG_ERR("zoap packet init error (err:%d)", ret); | ||
| return ret; |
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.
should be "goto cleanup;"
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.
nice catch. Done.
|
Thank you for the reviews / feedback. I will submit V3 on Tuesday / Wednesday (once I return from holiday):
|
subsys/net/lib/lwm2m/lwm2m_engine.c
Outdated
| if (net_pkt_family(pkt) == AF_INET6) { | ||
| net_ipaddr_copy(&net_sin6(&from_addr)->sin6_addr, | ||
| &NET_IPV6_HDR(pkt)->src); | ||
| net_sin6(&from_addr)->sin6_port = NET_TCP_HDR(pkt)->src_port; |
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.
NET_TCP_HDR() -> NET_UDP_HDR()
Assuming we are using UDP.
And in latest master branch, both have been removed check 3604c39
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.
Fixing all of these per master branch changes
subsys/net/lib/lwm2m/lwm2m_engine.c
Outdated
| if (net_pkt_family(pkt) == AF_INET) { | ||
| net_ipaddr_copy(&net_sin(&from_addr)->sin_addr, | ||
| &NET_IPV4_HDR(pkt)->src); | ||
| net_sin(&from_addr)->sin_port = NET_TCP_HDR(pkt)->src_port; |
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.
Same here
| if (net_pkt_family(pkt) == AF_INET6) { | ||
| net_ipaddr_copy(&net_sin6(&from_addr)->sin6_addr, | ||
| &NET_IPV6_HDR(pkt)->src); | ||
| net_sin6(&from_addr)->sin6_port = NET_TCP_HDR(pkt)->src_port; |
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.
same here
| if (net_pkt_family(pkt) == AF_INET) { | ||
| net_ipaddr_copy(&net_sin(&from_addr)->sin_addr, | ||
| &NET_IPV4_HDR(pkt)->src); | ||
| net_sin(&from_addr)->sin_port = NET_TCP_HDR(pkt)->src_port; |
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.
same here
| struct oma_tlv tlv; | ||
| s16_t net_value; | ||
|
|
||
| net_value = sys_cpu_to_be32(value); |
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.
sys_cpu_to_be32() -> sys_cpu_to_be16()
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.
nice catch. done.
|
Hi all, I've posted V3 changes for this PR. I apologize for the sizable difference from V2. There was a lot of renaming and refactoring due to review comments. HIGH LEVEL CHANGES FOR V3:
OPEN QUESTIONS:
OPEN JIRA SUB-TASKS:
|
|
V4 update specifically addresses the ZoAP id/token matching commit "net: zoap: use message id for reply matching" NOTES:
|
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 really good. I had couple of comments but those can be done later. So if there is no objections from anyway this could be commited.
| * zoap expects that buffer->data starts at the | ||
| * beginning of the CoAP header | ||
| */ | ||
| header_len = net_pkt_appdata(pkt) - pkt->frags->data; |
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.
There is also a zoap specific Jira ticked that is related to this issue https://jira.zephyrproject.org/browse/ZEP-2210
subsys/net/lib/lwm2m/lwm2m_engine.c
Outdated
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.
Is this to be implemented or just missing atm? Please add "TODO" here if this needs to be implemented later.
subsys/net/lib/lwm2m/lwm2m_engine.c
Outdated
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.
BTW, if we want to have transparent DTLS support, then the net-app API net_app_send_pkt() should be used here, instead of direct call to net_context_sendto(). This can be changed in later PR of course.
|
I'm going to add a V5 which:
And I'll add a JIRA subtask to track net app API conversion (send pkt) |
|
Apparently if the license is in the source files, then an entry in doc/LICENSING.rst is not needed (see subsys/net/ip/rpl.c) |
|
Patchset V5 diff: https://hastebin.com/ajicibemij.diff NOTE: I'm seeing a page fault currently when testing the LwM2M client on qemu x86. I'm currently debugging why CONFIG_X86_STACK_PROTECTION is causing this. |
|
FYI: There were 3 edits to fix checkpatch warnings for long lines not mentioned. |
|
Pushed V7 with only patch description changes to fix SHIPPABLE warnings (I hope). |
|
@nashif We could merge this now but @tbursztyka earlier change request prevents merging, can you override that and merge this PR (Tomasz is on vacation right now)? |
|
@nashif mentioned on IRC that the "net: lwm2m: initial library support for LWM2M" commit is missing a block in the commit description similar to: Origin: SICS-IoT / Contiki OS However, this is only true for the 9 files with 3-clause BSD licenses still attached. How should I clarify that? |
We currently support converting from cpu format to BE for u16_t and u32_t. Let's add u64_t as well. NOTE: This will be used in LWM2M subsys later. Signed-off-by: Michael Scott <[email protected]>
In the 08 Feb 2017 V1.0 LwM2M specification page 80 mentions: in response to a "Notify" operation for which it is not interested in any more, the LwM2M Server can send a "Reset Message". Leshan server sends this CoAP RST response and it does not contain the originating message token (which is also how the packet flow looks on page 81 of the LwM2M spec). Using the current ZoAP sources, the client has no way of matching back to observation which needs to be cancelled. Let's add a match for message ID of a reply where there is no token to handle this case. Signed-off-by: Michael Scott <[email protected]> [[email protected]: Handle both piggybackend and separate response (id doesn't need to match, only token).] Signed-off-by: Ricardo Salveti <[email protected]>
Origin: SICS-IoT / Contiki OS URL: https://github.com/sics-iot/lwm2m-contiki/tree/lwm2m-standalone-dtls commit: d07b0bcd77ec7e8b93787669507f3d86cfbea64a Purpose: Introduction of LwM2M client library. Maintained-by: Zephyr Lightweight Machine-to-Machine (LwM2M) is a protocol stack extension of the Constrained Application Protocol (CoAP) which uses UDP transmission packets. This library was based on source worked on by Joakim Eriksson, Niclas Finne and Joel Hoglund which was adopted by Contiki and then later revamped to work as a stand-alone library. A VERY high level summary of the changes made: - [ALL] sources were re-formatted to Zephyr coding standards - [engine] The engine portion was re-written due to the heavy reliance on ER-CoAP APIs which are not compatible to the Zephyr CoAP APIs as well as other Zephyr specific needs. - [engine] All LWM2M/IPSO object data is now abstracted into resource data which stores information like the data type, length, callbacks to help with read/write. The engine modifies this data directly (or makes callbacks) instead of all of the logic for this living in each object's code. (This wasn't scaling well as I was implementing changes). - [engine] Related to the above change, I also added a generic set of getter/setter functions that user applications can call to change the object data instead of having to add getter/setting methods in each object. - [engine] The original sources shared the engine's context structure quite extensively causing a problem with portability. I broke up the context into it's individual parts: LWM2M path data, input data and output data and pass only the needed data into each set of APIs. - [content format read/writer] sources were re-organized into single .c/h files per content formatter. - [content format read/writer] sources were re-written where necessary to remove the sharing of the lwm2m engine's context and instead only requires the path and input or output data specific to it's function. - [LwM2M objects] re-written using the new engine's abstractions Signed-off-by: Michael Scott <[email protected]>
This sample utilizes the new LwM2M library by setting up default values for LwM2M device and firmware objects and then establisting a connection to a LwM2M server (for example Leshan Demo Server) via the registration interface. To use QEMU for this purpose please see: doc/subsystems/networking/qemu_setup.rst NOTE: This sample currently does not demonstrate DTLS/bootstrap as neither of these is supported by the LwM2M library. Signed-off-by: Michael Scott <[email protected]>
IPSO Smart Objects are a set of template objects based on the LwM2M object framework which are designed to represent standard hardware such as temperature and humidity sensors or light controls. Let's add a place for these objects to live as well as an initial temperature sensor object. Signed-off-by: Michael Scott <[email protected]>
This commit adds IPSO temperature support to the LwM2M client sample. NOTE: A dummy value of 25C is set during initialization and does not change. Signed-off-by: Michael Scott <[email protected]>
Signed-off-by: Michael Scott <[email protected]>
Signed-off-by: Michael Scott <[email protected]>
Signed-off-by: Michael Scott <[email protected]>
|
Pushed V8 to address the contribution guide requirement for the above mentioned block in the "net: lwm2m: initial library support for LWM2M" patch. That was the only change. |
|
Correction: I also rebased on today's master branch. |
|
Ugh, SHIPPABLE complaining about the URL line length in the new change to the patch description: NOTE: running gitlint in my local repo does not show this error. :/ |
|
@nashif Could you force merge this as shippable complains about non-issue? |
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.
+1 for doc so far, but_lwm2m-client-sample is still empty :(
Signed-off-by: James Prestwood <[email protected]>
Hello all,
I'm submitting this series as RFC to start discussion / early review process to get an LWM2M implementation merged into Zephyr mainline.
A bit of history:
After doing quite a bit of research to select a good starting point for a Zephyr-based LWM2M stack, I selected the following source: https://github.com/sics-iot/lwm2m-contiki/tree/lwm2m-standalone-dtls/apps/oma-lwm2m
Created by: Joakim Eriksson, Niclas Finne and Joel Hoglund as a stand-alone and/or Contiki based solution. The stack they've put together is very robust and looked to be a good candidate.
Here's a small summary of my changes from the original sources:
The sample for LWM2M can be run via QEMU by following the instructions here:
https://github.com/zephyrproject-rtos/zephyr/blob/master/doc/subsystems/networking/qemu_setup.rst
(Quick Instructions)
To start with you will want to use Leshan LWM2M demo server:
[terminal #1]
$ wget https://hudson.eclipse.org/leshan/job/leshan/lastSuccessfulBuild/artifact/leshan-server-demo.jar
$ java -jar ./leshan-server-demo.jar
(You can now open a web browser to http://localhost:8080/ to watch connections)
Next, checkout https://github.com/zephyrproject-rtos/net-tools and follow the build instructions referenced above. After they are built, start up the slip tools:
[terminal #2]
$ ./loop-socat.sh
[terminal #3]
$ sudo ./loop-slip-tap.sh
Now you can cd to the Zephyr sources and build the sample for QEMU target:
$ make -C samples/net/lwm2m_client qemu
You should see the sample app connect to Leshan and register twice (IPv4 connection and IPv6 connection). Feel free to navigate around Leshan and play with the QEMU LWM2M client.
I look forward to your comments.