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

gcoap: multi-transport support #16688

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jul 27, 2021

Contribution description

#15549 introduced DTLS support for CoAP, but did not make it possible to select the transport on request. Since switching between CoAP and CoAPS (CoAP-over-DTLS) as client is a valid use case (one might want to e.g. talk to one server over CoAP and to another over CoAPS), this change makes that possible.

This is only a draft for now.

TODOs:

  • Testing.
  • Also make listeners configurable at run-time.

Testing procedure

Untested for now.

Issues/PRs references

Follow-up on #15549

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jul 27, 2021
@miri64 miri64 added this to the Release 2021.10 milestone Jul 27, 2021
@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels Jul 27, 2021
@miri64
Copy link
Member Author

miri64 commented Jul 27, 2021

That was supposed to be a draft PR... Oh well... clicked to quickly.

@miri64
Copy link
Member Author

miri64 commented Jul 28, 2021

Sending messages using both the gcoap and gcoap_dtls example seems to work now. I can also observe the respective /cli/stats resource using aiocoap-client.

@github-actions github-actions bot added the Area: examples Area: Example Applications label Jul 28, 2021
@miri64
Copy link
Member Author

miri64 commented Jul 28, 2021

I added support for listeners now, but I am not 100% happy yet, as /.well-known/core still advertises all resources, regardless if they are available with the used transport or not as the transport type is currently unknown when the resource collection function is called here (there is however now a transport type aware version of that function):

plen += gcoap_get_resource_list(pdu->payload, (size_t)pdu->payload_len,
COAP_FORMAT_LINK);

To fix this, the only thing I can think of at the moment would require to expose the transport type to the request handler (currently the knowledge of those stops in _handle_req which calls the request handlers). I would prefer not to do that, as this would increase the number of parameters (meaning ARM and co would start using the stack instead of the general purpose registers for them) and currently this is the only use-case I can think of where this information could be useful (as usually one knows which resource they registered for which transport).

@miri64
Copy link
Member Author

miri64 commented Jul 28, 2021

I know there is draft-amsuess-core-transport-indication, so maybe with that, this becomes a non-issue. But maybe @chrysn can give some insight here.

@miri64
Copy link
Member Author

miri64 commented Jul 28, 2021

Heavily edited #16688 (comment) to provide some better context.

@miri64 miri64 added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Jul 30, 2021
@miri64
Copy link
Member Author

miri64 commented Jul 30, 2021

Input on the proposed design is welcome.

@chrysn
Copy link
Member

chrysn commented Jul 30, 2021 via email

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.

First test with a gcoap_dtls server and both a gcoap_dtls and gcoap client is successful.
Both can access resources on the server via UDP and DTLS.

With this small change

--- a/examples/gcoap/gcoap_cli.c
+++ b/examples/gcoap/gcoap_cli.c
@@ -67,6 +67,9 @@ static ssize_t _riot_board_handler(coap_pkt_t* pdu, uint8_t *buf, size_t len, vo
 /* CoAP resources. Must be sorted by path (ASCII order). */
 static const coap_resource_t _resources[] = {
     { "/cli/stats", COAP_GET | COAP_PUT, _stats_handler, NULL },
+};
+
+static const coap_resource_t _resources_secure[] = {
     { "/riot/board", COAP_GET, _riot_board_handler, NULL },
 };
 
@@ -78,7 +81,16 @@ static const char *_link_params[] = {
 static gcoap_listener_t _listener = {
     &_resources[0],
     ARRAY_SIZE(_resources),
-    GCOAP_SOCKET_TYPE_UNSPEC,
+    GCOAP_SOCKET_TYPE_UNDEF,
+    _encode_link,
+    NULL,
+    NULL
+};
+
+static gcoap_listener_t _listener_secure = {
+    &_resources_secure[0],
+    ARRAY_SIZE(_resources_secure),
+    GCOAP_SOCKET_TYPE_DTLS,
     _encode_link,
     NULL,
     NULL
@@ -491,4 +503,5 @@ void gcoap_cli_init(void)
 #endif
 
     gcoap_register_listener(&_listener);
+    gcoap_register_listener(&_listener_secure);
 }

The /riot/board resource is only available using DTLS while /cli/stats remains accessible using both UDP and DTLS.

It's unfortunately not possible for the gcoap_dtls example to make unencrypted requests, but that's a limitation of the CLI command. Would probably be better if the coap command accepted an URI instead of <addr>[%iface] <port> <path> but this is for another PR.

examples/gcoap/gcoap_cli.c Outdated Show resolved Hide resolved
@miri64 miri64 changed the title gcoap: multi-transport support for clients gcoap: multi-transport support Aug 21, 2021
@miri64
Copy link
Member Author

miri64 commented Aug 24, 2021

Sorry, I'll get to it at earliest in 10 days :-(

@chrysn ping?

@miri64
Copy link
Member Author

miri64 commented Aug 25, 2021

Ok, I now added the transport filter for .well-known/core, but I think it is a bit hacky: For the _well_known_core_handler a dynamic context object is now added (the gcoap_socket_t of the current connection), while the context behavior for all other resources stays static and is taken from the coap_resource_t struct's context pointer, so as it was before.

@benpicco
Copy link
Contributor

I now added the transport filter for .well-known/core

Why does that need special handling?

@miri64
Copy link
Member Author

miri64 commented Aug 25, 2021

I now added the transport filter for .well-known/core

Why does that need special handling?

See #16688 (comment)

@benpicco
Copy link
Contributor

benpicco commented Aug 25, 2021

I wouldn't find it too bad if all handlers could know whether they've been accessed via DTLS or plain UDP.
That way a handler could return different results based on the transport.

@benpicco
Copy link
Contributor

Another option would be to just duplicate the endpoint

/* Internal variables */
const coap_resource_t _default_resources[] = {
    { "/.well-known/core", COAP_GET, _well_known_core_handler, GCOAP_SOCKET_TYPE_UDP },
};

const coap_resource_t _default_secure_resources[] = {
    { "/.well-known/core", COAP_GET, _well_known_core_handler, GCOAP_SOCKET_TYPE_DTLS },
};

static gcoap_listener_t _default_listener = {
    &_default_resources[0],
    ARRAY_SIZE(_default_resources),
    GCOAP_SOCKET_TYPE_UDP,
    NULL,
    NULL,
    _request_matcher_default
};

static gcoap_listener_t _default_secure_listener = {
    &_default_resources[0],
    ARRAY_SIZE(_default_secure_resources),
    GCOAP_SOCKET_TYPE_DTLS,
    NULL,
    NULL,
    _request_matcher_default
};

@miri64
Copy link
Member Author

miri64 commented Aug 25, 2021

Another option would be to just duplicate the endpoint

👎 While it looks simple now, just imagine additional transports coming in the future (TCP, TCP+TLS, SLIPMUX, BLE, QUIC ...). This simply does not scale.

@miri64
Copy link
Member Author

miri64 commented Aug 26, 2021

I wouldn't find it too bad if all handlers could know whether they've been accessed via DTLS or plain UDP.
That way a handler could return different results based on the transport.

Mh with regards to e.g. block-wise and TCP transports, this could indeed make sense...

@miri64
Copy link
Member Author

miri64 commented Sep 16, 2021

Do you think we can get this into the 2021.10 release?

No time to get it out of WIP ATM + I to me it make sense to wait at least until #16855 got somewhat started so I can put the new struct member of coap_pkt_t directly into gcoap_pkt_t, so we don't have to endure a deprecation round for that as well. So I would tend to say bump (even though it is still some time for hard feature freeze).

@benpicco benpicco removed this from the Release 2021.10 milestone Sep 16, 2021
@benpicco benpicco linked an issue Sep 16, 2021 that may be closed by this pull request
@benpicco benpicco added this to the Release 2022.01 milestone Sep 22, 2021
@benpicco
Copy link
Contributor

is there anything that can be done to help with this PR?

@miri64
Copy link
Member Author

miri64 commented Dec 23, 2021

is there anything that can be done to help with this PR?

As stated in #16688 (comment), we should work on #16855. I started some of that work in #17168.

@miri64
Copy link
Member Author

miri64 commented Dec 23, 2021

is there anything that can be done to help with this PR?

Other than that, some testing application would be great!

@benpicco
Copy link
Contributor

Want to give this a rebase? Feel free to squash while you're at it.

@miri64
Copy link
Member Author

miri64 commented Mar 1, 2022

Squashed first to make rebase less of a pain.

23a8659 introduced DTLS support for
CoAP, but did not make it possible to select the transport on request.
Since switching between CoAP and CoAPS (CoAP-over-DTLS) as client is a
valid use case (one might want to e.g. talk to one server over CoAP and
to another over CoAPS), this change makes that possible.
@miri64
Copy link
Member Author

miri64 commented Mar 1, 2022

Rebased and it at least still compiles. However, with the recent rework in #17471, the gcoap_dtls now does not seem to work in current master (the client sends out unencrypted CoAP messages), and as such I was unable to test if my rebase worked. I was very careful, so I don't think I broke anything. IMHO, once we can test it (i.e. the error in allegedly introduced in #17471 is fixed), this is ready to merge: Sure we can wait until we reworked and split nanocoap and gcoap, but lets not be perfect the enemy of good enough ;-).

@miri64
Copy link
Member Author

miri64 commented Mar 2, 2022

Ok, I think I was just tired yesterday. I tested it again today, and now it works

RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

NETOPT_RX_END_IRQ not implemented by driver
main(): This is RIOT! (Version: 2022.04-devel-577-g287bfd-gcoap/enh/multitransport)
gcoap example app
All up, running the shell now
> ifconfig
ifconfig
Iface  7  HWaddr: DA:27:1D:A8:64:24 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::d827:1dff:fea8:6424  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ffa8:6424
          
> coap info
coap info
CoAP server is listening on port 5684
Connection secured with DTLS
Free DTLS session slots: 1/1
 CLI requests sent: 0
CoAP open requests: 0
Configured Proxy: None
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

NETOPT_RX_END_IRQ not implemented by driver
main(): This is RIOT! (Version: 2022.04-devel-577-g287bfd-gcoap/enh/multitransport)
gcoap example app
All up, running the shell now
> coap get fe80::d827:1dff:fea8:6424 5684 /.well-known/core
coap get fe80::d827:1dff:fea8:6424 5684 /.well-known/core
gcoap_cli: sending msg ID 61972, 23 bytes
> gcoap: response Success, code 2.05, 46 bytes
</cli/stats>;ct=0;rt="count";obs,</riot/board>

image

@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 2, 2022
@benpicco benpicco merged commit c411ede into RIOT-OS:master Mar 3, 2022
@miri64 miri64 deleted the gcoap/enh/multitransport branch March 3, 2022 14:11
@miri64
Copy link
Member Author

miri64 commented Mar 3, 2022

🎉

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Feb 21, 2023
This is an API change in the latter, which would typically now take an
extra argument GCOAP_SOCKET_TYPE_UNDEF.

Follow-Up-For: RIOT-OS#16688
chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Feb 21, 2023
This is an API change in the latter, which would typically now take an
extra argument GCOAP_SOCKET_TYPE_UNDEF.

Follow-Up-For: RIOT-OS#16688
bors bot added a commit that referenced this pull request Feb 21, 2023
18392: drivers/servo: reimplement with high level interface r=benpicco a=maribu

### Contribution description

The previous servo driver didn't provide any benefit over using PWM directly, as users controlled the servo in terms of PWM duty cycles. This changes the interface to provide a high level interface that abstracts the gory PWM details.

In addition, a SAUL layer and auto-initialization is provided.

### Testing procedure

The test application provides access to the servo driver via the `saul` shell command.

```
> saul
2022-08-02 22:12:31,826 # saul
2022-08-02 22:12:31,827 # ID	Class		Name
2022-08-02 22:12:31,830 # #0	ACT_SWITCH	LD1(green)
2022-08-02 22:12:31,832 # #1	ACT_SWITCH	LD2(blue)
2022-08-02 22:12:31,834 # #2	ACT_SWITCH	LD3(red)
2022-08-02 22:12:31,837 # #3	SENSE_BTN	B1(User button)
2022-08-02 22:12:31,838 # #4	ACT_SERVO	servo
> saul write 4 0
2022-08-02 22:12:41,443 # saul write 4 0
2022-08-02 22:12:41,445 # Writing to device #4 - servo
2022-08-02 22:12:41,447 # Data:	             0 
2022-08-02 22:12:41,450 # [servo] setting 0 to 2949 (0 / 255)
2022-08-02 22:12:41,453 # data successfully written to device #4
> saul write 4 256
2022-08-02 22:12:45,343 # saul write 4 256
2022-08-02 22:12:45,346 # Writing to device #4 - servo
2022-08-02 22:12:45,347 # Data:	           256 
2022-08-02 22:12:45,351 # [servo] setting 0 to 6865 (255 / 255)
2022-08-02 22:12:45,354 # data successfully written to device #4
```

Each write resulted in the MG90S servo that I connected to move to the corresponding position.

### Issues/PRs references

19292: sys/phydat: Fix unit confusion r=benpicco a=maribu

### Contribution description

Previously, `UNIT_G` was used for g-force with the correct symbol `g`, `UNIT_GR` for gram (as in kilogram) with the incorrect symbol `G` (which would be correct for Gauss), and `UNIT_GS` for Gauss with symbol `Gs` (which is an alternative correct symbol).

To avoid confusion between G-Force, Gauss, and Gram the units have been renamed to `UNIT_G_FORCE`, `UNIT_GRAM`, and `UNIT_GAUSS`. In addition, gram now uses the correct symbol `g`; which sadly is the same as for g-force. But usually there is enough context to tell them apart.

### Testing procedure

Green CI

### Issues/PRs references

None

19294: sys/shell: don't include suit command by default r=benpicco a=benpicco



19295: gcoap: Finish the gcoap_get_resource_list_tl -> gcoap_get_resource_list renaming r=benpicco a=chrysn

### Contribution description

In #16688, an argument was added to the `gcoap_get_resource_list` function by creating a new function `gcoap_get_resource_list_tl` with a deprecation and roll-over plan.

This plan has not been acted on so far.

This PR shortens the original plan by just adding the argument to `gcoap_get_resource_list` and removing `gcoap_get_resource_list_tl` in a single go. The rationale for this deviation is that while it's a public API, its only two practical consumers are the (built-in) well-known/core implementation, and the (built-in) CoRE Resource Directory (cord) endpoint. Moreover, a further change to this API (switching over to `coap_block_slicer_t`) is expected to happen within this release cycle, which would take something like 4 total releases to get through otherwise, which is unrealistic for an API that there are no known external users of.

A second commit clean up ToDo items (in the changed function's documentation) that referred to a IETF draft that has long been abandoned by the CoRE WG.

### Testing procedure

Plain inspection and CI passing should suffice.

### AOB

There is a second analogous pair left over from #16688, `gcoap_req_send` / `gcoap_req_send_tl`. As that *is* expected to be used widely, I prefer not to mix these two concerns, and get the present one through without unnecessary hold-up.

Co-authored-by: Marian Buschsieweke <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
Co-authored-by: chrysn <[email protected]>
bors bot added a commit that referenced this pull request Feb 21, 2023
19294: sys/shell: don't include suit command by default r=benpicco a=benpicco



19295: gcoap: Finish the gcoap_get_resource_list_tl -> gcoap_get_resource_list renaming r=benpicco a=chrysn

### Contribution description

In #16688, an argument was added to the `gcoap_get_resource_list` function by creating a new function `gcoap_get_resource_list_tl` with a deprecation and roll-over plan.

This plan has not been acted on so far.

This PR shortens the original plan by just adding the argument to `gcoap_get_resource_list` and removing `gcoap_get_resource_list_tl` in a single go. The rationale for this deviation is that while it's a public API, its only two practical consumers are the (built-in) well-known/core implementation, and the (built-in) CoRE Resource Directory (cord) endpoint. Moreover, a further change to this API (switching over to `coap_block_slicer_t`) is expected to happen within this release cycle, which would take something like 4 total releases to get through otherwise, which is unrealistic for an API that there are no known external users of.

A second commit clean up ToDo items (in the changed function's documentation) that referred to a IETF draft that has long been abandoned by the CoRE WG.

### Testing procedure

Plain inspection and CI passing should suffice.

### AOB

There is a second analogous pair left over from #16688, `gcoap_req_send` / `gcoap_req_send_tl`. As that *is* expected to be used widely, I prefer not to mix these two concerns, and get the present one through without unnecessary hold-up.

Co-authored-by: Benjamin Valentin <[email protected]>
Co-authored-by: chrysn <[email protected]>
zhaolanhuang pushed a commit to zhaolanhuang/RIOT that referenced this pull request Dec 6, 2023
This is an API change in the latter, which would typically now take an
extra argument GCOAP_SOCKET_TYPE_UNDEF.

Follow-Up-For: RIOT-OS#16688
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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gcoap_dtls: Selecting transport at run time is not possible
4 participants