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: Create confirm request infrastructure and adapt existing messaging #7336

Merged
merged 5 commits into from
Dec 1, 2017

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Jul 9, 2017

This PR implements the second step in the confirmable messaging plan in #7192. It extends the request memo struct to support sending, and resending, a message of type CON. In addition, the existing message sending code has been adapted to use the changed struct.

This PR allows sending a request of CON type, but the message will not be resent if the response times out. The gcoap example also has been updated to specify use of the CON type, by addition of a -c option. Notice the call to coap_hdr_set_type() after initializing the PDU in gcoap_cli_cmd().

The resend implementation and documentation on sending a confirmable request are the subjects of the next PR in the series. I have started to implement it as a WIP (#7337) to verify that the present PR provides what's required.

@smlng smlng requested review from smlng and haukepetersen July 11, 2017 08:44
@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Jul 11, 2017
@aabadie aabadie modified the milestone: Release 2017.10 Jul 12, 2017
@haukepetersen
Copy link
Contributor

Just been playing a little with this PR and I think I found an issue: I am sending a CON request (POST to /.well-known/core to do a simplified registration with a RD). The server responds with an ACK., that has an empty payload. This behavior is intended, but gcoap gets caught by empty messages not handled yet...

As I understood this PR, it should actually handle empty messages (or at least empty ACK messages by now), right?

@kb2ma
Copy link
Member Author

kb2ma commented Jul 24, 2017

An empty (code 0.00) ACK indicates the server intends to send the separate response later. See Sec. 2.2 of the spec. This scenario is not included in this PR because it would require the client to retain the request memo after the initial ACK response. Handling for a separate response is the sixth item in #7192.

@haukepetersen
Copy link
Contributor

Yes, makes sense. Seems like I got something wrong when debugging, now I can't even reproduce the behavior I've seen on Monday... So never mind I guess.

@kb2ma kb2ma force-pushed the gcoap/confirm_infrastructure branch from d9ed185 to 47569a3 Compare August 17, 2017 11:42
@kb2ma
Copy link
Member Author

kb2ma commented Aug 17, 2017

Rebased.

@haukepetersen
Copy link
Contributor

Sorry for not giving this a proper review yet, hope to get to it soon.

@kb2ma
Copy link
Member Author

kb2ma commented Aug 17, 2017

Thanks, @haukepetersen. This PR is a stepping stone to the main goal of merging #7337. I plan to rebase that one in the next day or so.

@kb2ma
Copy link
Member Author

kb2ma commented Aug 17, 2017

Apologies, all. I dropped a commit in the rebase. Must redo it.

@jnohlgard
Copy link
Member

@kb2ma you can use the git reflog command to find your lost commit of you haven't manually pruned your local clone. Use git cherry-pick to reapply the commit.

@kb2ma
Copy link
Member Author

kb2ma commented Aug 18, 2017

Actually, the commits here are OK. I got confused between what belongs in this PR and what belongs in #7337.

@gebart -- thanks for the tip. My first rule of rebasing is to copy the local repository before starting. ;-)

@kaspar030
Copy link
Contributor

My first rule of rebasing is to copy the local repository before starting. ;-)

Using a temporary branch is usually much faster. git branch <whatever>_tmp; git checkout <whatever>_tmp; git rebase <other>;, and if everything works out, git checkout <whatever>; git reset --hard <whatever_tmp>; git branch -D <whatever_tmp>

@miri64
Copy link
Member

miri64 commented Aug 18, 2017

My first rule of rebasing is to copy the local repository before starting. ;-)

Why? As long as you don't git gc --purge on purpose git is storing everything you put into the index at some point. In case you mess up your rebase: git reflog (log of all previous HEAD commits) is your friend ;-)

}

if (coap_get_token_len(memo_pdu) == cmplen) {
if (cmplen) {
Copy link
Member

Choose a reason for hiding this comment

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

this can be optimised, i.e., by merging the 2 if to if (cmplen && (coap_get_token_len(memo_pdu) == cmplen)) {. However, I would rather put this into a helper function _cmp_hdr_token(token A, token B) which returns -1, 0, or 1 like typical compare functions. That would make this a one line and this whole for-loop a lot more readable (IMHO).

Copy link
Member Author

Choose a reason for hiding this comment

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

I just looked deeper into memcmp(), and I was wrong about the need to explicitly test when the comparison length is zero, as explained in this SO question.

So, the token comparison can be simplified to:

        if (coap_get_token_len(memo_pdu) == cmplen) {
            memo_pdu->token = &memo_pdu->hdr->data[0];
            if (memcmp(src_pdu->token, memo_pdu->token, cmplen) == 0) {
                *memo_ptr = memo;
                break;
            }
        }

Good enough?

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.

tested NONCON and CON message do work.

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Nov 7, 2017
@smlng
Copy link
Member

smlng commented Nov 7, 2017

please squash

Add support for a confirmable request and standardize token match.
@kb2ma kb2ma force-pushed the gcoap/confirm_infrastructure branch from 1986248 to 37f272b Compare November 10, 2017 16:19
@kb2ma
Copy link
Member Author

kb2ma commented Nov 10, 2017

Squashed and rebased.

@smlng smlng added GNRC CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 29, 2017
@smlng
Copy link
Member

smlng commented Nov 29, 2017

@haukepetersen can you spare some time and have a look too, I think we should move this and related PRs by @kb2ma forward, otherwise I'm also fine to push this on my own.

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 1, 2017
@smlng smlng merged commit 73d9c46 into RIOT-OS:master Dec 1, 2017
@kb2ma kb2ma deleted the gcoap/confirm_infrastructure branch February 4, 2019 11:27
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.

7 participants