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

nanocoap: allow to define CoAP resources as XFA #19296

Merged
merged 7 commits into from
Apr 25, 2023

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Feb 21, 2023

Contribution description

Having to define all CoAP resources in one continuous array is a bit of a hassle.
We have XFA now which is a perfect fit for this kind of thing.

The only caveat is that coap_handle_req() lives in nanocoap.c which is shared by all CoAP users.
By the nature of XFAs, we can't have them garbage collected by the linker as the linker is what brings them into existence.
So I added a new nanocoap_resources pseudo-module that needs to be selected to enable the XFA. This has the advantage that we can use the absence of this module to fall-back to the previous implementation, so existing users are not forced to change their code.

Testing procedure

examples/nanocoap_server still works:

$ aiocoap-client coap://[fe80::7837:fcff:fe7d:1aaf%tapbr0]/.well-known/core
application/link-format content was re-formatted
</sha256>,
</riot/ver>,
</riot/value>,
</riot/board>,
</echo/>,
</.well-known/core>

unmodified tests/pkg_edhoc_c also still works

$ aiocoap-client coap://[fe80::7837:fcff:fe7d:1aaf%tapbr0]/.well-known/core
application/link-format content was re-formatted
</.well-known/core>,
</.well-known/edhoc>

Issues/PRs references

@benpicco benpicco requested a review from chrysn February 21, 2023 20:44
@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 Area: tests Area: tests and testing framework labels Feb 21, 2023
@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 Feb 21, 2023
@riot-ci
Copy link

riot-ci commented Feb 21, 2023

Murdock results

✔️ PASSED

9495dc4 nanocoap_server: add nanocoap_server_auto_init

Success Failures Total Runtime
6881 0 6882 09m:37s

Artifacts

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.

lgtm. I do not like to have two competing mechanisms in place, could we deprecate the non-XFA variant of resource management eventually get rid of the old way to provide resources?

sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
examples/nanocoap_server/coap_handler.c Outdated Show resolved Hide resolved
tests/nanocoap_cli/request_handlers.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor Author

benpicco commented Feb 21, 2023

could we deprecate the non-XFA variant of resource management eventually get rid of the old way to provide resources?

Sure, this also still needs some better documentation.
We might also just name the module nanocoap_server if it is going to be the only nanoCoAP server module in the long term.

@chrysn
Copy link
Member

chrysn commented Feb 22, 2023

I haven't manage to look through this yet, but not finding it on a brief grep: Any chance this would consistently work both on gcoap and nanocoap?

@benpicco
Copy link
Contributor Author

benpicco commented Feb 22, 2023

Any chance this would consistently work both on gcoap and nanocoap?

In principle yes, since coap_resource_t is shared among both implementations.
It would require a

static gcoap_listener_t _listener = {
    .resources = coap_resources_xfa,
    .resources_len = XFA_LEN(coap_resource_t, coap_resources_xfa),
};

I added it as cc30b81

@benpicco
Copy link
Contributor Author

The last two commits are a bit more experimental, but they do work.

@benpicco
Copy link
Contributor Author

I changed the name of the module to nanocoap_resources as suggested by @miri64

@benpicco benpicco force-pushed the coap_resources_xfa branch 3 times, most recently from 0e2e879 to 6caf955 Compare March 10, 2023 00:20
@chrysn chrysn mentioned this pull request Mar 17, 2023
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.

Code looks good to me, trusting your testing

@benpicco
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Apr 25, 2023
18620: core: add core_mutex_debug to aid debugging deadlocks r=benpicco a=maribu

### Contribution description

Adding `USEMODULE += core_mutex_debug` to your `Makefile` results in
on log messages such as

    [mutex] waiting for thread 1 (pc = 0x800024d)

being added whenever `mutex_lock()` blocks. This makes tracing down
deadlocks easier.

### Testing procedure

Run e.g.

```sh
USEMODULE=core_mutex_debug BOARD=nucleo-f767zi make -C tests/mutex_cancel flash test
```

which should provide output such as

```
Welcome to pyterm!
Type '/exit' to exit.
READY
s
[mutex] waiting for thread 1 (pc = 0x8000f35)
START
main(): This is RIOT! (Version: 2022.10-devel-841-g5cc02-core/mutex/debug)
Test Application for mutex_cancel / mutex_lock_cancelable
=========================================================

Test without cancellation: OK
Test early cancellation: OK
Verify no side effects on subsequent calls: [mutex] waiting for thread 1 (pc = 0x800024d)
OK
Test late cancellation: [mutex] waiting for thread 1 (pc = 0x0)
OK
TEST PASSED
```

```sh
$ arm-none-eabi-addr2line -a 0x800024d -e tests/mutex_cancel/bin/nucleo-f767zi/tests_mutex_cancel.elf 
0x0800024d
/home/maribu/Repos/software/RIOT/tests/mutex_cancel/main.c:51
```

### Issues/PRs references

Depends on and includes #18619

19296: nanocoap: allow to define CoAP resources as XFA r=benpicco a=benpicco



19504: cpu/cc26xx_cc13xx: Fix bogus array-bound warning r=benpicco a=maribu

### Contribution description

GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for `uart == 0` and `uart == 1`, `uart` can indeed be `1`. There is an `assert(uart < UART_NUMOF)` above that would blow up prior to any out of bounds access.

In any case, optimizing out the special handling of `uart == 1` for when `UART_NUMOF == 1` likely improves the generated code and fixes the warning.

    /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds]
       88 |     ctx[uart].rx_cb = rx_cb;
          |     ~~~^~~~~~
    /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx'
       52 | static uart_isr_ctx_t ctx[UART_NUMOF];
          |                       ^~~
    /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds]
       89 |     ctx[uart].arg = arg;
          |     ~~~^~~~~~
    /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx'
       52 | static uart_isr_ctx_t ctx[UART_NUMOF];
          |                       ^~~

### Testing procedure

The actual change is a pretty obvious one-liner, so that code review and a green CI should be sufficient. If not, running any UART example app without regression should do.

### Issues/PRs references

None

19506: tools/openocd: Fix handling of OPENOCD_CMD_RESET_HALT r=maribu a=maribu

### Contribution description

The OPENOCD_CMD_RESET_HALT was not longer correctly passed to the script. This fixes the issue.

### Testing procedure

Flashing of e.g. the `cc2650-launchpad` with upstream OpenOCD should work again.

### Issues/PRs references

The change was added to #19050 after testing the PR and before merging. I'm not sure if the fix never worked because of this, or if behavior of `target-export-variables` or GNU Make changed.

19507: cpu/cc26x0_cc13x0: Drop feature cortexm_mpu r=benpicco a=maribu

### Contribution description

At least the CC2650 doesn't have an MPU, I assume this is also true for the rest of the family.

The CC2652 does have an MPU according to the datasheet. So I keep the feature there in place.

### Testing procedure

E.g.

```
make BOARD=cc2650-launchpad -C tests/mpu_noexec_ram flash test
```

fails. (Note: A successful test run would also crash but with a mem manage handler rather than a hardfault due to an invalid instruction on the stack being executed.)

It would be nice to also test the same for a `cc2652-launchpad`, for which the MPU should work.

### Issues/PRs references

None

Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>
Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
@bors
Copy link
Contributor

bors bot commented Apr 25, 2023

Build failed (retrying...):

bors bot added a commit that referenced this pull request Apr 25, 2023
18620: core: add core_mutex_debug to aid debugging deadlocks r=benpicco a=maribu

### Contribution description

Adding `USEMODULE += core_mutex_debug` to your `Makefile` results in
on log messages such as

    [mutex] waiting for thread 1 (pc = 0x800024d)

being added whenever `mutex_lock()` blocks. This makes tracing down
deadlocks easier.

### Testing procedure

Run e.g.

```sh
USEMODULE=core_mutex_debug BOARD=nucleo-f767zi make -C tests/mutex_cancel flash test
```

which should provide output such as

```
Welcome to pyterm!
Type '/exit' to exit.
READY
s
[mutex] waiting for thread 1 (pc = 0x8000f35)
START
main(): This is RIOT! (Version: 2022.10-devel-841-g5cc02-core/mutex/debug)
Test Application for mutex_cancel / mutex_lock_cancelable
=========================================================

Test without cancellation: OK
Test early cancellation: OK
Verify no side effects on subsequent calls: [mutex] waiting for thread 1 (pc = 0x800024d)
OK
Test late cancellation: [mutex] waiting for thread 1 (pc = 0x0)
OK
TEST PASSED
```

```sh
$ arm-none-eabi-addr2line -a 0x800024d -e tests/mutex_cancel/bin/nucleo-f767zi/tests_mutex_cancel.elf 
0x0800024d
/home/maribu/Repos/software/RIOT/tests/mutex_cancel/main.c:51
```

### Issues/PRs references

Depends on and includes #18619

19296: nanocoap: allow to define CoAP resources as XFA r=benpicco a=benpicco



Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>
Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
@bors
Copy link
Contributor

bors bot commented Apr 25, 2023

This PR was included in a batch that was canceled, it will be automatically retried

@maribu
Copy link
Member

maribu commented Apr 25, 2023

bors r-

@bors
Copy link
Contributor

bors bot commented Apr 25, 2023

Canceled.

@maribu
Copy link
Member

maribu commented Apr 25, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Apr 25, 2023

Build succeeded:

@bors bors bot merged commit 46af92d into RIOT-OS:master Apr 25, 2023
@benpicco benpicco deleted the coap_resources_xfa branch April 25, 2023 19:14
@benpicco
Copy link
Contributor Author

benpicco commented Apr 25, 2023

Thank you for the review!

@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
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 Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants