MAJOR CHANGE: uTimeout API. #225
Conversation
A new common API, uTimeout, is added and all timeouts are routed through it. This API performs time comparisons correctly with unsigned 32 bit arithmetic to ensure that timeouts expire correctly if they happen to cross the 32-bit wrap point, which will occur every 50 days for a millisecond tick; otherwise there were instances where a timeout could potentially get "stuck" for 25 days (the unsigned 32-bit wrap length for a millisecond tick). The implementation of the API includes the ability to speed up the apparent wrap-rate of the underlying tick; this is used on test instance 23 (which includes cellular, GNSS and short-range module types) to give it a good thrashing. Our thanks to warasilapm for raising this issue and _my_ thanks to mazgch for providing the solution.
RobMeades
force-pushed
the
preview_fix_tick_wrap_2_rmea
branch
from
e7bb366
to
8b2e457
Compare
There was a problem hiding this comment.
@RobMeades Added several small comments and suggestions, but it looks really good and in my view cleaner.
| x = send(sock, (const void *) pData, sizeBytes - sentSizeBytes, 0); | ||
| if (x > 0) { | ||
| sentSizeBytes += x; | ||
| pData += x; | ||
| U_TEST_PRINT_LINE("sent %d byte(s) of TCP data @%d ms.", | ||
| sentSizeBytes, (int32_t) uPortGetTickTimeMs()); | ||
| sentSizeBytes, uPortGetTickTimeMs()); |
There was a problem hiding this comment.
so me need absolute ms here ?
| @@ -254,7 +256,7 @@ static void rxTask(void *pParameter) | |||
| pTestConfig->bytesReceived, 0); | |||
| if (sizeBytes > 0) { | |||
| U_TEST_PRINT_LINE("received %d byte(s) of data @%d ms.", | |||
| sizeBytes, (int32_t) uPortGetTickTimeMs()); | |||
| sizeBytes, uPortGetTickTimeMs()); | |||
| x = zsock_send(sock, (const void *) pData, sizeBytes - sentSizeBytes, 0); | ||
| if (x > 0) { | ||
| sentSizeBytes += x; | ||
| pData += x; | ||
| U_TEST_PRINT_LINE("sent %d byte(s) of TCP data @%d ms.", | ||
| sentSizeBytes, (int32_t) uPortGetTickTimeMs()); | ||
| sentSizeBytes, uPortGetTickTimeMs()); |
There was a problem hiding this comment.
remoive @ms or make it relative ?
| @@ -1511,7 +1514,7 @@ U_PORT_TEST_FUNCTION("[port]", "portOs") | |||
| U_PORT_TEST_ASSERT(errorCode == 0); | |||
| U_PORT_TEST_ASSERT(gTaskHandle != NULL); | |||
|
|
|||
| U_TEST_PRINT_LINE("time now %d ms.", (int32_t) uPortGetTickTimeMs()); | |||
| U_TEST_PRINT_LINE("time now %d ms.", uPortGetTickTimeMs()); | |||
There was a problem hiding this comment.
should this all have relative times to the start ? use uTimeoutStart and elapsed ?
Just consistency not anything wrong here.
There was a problem hiding this comment.
I found it useful to have the timestamps in front of me when initially debugging this, kind of easier to visualise.
There was a problem hiding this comment.
I see, you want to see how the packet timing when the go out, and at the same time wouldn't an elapsed ms time since start be even more useful, so it keeps the numbers less random and small ?
There was a problem hiding this comment.
Maybe; I think it probably depends how you want to look at things. Given a set of times I can write them on a line and get a feeling for the pattern; provided one isn't running ubxlib on Windows or Linux, the MCU we are running on will have been booted just a short time before and so the numbers will be small. I know I could do the same with time differences, just that I recall finding this clearer to stuff into my tiny brain :-).
Test: 1 2 4.1 4.2 4.3.0 4.3.1 4.4.0 4.4.1 5.1.0 5.1.1 5.2.0 5.2.1 5.3 5.4 6.1.0 6.1.1 6.2.0 6.2.1 7 8
|
@RobMeades in the latest commit where you removed the durationMs in at client test aroud lines 950-970 you are once using utimeoutStop.timeoutStart and in the timeoutStart to replace the similar durationMs timeoutCheck |
|
@mazgch: yes, well spotted, the subset of tests I ran on that commit didn't include ones that ran that particular AT Client test, when I ran a more complete test that one failed; fixed on my local branch, didn't bother pushing it back here. |
A new common API,
uTimeout, is added and all timeouts are routed through it. This API performs time comparisons correctly with unsigned 32 bit arithmetic to ensure that timeouts are not affected if they happen to cross the 32-bit wrap point, which will occur every 50 days for a millisecond tick; otherwise there were instances where a timeout could potentially get "stuck" for 25 days (the unsigned 32-bit wrap length for a millisecond tick).The implementation of the API includes the ability to speed up the apparent wrap-rate of the underlying tick; this is used on test instance 23 (which includes cellular, GNSS and short-range module types) to give it a good thrashing.
Our thanks to warasilapm for raising this issue and my thanks to mazgch for providing the solution.
NOTE: this PR will not be merged it is simply posted here to make it possible for warasilapm to comment on it directly and for him to be able to see any comments made by mazgch.