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.

Sign up for GitHub

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

Jump to bottom

pkg/tinydtls: allow to set buffer size from application again #19892

Merged
merged 3 commits into from
Aug 24, 2023
Merged

pkg/tinydtls: allow to set buffer size from application again #19892

merged 3 commits into from
Aug 24, 2023

Conversation

leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented Aug 21, 2023 •
edited
Loading

Contribution description

Currently the buffer size on tinydtls is set in its Makefile whenever gcoap module is present. This limits the ability of the user to override the value. This adds a pre-check of the CFLAGS to see if it was set before.

Testing procedure

Try setting CFLAGS += -DDTLS_MAX_BUF=<some_value> DTLS_MAX_BUF=<some_value> on examples/gcoap_dtls, you should be able to override the default value without errors.

Issues/PRs references

Reported in #19838

@leandrolanzieri leandrolanzieri added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: pkg Area: External package ports labels Aug 21, 2023
@leandrolanzieri leandrolanzieri mentioned this pull request Aug 21, 2023
Open
benpicco
benpicco reviewed Aug 21, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This feels a bit hacky.
If applications currently don't (can't) set DTLS_MAX_BUF directly (and we didn't document that they should, so how would they even find out) we could instead add a variable:

ifneq (,$(filter gcoap,$(USEMODULE)))
  DTLS_MAX_BUF ?= $(CONFIG_GCOAP_PDU_BUF_SIZE) + 36)
else
  DTLS_MAX_BUF ?= 200
endif
CFLAGS += "-DDTLS_MAX_BUF=$(DTLS_MAX_BUF)"

and document that applications can just set the DTLS_MAX_BUF variable (or maybe chose a different name to avoid confusion with the CFLAG).

P.S.: I think this should be set to CONFIG_NANOCOAP_BLOCKSIZE_DEFAULT when nanocoap_dtls is used.

@leandrolanzieri leandrolanzieri force-pushed the pr/pkg/tinydtls_buffer_cflag branch from 5e95c9b to 0e8ae62 Compare August 21, 2023 11:43
@github-actions github-actions bot added Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Aug 21, 2023
@leandrolanzieri leandrolanzieri force-pushed the pr/pkg/tinydtls_buffer_cflag branch from 0e8ae62 to d721630 Compare August 21, 2023 11:45
benpicco
benpicco approved these changes Aug 21, 2023
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.

Please squash

@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 Aug 21, 2023
@leandrolanzieri leandrolanzieri force-pushed the pr/pkg/tinydtls_buffer_cflag branch from 5d58657 to e65f1c4 Compare August 21, 2023 15:21
@riot-ci
Copy link

riot-ci commented Aug 21, 2023 •
edited
Loading

Murdock results

✔️ PASSED

e65f1c4 sys/net/gcoap: configure DTLS buffer

Success Failures Total Runtime
7939 0 7939 13m:54s

Artifacts

@leandrolanzieri
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Aug 22, 2023
@bors @leandrolanzieri
Merge #19892
16ddac6 
19892: pkg/tinydtls: allow to set buffer size from application again r=leandrolanzieri a=leandrolanzieri

### Contribution description

Currently the buffer size on tinydtls is set in its Makefile whenever `gcoap` module is present. This limits the ability of the user to override the value. This adds a pre-check of the `CFLAGS` to see if it was set before.

### Testing procedure

Try setting `CFLAGS += -DDTLS_MAX_BUF=<some_value>` on `examples/gcoap_dtls`, you should be able to override the default value without errors.


### Issues/PRs references
Reported in #19838


Co-authored-by: Leandro Lanzieri <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 22, 2023

Build failed:

@benpicco
Copy link
Contributor

bors merge

bors bot added a commit that referenced this pull request Aug 23, 2023
@bors @gschorcht @leandrolanzieri
Merge #19886 #19892
35e69c6 
19886: cpu/efm32: fix DAC configuration r=benpicco a=gschorcht

### Contribution description

The EFM32 MCU allows the reference voltage to be configured per DAC device, not per DAC channel. Also, the DAC reference voltage was defined in the configuration but not used anywhere.

At the moment we have only defined one board (`stwstk6220a`) that uses the DAC, so changing the configuration interface shouldn't be critical.

### Testing procedure

`tests/periph/dac` should still work for the `stwstk6220a`
```
BOARD=slwstk6220a make -j8 -C tests/periph/dac flash
```
I don't have a `stwstk6220a` board (EFM32 Series 0) so that I can't test it. I could only test it for the `sltb009a` board (EFM32 Series 1) with the change for VDAC in PR #19887.

### Issues/PRs references


19892: pkg/tinydtls: allow to set buffer size from application again r=benpicco a=leandrolanzieri

### Contribution description

Currently the buffer size on tinydtls is set in its Makefile whenever `gcoap` module is present. This limits the ability of the user to override the value. This adds a pre-check of the `CFLAGS` to see if it was set before.

### Testing procedure

Try setting `CFLAGS += -DDTLS_MAX_BUF=<some_value>` on `examples/gcoap_dtls`, you should be able to override the default value without errors.


### Issues/PRs references
Reported in #19838


Co-authored-by: Gunar Schorcht <[email protected]>
Co-authored-by: Leandro Lanzieri <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 23, 2023

Build failed (retrying...):

bors bot added a commit that referenced this pull request Aug 23, 2023
@bors @leandrolanzieri
Merge #19892
2c9b100 
19892: pkg/tinydtls: allow to set buffer size from application again r=benpicco a=leandrolanzieri

### Contribution description

Currently the buffer size on tinydtls is set in its Makefile whenever `gcoap` module is present. This limits the ability of the user to override the value. This adds a pre-check of the `CFLAGS` to see if it was set before.

### Testing procedure

Try setting `CFLAGS += -DDTLS_MAX_BUF=<some_value>` on `examples/gcoap_dtls`, you should be able to override the default value without errors.


### Issues/PRs references
Reported in #19838


Co-authored-by: Leandro Lanzieri <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 23, 2023

Build failed:

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 24, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit d1edbd9 into RIOT-OS:master Aug 24, 2023
@leandrolanzieri leandrolanzieri deleted the pr/pkg/tinydtls_buffer_cflag branch October 30, 2023 13:20
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.10 milestone Nov 17, 2023
@benpicco
Copy link
Contributor

arg, this breaks examples/gcoap_dtls <-> tests/net/nanocoap_cli:

> ncget coaps://[fe80::581a:98ff:fe23:2d6c]/.well-known/core -
cannot send ClientHello
cannot send ClientHello
cannot send ClientHello

benpicco
benpicco reviewed Feb 26, 2024
View reviewed changes
sys/net/application_layer/nanocoap/Makefile.include
@@ -0,0 +1,4 @@
ifneq (,$(filter nanocoap_dtls,$(USEMODULE)))
CONFIG_NANOCOAP_BLOCKSIZE_DEFAULT := $(or $(CONFIG_NANOCOAP_BLOCKSIZE_DEFAULT),2)
DTLS_MAX_BUF ?= ((1 << ($(CONFIG_NANOCOAP_BLOCKSIZE_DEFAULT) + 3)) + 36)
Copy link
Contributor

Choose a reason for hiding this comment

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

The handshake buffer size does not depend on the block size

@benpicco benpicco mentioned this pull request Feb 26, 2024
Merged
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

@chrysn chrysn Awaiting requested review from chrysn

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

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2023.10
Development

Successfully merging this pull request may close these issues.
4 participants
@leandrolanzieri @riot-ci @benpicco @MrKevinWeiss