Skip to content

pkg/wakaama: add get set functions and cleanup client connection#16203

Merged
jia200x merged 4 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/pkg/wakaama_add_get_set_functions
Jul 13, 2021
Merged

pkg/wakaama: add get set functions and cleanup client connection#16203
jia200x merged 4 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/pkg/wakaama_add_get_set_functions

Conversation

@leandrolanzieri
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri commented Mar 17, 2021

Contribution description

This adds generic get/set functions to interact with resources of the LwM2M client. Also, the client connection code is refactored to use these functions and the URI parser module.

Testing procedure

The examples/wakaama application should work as usual.

Issues/PRs references

None

@leandrolanzieri leandrolanzieri added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: pkg Area: External package ports labels Mar 17, 2021
@leandrolanzieri leandrolanzieri force-pushed the pr/pkg/wakaama_add_get_set_functions branch from 9f72f0c to 4cb77ee Compare March 17, 2021 15:12
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Mar 17, 2021

I'm piggybacking the fixes to the issues spotted by the static test

@leandrolanzieri leandrolanzieri mentioned this pull request Mar 25, 2021
Merged
PeterKietzmann
PeterKietzmann reviewed Apr 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

This is not a full review, just some early comments

pkg/wakaama/contrib/objects/common.c
@@ -0,0 +1,413 @@
/*
Copy link
Copy Markdown
Member

@PeterKietzmann PeterKietzmann Apr 6, 2021

Choose a reason for hiding this comment

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

This file is common to wakaama, right? Maybe prefix.

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What should I prefix? The file name? It is contained in the contrib folder, is it really necessary ?

jia200x
jia200x requested changes Jun 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

Some minor comments

pkg/wakaama/contrib/objects/common.c Outdated
case LWM2M_TYPE_OPAQUE:
if (data->value.asBuffer.length > out_len) {
DEBUG("[lwm2m:get_data] not enough space in buffer\n");
result = -1;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
result = -1;
result = -ENOMEM;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Changed!

pkg/wakaama/contrib/objects/common.c Outdated
lwm2m_object_t *object = lwm2m_get_object_by_id(client_data, uri->objectId);
if (!object || !object->readFunc) {
DEBUG("[lwm2m:get_data] could not find object with ID %d\n", uri->objectId);
result = -1;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
result = -1;
result = -ENOENT;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Changed!

pkg/wakaama/contrib/objects/common.c Outdated
/* read the resource from the specified instance */
uint8_t res = object->readFunc(uri->instanceId, &data_num, &data, object);
if (res != COAP_205_CONTENT || data->type != expected_type) {
result = -1;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
result = -1;
result = -EINVAL;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Changed!

pkg/wakaama/contrib/objects/common.c Outdated
lwm2m_uri_t uri;
if (!lwm2m_stringToUri(path, path_len, &uri)) {
DEBUG("[lwm2m:get_resource] malformed path\n");
return -1;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
return -1;
return -EINVAL;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Changed!

pkg/wakaama/contrib/objects/common.c Outdated

default:
DEBUG("[lwm2m:get_data] not supported type\n");
result = -1;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
result = -1;
result = -ENOTSUP;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Changed!

pkg/wakaama/contrib/objects/common.c Outdated

default:
DEBUG("[lwm2m:get_data] not supported type\n");
result = -1;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
result = -1;
result = -ENOTSUP;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Changed!

pkg/wakaama/contrib/objects/common.c Outdated
/* write the resource of the specified instance */
uint8_t res = object->writeFunc(uri->instanceId, 1, data, object);
if (res != COAP_204_CHANGED) {
result = -1;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
result = -1;
result = -EINVAL;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Changed!

pkg/wakaama/contrib/objects/common.c Outdated
lwm2m_uri_t uri;
if (!lwm2m_stringToUri(path, path_len, &uri)) {
DEBUG("[lwm2m:set_resource] malformed path\n");
return -1;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
return -1;
return -EINVAL;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Changed!

pkg/wakaama/contrib/objects/common.c
default:
DEBUG("[lwm2m:get_data] not supported type\n");
result = -1;
break;
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Just in case someone else adds code right after the switch-case

Suggested change
break;
goto free_out;
break;

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

I simplified the return routine, now there is only on out point.

pkg/wakaama/contrib/objects/common.c
if (data->value.asBuffer.length > out_len) {
DEBUG("[lwm2m:get_data] not enough space in buffer\n");
result = -1;
}
Copy link
Copy Markdown
Member

@jia200x jia200x Jun 17, 2021

Choose a reason for hiding this comment

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

Suggested change
}
goto free_out;
}

Copy link
Copy Markdown
Contributor Author

@leandrolanzieri leandrolanzieri Jul 8, 2021

Choose a reason for hiding this comment

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

Same for this one

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@leandrolanzieri leandrolanzieri force-pushed the pr/pkg/wakaama_add_get_set_functions branch from df0d031 to afda4ef Compare July 8, 2021 08:22
@github-actions github-actions bot added Area: examples Area: Example Applications and removed Area: network Area: Networking labels Jul 8, 2021
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Jul 8, 2021

@jia200x I think I addressed your comments. I updated the documentation to reflect the new possible return values. Also added checks when allocating data structure.

Finally, I rebased to resolve conflicts.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jul 12, 2021

It works:

Captura de Pantalla 2021-07-12 a la(s) 14 04 19

Captura de Pantalla 2021-07-12 a la(s) 14 02 57

Also, the lwm2m mem command doesn't seem to show leaks.

jia200x
jia200x approved these changes Jul 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

ACK

@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 Jul 12, 2021
@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jul 12, 2021

please squash

@leandrolanzieri leandrolanzieri force-pushed the pr/pkg/wakaama_add_get_set_functions branch from afda4ef to 327da03 Compare July 13, 2021 06:10
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Jul 13, 2021

Squashed!

@jia200x jia200x merged commit a53e1a3 into RIOT-OS:master Jul 13, 2021
@leandrolanzieri leandrolanzieri deleted the pr/pkg/wakaama_add_get_set_functions branch July 13, 2021 15:20
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Jul 13, 2021

Thanks for reviewing!

@leandrolanzieri leandrolanzieri mentioned this pull request Jul 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PeterKietzmann PeterKietzmann PeterKietzmann left review comments

@jia200x jia200x jia200x approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees

No one assigned

Labels

Area: examples Area: Example Applications 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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2021.10

Development

Successfully merging this pull request may close these issues.

4 participants

@leandrolanzieri @jia200x @PeterKietzmann @MrKevinWeiss