pkg/tinydtls: migrate to ztimer64_msec by fjmolinas · Pull Request #17564 · RIOT-OS/RIOT

fjmolinas · 2022-01-25T07:52:42Z

Contribution description

tinydtls is usins some xtimer externals which prevent a drop in replacement with ztimer_xtimer_compat, so this PR migrate it to use ztimer64_msec instead of xtimer.

The patch would be upstreamed later, but I think it makes more sense to get it reviewed here first.

Testing procedure

Running some dtls tests

Issues/PRs references

Split form #17365

fjmolinas · 2022-01-31T08:51:10Z

The dtls-echo test works:

> ifconfig
2022-01-31 09:49:28,752 # ifconfig
2022-01-31 09:49:28,755 # Iface  5  HWaddr: 1F:2A  Channel: 26  Page: 0  NID: 0x23  PHY: O-QPSK
2022-01-31 09:49:28,756 #
2022-01-31 09:49:28,758 #           Long HWaddr: EA:5C:0D:4F:EB:A2:1F:2A
2022-01-31 09:49:28,761 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4
2022-01-31 09:49:28,768 #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  6LO
2022-01-31 09:49:28,768 #           IPHC
2022-01-31 09:49:28,769 #           Source address length: 8
2022-01-31 09:49:28,770 #           Link type: wireless
2022-01-31 09:49:28,771 #           inet6 addr: fe80::e85c:d4f:eba2:1f2a  scope: link  VAL
2022-01-31 09:49:28,772 #           inet6 group: ff02::1
2022-01-31 09:49:28,772 #
> 2022-01-31 09:50:34,043 #
2022-01-31 09:50:34,060 # Server: got DTLS Data App: --- some very important data ---   (echo!)
2022-01-31 09:50:34,076 # Received alert from client

2022-01-31 09:50:30,500 # ifconfig
2022-01-31 09:50:30,507 # Iface  5  HWaddr: 29:BA  Channel: 26  Page: 0  NID: 0x23  PHY: O-QPSK
2022-01-31 09:50:30,508 #
2022-01-31 09:50:30,513 #           Long HWaddr: 00:04:25:19:18:01:A9:BA
2022-01-31 09:50:30,520 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4
2022-01-31 09:50:30,528 #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102  MTU:1280  HL:64  6LO
2022-01-31 09:50:30,529 #           IPHC
2022-01-31 09:50:30,532 #           Source address length: 8
2022-01-31 09:50:30,534 #           Link type: wireless
2022-01-31 09:50:30,540 #           inet6 addr: fe80::204:2519:1801:a9ba  scope: link  VAL
2022-01-31 09:50:30,543 #           inet6 group: ff02::1
2022-01-31 09:50:30,544 #
dtlsc fe80::e85c:d4f:eba2:1f2a%5 "some very important data"
2022-01-31 09:50:33,956 # dtlsc fe80::e85c:d4f:eba2:1f2a%5 "some very important data"
2022-01-31 09:50:34,058 # Client: got DTLS Data App -- some very important data --

fjmolinas · 2022-01-31T08:51:32Z

I would merge this PR with the patch and then PR the patch upstream, and then in another PR remove the patch.

kfessel

at first glance this does not need ztimer64_usec
but should use ztimer_msec

kfessel · 2022-01-31T16:12:56Z

pkg/tinydtls/patches/0001-dtls_time-migrate-to-ztimer64_usec.patch

+ }
+
+ void
+ dtls_ticks(dtls_tick_t *t) {


in the upstream default configuration this provides msec ticks

https://github.com/eclipse/tinydtls/blob/706888256c3e03d9fcf1ec37bb1dd6499213be3c/dtls_time.h#L52

maybe the whole package can use ztimer msec

for contiki the dtls_tick_t is uint_32

seems like there might be no need for ztimer64
at least not for ztimer64_usec

Do you mean ztimer64_msec then? Because I'm a bit worried about the rollover with 32bit msec.

The tinydtls people are not concerned over the 32Bit msec (at least in the contiki case) i don't see time beeing part of the messages of rfc4347 and all timout are in the range of a couple of seconds -> why go 64 bit.
If tinydtls would have a problem going with 32Bit msecs this would be a bug in tinydtls which should also be fixed in interest of contiki.

https://github.com/eclipse/tinydtls/blob/706888256c3e03d9fcf1ec37bb1dd6499213be3c/dtls_time.h#L46

Shall I squash @kfessel?

@kfessel since eclipse-tinydtls/tinydtls#123 (comment) It makes me thinks in a first stage its better to leave it in ms, what about ztimer64_msec, as a first step, once questions in eclipse-tinydtls/tinydtls#123 (comment) are clarified maybe we can remove the 64bit part.

sorry for that late answer:

we can certainly go that route i think they will sort that out and i am much less against ztimer64_msec use (than zt64us)

they also will sort out eclipse-tinydtls/tinydtls#124

fjmolinas · 2022-02-01T11:40:22Z

Ok, changed to use msec

RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
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-125-ge374c-pr_tinydyls_migrate_to_ztimer)
RIOT (Tiny)DTLS testing implementation
All up, running the shell now
> dtlsc fe80::24a7:99ff:fe9f:45ae "some very important data"
dtlsc fe80::24a7:99ff:fe9f:45ae "some very important data"
Client: got DTLS Data App -- some very important data --

main(): This is RIOT! (Version: 2022.04-devel-125-ge374c-pr_tinydyls_migrate_to_ztimer)
RIOT (Tiny)DTLS testing implementation
All up, running the shell now
> dtlss start
dtlss start
> ifconfig
ifconfig
Iface  5  HWaddr: 26:A7:99:9F:45:AE
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::24a7:99ff:fe9f:45ae  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ff9f:45ae

>
Server: got DTLS Data App: --- some very important data ---     (echo!)
Received alert from client

fjmolinas · 2022-02-09T07:20:18Z

I switched to ztimer64_msec

kfessel

This is a simple direct replacement, the author ran the tests. (documented above)

fjmolinas · 2022-02-09T10:46:29Z

Re-tested, all OK!, Go!

fjmolinas · 2022-02-09T10:46:41Z

Thanks @kfessel!

benpicco · 2023-03-04T21:54:34Z

Do we really need ztimer64 here?
32 bit ztimer_msec gives us a range of 49 days which is longer than any DTLS session will ever be.

kfessel · 2023-03-06T13:03:27Z

Upstream had or has some 32 bit time-overflow problem (it is most likely fixed with eclipse-tinydtls/tinydtls#131). But upstream seems to be not sure eclipse-tinydtls/tinydtls#125 until a unit-test for the overflow exists. I am not sure if we want to go 32 Bit or wait until upstream is sure.

benpicco · 2023-03-06T13:08:06Z

But upstream doesn't even use ztimer64: dtls_time.c#L44

fjmolinas requested review from kfessel and pokgak January 25, 2022 07:52

fjmolinas requested a review from leandrolanzieri as a code owner January 25, 2022 07:52

github-actions bot added the Area: pkg Area: External package ports label Jan 25, 2022

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 31, 2022

kfessel reviewed Jan 31, 2022

View reviewed changes

fjmolinas mentioned this pull request Feb 2, 2022

dtls_time: migrate to ztimer_msec eclipse-tinydtls/tinydtls#123

Merged

pkg/tinydtls: migrate to ztimer64_msec

e6822e0

fjmolinas force-pushed the pr_tinydyls_migrate_to_ztimer branch from 43ad1c7 to e6822e0 Compare February 9, 2022 07:19

fjmolinas changed the title ~~pkg/tinydtls: migrate to ztimer64_usec~~ pkg/tinydtls: migrate to ztimer64_msec Feb 9, 2022

kfessel approved these changes Feb 9, 2022

View reviewed changes

fjmolinas merged commit 74741fb into RIOT-OS:master Feb 9, 2022

fjmolinas deleted the pr_tinydyls_migrate_to_ztimer branch February 9, 2022 10:46

fjmolinas mentioned this pull request Feb 18, 2022

pkg/tinydtls/sock_dtls: use ztimer_usec #17677

Merged

OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022

kfessel mentioned this pull request Mar 6, 2023

pkg/tinydtls: don't require ztimer64 #19352

Merged

Conversation

fjmolinas commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

fjmolinas commented Jan 31, 2022

Uh oh!

fjmolinas commented Jan 31, 2022

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

kfessel Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

kfessel Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

kfessel Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 1, 2022

Uh oh!

fjmolinas commented Feb 9, 2022

Uh oh!

kfessel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 9, 2022

Uh oh!

fjmolinas commented Feb 9, 2022

Uh oh!

benpicco commented Mar 4, 2023

Uh oh!

kfessel commented Mar 6, 2023

Uh oh!

benpicco commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fjmolinas commented Jan 25, 2022 •

edited

Loading

kfessel Jan 31, 2022 •

edited

Loading

kfessel left a comment •

edited

Loading

benpicco commented Mar 6, 2023 •

edited

Loading