-
Notifications
You must be signed in to change notification settings - Fork 57
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
dtls_time: migrate to ztimer_msec #123
Conversation
Hi Francisco, for contributions to Eclipse you need a ECA, please read CONTRIBUTING for more. About 32 bit milliseconds: AFAIK that should work, but requires special care for potential overflow after about 49 days.
If I remember well, that should be:
In the case of an overflow |
Hi @boaks thanks for your answer, since you seem to point out at least one occurrence where 64bit (or at least no overflow) are assumed, could there be more? I can also leave the values in 64bits, the ztimer_msec has a 64bit version I can use here as well. |
FMPOV, a 32 bit retransmission timer should not be too bad. That's only my opinion.
Sure, e.g. |
i just searched trough tinydtls all dtls_ticks( ) uses are in dtls.c l2076: produce 4 bytes of of "random" for the dtls handshake (server) (tries to get seconds -> we are loosing 10Bit) l4248: inits prng (not relevant to riot it is ignored, initilised by riot) l4395: calculate another timestamp (timeout) see l1692: l4458: walks the packet resend queue until now is reached uses the timstamps from (l1692 and l4395) needs to be made 32 bit safe netqc: netq_insert_node: |
I think, checking for the usage of |
(Even, if |
dtls_time.h: l59 has |
@kfessel Thanks for looking up the uses of the timer functions in the code. It would have been easier to read if you had used the current develop head or generated persistent links. Regarding the use of 32 bit unix time stamp in ClientHello and ServerHello: DTLS 1.2 is defined as a sort of "update" to TLS 1.2. The code in |
Sorry i went a bit quick with the write down. And used current riot checkout |
just in the case you're interested: for DTLS 1.2 there is a upcoming new feature, RFC9146, which overcomes issues caused by address changes. If you're interested to test that with RIOT, that would be great. The complex part is the server-side, and currently implemented by me in Eclipse/Californium. A simple, client-only implementation is available here in branch feature/connection_id and my PR #108. I will try to update my PR, and maybe afterwards, Olaf may reset the branch to the update PR. |
i created an issue about that 32 Bit overflow concerns: #125 @boaks the feature/connection_id branch seems to carry the same issue |
sure. it was not about testing a fix for this, it was about interest in DTLS CID / RFC9146. |
@fjmolinas I believe you still need to accept the legal agreement and force push so your changes can be accepted ;) |
You still need to register and sign a ECA, please read CONTRIBUTING for more. Please use the e-mail of your commits for that. |
With the potential 32bit issues mentioned above, I'm not sure this makes sense now. |
Thanks for signing the ECA! |
Yes, step 1 would be resolving the conflict. |
o i didn't see the conflict lets alert @fjmolinas so he is aware of it |
d4ac77b
to
1c182db
Compare
Rebased to main! |
dtls.c
Outdated
@@ -1,6 +1,6 @@ | |||
/******************************************************************************* | |||
* | |||
* Copyright (c) 2011-2021 Olaf Bergmann (TZI) and others. | |||
* Copyright (c) 2011-2022 Olaf Bergmann (TZI) and others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really wrong, but I would prefer to have that listed for the code-cleanup, issue #143
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think its because I rebased maybe against the wrong branch, I guess I should have done it against develop....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed I wrongly rebased to main which had pulled in unrelated changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your right, main has 2022 and develop has 2021.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR had been opened against develop I kept it that way, or should I re-open against main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FMPOV, that's a detail of issue #140.
For now it's a "raw idea", to have a "main" and a "develop", mainly to relax the time constraints for team members. Though this project works different with git as I'm used to it, I'm not sure, if my way will the right one. (My way would be, PR against "main", cherrypick in advance to "develop", if indicated by time constraints.)
Anyway, though this PR is dated before #140, I don't think, it's required to recreate a PR against main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, so we can move forward with this one as is.
1c182db
to
1c479d1
Compare
LGTM (I merge it to develop, we merge it to main with obgm LGTM). |
LGTM (I merge it to develop, we merge it to main with obgm LGTM). |
Thanks for the patience and the review! |
Looks good, can be merged into |
Merged to main (cherrypick). |
RIOT is currently migrating from its xtimer to ztimer high level timer abstraction.
The new API allows better sleep management.
xtimer
was based in microseconds and in this library-port, it was using 64bits. This PR is changing it to useztimer_msec
, so milliseconds based. It's also using 32bits timestamps instead of 64bits used byxtimer
.Since
contiki
also uses 32bits my assumption is that a 32bit millisecond timer should be enough as a backend for thedtls_time
api, is there any reason why this assumption would be wrong?I have provided test results for this in the PR making this change in RIOT-OS/RIOT, (see RIOT-OS/RIOT#17564). In that PR this change is provided as a patch, but if this PR is accepted I can simply change the commit hash.