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

09-coap: add CoAP release specs #77

Merged
merged 1 commit into from
Jan 23, 2019
Merged

09-coap: add CoAP release specs #77

merged 1 commit into from
Jan 23, 2019

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Oct 26, 2018

Contribution description

Basic coverage for CoAP messaging as described in #73. The goal for this PR is to include non-confirmable and confirmable messaging, block extension, and observe extension. Includes some tests for both gcoap and nanocoap. Includes test of RIOT Resource Directory client implementation (CORD).

Initial commit probably premature, but I wanted to get the message out that I'm working on it for the 2018.10 release. :-)

Issues/PRs references

Resolves #73

@kb2ma kb2ma requested review from miri64 and jia200x October 26, 2018 13:10
@kb2ma
Copy link
Member Author

kb2ma commented Oct 29, 2018

Making good progress; one more task to go. I did just find a bug in nanocoap that probably should be fixed for the release, so I'll create a PR for that before continuing with the test spec.

@cladmi
Copy link
Contributor

cladmi commented Oct 29, 2018

Good to have dedicated tests using coap for the release.

A remark that shows a flaw in my previous naming, maybe my task 09 should be named 99 as I wanted it to be an optional bonus task. This way your task would be a more important task directly after the others. I can rename it.

@cladmi cladmi mentioned this pull request Oct 29, 2018
54 tasks
@jia200x
Copy link
Member

jia200x commented Oct 29, 2018

thanks @kb2ma !!

@kb2ma
Copy link
Member Author

kb2ma commented Oct 29, 2018

Sure thing @jia200x, glad to get this in. It gave me an excuse to work with aiocoap, which is worthwhile.

@cladmi, I'll follow your lead on the naming for the directory. If 09-compile... goes away, I'll rename 10-coap to take its place.

@cladmi
Copy link
Contributor

cladmi commented Oct 29, 2018

It went away :)

@kb2ma kb2ma changed the title 10-coap: WIP to add CoAP release specs 09-coap: add CoAP release specs Oct 30, 2018
@kb2ma
Copy link
Member Author

kb2ma commented Oct 30, 2018

Added Observe task, renamed to 09-coap. Squashed and ready for review. The content should be usable by others.

Also, from my perspective gcoap and nanocoap work fine in 2018.10-branch with the fix in RIOT #10298.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

These specifications are providing good tests of the basic CoAP features available in RIOT. I tried all of them with success by following the description given. Maybe adding a note on how to setup the tap (like it's done gnrc_networking example) could also help.

I found minor typos in the documentation and I also think the lines could be split to 80 characters width.

It would be great to have these CoAP release specs also tested for the next release (2019.01).

09-coap/coap.md Outdated
==============================
### Description

gcoap sends a CON GET requst to an aiocoap server. The test runner delays startup of aiocoap to trigger gcoap to resend the message.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/requst/request/ + 2 spaces between runner and delays

09-coap/coap.md Outdated
=================
### Description

aiocoap slices a payload into a sequence of Block1 encoded POSTs to nanocoap. nanocoap computes and returns the SHA-256 message digest on the payload.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/on the payload/of the payload/ ?

@kb2ma
Copy link
Member Author

kb2ma commented Jan 20, 2019

Thanks for the review. I added a couple of fixups to address your concerns. Rather than fully explain the tap setup, I included scripts to set up and teardown, and referenced them in the instructions. Let me know if that is good enough.

Also FWIW, RIOT #10671 added a feature to tests/nanocoap_cli to specify a number of requests to ignore. This will make test #02 cleaner to run. I'll provide an update after we merge the present PR.

@kb2ma kb2ma closed this Jan 20, 2019
@kb2ma
Copy link
Member Author

kb2ma commented Jan 20, 2019

Pushed the wrong button. :-)

@kb2ma kb2ma reopened this Jan 20, 2019
@kb2ma
Copy link
Member Author

kb2ma commented Jan 20, 2019

And of course I'm happy to run these tests for the release.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Thanks for updating and adding the setup script @kb2ma. This will be useful.

Please squash!

ACK

@kb2ma
Copy link
Member Author

kb2ma commented Jan 23, 2019

Squashed, looks good to me.

@aabadie
Copy link
Contributor

aabadie commented Jan 23, 2019

Thanks, let's merge!

@aabadie aabadie merged commit 12e9d32 into RIOT-OS:master Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants