[riot-notifications] [RIOT-OS/RIOT] nanocoap: Allow accessing opaque and other written options (#11437)

chrysn notifications at github.com
Tue May 14 20:45:49 CEST 2019


Thanks for your input -- had I read it earlier I wouldn't have stumbled over the trouble with reading options twice myself just now.

I've implemented a `coap_opt_get_next` roughly as you suggested. There are some areas of public facing interface I'm not yet happy with:

* Checking whether the result is valid requires looking into a *pointer passed rather than inspecting the return value, this feels counter-intuitive. (It might make sense to return the start and set *length if start != NULL).
* The struct needs to be zero-initialized by the caller, and offset being zero is explicitly checked and set to the "right" zero (the offset of after the token). I think it would make sense to have a `coap_opt_iter_init(&pkt)` function that does that initialization right. That would return a struct, which C allows but is often discouraged -- would that be OK here? (I expect that initialization to be inlined all the time).
* Without using the Packet API, the "reading options" use case is a bit weaker as we're always iterating over the whole thing. If we have `coap_opt_iter_init`, there could also be a `coap_opt_iter_init_with_optno` that requires the packet to follow the Packet API and uses the options array to find a proper initialization value quickly.

(I'm not sure about whether the implementation is the best there can be, but that's for later review)

> I agree on the usefulness for the ETag example, although it would only work with the gcoap API when writing a message because it requires a coap_pkt_t.

I could use a little more explanation here, I both Packet and Buffer API were part of nanocoap (which is only utilized by gcoap) and both use coap_pkt_t, will that not stay so?

> Example error handling -- Is it useful to reply on error? Why not just print an error message?

The "error" label on the goto was probably a misnomer; it's not so much an error reply but more a "indicate that the result was truncated and continue" way out of options that are too long to display in the buffer. (In general I think it makes sense to provide good error responses via CoAP -- whatever that thing will be tested on, I don't know whether it'll have stdio, but it sure will have network, and whoever requested the result will have an easier time correlating the error response with the request than interpreting stderr).

New commits to the PR are coming in, rebasing to master may need some more time unless it's trivial. The changes will need squashing as they'll make little sense like that in the history, does that warrant a "WIP" note in the title?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11437#issuecomment-492361252
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190514/14a9deb4/attachment.html>


More information about the notifications mailing list