net/nanocoap: sort Options

[kb2ma]

Contribution description

Allows the user to add CoAP Options out of option number order via the coap_opt_add_[string|buf|uint](pkt, ...) functions. Provides a flag for coap_opt_finish() to sort the options. This code for this capability is compiled conditionally by enabling a new macro, COAP_OPTIONS_SORT, which is disabled by default. This conditional approach allows the user to trade off API simplicity for code/memory space and speed.

Issues/PRs references

Implements ideas described in #9048 and #8831.

[bergzand]

@kb2ma From a high level perspective, I have two remarks:

• Would it be possible to implement the ifdef as a submodule, then gcoap and whathever module assumes this option could just depend on the submodule.
• From an API simplicity perspective, I'd rather have the sorting always done if the submodule is enabled. How large is the impact if the sorting is done on an already sorted options list?

[bergzand]

What is supposed to happen if the sort option is not enabled at compile time, but the caller requests it? Return an error code? Assertion failure?

[kb2ma]

[ 2018-06-14 3:00 UTC -- significant edit to last paragraph to refine solution ]

I was unaware of submodules, so thanks for that. Overall though, I am ambivalent about sorting in general as well as the 'always on' approach if the submodule is included.

My perspective is that the sort routine became a signifcant amount of code for an embedded implementation. However IMO, conditional use via COAP_OPTIONS_SORT makes it a useful tool for a developer if necessary. So to minimize the resource impact, the idea was to let the developer decide when to use it.

I think it will be difficult to rely solely on the submodule mechanism. The feature still needs a switch for behavior in coap_put_option() in the base module. If sorting is enabled, it must set the option delta to zero if the option is out of order. If sorting is disabled, it must trigger an assert if the option is out of order. In addition, the current implementation of the sort function is expensive to use if the list already is sorted. The function always allocates and writes the options to the scratch buffer, and then uses memcpy() to write it back to the message buffer. We could add a small function to pre-validate the order of options, but then that would need to be executed for each message.

An inclusive solution might be to use a submodule for the sort function, and then generate the COAP_OPTIONS_SORT macro from its presence. This allows us to implement the required conditional code in coap_put_option(). On top of that, define a COAP_OPT_FINISH_DEFAULTS macro with flags that always are ORed with the flags parameter in coap_opt_finish(). By default the defaults macro includes COAP_OPT_FINISH_SORT. This would satisfy your approach of automatic sorting, including the pre-validation function, just by inclusion of the submodule. On the other hand, the user could redefine the defaults macro to exclude COAP_OPT_FINISH_SORT, which would be my preferred approach.

[kb2ma]

What is supposed to happen if the sort option is not enabled at compile time, but the caller requests it?

Sounds like a programming error to me, so I'll add an assert.

[bergzand]

I think it will be difficult to rely solely on the submodule mechanism. The feature still needs a switch for behavior in coap_put_option() in the base module. If sorting is enabled, it must set the option delta to zero if the option is out of order. If sorting is disabled, it must trigger an assert if the option is out of order.

This could become something like this right?

#ifdef MODULE_NANOCOAP_SORT_OPTIONS
    unsigned delta = (lastonum <= onum) ? (onum - lastonum) : 0;
#else
     assert(lastonum <= onum);
     unsigned delta = (onum - lastonum);
#endif

the current implementation of the sort function is expensive to use if the list already is sorted. The function always allocates and writes the options to the scratch buffer, and then uses memcpy() to write it back to the message buffer.

Do we have any clue what the real world impact is here? Most CoAP packets don't include that many options, and the options are usually not that large. What I'm trying to say is that if the majority of the packets contain at most 5(Just guessing here) options, the actual impact of always sorting might be negligible.

[kb2ma]

This could become something like this right?

Yes, that was the idea.

Do we have any clue what the real world impact is here?

I hear you on premature optimization. I'll create the pre-validation function, but will not systematically call it yet in the 'automatic sorting' case.

[bergzand]

Yes, that was the idea.

Ack, just verifying that we're on the same page here.

I hear you on premature optimization. I'll create the pre-validation function, but will not systematically call it yet in the 'automatic sorting' case.

Fine with me, let's see how it works out :)

[kb2ma]

Rebased since #9085 has been merged. Starting to implement refinements discussed above.

[kb2ma]

Added fixups from @bergzand's review. As we discussed, addition of the nanocoap_options_sort submodule automatically sorts options. However, the user may redefine COAP_OPT_FINISH_DEFAULTS to change this behavior. I also started to define the module documentation, based on the linked example.

I also measured impact on code space, stack, and execution time on a SAMR21-Xpro board for my nanocoap CLI tool (see l.170), with the results below.

Within the nanocoap_options_sort submodule, _sort_options() adds approx. 250 bytes to the text section, and the coap_opt_sorted() test adds 36 bytes.

Sorting in coap_opt_finish() had no impact on the stack high water mark in the main thread of the CLI app, as reported by the ps command. Apparently, other activity when generating or sending the message used up more.

I measured execution time for this sequence:

• coap_build_hdr()
• coap_pkt_init()
• coap_opt_add_string() for Uri-Path with 2 segments
• coap_opt_add_uint(), for Content-Format
• coap_opt_finish()

Without sorting, this took 51 usec. Addition of automatic sorting increased the time to 81 usec, which is significant. In contrast, use of manual sorting guarded by coap_opt_sorted() reduced the time to 53 usec when the options already were sorted (so sorting was not executed).

Based on these results, I'm still OK with automatically sorting options by default just by activating the submodule because it is simple for the user. However, the ability to sort manually definitely is valuable. I guess it all depends on the use case. The module documentation provides enough hints to support the user.

[bergzand]

@kb2ma Awesome, thanks for those measurements! Really helps judging the impact of this PR.

I've opened a PR on your branch which rewrites this to use a pseudomodule instead of a submodule. Please let me know what you think and which solution is cleaner.

I have some time for an actual review this weekend or beginning of next week.

[kb2ma]

@bergzand, thanks for the pseudomodule-based PR. I really appreciate your guides to idiomatic RIOT constructs. :-) I prefer the original submodule version in this case though. I see the ability to sort options as clearly optional/add-on functionality. In addition, sorting essentially is contained in a function, which lends itself to implementation in a separate file. In contrast, I saw other use of pseudomodules within a single function, like in the CBOR module. This use seems more appropriate for a pseudomodule.

I think submodules also will work well for the message writing APIs, described in the example documentation. These APIs also will work well implemented as separate files. I also like use of separate files because it helps someone learning or using nanocoap to narrow down how much code they need to read. 1000 line implementation files are not friendly.

I also have been thinking more about the code/memory/time measurements above. The lesson I get is that the coap_opt_sorted() test is trivial, and it's not worth separately calling that function. So, I reimplemented this PR in a simpler way on my nanocoap/sort_options_simple branch. The user no longer needs to include COAP_OPT_FINISH_SORT; _sort_opts() always is called. _sort_opts() always tests if the options already are sorted. So, there is no more need for the concept of default flags. The result is that use of this module is super simple. Just enable the submodule, and options always are sorted in coap_opt_finish() when necessary. And with the measurements, the simplicity comes guilt free. :-)

[kb2ma]

I just updated the nanocoap/sort_options_simple branch. I moved the functions from common.c back into nanocoap.c. I realized that the work to generate submodules for the options API will move a lot of code out of nanocoap.c, and that file is the most logical place for common functions.

I plan to create a PR soon for the struct-based options API submodule. You can see the nanocoap/opt_api_module branch in my repository

[kb2ma]

Closing in favor of #9475, which essentially implements the ideas in the nanocoap/sort_options_simple branch mentioned above.