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

pkg/wakaama: Add basic LWM2M client implementation #11036

Merged
merged 3 commits into from
Nov 29, 2019

Conversation

leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented Feb 20, 2019

Contribution description

This PR is a part of a rework of #8610 by @moenoel (see #8610 (comment)).

This PR adds

Contrib code

  • Adds a LWM2M client implementation
  • Adds a basic Device Object implementation
  • Adds a basic Light Control Object implementation

Example

  • Adds an example of LWM2M client

For now the client only implements no security connections. In future PRs DTLS support will be added.

Testing procedure

Run the examples/wakaama application

SERVER_URI='\"coap://[<ADDRESS_OF_SERVER>]\"' make all term -C examples/wakaama/

Issues/PRs references

#8610
Depends on #10738

@leandrolanzieri leandrolanzieri added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports labels Feb 20, 2019
@jia200x jia200x added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 20, 2019
@RIOT-OS RIOT-OS deleted a comment Feb 20, 2019
@leandrolanzieri leandrolanzieri added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 25, 2019
@leandrolanzieri leandrolanzieri force-pushed the pr/pkg/wakaama_rework branch 2 times, most recently from bc9957d to feafb2e Compare February 27, 2019 10:57
@leandrolanzieri leandrolanzieri removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 27, 2019
@leandrolanzieri
Copy link
Contributor Author

@kb2ma you mentioned you would be able to review the rework of wakaama?

@kb2ma kb2ma self-requested a review March 1, 2019 13:18
@kb2ma
Copy link
Member

kb2ma commented Mar 1, 2019

Thanks for the reminder. I'll start digging in.

Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

@leandrolanzieri, it will be great to have an LwM2M client example! I have started by testing against only a registration server, with no bootstrap server. I was able to run the client and server, manipulate the LEDs, and interact with the Leshan demo server. See the inline comments and question below on issues that arose.

How did you set up the tap interface on native? I usually assign a ULA on the tap interface, like fd00:dead:beef::1. I then add the native instance to that network, at fd00:dead:beef::2, via RIOT's ifconfig command. The Leshan server was able to receive CoAP messages in this case. However, to send a message to read a value, the Leshan server used a different local address on the same network, like fd00:dead:beef::919f:e3c:0:2721, which the RIOT native instance did not respond to. I found that I had to set the local address of the server via the --coaphost option, like '--coaphost [fd00:dead:beef::1]' to get this to work.

examples/wakaama/README.md Show resolved Hide resolved
examples/wakaama/README.md Show resolved Hide resolved
examples/wakaama/README.md Show resolved Hide resolved
@leandrolanzieri
Copy link
Contributor Author

I have started by testing against only a registration server, with no bootstrap server. I was able to run the client and server, manipulate the LEDs, and interact with the Leshan demo server.

Thanks for testing!

How did you set up the tap interface on native?

What I have been doing so far (though it may not be the correct way) is to create the tap interfaces and use the address of the tapbr0 as the server. Then I run the Leshan server without setting the coaphost option.

@kb2ma
Copy link
Member

kb2ma commented Mar 5, 2019

Thanks, I'll try use of tapbr0 address.

examples/wakaama/README.md Outdated Show resolved Hide resolved
examples/wakaama/README.md Outdated Show resolved Hide resolved
@kb2ma
Copy link
Member

kb2ma commented Mar 22, 2019

I got the bootstrap server to work after resolving the issue with the port noted inline. I have started to look at the code, but there is a lot to review! I hope we can make some time at IETF over the next week to make progress.

@MrKevinWeiss MrKevinWeiss self-requested a review May 16, 2019 06:52
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

I ran through the example with and without bootstrap on both native and samr21-xpro. The only thing that may be nice is to link to some general tutorial that explains lwm2m for those who are starting from scratch. Not everyone has a @leandrolanzieri across the desk.

Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

First, I don't know LWM2M, or waakama. My comments are thus only related to general coding stuff and the RIOT integration.

The most important thing here is that this is ALOT of code, so I probably missed many things. At the same time, I feel its size is not so much because of what it does, but how it does it:

  • A pooled allocator implementation that AFAICT does not belong here.
  • Bit switch...case that can be replaced by table lookups.
  • A big URI parsing routine.
  • The lights stuff does not belong to the package.

Because of the last point, I did not go deep into the lights code.

* @brief Device name used to register at the LWM2M server
*/
#ifndef LWM2M_DEVICE_NAME
#define LWM2M_DEVICE_NAME "testRIOTDevice"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a good idea to give default values for parameters other than port? I can imagine loads of devices showing up as "testRIOTDevice".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the exposed compile configurations for the module, should be changed by the user when using multiple nodes, I think it makes things easier to have a default one.

pkg/wakaama/Makefile.dep Outdated Show resolved Hide resolved
pkg/wakaama/Makefile.dep Outdated Show resolved Hide resolved
pkg/wakaama/include/lwm2m_objects/light_control.h Outdated Show resolved Hide resolved
return 0;
}

static lwm2m_client_connection_t *_connection_create(int instance_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to roll out a URI parser from scratch specifically for this module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it could be eventually refactored to a module or replaced by an existing pkg.

pkg/wakaama/Makefile.dep Show resolved Hide resolved
pkg/wakaama/contrib/lwm2m_objects/device/device.c Outdated Show resolved Hide resolved
pkg/wakaama/contrib/lwm2m_objects/device/device.c Outdated Show resolved Hide resolved
pkg/wakaama/contrib/lwm2m_objects/device/device.c Outdated Show resolved Hide resolved
@MrKevinWeiss
Copy link
Contributor

release status ping, should I change the milestone?

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Blocking until hard feature freeze

@MrKevinWeiss MrKevinWeiss added this to the Release 2019.10 milestone Jun 29, 2019
@kb2ma
Copy link
Member

kb2ma commented Nov 22, 2019

The latest code tests fine without the internal Wakaama logging. However, with LWM2M_WITH_LOGS defined, I see the output below when registering without a bootstrap server. The garbage characters continue indefinitely.

It looks like some problem with LWM2M_ALT_PATH defined as NULL. I saw you added a patch for something similar in 2cc4ec4. What changed?

[registration_start:477] State: STATE_REGISTER_REQUIRED
[object_getRegisterPayloadBufferLength:508] Entering
[object_getRegisterPayload:579] Entering
[lwm2m_data_new:143] size: 1
[lwm2m_data_encode_string:195] "coap://[fd00:bbbb::1]"
[lwm2m_data_free:161] size: 1
[transaction_new:156] method: 2, altPath: "0 yU��e�������������������	 ��������

@leandrolanzieri
Copy link
Contributor Author

The latest code tests fine without the internal Wakaama logging. However, with LWM2M_WITH_LOGS defined, I see the output below when registering without a bootstrap server. The garbage characters continue indefinitely.

It looks like some problem with LWM2M_ALT_PATH defined as NULL. I saw you added a patch for something similar in 2cc4ec4. What changed?

Hmm I patched lwm2m_configure because it was not checking if the pointer was set to NULL before logging the parameters. Looks like you found another case where this is happening. I will add the patch.

It's odd because this functions accept NULL when the parameter is not needed (the code has NULL checks), but the logging is being done before 😕

@kb2ma
Copy link
Member

kb2ma commented Nov 22, 2019

I just checked out an early commit from this PR, and I see that LWM2M_ALT_PATH was defined as NULL there also. I know I have tested with LWM2M_WITH_LOGS before, but I don't remember this issue with the garbage output. It might be worth digging a little to determine if something about RIOT has changed that now is triggering this issue.

@leandrolanzieri
Copy link
Contributor Author

I just checked out an early commit from this PR, and I see that LWM2M_ALT_PATH was defined as NULL there also. I know I have tested with LWM2M_WITH_LOGS before, but I don't remember this issue with the garbage output. It might be worth digging a little to determine if something about RIOT has changed that now is triggering this issue.

I tested this on latest release (without the logging fix patch) and the logs seem to be OK:

lwm2m start
[lwm2m_init:64] Entering
[lwm2m_configure:264] endpointName: "testRIOTDevice", msisdn: "(null)", altPath: "(null)", numObject: 3
[lwm2m_step:372] timeoutP: 6226783043881795585
[lwm2m_step:377] State: STATE_INITIAL

The NULL pointer is recognized and no garbage is printed. I will try to bisect.

@leandrolanzieri
Copy link
Contributor Author

I just checked out an early commit from this PR, and I see that LWM2M_ALT_PATH was defined as NULL there also. I know I have tested with LWM2M_WITH_LOGS before, but I don't remember this issue with the garbage output. It might be worth digging a little to determine if something about RIOT has changed that now is triggering this issue.

I tested this on latest release (without the logging fix patch) and the logs seem to be OK:

Disregard this. It does not work for samr21-xpro. It seems it really depends on the implementation of printf. In any case, we should fix these debug messages as the behaviour of printf is not defined on a NULL pointer.

Do you remember if the moment when you tested this with logs you were compiling for native or a different platform?

@kb2ma
Copy link
Member

kb2ma commented Nov 26, 2019

In any case, we should fix these debug messages as the behaviour of printf is not defined on a NULL pointer.

OK, sounds good. It really needs to be fixed. You can see that upstream handles this case in internals.h for LOG_URI.

Do you remember if the moment when you tested this with logs you were compiling for native or a different platform?

No, I don't remember.

@leandrolanzieri
Copy link
Contributor Author

@kb2ma I added the patch

@kb2ma
Copy link
Member

kb2ma commented Nov 27, 2019

I don't see the error any more with this patch. Thanks!

I have discovered one more problem with logging on samr21-xpro. There are lines like this in the code:

    LOG_ARG("value: %" PRId64 "", value);

which result in the following in the log:

[lwm2m_data_encode_int:272] value: ld

@leandrolanzieri
Copy link
Contributor Author

I don't see the error any more with this patch. Thanks!

I have discovered one more problem with logging on samr21-xpro. There are lines like this in the code:

    LOG_ARG("value: %" PRId64 "", value);

which result in the following in the log:

[lwm2m_data_encode_int:272] value: ld

So this seems to be a limitation of the implementation of printf that is being used. See #1891. We could replace this with fmt, but it seems a bit odd to use it in the package. Any suggestions?

@kb2ma
Copy link
Member

kb2ma commented Nov 27, 2019

I can only suggest to do the simplest thing to make it work. I got the code below to compile in Wakaama's internals.h by copying the current fmt.(c|h) files into the wakaama 'shared' directory. I created the LOG_ARG1 macro to avoid having to deal with __VAR_ARGS__.

Maybe you could just patch the code for fmt_s64_dec() into the Wakaama utils.(c|h).

I know it's a pain in the neck, but I think it will be useful for debugging to see these values.

#include "fmt.h"
#define LOG_ARG1(FMT, VALUE)                                                        \
{                                                                                   \
    if (!strcmp(FMT, "PRId64"))                                                     \
    {                                                                               \
        char int64_str[20];                                                         \
        int64_str[fmt_s64_dec(int64_str, (VALUE))] = '\0';                          \
        lwm2m_printf("[%s:%d] %s \r\n", __func__ , __LINE__ , int64_str);           \
    }                                                                               \
    else lwm2m_printf("[%s:%d] " FMT "\r\n", __func__ , __LINE__ , (VALUE));        \
}

@kb2ma
Copy link
Member

kb2ma commented Nov 29, 2019

Looks good. Tests fine for bootstrap and non-bootstrap. Please squash!

@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 29, 2019
@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 29, 2019
@leandrolanzieri
Copy link
Contributor Author

@kb2ma done, everything looks good

@kb2ma
Copy link
Member

kb2ma commented Nov 29, 2019

Before I press the button, what were the problems? I saw some out of memory issues, LLVM issues.

@leandrolanzieri
Copy link
Contributor Author

Before I press the button, what were the problems? I saw some out of memory issues, LLVM issues.

Some boards missing in the 'insufficient memory list' (some new boards and renaming of nucleo boards), some new 8-bits boards had to be added to the blacklist.

LLVM didn't like the lack of a newline at the end of one file in the package (thus the new patch 0021) and needs an attribute line for the printf function, that performs a check on all the calls to make sure that the format string is ok for printf.

@kb2ma
Copy link
Member

kb2ma commented Nov 29, 2019

Finally the embedded OS deities have been satisfied. 126 comments to bring in a package. Is @leandrolanzieri getting stronger or is he about to fold? Only time will tell. ;-)

At any rate, we mortals appreciate all the work you put into this!

@kb2ma kb2ma merged commit 353c0e9 into RIOT-OS:master Nov 29, 2019
@leandrolanzieri
Copy link
Contributor Author

Thanks everyone!! 🎉 Especially @kb2ma for your thorough testing and reviewing! 🚀

@leandrolanzieri leandrolanzieri deleted the pr/pkg/wakaama_rework branch November 29, 2019 21:35
@emmanuelsearch
Copy link
Member

@leandrolanzieri yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants