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

CoAP: add remote and local endpoint data to coap_pkt_t #16827

Closed

Conversation

fabian18
Copy link
Contributor

@fabian18 fabian18 commented Sep 9, 2021

Contribution description

This is an alternative attempt to expose local and remote IP addresses and ports to the CoAP request/response handlers. The CoAP PDU now contains pointers to local and remote endpoint data.

Testing procedure

examples/gcoap:

> coap get fe80::4c49:6fff:fe54:5c%6 5683 /.well-known/core
2021-09-09 09:42:24,179 # gcoap_cli: sending msg ID 10865, 23 bytes
2021-09-09 09:42:24,183 # gcoap_remote [fe80::4c49:6fff:fe54:5c%6]:5683
2021-09-09 09:42:24,186 # local [fe80::4c49:6fff:fe54:66]:5683
2021-09-09 09:42:24,190 # gcoap: response Success, code 2.05, 46 bytes
2021-09-09 09:42:24,194 # </cli/stats>;ct=0;rt="count";obs,</riot/board>

> coap get fe80::4c49:6fff:fe54:66%6 5683 /.well-known/core
2021-09-09 09:42:25,593 # gcoap_cli: sending msg ID 35428, 23 bytes
2021-09-09 09:42:25,597 # gcoap_remote [fe80::4c49:6fff:fe54:66%6]:5683
2021-09-09 09:42:25,601 # local [fe80::4c49:6fff:fe54:5c]:5683
2021-09-09 09:42:25,605 # gcoap: response Success, code 2.05, 46 bytes
2021-09-09 09:42:25,609 # </cli/stats>;ct=0;rt="count";obs,</riot/board>

Issues/PRs references

Replaces #16429
Trying to solve #15686

@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 Sep 9, 2021
@fabian18 fabian18 force-pushed the coap_add_remote_and_local_ep_to_pdu branch from 69e14d7 to 65877a2 Compare September 9, 2021 07:58
@benpicco benpicco requested a review from chrysn September 9, 2021 08:07
@@ -91,7 +90,7 @@ static void _on_register(const gcoap_request_memo_t *memo, coap_pkt_t* pdu,
/* read the location header and save the RD details on success */
if (coap_get_location_path(pdu, (uint8_t *)_rd_loc,
sizeof(_rd_loc)) > 0) {
memcpy(&_rd_remote, remote, sizeof(_rd_remote));
memcpy(&_rd_remote, pdu->remote, sizeof(_rd_remote));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to have a dedicated function for this. This function could assert() that the endpoint indeed of a UDP flavor, once multiple protocols are supported.

@bergzand
Copy link
Member

bergzand commented Sep 9, 2021

At a first glance this looks similar to #13621. Did things change enough in the mean time to reconsider adding this?

@maribu
Copy link
Member

maribu commented Sep 9, 2021

At a first glance this looks similar to #13621. Did things change enough in the mean time to reconsider adding this?

That was closed in favor of another PR that was closed in the meantime. However, the feature was added by exposing the remote addr as an additional argument. The same could be done here, which is what @fabian18 first did - but that is an API change (PR still open).

I take it that you prefer that version?

@miri64
Copy link
Member

miri64 commented Sep 9, 2021

I think we should at least wait for the outcome of tomorrow's CoAP API breakout session at the summit, before we can conclude on this definitively.

@bergzand
Copy link
Member

bergzand commented Sep 9, 2021

I take it that you prefer that version?

I'm just passing by and noticed similarities, I haven't looked good enough at all the options to have a good opinion or preference.

@miri64
Copy link
Member

miri64 commented Sep 9, 2021

(see https://hackmd.io/o_y0nKfGRSqrkg-YTCf97g for some initial notes)

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Blocking until breakout session during the summit concludes

@miri64
Copy link
Member

miri64 commented Sep 10, 2021

Blocking until breakout session during the summit concludes

I think the conclusion was that longterm we move to another API, but short-term we move out the gcoap members out of nanocoap into a new gcoap_pkt_t struct. IMHO, before we worsen things for nanocoap, this should be done first, but I don't hard-NACK the approach of merging this first and cleaning up later.

@stale
Copy link

stale bot commented Apr 16, 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 Apr 16, 2022
@stale stale bot closed this Jun 12, 2022
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 Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System State: stale State: The issue / PR has no activity for >185 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants