few tick timeout comparison and other issues/improvements:

[mazgch]

Hi @RobMeades

I had some time to screen the https://github.com/u-blox/ubxlib/tree/preview_fix_tick_wrap_rmea branch a bit and found some issues many of them timeout related but not only. Towards the end you also find a suggestion how to avoid these mistakes in the future with an improved timeout API that hides the MS handling from the user.

1. ubxlib/cell/test/u_cell_geofence_test.c

   Line 404 in 3cf5348

   (uPortGetTickTimeMs() < gStopTimeMs) &&

   
   change: (uPortGetTickTimeMs() < gStopTimeMs)
   to: (((uPortGetTickTimeMs - gStopTimeMs) < 0)
   suggest to use uPortTickTimeBeyondStopMs

2. ubxlib/cell/test/u_cell_loc_test.c

   Line 779 in 3cf5348

   while ((gErrorCode == 0xFFFFFFFF) && (uPortGetTickTimeMs() < gStopTimeMs) &&

   
   change: (uPortGetTickTimeMs() < gStopTimeMs)
   to: (((uPortGetTickTimeMs - gStopTimeMs) < 0)
   or use uPortTickTimeBeyondStopMs

3. ubxlib/common/geofence/src/u_geofence.c

   Line 1906 in 3cf5348

   dynamicsMinDistance.lastStatus.distanceMillimetres = uPortGetTickTimeMs();

   
   review: dynamicsMinDistance.lastStatus.distanceMillimetres = uPortGetTickTimeMs();
   possibly change to dynamicsMinDistance.lastStatus.timeMs = uPortGetTickTimeMs();

4. ubxlib/common/mqtt_client/test/u_mqtt_client_test.c

   Line 471 in 3cf5348

   (uPortGetTickTimeMs() < startTimeMs +

   
   change: (uPortGetTickTimeMs() < startTimeMs +
   (U_MQTT_CLIENT_RESPONSE_WAIT_SECONDS * 1000))
   to: (uPortGetTickTimeMs() - startTimeMs < (U_MQTT_CLIENT_RESPONSE_WAIT_SECONDS * 1000))
   or use uPortTickTimeExpiredMs

5. ubxlib/gnss/src/lib_mga/u_lib_mga.c

   Line 1191 in 3cf5348

   if (now > pMsgInfo->timeOut)

   
   change: if (now > pMsgInfo->timeOut)
   to: if (now - pMsgInfo->timeOut > 0)
   suggest to rename now to startTimeMs and timeOut StopTimeMs
   sugguest to use uPortTickTimeBeyondStopMs

many more occasions throughout this file ....

ubxlib/gnss/src/lib_mga/u_lib_mga.c

Line 1237 in 3cf5348

if (now > s_pLastFlashBlockSent->timeOut)

ubxlib/gnss/src/lib_mga/u_lib_mga.c

Line 2246 in 3cf5348

if (now > s_aidingTimeout)

ubxlib/gnss/src/lib_mga/u_lib_mga.c

Line 2265 in 3cf5348

if (now > s_pLastFlashBlockSent->timeOut)

ubxlib/gnss/src/lib_mga/u_lib_mga.c

Line 2291 in 3cf5348

if (now > s_aidingTimeout)

... potentially more ..

6. ubxlib/gnss/test/u_gnss_geofence_test.c

   Line 901 in 3cf5348

   while ((gCallbackParameters.called < 2) && (uPortGetTickTimeMs() < gStopTimeMs)) {

   
   

   ubxlib/gnss/test/u_gnss_geofence_test.c

   Line 920 in 3cf5348

   while ((gCallbackParameters.called < 2) && (uPortGetTickTimeMs() < gStopTimeMs)) {

   
   change: (uPortGetTickTimeMs() < gStopTimeMs)
   to: (uPortGetTickTimeMs() - gStopTimeMs < 0)
   suggest use of use uPortTickTimeBeyondStopMs

7. ubxlib/gnss/test/u_gnss_pos_test.c

   Line 777 in 3cf5348

   while ((gErrorCode != 0) && (uPortGetTickTimeMs() < gStopTimeMs)) {

   
   chnage: (uPortGetTickTimeMs() < gStopTimeMs)
   to: (uPortGetTickTimeMs() - gStopTimeMs < 0)
   suggest use of use uPortTickTimeBeyondStopMs

maybe there are even more of these things ...

To avoid all such mistakes my suggestion is to have tick uPortGetTickTimeMs return a "handle" and move all the comparison functions into the function where this is allways properly handled. properly, this way the develpper can't make any mistakes as he does not know what using this handle has and how to convert it

TIMEOUTHANDLE_t uPortGetTimeoutHandle() {
    return (TIMEOUTHANDLE_t) uPortGetTickTimeMs();
}

You could extend the tick API with a elapsed millisecond and use this throughout the code in all the printfs

uint32_t uPortGetElapsedMs(TIMEOUTHANDLE_t timeoutHandle)
{ 
   return (uint32_t)(uPortGetTickTimeMs() - (int32-t)timeoutHandle); 
}
 
uint32_t uPortGetElapsedSeconds(TIMEOUTHANDLE_t timeoutHandle)
{ 
   return uPortGetElapsedMs() / 1000; 
}

of course the current API would also take only these handles

[RobMeades]

Very useful indeed, thanks Michael, give me time to absorb and I will comment here, tagging issue #209 so that warasilapm sees this.

[mazgch]

Some of the issues are just in test code, so will not affect a real application.

[mazgch]

Here is some more:

8. ubxlib/cell/src/u_cell_net.c

   Line 751 in 3cf5348

   if ((pInstance->startTimeMs > 0) &&

   
   likely a bad idea, just remove?: (pInstance->startTimeMs > 0) &&
   either you need an extra bool flag to indicate that startTimeMs was never set.
   all the startTimeMs = 0 in this file are very supicious ... values <0 are valid values that uPortGetTickTimeMs can return

9. ubxlib/common/location/test/u_location_test.c

   Line 332 in 3cf5348

   int32_t startTimeMs = 0;

   
   probably better to init with uPortGetTickTimeMs or maybe this = 0 is not needed at all, but seting 0 here is not better than having a random value.
   same here...
   

   ubxlib/wifi/test/u_wifi_loc_test.c

   Line 295 in 3cf5348

   int32_t startTimeMs = 0;

10. ubxlib/common/short_range/src/u_short_range.c

    Line 240 in 3cf5348

    pInstance->startTimeMs = 500;

    
    looks suspicious but not sure ...

11. ubxlib/wifi/test/u_wifi_captive_portal_test.c

    Line 131 in 3cf5348

    static int32_t gStartTimeMs = -1;

    
    again supicious probably need a separate flag to track if gStartTimeMs was ever set.

12. ubxlib/common/short_range/src/u_short_range_private.h

    Line 153 in 3cf5348

    int32_t ticksLastRestart;

    
    reconsider if this is needed: ticksLastRestart
    it does not really do something maybe just there for debugging and can be removed:
    

    ubxlib/common/short_range/src/u_short_range.c

    Line 188 in 3cf5348

    pInstance->ticksLastRestart = uPortGetTickTimeMs();

    
    

    ubxlib/common/short_range/src/u_short_range.c

    Line 259 in 3cf5348

    pInstance->ticksLastRestart = 0;

[warasilapm]

Yeah I've been noticing this repeatedly as well - though we've not been reviewing the test code applications. Zephyr does something similar as suggested by wrapping all "times" in an "opaque" (but not really) struct that is supposed to be passed into macros/static inlines to handle the encapsulated values. You can see the encapsulation in k_timeout_t here. I imagine you'd end up with a uTick_t struct similar to this.

Note Zephyr does some things differently as it generally leverages int64_t to avoid any wraparound issues. We've found a couple more edge cases to share on the other issue later today after we do some more investigation.

[RobMeades]

Implementation in #225.

[RobMeades]

The fix for this issue, as proposed (and reviewed) by @mazgch, can be found in commit 79e808e.