Skip to content

examples/gcoap: replace _parse_endpoint by sock_udp_name2ep#17870

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
HendrikVE:pr/examples/gcoap/replace_by_sock_udp_name2ep
Nov 23, 2022
Merged

examples/gcoap: replace _parse_endpoint by sock_udp_name2ep#17870
benpicco merged 1 commit intoRIOT-OS:masterfrom
HendrikVE:pr/examples/gcoap/replace_by_sock_udp_name2ep

Conversation

@HendrikVE
Copy link
Contributor

@HendrikVE HendrikVE commented Mar 26, 2022

Contribution description

Replace custom endpoint parsing function with the use of sock_udp_name2ep.
This changes the way be input the address and the port. These are now not two arguments anymore but only a single one.

Issues/PRs references

Dependencies:

@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System labels Mar 26, 2022
@HendrikVE HendrikVE changed the title Pr/examples/gcoap/replace by sock udp name2ep examples/gcoap: replace _parse_endpoint by sock_udp_name2ep Mar 26, 2022
@HendrikVE HendrikVE requested a review from benpicco March 26, 2022 21:09
@HendrikVE HendrikVE mentioned this pull request Mar 26, 2022
Merged
@HendrikVE
Copy link
Contributor Author

HendrikVE commented Mar 26, 2022

The help entry for IPv6 is a bit weird now: coap proxy set <[[IPv6 addr[%iface]]]:port>
The square bracket for the IPv6 clashes with the meaning of an optional argument. I doubled it to "escape" it.

@benpicco
Copy link
Contributor

benpicco commented Mar 26, 2022

Eh it's just a standard scheme now and it doesn't even have to be an IP address anymore.
So it's coap proxy set <host:port> with host being either a hostname, a global address or a link-local address with an interface ID - no need to explain the convention.

What I don't quite understand: Are you implying that you can set the port without the host? How would that make sense?

benpicco
benpicco reviewed Mar 26, 2022
View reviewed changes
@HendrikVE
Copy link
Contributor Author

HendrikVE commented Mar 26, 2022

it doesn't even have to be an IP address anymore

True

What I don't quite understand: Are you implying that you can set the port without the host? How would that make sense?

No I didn't mean to imply that^^ Better use coap proxy set <host>:<port> then?

@benpicco
Copy link
Contributor

benpicco commented Mar 26, 2022

If you want to be extra nice also allow to leave the port out, then fall back to use COAP_PORT - it's super annoying always having to look that up when using the gcoap example.

@HendrikVE
Copy link
Contributor Author

HendrikVE commented Mar 26, 2022

If you want to be extra nice also allow to leave the port out, then fall back to use COAP_PORT - it's super annoying always having to look that up when using the gcoap example.

That would be coap proxy set <host>[:port] then? 😄

@HendrikVE HendrikVE force-pushed the pr/examples/gcoap/replace_by_sock_udp_name2ep branch from ebfef8e to 64ee747 Compare March 26, 2022 21:58
@HendrikVE
Copy link
Contributor Author

HendrikVE commented Mar 26, 2022

Adding default ports is a useful addition. The example will now automatically set the port either to CONFIG_GCOAP_PORT or to CONFIG_GCOAPS_PORT depending if we are compiling gcoap or gcoap_dtls.

benpicco
benpicco reviewed Mar 26, 2022
View reviewed changes
HendrikVE
HendrikVE commented Mar 26, 2022
View reviewed changes
examples/gcoap/client.c
return _print_usage(argv);
}

if (tmp_remote.port == 0) {
Copy link
Contributor Author

@HendrikVE HendrikVE Mar 26, 2022

Choose a reason for hiding this comment

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

Here it would also be possible to remove the port completely from the string.

@HendrikVE HendrikVE force-pushed the pr/examples/gcoap/replace_by_sock_udp_name2ep branch from 64ee747 to 4c19acf Compare March 26, 2022 23:31
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 27, 2022
@benpicco benpicco requested a review from chrysn March 27, 2022 20:45
benpicco
benpicco reviewed Mar 27, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

That's much nicer to use 👍
But README.md now needs an update too.

@HendrikVE HendrikVE force-pushed the pr/examples/gcoap/replace_by_sock_udp_name2ep branch from 4c19acf to 63a55be Compare March 29, 2022 15:01
@HendrikVE HendrikVE requested a review from jia200x as a code owner March 29, 2022 15:01
@github-actions github-actions bot added the Area: doc Area: Documentation label Mar 29, 2022
@HendrikVE HendrikVE force-pushed the pr/examples/gcoap/replace_by_sock_udp_name2ep branch from 63a55be to 9a51e2d Compare March 29, 2022 15:04
@HendrikVE
Copy link
Contributor Author

HendrikVE commented Mar 29, 2022

But README.md now needs an update too.

I updated the README to include all three possible query types

@miri64
Copy link
Member

miri64 commented Mar 29, 2022

Should we maybe rather go with a URI here? This way we can also use CoAP and CoAPS as client at the same time ;-).

@HendrikVE
Copy link
Contributor Author

HendrikVE commented Mar 30, 2022

Unfortunately the gcoap examples are not yet adapted to #16688. They use gcoap_req_send and rely on the fact that GCOAP_SOCKET_TYPE_UNDEF will be used as GCOAP_SOCKET_TYPE_DTLS when MODULE_GCOAP_DTLS is included, so this would need to be done first before we can dynamically handle different schemes.
Would be nice feature, because that way we could also call http:// and https:// endpoints via proxies with the examples. Should we require the scheme then? Could get annoying really fast, but we could also assume coap:// for gcoap coaps:// and gcoap_dtls as defaults, similiar to the ports. uri_parser would be a good choice then. sock_urlsplit does not give us the scheme back (yet). So, we could use uri_parser to split the input into its parts and then we input a part of the URI (host, netif and port) to sock_udp_name2ep to get our sock_udp_ep_t.
I'd say all of that should be postponed to a follow-up PR.

@HendrikVE HendrikVE force-pushed the pr/examples/gcoap/replace_by_sock_udp_name2ep branch from 9a51e2d to 4339f06 Compare March 30, 2022 13:59
@stale
Copy link

stale bot commented Nov 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 2, 2022
@HendrikVE HendrikVE added State: don't stale State: Tell state-bot to ignore this issue and removed State: stale State: The issue / PR has no activity for >185 days labels Nov 2, 2022
@benpicco
Copy link
Contributor

benpicco commented Nov 2, 2022

This needs a rebase

@HendrikVE HendrikVE force-pushed the pr/examples/gcoap/replace_by_sock_udp_name2ep branch from 4339f06 to a673d50 Compare November 14, 2022 16:16
@github-actions github-actions bot removed Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System Area: network Area: Networking labels Nov 14, 2022
@riot-ci
Copy link

riot-ci commented Nov 14, 2022 •
edited
Loading

Murdock results

✔️ PASSED

a673d50 examples/gcoap: replace _parse_endpoint by sock_udp_name2ep

Success Failures Total Runtime
958 0 958 02m:41s

Artifacts

@HendrikVE
Copy link
Contributor Author

HendrikVE commented Nov 14, 2022

I'd love to fix whatever is wrong with this PR, but the new CI does not show me any output :(

@kaspar030 kaspar030 removed the State: don't stale State: Tell state-bot to ignore this issue label Nov 15, 2022
@kaspar030
Copy link
Contributor

kaspar030 commented Nov 15, 2022

I'd love to fix whatever is wrong with this PR, but the new CI does not show me any output :(

working on it. I think new murdock stumbled over the tick in that "don't stale" label.

@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: don't stale State: Tell state-bot to ignore this issue and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: don't stale State: Tell state-bot to ignore this issue labels Nov 15, 2022
benpicco
benpicco approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks much cleaner now.

@benpicco benpicco merged commit ab342d0 into RIOT-OS:master Nov 23, 2022
@HendrikVE
Copy link
Contributor Author

HendrikVE commented Nov 24, 2022

Thanks for the review!

@HendrikVE HendrikVE deleted the pr/examples/gcoap/replace_by_sock_udp_name2ep branch November 24, 2022 20:58
@miri64
Copy link
Member

miri64 commented Dec 5, 2022

This might have broken release tests: https://github.com/RIOT-OS/RIOT/actions/runs/3606686090/jobs/6078208407#step:14:443

@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@miri64 miri64 Awaiting requested review from miri64

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@cgundogan cgundogan Awaiting requested review from cgundogan

@chrysn chrysn Awaiting requested review from chrysn

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

Assignees

No one assigned

Labels

Area: doc Area: Documentation Area: examples Area: Example Applications CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2023.01

Development

Successfully merging this pull request may close these issues.

6 participants

@HendrikVE @benpicco @miri64 @riot-ci @kaspar030 @aabadie