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.

Sign up for GitHub

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

Jump to bottom

coap: Update list of defines for "PATCH and FETCH Methods" and SenML #10193

Merged
merged 3 commits into from
Sep 8, 2019
Merged

coap: Update list of defines for "PATCH and FETCH Methods" and SenML #10193

merged 3 commits into from
Sep 8, 2019

Conversation

bergzand
Copy link
Member

Contribution description

Updated our list of defines with the ones from rfc8132 (Fetch, patch and ipatch) and accompanying response codes. I decided to rewrite the existing hexadecimal response codes to decimal to better match the formatting of the IANA response code list

Also added content types from rfc8428 (senml).

Decided to bundle it in a single PR because it is defines only.

Testing procedure

Test if nanocoap and/or gcoap still compiles.

Issues/PRs references

None

@bergzand bergzand added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: CoAP Area: Constrained Application Protocol implementations labels Oct 18, 2018
@bergzand bergzand changed the title coap: Update list of defines for coap: Update list of defines for "PATCH and FETCH Methods" and SenML Oct 18, 2018
@bergzand
Copy link
Member Author

Now with a sensible subject 😆

@bergzand bergzand requested a review from kb2ma October 18, 2018 10:19
kb2ma
kb2ma reviewed Oct 18, 2018
View reviewed changes
Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

Just took a quick look, and this looks straightforward and valuable. I agree that the use of decimal for response codes works better.

Give me a chance to browse the RFCs, and I'll provide a detailed review.

@kb2ma
Copy link
Member

kb2ma commented Oct 18, 2018

What to do about the nanocoap method flag macros for coap_resource_t, defined in nanocoap.h? All things being equal, I would add them in this PR like:

COAP_FETCH  (0x10)

However, this need might be a good opportunity also to rename the prefix for these macros. IMO the current naming scheme is confusing relative to the COAP_METHOD_xxx macros. (I'm thinking @haukepetersen will agree here :-)). So, the new prefix could be NANOCOAP_FLAG_xxx, for example

NANOCOAP_FLAG_FETCH  (0x10)

For existing values we could deprecate the old names and add new names, and also add the new ones for fetch and patch at the same time.

We could do this work in a separate commit in this PR, or in a separate PR for increased visibility since they are used extensively. What do you think?

kb2ma
kb2ma reviewed Oct 18, 2018
View reviewed changes
kb2ma
kb2ma requested changes Oct 18, 2018
View reviewed changes
Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

When I first saw this PR, I thought "new macros, this should be easy". Oh well, it is what it is. :-) Definitely worth a little discussion.

@bergzand
Copy link
Member Author

When I first saw this PR, I thought "new macros, this should be easy". Oh well, it is what it is. :-) Definitely worth a little discussion.

Yeah, I was hoping to easily get this merged, but your points on the name and nanocoap.h are valid.

@bergzand
Copy link
Member Author

IMO the current naming scheme is confusing relative to the COAP_METHOD_xxx macros. (I'm thinking @haukepetersen will agree here :-)). So, the new prefix could be NANOCOAP_FLAG_xxx,

I think I agree with you here.

We could do this work in a separate commit in this PR, or in a separate PR for increased visibility since they are used extensively. What do you think?

Yes please! I really don't want to piggy-back an API change here :)

@bergzand
Copy link
Member Author

@kb2ma I've added a commit to add the flags to nanocoap.h

@kb2ma
Copy link
Member

kb2ma commented Oct 24, 2018

Sounds good -- agreed on separate PR to convert COAP_xxx macros to NANOCOAP_FLAG_xxx.

kb2ma
kb2ma reviewed Oct 24, 2018
View reviewed changes
@kb2ma
Copy link
Member

kb2ma commented Oct 24, 2018 •
edited
Loading

Don't we also need to add COAP_FORMAT_xxx codes for json-patch+json and merge-patch+json defined in 8132? I have not looked closely, just noticed them in the IANA Considerations section.

@bergzand
Copy link
Member Author

bergzand commented Nov 7, 2018

Don't we also need to add COAP_FORMAT_xxx codes for json-patch+json and merge-patch+json defined in 8132? I have not looked closely, just noticed them in the IANA Considerations section.

Yeah, nothing against adding them. I didn't bother with it in the first place because we don't use really use JSON a lot with RIOT

@bergzand
Copy link
Member Author

bergzand commented Nov 7, 2018

Don't we also need to add COAP_FORMAT_xxx codes for json-patch+json and merge-patch+json defined in 8132? I have not looked closely, just noticed them in the IANA Considerations section.

Yeah, nothing against adding them. I didn't bother with it in the first place because we don't use really use JSON a lot with RIOT

After a second look, those are from a different RFC, I'll add them to the list here, but that's probably the original reason why I skipped them

kb2ma
kb2ma reviewed Nov 29, 2018
View reviewed changes
sys/include/net/coap.h Outdated
#define COAP_FORMAT_JSON (50)
#define COAP_FORMAT_JSON_PATCH_JSON (51)
#define COAP_FORMAT_MERGE_PATCH_JSON (52)
#define COAP_FORMAT_JSON (50)
Copy link
Member

Choose a reason for hiding this comment

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

Only need to define once ;-)

@kb2ma
Copy link
Member

kb2ma commented Nov 29, 2018

Except for the duplicate COAP_FORMAT_JSON, looks fine to me. Please squash with the fix, and I think we'll be good to go.

I just did some automated testing on native -- unit tests and release specs.

@kb2ma kb2ma added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Nov 29, 2018
@kb2ma kb2ma mentioned this pull request Mar 5, 2019
Merged
@bergzand bergzand force-pushed the pr/coap/include_rfc8428_rfc8132 branch 2 times, most recently from dc284a8 to 7730517 Compare March 13, 2019 09:45
@bergzand
Copy link
Member Author

@kb2ma squashed and rebased!

@kb2ma
Copy link
Member

kb2ma commented Jul 4, 2019

@bergzand ping

As soon as you fix the duplicate COAP_FORMAT_JSON, and rebase and resolve the conflicts (easy), I am happy to ACK.

@bergzand
Copy link
Member Author

As soon as you fix the duplicate COAP_FORMAT_JSON, and rebase and resolve the conflicts (easy), I am happy to ACK.

Removed the duplicate define and fixed the alignment. Took a bit too long to fix it. I'll rework the commits to a sensible set of changes when squashing.

@kb2ma
Copy link
Member

kb2ma commented Aug 25, 2019

Looks good to me. Please squash.

I added a little code to the gcoap example to test making a FETCH request, and that worked. LwM2M is on my personal roadmap, and v1.1 uses FETCH and iPATCH. Looking forward to addition of support for these methods.

kb2ma
kb2ma previously approved these changes Aug 26, 2019
View reviewed changes
@kb2ma kb2ma dismissed their stale review August 26, 2019 10:23

Conflicts still not resolved.

@kb2ma
Copy link
Member

kb2ma commented Aug 26, 2019

I was too quick to ACK due to the merge conflict. Apologies for the confusion.

@bergzand
coap: update defines for rfc8132
bf51f35 
Update the list of defines for methods and response codes with the new
codes as defined in [rfc8132](https://tools.ietf.org/html/rfc8132)
@bergzand bergzand force-pushed the pr/coap/include_rfc8428_rfc8132 branch from ca36d54 to 1721739 Compare September 5, 2019 10:57
@bergzand
Copy link
Member Author

bergzand commented Sep 5, 2019

Rebased

@bergzand bergzand force-pushed the pr/coap/include_rfc8428_rfc8132 branch from 1721739 to 7bd9e68 Compare September 5, 2019 11:01
@bergzand
Copy link
Member Author

bergzand commented Sep 5, 2019

Squashed

@bergzand
Copy link
Member Author

bergzand commented Sep 5, 2019

@kb2ma I think the set of should be good as they are now. Can you please check that I didn't mix different sets of defines in the single commit.

@kb2ma kb2ma added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 8, 2019
@kb2ma kb2ma merged commit 113dd64 into RIOT-OS:master Sep 8, 2019
@bergzand bergzand deleted the pr/coap/include_rfc8428_rfc8132 branch September 9, 2019 13:26
@bergzand
Copy link
Member Author

bergzand commented Sep 9, 2019

@kb2ma Thanks!

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kb2ma kb2ma kb2ma approved these changes

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
2 participants
@bergzand @kb2ma