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

Tick timer has a wrap-around every 49.71 days #2

Closed
danielinux opened this issue Jul 17, 2013 · 12 comments
Closed

Tick timer has a wrap-around every 49.71 days #2

danielinux opened this issue Jul 17, 2013 · 12 comments

Comments

@danielinux
Copy link
Contributor

This is issue 724 in internal redmine

System tick counter is 32 bit in size, and counts every ms..

2^32
/ 1000 --> ms
/ 3600 --> hours
/ 24 --> days

= 49.71 days.

I propose to increase the counter to an unsigned long long everywhere... That timer should never ever overflow.

@ghost ghost assigned kristofR Jul 17, 2013
@phalox
Copy link
Contributor

phalox commented Jul 17, 2013

Don't forget all current applications will also need to change their implementation.

@andreixc
Copy link
Contributor

andreixc commented Sep 5, 2013

Do all architectures support unsigned long long?

@danielinux
Copy link
Contributor Author

Reopening due to a regression introduced in 52f21a4

@danielinux danielinux reopened this Dec 4, 2013
@ghost ghost assigned bibireiulian Dec 4, 2013
@danielinux
Copy link
Contributor Author

Please add a unit test to verify that (uint64_t)(-1) can be safely assigned to pico_tick -- We don't want to have another regression on this anymore.

@andreixc
Copy link
Contributor

andreixc commented Dec 5, 2013

Created pico_time as synonym for uint64_t.
All the timestamps have the type pico_time.
Added unit test to check pico_tick = -1.

@andreixc andreixc closed this as completed Dec 5, 2013
@phalox
Copy link
Contributor

phalox commented Dec 5, 2013

Looking at our freshly introduced coding guidelines: Does this clash with any of them? We're typedeffing a known type. And the Ticks framework doesn't have any problems any more with these changes?

@andreixc
Copy link
Contributor

andreixc commented Dec 5, 2013

The only reason I did the typedef is in the future it needs to be changed you don't search in the whole stack but only in one place.
Another reason is that you can easily see which variables are timestamps.

@andreixc
Copy link
Contributor

andreixc commented Dec 5, 2013

Is this rule broken?

  1. typedef is advised for structs, enums, unions and standard types only. Do not use typedefs to hide dereferences (e.g. typedef int* intp)

uint64_t should be considered as standard type.

@bogdanlu bogdanlu reopened this Dec 6, 2013
@bogdanlu
Copy link
Contributor

bogdanlu commented Dec 6, 2013

This issue introduces a regression for AUTOTEST.
Reproducible 100%:

UDP TEST
./test/autotest.sh: line 20: 1833 Segmentation fault (core dumped) ./build/test/picoapp.elf --vde pic0:/tmp/pic0.ctl:10.40.0.9:255.255.0.0: -a udpclient:10.40.0.8:6667:6667:1400:100:10
Build step 'Execute shell' marked build as failure

@andreixc
Copy link
Contributor

andreixc commented Dec 6, 2013

Tested on my machine, sadly there unsigned long = uint64_t.
Root cause was in picoapp.c
This time the test was run on a 32 bit machine and passed.

andreixc pushed a commit that referenced this issue Dec 6, 2013
@andreixc
Copy link
Contributor

andreixc commented Dec 6, 2013

Please run autotest again, confirm and close.

@danielinux
Copy link
Contributor Author

Continuous integration on masterbranch autotest is back to normal. Issue closed. Thanks Andrei.

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

No branches or pull requests

6 participants