few tick timeout comparison and other issues/improvements: about ubxlib HOT 6 CLOSED

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

from ubxlib.

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

from ubxlib.

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;

from ubxlib.

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.

from ubxlib.

Implementation in #225.

from ubxlib.

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

from ubxlib.