Skip to content

net/gcoap: Provide piggybacked ACK response to confirmable request#7223

Merged
aabadie merged 3 commits intoRIOT-OS:masterfrom
kb2ma:gcoap/confirm_piggy_server
Jun 24, 2017
Merged

net/gcoap: Provide piggybacked ACK response to confirmable request#7223
aabadie merged 3 commits intoRIOT-OS:masterfrom
kb2ma:gcoap/confirm_piggy_server

Conversation

@kb2ma
Copy link
Copy Markdown
Member

@kb2ma kb2ma commented Jun 22, 2017

Implements the first task of #7192 for confirmable messaging. Also updated documentation and added a couple of unit tests.

@smlng smlng self-requested a review June 22, 2017 08:12
@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation GNRC Area: network Area: Networking labels Jun 22, 2017
size_t pdu_len = _handle_req(&pdu, buf, sizeof(buf), &remote);
if (pdu_len > 0) {
sock_udp_send(sock, buf, pdu_len, &remote);
if (coap_get_type(&pdu) == COAP_TYPE_NON
Copy link
Copy Markdown
Member

@smlng smlng Jun 22, 2017

Choose a reason for hiding this comment

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

you could use a switch-case over the possible coap types here, something like

switch(coap_get_type(&pdu)) {
case COAP_TYPE_NON:
case COAP_TYPE_CON:
    size_t pdu_len = _handle_req(&pdu, buf, sizeof(buf), &remote);
    if (pdu_len > 0) {
        sock_udp_send(sock, buf, pdu_len, &remote);
    }
    break;
case COAP_TYPE_RST:
case COAP_TYPE_ACK:
    DEBUG("gcoap: RST and ACK handler not implemented yet!\n");
    break;
default:
    DEBUG("gcoap: illegal request type: %u\n", coap_get_type(&pdu));
    return;
}

[edit] fixed indention above

@kb2ma
Copy link
Copy Markdown
Member Author

kb2ma commented Jun 23, 2017

@smlng, thanks for the quick review! I appreciate your debug output to indicate a message can't be handled yet. It made me realize that it wasn't right to say an empty message (code 0.00) is 'illegal'. So, I called that scenario out first, which means that a request code class message must be either NON or CON type. For reference see Table 1 at the end of sec. 4.3 in the spec.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 23, 2017

@kb2ma what about the switch-case proposed by @smlng? Other than that: tested on native, samr21-xpro and iotlab-m3.

@kb2ma
Copy link
Copy Markdown
Member Author

kb2ma commented Jun 23, 2017

@smlng's code is within the handling for a request-class code (GET, POST, etc). Since RST and ACK types are not valid for a request-class code, there are only two cases -- NON/CON and illegal. A case statement seemed like overkill for only two cases. Am I missing something?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 23, 2017

True. @smlng, do you agree?

@smlng
Copy link
Copy Markdown
Member

smlng commented Jun 24, 2017

yes, agreed - RFCs allows NON and CON as requests only. And at least with the current handling of CoAP message types, an if-else is fine.

Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

ACK!

@smlng smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 24, 2017
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 24, 2017

Let's go here.

@aabadie aabadie merged commit e71bf07 into RIOT-OS:master Jun 24, 2017
@aabadie aabadie modified the milestone: Release 2017.07 Jun 26, 2017
@kb2ma kb2ma mentioned this pull request Jul 10, 2017
6 tasks
@kb2ma kb2ma deleted the gcoap/confirm_piggy_server branch February 4, 2019 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants