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) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/cell/test/u_cell_geofence_test.c#L404 change: (uPortGetTickTimeMs() < gStopTimeMs) to: (((uPortGetTickTimeMs - gStopTimeMs) < 0) suggest to use uPortTickTimeBeyondStopMs

2) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/cell/test/u_cell_loc_test.c#L779 change: (uPortGetTickTimeMs() < gStopTimeMs) to: (((uPortGetTickTimeMs - gStopTimeMs) < 0) or use uPortTickTimeBeyondStopMs

3) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/common/geofence/src/u_geofence.c#L1906 review: dynamicsMinDistance.lastStatus.distanceMillimetres = uPortGetTickTimeMs(); possibly change to dynamicsMinDistance.lastStatus.timeMs = uPortGetTickTimeMs();

4) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/common/mqtt_client/test/u_mqtt_client_test.c#L471 change: (uPortGetTickTimeMs() < startTimeMs + (U_MQTT_CLIENT_RESPONSE_WAIT_SECONDS 1000)) to: (uPortGetTickTimeMs() - startTimeMs < (U_MQTT_CLIENT_RESPONSE_WAIT_SECONDS 1000)) or use uPortTickTimeExpiredMs

5) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/gnss/src/lib_mga/u_lib_mga.c#L1191 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 .... https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/gnss/src/lib_mga/u_lib_mga.c#L1237 https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/gnss/src/lib_mga/u_lib_mga.c#L2246 https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/gnss/src/lib_mga/u_lib_mga.c#L2265 https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/gnss/src/lib_mga/u_lib_mga.c#L2291 ... potentially more ..

6) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/gnss/test/u_gnss_geofence_test.c#L901 https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/gnss/test/u_gnss_geofence_test.c#L920 change: (uPortGetTickTimeMs() < gStopTimeMs) to: (uPortGetTickTimeMs() - gStopTimeMs < 0) suggest use of use uPortTickTimeBeyondStopMs

7) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/gnss/test/u_gnss_pos_test.c#L777 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) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/cell/src/u_cell_net.c#L751 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) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/common/location/test/u_location_test.c#L332 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... https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/wifi/test/u_wifi_loc_test.c#L295

10) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/common/short_range/src/u_short_range.c#L240 looks suspicious but not sure ...

11) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/wifi/test/u_wifi_captive_portal_test.c#L131 again supicious probably need a separate flag to track if gStartTimeMs was ever set.

12) https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/common/short_range/src/u_short_range_private.h#L153 reconsider if this is needed: ticksLastRestart it does not really do something maybe just there for debugging and can be removed: https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/common/short_range/src/u_short_range.c#L188 https://github.com/u-blox/ubxlib/blob/3cf5348e22fddd185f878eea49e3bb7526cccdf7/common/short_range/src/u_short_range.c#L259

[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 79e808eb8be3095b4f94efcffba97515ba1870bd.