Make CoAP block size configurable at build- and run-time #596

mlasch · 2021-04-26T15:51:05Z

This patch makes the block size used in CoAP block transfers configurable in two ways.

Build-time configurable default value via a cmake variable.
Run-time configurable via new -S command line argument for all examples. This will simplify (automated) testing with different block sizes.

⚠️ The new default value for the block size will now be 1024 bytes with no further configuration. The idea is to avoid block transfers in common use-cases. Any objections?

sbertin-telular · 2021-04-26T15:54:47Z

README.md

- -s Set the device management or bootstrap server Pre-Shared-Key. If not set use none secure mode
+```
+  -i Set the device management or bootstrap server PSK identity. If not set use none secure mode
+  -s Set the device management or bootstrap server Pre-Shared-Key. If not set use none secure mode


CoAP block size and Pre-Shared-Key are both -s.

I missed that. I would go with -S then. Another option would be -a since other obvious options are already used.

sbertin-telular · 2021-04-26T15:59:31Z

examples/lightclient/lightclient.c

@@ -57,6 +57,7 @@

 #include "liblwm2m.h"
 #include "connection.h"
+#include "commandline.h"


Why the additional include? I don't think the lightclient supports commands.

I put validate_coap_block_size in commandline.c because it is shared code for the examples. Therefore this include is needed in the lightclient. ~~Another place would be core/utils.c and liblwm2m.h.~~

sbertin-telular · 2021-04-26T16:02:09Z

examples/shared/commandline.c

+{
+	return coap_block_size_arg >= 16 && \
+		coap_block_size_arg <= 1024 && \
+		coap_block_size_arg % 2 == 0;


This would allow any even block size from 16 to 1024, not just 16, 32, 64, 128, 256, 512, 1024.

I will update this part and move into core/utils.c

sbertin-telular · 2021-04-26T16:07:52Z

coap/er-coap-13/er-coap-13.c

@@ -65,6 +65,7 @@
 /*- Variables -----------------------------------------------------------------------*/
 /*-----------------------------------------------------------------------------------*/
 static uint16_t current_mid = 0;
+uint16_t coap_block_size = COAP_DEFAULT_BLOCK_SIZE;


This is declared in liblwm2m.h. I think the storage and default value should be set in one of the core files, not here. I would suggest core/packet.c since that is where most of the usage occurs.

sbertin-telular · 2021-04-26T16:14:11Z

include/liblwm2m.h

@@ -97,6 +97,8 @@ extern "C" {
 #error "LWM2M_BOOTSTRAP and LWM2M_BOOTSTRAP_SERVER_MODE cannot be defined at the same time!"
 #endif

+extern uint16_t coap_block_size;


I don't think there are any other variables publicly exported by Wakaama. I would suggest moving this to core/internals.h and creating a function to set it. That would also give a point to notify a CoAP stack (if we support a different one in the future) of a change in the block size.

On another note, anything meant to be accessed by the user's code should start with lwm2m.

sbertin-telular · 2021-04-26T16:15:22Z

examples/client/lwm2mclient.c

@@ -1014,6 +1015,22 @@ int main(int argc, char *argv[])
        case '4':
            data.addressFamily = AF_INET;
            break;
+        case 's':


Duplicate case if WITH_TINYDTLS is defined.

@rettichschnidi, the CI didn't catch this. Possibly another area for improvement. I'm not sure if it is doing a DTLS build or not.

sbertin-telular · 2021-04-26T16:33:56Z

1024 bytes for the block size agrees with RFC7252 section 4.6

rettichschnidi · 2021-04-27T12:46:23Z

@mlasch Please rebase on the current master 😸

sbertin-telular · 2021-04-27T12:56:36Z

core/packet.c

+
+bool lwm2m_validate_block_size(uint16_t coap_block_size_arg)
+{
+	uint16_t valid_block_sizes[7] = {16, 32, 64, 128, 256, 512, 1024};


Please add const here. That will allow it to remain in flash on some devices rather than being copied to RAM and using more resources.

sbertin-telular · 2021-04-27T13:02:10Z

core/packet.c

+
+void lwm2m_set_block_size(uint16_t coap_block_size_arg)
+{
+	coap_block_size = coap_block_size_arg;


I'd like to see the set do the validation and return a true/false indication to prevent misuse and simplify the API for the user.

Sounds sensible. I will remove lwm2m_validate_block_size from the public interface then.

mlasch · 2021-04-27T21:20:53Z

Pushed again, let clang-format do the formatting.

sbertin-telular

LGTM

README.md

coap/er-coap-13/er-coap-13.h

core/internals.h

core/packet.c

rettichschnidi · 2021-04-29T16:07:24Z

examples/lightclient/lightclient.c

+            uint16_t coap_block_size_arg;
+            if (1 == sscanf(argv[opt], "%hu", &coap_block_size_arg) && lwm2m_set_block_size(coap_block_size_arg)) {
+                break;
+            } else {
+                print_usage();
+                return 0;
+            }


Same as above.

rettichschnidi · 2021-04-29T16:07:38Z

examples/server/lwm2mserver.c

@@ -1217,7 +1232,7 @@ int main(int argc, char *argv[])
                    in_port_t port;
                    connection_t * connP;

-					s[0] = 0;
+                    s[0] = 0;


Unwanted cosmetic change?

rettichschnidi · 2021-04-29T16:07:48Z

examples/server/lwm2mserver.c

+            uint16_t coap_block_size_arg;
+            if (1 == sscanf(argv[opt], "%hu", &coap_block_size_arg) && lwm2m_set_block_size(coap_block_size_arg)) {
+                break;
+            } else {
+                print_usage();
+                return 0;


Same as above.

rettichschnidi · 2021-04-29T16:08:07Z

include/liblwm2m.h

@@ -277,6 +277,11 @@ void lwm2m_list_free(lwm2m_list_t * head);
 #define LWM2M_LIST_FIND(H,I) lwm2m_list_find((lwm2m_list_t *)H, I)
 #define LWM2M_LIST_FREE(H) lwm2m_list_free((lwm2m_list_t *)H)

+/*
+ * Helper functions for coap block size settings.


Suggested change

* Helper functions for coap block size settings.

* Helper functions for CoAP block size settings.

coap/transaction.c

rettichschnidi · 2021-05-04T11:47:47Z

core/packet.c

@@ -93,6 +93,28 @@ Contains code snippets which are:

 #include <stdio.h>

+uint16_t coap_block_size = LWM2M_COAP_DEFAULT_BLOCK_SIZE;
+
+static bool validate_block_size(uint16_t coap_block_size_arg) {


Suggested change

static bool validate_block_size(uint16_t coap_block_size_arg) {

static bool validate_block_size(const uint16_t coap_block_size_arg) {

rettichschnidi · 2021-05-04T11:48:38Z

core/packet.c

+    return false;
+}
+
+bool lwm2m_set_coap_block_size(uint16_t coap_block_size_arg) {


Suggested change

bool lwm2m_set_coap_block_size(uint16_t coap_block_size_arg) {

bool lwm2m_set_coap_block_size(const uint16_t coap_block_size_arg) {

rettichschnidi · 2021-05-04T19:54:50Z

examples/bootstrap_server/bootstrap_server.c

+                return 0;
+            }
+            uint16_t coap_block_size_arg;
+            if (1 == sscanf(argv[opt], "%hu", &coap_block_size_arg) && lwm2m_set_coap_block_size(coap_block_size_arg)) {


Would rather use C99 format specifiers:

Suggested change

if (1 == sscanf(argv[opt], "%hu", &coap_block_size_arg) && lwm2m_set_coap_block_size(coap_block_size_arg)) {

if (1 == sscanf(argv[opt], "%" PRIu16, &coap_block_size_arg) && lwm2m_set_coap_block_size(coap_block_size_arg)) {

I guess you meant SCNu16 in this case. I can change those lines.

rettichschnidi · 2021-05-04T19:55:10Z

examples/client/lwm2mclient.c

+                return 0;
+            }
+            uint16_t coap_block_size_arg;
+            if (1 == sscanf(argv[opt], "%hu", &coap_block_size_arg) && lwm2m_set_coap_block_size(coap_block_size_arg)) {


Would rather use C99 format specifiers.

rettichschnidi · 2021-05-04T19:55:23Z

examples/lightclient/lightclient.c

+                return 0;
+            }
+            uint16_t coap_block_size_arg;
+            if (1 == sscanf(argv[opt], "%hu", &coap_block_size_arg) && lwm2m_set_coap_block_size(coap_block_size_arg)) {


Would rather use C99 format specifiers.

rettichschnidi · 2021-05-04T19:55:52Z

examples/server/lwm2mserver.c

+                return 0;
+            }
+            uint16_t coap_block_size_arg;
+            if (1 == sscanf(argv[opt], "%hu", &coap_block_size_arg) && lwm2m_set_coap_block_size(coap_block_size_arg)) {


Would rather use C99 format specifiers.

rettichschnidi · 2021-05-04T19:56:32Z

include/liblwm2m.h

+ * Helper functions for CoAP block size settings.
+ */
+bool lwm2m_set_coap_block_size(uint16_t coap_block_size_arg);
+uint16_t lwm2m_get_coap_block_size();


We should use prototypes in C:

Suggested change

uint16_t lwm2m_get_coap_block_size();

uint16_t lwm2m_get_coap_block_size(void);

rettichschnidi

One small fix/improvement I feel is worth to take into account. See comment. Everything else LGTM.

rettichschnidi · 2021-05-07T22:59:19Z

core/wakaama.cmake

+if (LWM2M_COAP_DEFAULT_BLOCK_SIZE)
+    add_compile_definitions(LWM2M_COAP_DEFAULT_BLOCK_SIZE=${LWM2M_COAP_DEFAULT_BLOCK_SIZE})
+endif()


The Wakaama code will neither compile nor work when LWM2M_COAP_DEFAULT_BLOCK_SIZE is zero. Therefore, this if should not exist.

Suggested change

if (LWM2M_COAP_DEFAULT_BLOCK_SIZE)

add_compile_definitions(LWM2M_COAP_DEFAULT_BLOCK_SIZE=${LWM2M_COAP_DEFAULT_BLOCK_SIZE})

endif()

add_compile_definitions(LWM2M_COAP_DEFAULT_BLOCK_SIZE=${LWM2M_COAP_DEFAULT_BLOCK_SIZE})

core/packet.c

qleisan · 2021-05-10T12:18:56Z

Once this PR is merged (with the modified default blocksize) other PRs related to integration tests can me merged

This patch makes the CoAP block size, used in block transfers, configurable in two ways. 1. Build-time configurable default value via a cmake variable. LWM2M_COAP_DEFAULT_BLOCK_SIZE 2. Run-time configurable via new -S command line argument. Add this new function to the interface: - lwm2m_set_coap_block_size() - lwm2m_get_coap_block_size() The new default value for the block size will now be 1024 with no further configuration. This avoids block transfers in common use-cases. Signed-off-by: Marc Lasch <marc.lasch@husqvarnagroup.com>

clang-format enforced changes for unkown reasons. Signed-off-by: Marc Lasch <marc.lasch@husqvarnagroup.com>

Adjust git blame ignores Signed-off-by: Marc Lasch <marc.lasch@husqvarnagroup.com>

sbertin-telular suggested changes Apr 26, 2021

View reviewed changes

rettichschnidi mentioned this pull request Apr 26, 2021

CI: Cover all defines #588

Open

mlasch force-pushed the ml/make-block-size-configurable branch from 824e7be to a08f7d7 Compare April 27, 2021 11:27

mlasch requested a review from rettichschnidi April 27, 2021 12:43

mlasch force-pushed the ml/make-block-size-configurable branch from a08f7d7 to b65fb0f Compare April 27, 2021 13:00

sbertin-telular suggested changes Apr 27, 2021

View reviewed changes

mlasch self-assigned this Apr 27, 2021

mlasch force-pushed the ml/make-block-size-configurable branch from b65fb0f to dffbfb7 Compare April 27, 2021 21:18

sbertin-telular approved these changes Apr 28, 2021

View reviewed changes

rettichschnidi requested changes Apr 29, 2021

View reviewed changes

mlasch force-pushed the ml/make-block-size-configurable branch from dffbfb7 to d92c2c4 Compare May 4, 2021 10:04

rettichschnidi reviewed May 4, 2021

View reviewed changes

mlasch force-pushed the ml/make-block-size-configurable branch from d92c2c4 to 1ca6295 Compare May 4, 2021 12:29

rettichschnidi reviewed May 4, 2021

View reviewed changes

mlasch force-pushed the ml/make-block-size-configurable branch from 1ca6295 to f93b8c1 Compare May 5, 2021 18:19

mlasch requested a review from rettichschnidi May 5, 2021 18:19

mlasch force-pushed the ml/make-block-size-configurable branch from f93b8c1 to e8d92c7 Compare May 5, 2021 18:37

rettichschnidi requested changes May 7, 2021

View reviewed changes

mlasch added 3 commits May 10, 2021 15:55

Cosmetic changes enforced by tooling

6a8a2b4

clang-format enforced changes for unkown reasons. Signed-off-by: Marc Lasch <marc.lasch@husqvarnagroup.com>

[eclipse-wakaama#601] Adjust git blame ignores

17b6ac6

Adjust git blame ignores Signed-off-by: Marc Lasch <marc.lasch@husqvarnagroup.com>

mlasch force-pushed the ml/make-block-size-configurable branch from e8d92c7 to 17b6ac6 Compare May 10, 2021 13:59

rettichschnidi approved these changes May 10, 2021

View reviewed changes

mlasch merged commit ec3bc98 into eclipse-wakaama:master May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CoAP block size configurable at build- and run-time #596

Make CoAP block size configurable at build- and run-time #596

mlasch commented Apr 26, 2021 •

edited

Loading

sbertin-telular Apr 26, 2021

mlasch Apr 27, 2021

sbertin-telular Apr 26, 2021

mlasch Apr 27, 2021 •

edited

Loading

sbertin-telular Apr 26, 2021 •

edited

Loading

mlasch Apr 27, 2021

sbertin-telular Apr 26, 2021

sbertin-telular Apr 26, 2021

sbertin-telular Apr 26, 2021

sbertin-telular Apr 26, 2021

sbertin-telular commented Apr 26, 2021

rettichschnidi commented Apr 27, 2021

sbertin-telular Apr 27, 2021

sbertin-telular Apr 27, 2021

mlasch Apr 27, 2021

mlasch commented Apr 27, 2021

sbertin-telular left a comment

rettichschnidi Apr 29, 2021

rettichschnidi Apr 29, 2021

rettichschnidi Apr 29, 2021

rettichschnidi Apr 29, 2021

rettichschnidi May 4, 2021

rettichschnidi May 4, 2021

rettichschnidi May 4, 2021

mlasch May 5, 2021

rettichschnidi May 4, 2021

rettichschnidi May 4, 2021 •

edited

Loading

rettichschnidi May 4, 2021

rettichschnidi May 4, 2021

rettichschnidi left a comment

rettichschnidi May 7, 2021

qleisan commented May 10, 2021

	* Helper functions for coap block size settings.
	* Helper functions for CoAP block size settings.

	static bool validate_block_size(uint16_t coap_block_size_arg) {
	static bool validate_block_size(const uint16_t coap_block_size_arg) {

	bool lwm2m_set_coap_block_size(uint16_t coap_block_size_arg) {
	bool lwm2m_set_coap_block_size(const uint16_t coap_block_size_arg) {

	if (1 == sscanf(argv[opt], "%hu", &coap_block_size_arg) && lwm2m_set_coap_block_size(coap_block_size_arg)) {
	if (1 == sscanf(argv[opt], "%" PRIu16, &coap_block_size_arg) && lwm2m_set_coap_block_size(coap_block_size_arg)) {

	uint16_t lwm2m_get_coap_block_size();
	uint16_t lwm2m_get_coap_block_size(void);

Make CoAP block size configurable at build- and run-time #596

Make CoAP block size configurable at build- and run-time #596

Conversation

mlasch commented Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlasch Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

sbertin-telular Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbertin-telular commented Apr 26, 2021

rettichschnidi commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlasch commented Apr 27, 2021

sbertin-telular left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rettichschnidi May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rettichschnidi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qleisan commented May 10, 2021

mlasch commented Apr 26, 2021 •

edited

Loading

mlasch Apr 27, 2021 •

edited

Loading

sbertin-telular Apr 26, 2021 •

edited

Loading

rettichschnidi May 4, 2021 •

edited

Loading