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/coap: Packet API function to add Uri-Query option #13213

Merged
merged 3 commits into from
Jan 29, 2020

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Jan 27, 2020

Contribution description

#9309 is a tracking issue for the migration of CoAP Option functionality to the nanocoap Packet API. The last remaining function for the migration is gcoap_add_qstring(). This PR adds coap_opt_add_uquery() to nanocoap, renamed to be more descriptive and conform to the Packet API naming convention. We plan to create a follow-on PR to migrate existing uses and deprecate gcoap_add_qstring().

Testing procedure

Build the documentation to ensure the new function appears as part of the Options Write Packet API section of the nanocoap documentation. Also run the gcoap unit tests.

Issues/PRs references

Part of #9309.

@kb2ma kb2ma added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Jan 27, 2020
@kb2ma kb2ma added this to the Release 2020.04 milestone Jan 27, 2020
@benpicco benpicco 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 Jan 27, 2020
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

It looks good. The unit tests still pass. Just a small typo. Also, the prefix on the second commit is a bit misleading, as you are using the new query function in the unit test and not in gcoap itself.

I think it also would be good to modify nanocoap's query unit tests to use this function now.

Unit test result
> make tests-gcoap test
Building application "tests_unittests" for "native" with MCU "native".

"make" -C /home/leandro/Work/RIOT/boards/native
"make" -C /home/leandro/Work/RIOT/boards/native/drivers
"make" -C /home/leandro/Work/RIOT/core
"make" -C /home/leandro/Work/RIOT/cpu/native
"make" -C /home/leandro/Work/RIOT/cpu/native/periph
"make" -C /home/leandro/Work/RIOT/cpu/native/stdio_native
"make" -C /home/leandro/Work/RIOT/cpu/native/vfs
"make" -C /home/leandro/Work/RIOT/drivers
"make" -C /home/leandro/Work/RIOT/drivers/periph_common
"make" -C /home/leandro/Work/RIOT/sys
"make" -C /home/leandro/Work/RIOT/sys/div
"make" -C /home/leandro/Work/RIOT/sys/embunit
"make" -C /home/leandro/Work/RIOT/sys/evtimer
"make" -C /home/leandro/Work/RIOT/sys/fmt
"make" -C /home/leandro/Work/RIOT/sys/luid
"make" -C /home/leandro/Work/RIOT/sys/net/application_layer/gcoap
"make" -C /home/leandro/Work/RIOT/sys/net/application_layer/nanocoap
"make" -C /home/leandro/Work/RIOT/sys/net/crosslayer/inet_csum
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/netapi
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/netif
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/netif/hdr
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/netreg
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/network_layer/icmpv6
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/network_layer/ipv6
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/network_layer/ipv6/hdr
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/network_layer/ipv6/nib
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/network_layer/ndp
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/pkt
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/pktbuf
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/pktbuf_static
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/sock
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/sock/udp
"make" -C /home/leandro/Work/RIOT/sys/net/gnrc/transport_layer/udp
"make" -C /home/leandro/Work/RIOT/sys/net/link_layer/l2util
"make" -C /home/leandro/Work/RIOT/sys/net/netif
"make" -C /home/leandro/Work/RIOT/sys/net/network_layer/icmpv6
"make" -C /home/leandro/Work/RIOT/sys/net/network_layer/ipv6/addr
"make" -C /home/leandro/Work/RIOT/sys/net/network_layer/ipv6/hdr
"make" -C /home/leandro/Work/RIOT/sys/net/sock
"make" -C /home/leandro/Work/RIOT/sys/net/transport_layer/udp
"make" -C /home/leandro/Work/RIOT/sys/posix/inet
"make" -C /home/leandro/Work/RIOT/sys/random
"make" -C /home/leandro/Work/RIOT/sys/random/tinymt32
"make" -C /home/leandro/Work/RIOT/sys/test_utils/interactive_sync
"make" -C /home/leandro/Work/RIOT/sys/xtimer
"make" -C /home/leandro/Work/RIOT/tests/unittests/tests-gcoap
   text	  data	   bss	   dec	   hex	filename
  54727	  1284	 47840	103851	 195ab	/home/leandro/Work/RIOT/tests/unittests/bin/native/tests_unittests.elf
r
/home/leandro/Work/RIOT/tests/unittests/bin/native/tests_unittests.elf  
RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2020.04-devel-110-g8ccc4-coap/add_uquery_opt)
Help: Press s to start test, r to print it is ready
READY
s
START
...........
OK (11 tests)

tests/unittests/tests-gcoap/tests-gcoap.c Outdated Show resolved Hide resolved
@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jan 28, 2020
@kb2ma
Copy link
Member Author

kb2ma commented Jan 28, 2020

Also, the prefix on the second commit is a bit misleading, as you are using the new query function in the unit test and not in gcoap itself.

So, change the second commit to this?

net/coap: use new function to add query option

I think it also would be good to modify nanocoap's query unit tests to use this function now.

I like test_nanocoap__get_query() as is because it's the only test with an option string that does not include the delimiter. How about if I replace the contents of test_nanocoap__get_multi_query() with
test_gcoap__client_get_query(), and drop that test in gcoap? [edit: Might rework the second commit above based on suggestion.]

In general as a result of #9309 there's a larger issue of what tests belong in nanocoap vs. gcoap. nanocoap also has become so large that its tests can be sub-divided along functional lines in the future.

@leandrolanzieri
Copy link
Contributor

How about if I replace the contents of test_nanocoap__get_multi_query() with
test_gcoap__client_get_query(), and drop that test in gcoap? [edit: Might rework the second commit above based on suggestion.]

I agree, let's do it this way, as test_gcoap__client_get_query() is now testing mostly nanocoap functionalities.

@kb2ma
Copy link
Member Author

kb2ma commented Jan 29, 2020

Pushed test fixups as discussed. There are four commits now. If it looks good, I will squash down to these three:

  1. net/coap: move/rename function to add query option
  2. net/nanocoap: use new function in query test
  3. net/gcoap: remove test migrated to nanocoap

Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Changes look good. Unit tests for nanocoap and gcoap still pass. ACK.

@kb2ma please squash down to the commits you mentioned.

@kb2ma kb2ma force-pushed the coap/add_uquery_opt branch from 82563d8 to bd73f12 Compare January 29, 2020 14:16
@kb2ma kb2ma force-pushed the coap/add_uquery_opt branch from bd73f12 to b354722 Compare January 29, 2020 14:28
@kb2ma
Copy link
Member Author

kb2ma commented Jan 29, 2020

Squashed and rebased due to recent changes for Kconfig.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants