Skip to content

net: coap: Use 64bit timestamps #61026

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
Merged
merged 2 commits into from
Aug 1, 2023
Merged

net: coap: Use 64bit timestamps #61026

merged 2 commits into from
Aug 1, 2023

Conversation

@SeppoTakalo
Copy link
Contributor

Use 64bit timestamps from k_uptime_get() so they don't roll over during the expected device lifetime.

Fixes #60826

@SeppoTakalo SeppoTakalo added the DNM This PR should not be merged (Do Not Merge) label Aug 1, 2023
@zephyrbot zephyrbot requested review from pdgendt and ssharks August 1, 2023 09:34
@SeppoTakalo
Copy link
Contributor Author

This PR should not be merged before #60887 but I just posted it to review before that.
It needs one line of change after the mentioned PR is merged as that fixes LwM2M engine but leaves one TODO comment.

@carlescufi
Copy link
Member

Can you please add a release note about the tickless operation and the 64-bit timestamps?

@SeppoTakalo SeppoTakalo removed the DNM This PR should not be merged (Do Not Merge) label Aug 1, 2023
rlubos
rlubos previously approved these changes Aug 1, 2023
View reviewed changes
@SeppoTakalo SeppoTakalo requested a review from nashif as a code owner August 1, 2023 10:21
@SeppoTakalo
Copy link
Contributor Author

Can you please add a release note about the tickless operation and the 64-bit timestamps?

Added release notes as well.

@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Aug 1, 2023
ssharks
ssharks approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@ssharks ssharks left a comment

Choose a reason for hiding this comment

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

LGTM

rlubos
rlubos previously approved these changes Aug 1, 2023
View reviewed changes
@SeppoTakalo
Copy link
Contributor Author

Fixed the documentation build failure (minor whitespace problem).

rlubos
rlubos previously approved these changes Aug 1, 2023
View reviewed changes
pdgendt
pdgendt previously approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

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

Should we move to an implementation using the new timepoint API (#60232)?

@carlescufi
Copy link
Member

Should we move to an implementation using the new timepoint API (#60232)?

@SeppoTakalo @rlubos could you take a look and consider it as a follow-up?

@SeppoTakalo
net: coap: Use 64bit timestamps
a57ee89 
Use 64bit timestamps from k_uptime_get() so they don't
roll over during the expected device lifetime.

Fixes zephyrproject-rtos#60826

Signed-off-by: Seppo Takalo <seppo.takalo@nordicsemi.no>
@SeppoTakalo
doc: release-notes: Add release notes about LwM2M and CoAP changes
92b5f92 
CoAP fixes contain 64bit timer values.
LwM2M now supports tickless mode.

Signed-off-by: Seppo Takalo <seppo.takalo@nordicsemi.no>
@SeppoTakalo SeppoTakalo dismissed stale reviews from pdgendt and rlubos via 92b5f92 August 1, 2023 11:36
@SeppoTakalo
Copy link
Contributor Author

Apparently I left one bug in coap_pending_next_to_expire() as it was looking like it always returns one pointer. But there was a case when it returns NULL which I did not handle.
Should be fixed now.

@SeppoTakalo
Copy link
Contributor Author

Should we move to an implementation using the new timepoint API (#60232)?

@SeppoTakalo @rlubos could you take a look and consider it as a follow-up?

I need to take a look. This is a new API for me which I was not aware of.

@SeppoTakalo
Copy link
Contributor Author

Using these timepoints API would fix potential integer conversion problems. But there is one feature missing, comparison.
There is only a function to check equality of timepoints (and timeouts) but no comparison of which one comes first.

In CoAP and LwM2M engine there are lists of potential timeouts and it would need a function to compare those and return a next timestamp to expire.

@carlescufi carlescufi merged commit a451a03 into zephyrproject-rtos:main Aug 1, 2023
@carlescufi
Copy link
Member

Using these timepoints API would fix potential integer conversion problems. But there is one feature missing, comparison. There is only a function to check equality of timepoints (and timeouts) but no comparison of which one comes first.

In CoAP and LwM2M engine there are lists of potential timeouts and it would need a function to compare those and return a next timestamp to expire.

Could you please open an issue with the missing functionality and assign it to @npitre ?

@SeppoTakalo SeppoTakalo deleted the coap_timeout branch August 1, 2023 13:43
@SeppoTakalo
Copy link
Contributor Author

Actually, I just create a proposal PR, as this was (looking like) trivial problem #61037

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jukkar jukkar jukkar approved these changes

@ssharks ssharks ssharks approved these changes

@rlubos rlubos rlubos approved these changes

@pdgendt pdgendt pdgendt left review comments

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@nashif nashif Awaiting requested review from nashif

@fabiobaltieri fabiobaltieri Awaiting requested review from fabiobaltieri

@jhedberg jhedberg Awaiting requested review from jhedberg

Assignees

@rlubos rlubos

Labels

area: LWM2M area: Networking area: Sockets Networking sockets Release Notes To be mentioned in the release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CoAP pendings use 32bit timer for timestamp, possibility to roll over

7 participants

@SeppoTakalo @carlescufi @jukkar @pdgendt @ssharks @rlubos @zephyrbot