Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 24, 2017

Conversation

kb2ma
Copy link
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
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
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
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
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
Member

miri64 commented Jun 23, 2017

True. @smlng, do you agree?

@smlng
Copy link
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
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
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