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

net: lwm2m: Add new API to set multiple resource values #68988

Merged
merged 4 commits into from Feb 26, 2024

Conversation

juhaylinen
Copy link
Contributor

Add new API lwm2m_set_bulk() to set multiple resource values in one function call.

Update lwm2m sample and add test case.

Add entry to release notes.

Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Please check compliance, there are a few issues reported.

@@ -577,6 +577,10 @@ Networking

* LwM2M:

* Added new API function to set multiple resource values:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this won't make it to 3.6, will need to move this entry to 3.7 release notes, once available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Removed the change note entry for now.

int lwm2m_set_bulk(const struct lwm2m_res_item res_list[], size_t res_list_size)
{
int ret;
for (int i = 0; i < res_list_size; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make sense to take the registry lock before iterating? That way we could set several resources in an "atomic" way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added registry lock

@henrikbrixandersen henrikbrixandersen added this to the v3.7.0 milestone Feb 14, 2024
@juhaylinen juhaylinen force-pushed the lwm2m_set_bulk branch 2 times, most recently from 5428957 to eca2377 Compare February 15, 2024 08:08
@juhaylinen juhaylinen changed the title Lwm2m set bulk net: lwm2m: Add new API to set multiple resource values Feb 15, 2024
rlubos
rlubos previously approved these changes Feb 15, 2024
Copy link
Collaborator

@SeppoTakalo SeppoTakalo left a comment

Choose a reason for hiding this comment

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

After an internal discussion, we realized that this needs special handling for strings.

When target type is string or opaque, the length must be different. See lwm2m_set_string().

I have no idea how to detect from the API that the given pointer points to string. So it should be properly documented.

Copy link
Collaborator

@SeppoTakalo SeppoTakalo left a comment

Choose a reason for hiding this comment

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

After a second thought, I don't think we can detect the proper use of strings, so it should be documented.

There is no type in the struct lwm2m_res_item so it is impossible to know if the developer is doing the right thing when writing to opaque resources.
At least current lwm2m_set_string() API allows writing strings to opaque resources, for example PSK ID is sometimes written like this. This should be allowed.

struct lwm2m_res_item {
struct lwm2m_obj_path *path; /**< Pointer to LwM2M path as a struct */
void *value; /**< Pointer to resource value */
uint16_t len; /**< Length of value */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uint16_t len; /**< Length of value */
uint16_t size; /**< Size of the value. For string resources, it should contain the null-terminator. */

Comment on lines 1218 to 1251
* @brief Set multiple resource (instance) values
*
* @param[in] res_list LwM2M resource item list
* @param[in] res_list_size Length of resource list
*
* @return 0 for success or negative in case of error.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @brief Set multiple resource (instance) values
*
* @param[in] res_list LwM2M resource item list
* @param[in] res_list_size Length of resource list
*
* @return 0 for success or negative in case of error.
*/
* @brief Set multiple resource (instance) values
*
* When writing strings to opaque resources, the null-terminator should not be counted in the size field.
* When writing strings to string resources, the null-terminator should be included in the size.
*
* @param[in] res_list LwM2M resource item list
* @param[in] res_list_size Length of resource list
*
* @return 0 for success or negative in case of error.
*/

SeppoTakalo
SeppoTakalo previously approved these changes Feb 15, 2024
SeppoTakalo
SeppoTakalo previously approved these changes Feb 15, 2024
rlubos
rlubos previously approved these changes Feb 15, 2024
@henrikbrixandersen henrikbrixandersen added Release Notes Required Release notes required for this change and removed Release Notes To be mentioned in the release notes labels Feb 24, 2024
@henrikbrixandersen henrikbrixandersen removed their request for review February 24, 2024 21:02
Add new API lwm2m_set_bulk() to set multiple resource values in
one function call.

Signed-off-by: Juha Ylinen <juha.ylinen@nordicsemi.no>
Add an example how to set multiple resource values using
lwm2m_set_bulk() function.

Signed-off-by: Juha Ylinen <juha.ylinen@nordicsemi.no>
Add test case for lwm2m_set_bulk() function.

Signed-off-by: Juha Ylinen <juha.ylinen@nordicsemi.no>
Add new API function lwm2m_set_bulk.

Signed-off-by: Juha Ylinen <juha.ylinen@nordicsemi.no>
@juhaylinen juhaylinen dismissed stale reviews from rlubos and SeppoTakalo via 7000880 February 26, 2024 08:06
@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Feb 26, 2024
@juhaylinen
Copy link
Contributor Author

Added entry to 3.7 release notes.

@fabiobaltieri fabiobaltieri merged commit 5a9a221 into zephyrproject-rtos:main Feb 26, 2024
24 checks passed
@juhaylinen juhaylinen deleted the lwm2m_set_bulk branch February 26, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: LWM2M area: Networking area: Samples Samples Release Notes Required Release notes required for this change Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants