nanocoap_sock: split generic request function of from nanocoap_get #9086

bergzand · 2018-05-06T12:46:08Z

Contribution description

The nanocoap_get function is refactored to split of the request part
into a separate function for reuse by other modules. Support for
retransmissions when the received frame is malformed is dropped as it
was broken anyway.

Issues/PRs references

None

kb2ma · 2018-05-06T13:45:01Z

I agree that it makes sense to separate the generic packet sending mechanism from sending/handling a simple GET.

Use of the coap_pkt_t for the nanocoap_request() meshes well with #9085, which provides a mechanism for building the packet struct. That PR will further simplify the packet building code in nanocoap_get().

I expect it's also nice to save that memmove() in the response handling if you have other plans. :-)

bergzand · 2018-05-08T09:22:14Z

I want to add the local sock_udp_ep_t endpoint to the arguments as well. Mostly because some CoAP servers use the tuple of addresses and udp ports to correlate requests. Most use cases can just leave it to NULL, but for example when sending a sequence of block2 requests, it can be useful to reuse the local endpoint.

I expect it's also nice to save that memmove() in the response handling if you have other plans. :-)

Yeah, but I don't think I can remove it without changing the current nanocoap_get api.

kb2ma · 2018-05-10T05:57:53Z

Koen, I reused your nanocoap_request() function in a nanocoap-based CLI app. See my riot-nano-cli repository. Thanks for making that separation from nanocoap_get()!

Please consider reworking how the request function determines the length of the outgoing PDU. Presently you are using the payload_len attribute of the struct, but it's really not the payload length. If you set the payload attribute in nanocoap_get() you can determine the length with the code below, assuming payload_len is set to the length of the actual payload.

(pkt->payload - (uint8_t *)pkt->hdr) + pkt->payload_len

smlng · 2018-05-24T09:58:32Z

sys/include/net/nanocoap_sock.h

+/**
+ * @brief   Simple synchronous CoAP request
+ *
+ * @param[inout]    pkt     Packet struct containing the request. Is reused for


IIRC use comma: in,out

smlng · 2018-05-24T09:59:57Z

sys/net/application_layer/nanocoap/sock.c

+
+    res = nanocoap_request(&pkt, remote, len);
+    if (res < 0)
+    {


coding style

smlng · 2018-05-24T10:07:07Z

sys/net/application_layer/nanocoap/sock.c

+        return res;
+    }
+    else {
+        res = coap_get_code(&pkt);


IMHO better use coap_get_code_class() and then compare res != COAP_CLASS_SUCCESS

oh, I see its copy paste from before and my suggestion would be an API change ... mhm

Maybe in a follow up then?

bergzand · 2018-05-24T10:51:22Z

Koen, I reused your nanocoap_request() function in a nanocoap-based CLI app. See my riot-nano-cli repository. Thanks for making that separation from nanocoap_get()!

Nice!

Please consider reworking how the request function determines the length of the outgoing PDU. Presently you are using the payload_len attribute of the struct, but it's really not the payload length. If you set the payload attribute in nanocoap_get() you can determine the length with the code below, assuming payload_len is set to the length of the actual payload.

Thanks, I wasn't sure whether I was using all the struct members correctly, thanks for clarifying. Could you please verify whether the current code is behaving correctly with respect to the struct members.

@smlng Thanks for the review, I've fixed your issues except for the API change.

kb2ma

Thanks for the improvement to the packet length calculation. I tested nanocoap_request() and nanocoap_get() in my CLI app, and they work as expected.

Funny what you learn when you test things though. ;-) I posted #9191 on some pre-existing logic errors, including the item @smlng pointed out. These should not hold up this PR though.

kb2ma

I approved too quickly though, will dismiss. Needs squash if @smlng agrees we're ready.

Need to squash first

bergzand · 2018-05-25T07:30:05Z

Forgot to change the doxygen issue, is fixed now :(

smlng

minor nit, please amend and squash directly

smlng · 2018-05-25T07:32:31Z

sys/include/net/nanocoap_sock.h

+/**
+ * @brief   Simple synchronous CoAP request
+ *
+ * @param[in,out]    pkt     Packet struct containing the request. Is reused for


indention slightly of

The nanocoap_get function is refactored to split of the request part into a separate function for reuse by other modules. Support for retransmissions when the received frame is malformed is dropped as it was broken anyway.

bergzand · 2018-05-25T07:41:13Z

amended and squashed!

smlng · 2018-05-25T08:09:16Z

@kaspar030 could you please look this over, too? Note the changed behaviour when a mal formed response is received, before this would trigger a retransmit now it returns with the appropriate error.

bergzand · 2018-06-20T07:50:54Z

@kaspar030 ping!

kaspar030

ACK.

bergzand · 2018-06-21T05:57:59Z

Thanks!

bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels May 6, 2018

bergzand requested a review from kaspar030 May 6, 2018 12:46

smlng reviewed May 24, 2018

View reviewed changes

kb2ma self-requested a review May 25, 2018 03:53

kb2ma mentioned this pull request May 25, 2018

Logic errors in nanocoap_get() #9191

Closed

kb2ma previously approved these changes May 25, 2018

View reviewed changes

kb2ma reviewed May 25, 2018

View reviewed changes

smlng suggested changes May 25, 2018

View reviewed changes

nanocoap_sock: split request function from get

6bb8a5c

The nanocoap_get function is refactored to split of the request part into a separate function for reuse by other modules. Support for retransmissions when the received frame is malformed is dropped as it was broken anyway.

bergzand force-pushed the pr/nanocoap_sock/client_split branch from 8019928 to 6bb8a5c Compare May 25, 2018 07:40

smlng approved these changes May 25, 2018

View reviewed changes

smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 25, 2018

bergzand added the Area: CoAP Area: Constrained Application Protocol implementations label May 25, 2018

kaspar030 approved these changes Jun 20, 2018

View reviewed changes

kaspar030 merged commit ddfc584 into RIOT-OS:master Jun 20, 2018

bergzand deleted the pr/nanocoap_sock/client_split branch June 21, 2018 05:58

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nanocoap_sock: split generic request function of from nanocoap_get #9086

nanocoap_sock: split generic request function of from nanocoap_get #9086

bergzand commented May 6, 2018

kb2ma commented May 6, 2018

bergzand commented May 8, 2018

kb2ma commented May 10, 2018 •

edited

Loading

smlng May 24, 2018

bergzand May 24, 2018

smlng May 24, 2018

bergzand May 24, 2018

smlng May 24, 2018

smlng May 24, 2018

bergzand May 24, 2018

bergzand commented May 24, 2018

kb2ma left a comment

kb2ma left a comment •

edited

Loading

bergzand commented May 25, 2018

smlng left a comment

smlng May 25, 2018

bergzand May 25, 2018

bergzand commented May 25, 2018

smlng commented May 25, 2018

bergzand commented Jun 20, 2018

kaspar030 left a comment

bergzand commented Jun 21, 2018

nanocoap_sock: split generic request function of from nanocoap_get #9086

nanocoap_sock: split generic request function of from nanocoap_get #9086

Conversation

bergzand commented May 6, 2018

Contribution description

Issues/PRs references

kb2ma commented May 6, 2018

bergzand commented May 8, 2018

kb2ma commented May 10, 2018 • 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

Choose a reason for hiding this comment

bergzand commented May 24, 2018

kb2ma left a comment

Choose a reason for hiding this comment

kb2ma left a comment • edited Loading

Choose a reason for hiding this comment

bergzand commented May 25, 2018

smlng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergzand commented May 25, 2018

smlng commented May 25, 2018

bergzand commented Jun 20, 2018

kaspar030 left a comment

Choose a reason for hiding this comment

bergzand commented Jun 21, 2018

kb2ma commented May 10, 2018 •

edited

Loading

kb2ma left a comment •

edited

Loading