search

tass-belgium / picotcp

PicoTCP is a free TCP/IP stack implementation
Other
1.17k stars 218 forks source link

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

Closed danielinux closed 10 years ago

danielinux commented 11 years ago

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.

phalox commented 11 years ago

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

andreixc commented 11 years ago

Do all architectures support unsigned long long?

danielinux commented 10 years ago

Reopening due to a regression introduced in 52f21a432e895f7f9ec9d3898a0ca0572d6892eb

danielinux commented 10 years ago

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 commented 10 years ago

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

phalox commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

Please run autotest again, confirm and close.

danielinux commented 10 years ago

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