MAJOR CHANGE: uTimeout API. (#1143)

MAJOR CHANGE: uTimeout API. IN CASE OF COMPILATION FAILURE: if you have copied an existing ubxlib .c file to create your own file, you may find it fails to compile after merging this commit: this is because, in our internal .c files, we do not include ubxlib.h, we bring in only the required headers, and so your .c file will be missing u_timeout.h. To fix this you should #include "u_timeout.h" nearish the top of the inclusions in your .c file, above where you would include things like u_at_client.h, below where you would include u_error_common.h (see the modified .c files in this commit for the pattern). A new common API, uTimeout, is added and all timeouts are routed through it. This API performs time comparisons 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.
CogniPilot · Apr 23, 2024 · 79e808e · 79e808e
1 parent f774a0b
commit 79e808e
Show file tree

Hide file tree

Showing 150 changed files with 1,624 additions and 835 deletions.
diff --git a/DEVELOPMENT_PRINCIPLES.md b/DEVELOPMENT_PRINCIPLES.md
@@ -67,8 +67,16 @@ Since the commit messages are our only change documentation, and since Github is
   - pull the PR into a branch of `ubxlib_priv` so that you can throw it at the test system to prove that it is all good,
   - make sure that `ubxlib` `master` is up to date with `ubxlib_priv` `master` (i.e. push `ubxlib_priv` `master` to `ubxlib` `master`, which should always be possible, see above),
   - do a rebase-merge of the customer PR into `ubxlib` `master` (i.e. directly, not going via `ubxlib_priv`),
-  - pull `ubxlib` `master` back into `ubxlib_priv` `master` (i.e. with the latest `ubxlib_priv` `master` on your machine, pull `ubxlib` `master` and then push that to `ubxlib_priv` `master`).
-  
+  - pull `ubxlib` `master` back into `ubxlib_priv` `master` (i.e. with the latest `ubxlib_priv` `master` checked-out on your machine, pull `ubxlib` `master` and then push that to `ubxlib_priv` `master`).
+
 The only exception to the above is when there has been active work on the `ubxlib_priv` `development` branch and that is ready to be brought into `master`: this should be brought into `ubxlib_priv` `master` through a normal (i.e. non-rebase, non-squash) merge since it will likely be a _very_ large commit of disparate things that will not be describable when in one big blob.
 
-As an aside, if `master` moves on underneath a branch **THAT YOU ALONE** are working on, please do a `rebase` of that development branch onto `master`, rather then merging the changes from `master` onto your branch, (i.e. checkout `master` locally, pull the latest `master` and then `rebase` your branch onto `master`); the reason for this is that, otherwise, the merge process can be confused and end up thinking that you intend to remove things that have just been added in the `master` branch.  If you share the branch with someone else, i.e. you are not working on it alone, then take care because rebasing obviously changes history; it may still be the right thing to do, 'cos the ground has indeed moved underneath you, history _has_ changed, but make sure that anyone else who is working on the branch with you is aware of what you have done when you push the branch back to the repo.
+As an aside, if `master` moves on underneath a branch **THAT YOU ALONE** are working on, please do a `rebase` of that development branch onto `master`, rather then merging the changes from `master` onto your branch, (i.e. checkout `master` locally, pull the latest `master` and then `rebase` your branch onto `master`); the reason for this is that, otherwise, the merge process can be confused and end up thinking that you intend to remove things that have just been added in the `master` branch.  If you share the branch with someone else, i.e. you are not working on it alone, then take care because rebasing obviously changes history; it may still be the right thing to do, 'cos the ground has indeed moved underneath you, history _has_ changed, but make sure that anyone else who is working on the branch with you is aware of what you have done when you push the branch back to the repo.
+
+# Beware The Wrap
+Embedded systems usually have no better than a 32 bit millisecond tick count, i.e. a 32 bit counter that will wrap at 2^32 - 1, or 4,294,967,295, or about 50 days (25 days if treated as signed); the return value of the port layer `uPortGetTickTimeMs()` is a 32-bit integer for this reason.
+
+The systems that `ubxlib` is built into will need to be up for longer than 50 days, so the `ubxlib` code must behave well around such a wrap and, specifically, not unintentionally become stuck for 50 days if the tick counter happens to wrap while the code is waiting on it.  For any timeouts or delays, **always** use the [uTimeout](/common/timeout/api/u_timeout.h) API, which defines an "anonymous" `uTimeoutStart_t` structure that can be populated with a call to `uTimeoutStart()` and then checked with the `uTimeoutExpiredMs()` or the `uTimeoutExpiredSeconds()` functions; these are designed to ensure that nothing will get stuck.
+
+# Be Explicit About Units
+Where a number represents a quantity it will have a unit: seconds, milliseconds, nanoseconds, Volts, milliVolts, decibels, decibels relative to one milliWatt (dBm), words (as opposed to bytes), kilo-sheep, etc.  You may recall the tale of the [Mars Climate Orbiter](https://en.wikipedia.org/wiki/Mars_Climate_Orbiter), a $327 million spacecraft that was lost because the NASA navigation software expected measurements in Newton-seconds while their contractor was providing measurements in pound-force seconds, a factor of 4.5 different; where a number represents a quantity, **be explicit** about the unit by including it in the variable/parameter name.  For instance, presented with a variable/parameter named `timeout`, you could get things wrong by three orders of magnitude or more when applying that parameter, without realising it; naming it something like `timeoutMs` or `timeoutSeconds` will make the intended usage clear.
diff --git a/ble/src/gen2/u_ble_cfg_extmod.c b/ble/src/gen2/u_ble_cfg_extmod.c
@@ -40,6 +40,8 @@
 #include "u_cfg_sw.h"
 #include "u_port_os.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_short_range_module_type.h"

diff --git a/ble/src/gen2/u_ble_gap_extmod.c b/ble/src/gen2/u_ble_gap_extmod.c
@@ -37,6 +37,7 @@
 #include "stdlib.h"  // strol(), atoi(), strol(), strtof()
 #include "string.h"  // memset(), strncpy(), strtok_r(), strtol()
 #include "u_error_common.h"
+#include "u_timeout.h"
 #include "u_at_client.h"
 #include "u_ble.h"
 #include "u_ble_cfg.h"

diff --git a/ble/src/gen2/u_ble_gatt_extmod.c b/ble/src/gen2/u_ble_gatt_extmod.c
@@ -37,6 +37,7 @@
 #include "stdlib.h"  // strol(), atoi(), strol(), strtof()
 #include "string.h"  // memset(), strncpy(), strtok_r(), strtol()
 #include "u_error_common.h"
+#include "u_timeout.h"
 #include "u_at_client.h"
 #include "u_ble.h"
 #include "u_ble_cfg.h"

diff --git a/ble/src/gen2/u_ble_sps_extmod.c b/ble/src/gen2/u_ble_sps_extmod.c
@@ -45,6 +45,7 @@
 #include "u_port_event_queue.h"
 #include "u_cfg_os_platform_specific.h"
 
+#include "u_timeout.h"
 #include "u_at_client.h"
 #include "u_ble_sps.h"
 #include "u_ble_private.h"

diff --git a/ble/src/u_ble_cfg_extmod.c b/ble/src/u_ble_cfg_extmod.c
@@ -40,6 +40,8 @@
 #include "u_cfg_sw.h"
 #include "u_port_os.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_short_range_module_type.h"

diff --git a/ble/src/u_ble_gap_extmod.c b/ble/src/u_ble_gap_extmod.c
@@ -37,6 +37,7 @@
 #include "stdlib.h"  // strol(), atoi(), strol(), strtof()
 #include "string.h"  // memset(), strncpy(), strtok_r(), strtol()
 #include "u_error_common.h"
+#include "u_timeout.h"
 #include "u_at_client.h"
 #include "u_ble.h"
 #include "u_ble_cfg.h"

diff --git a/ble/src/u_ble_gatt_extmod.c b/ble/src/u_ble_gatt_extmod.c
@@ -37,6 +37,7 @@
 #include "stdlib.h"  // strol(), atoi(), strol(), strtof()
 #include "string.h"  // memset(), strncpy(), strtok_r(), strtol()
 #include "u_error_common.h"
+#include "u_timeout.h"
 #include "u_at_client.h"
 #include "u_ble.h"
 #include "u_ble_cfg.h"

diff --git a/ble/src/u_ble_nus.c b/ble/src/u_ble_nus.c
@@ -37,6 +37,7 @@
 #include "stdlib.h"  // strol(), atoi(), strol(), strtof()
 #include "string.h"  // memset(), strncpy(), strtok_r(), strtol()
 #include "u_error_common.h"
+#include "u_timeout.h"
 #include "u_at_client.h"
 #include "u_ble.h"
 #include "u_ble_cfg.h"

diff --git a/ble/src/u_ble_sps_extmod.c b/ble/src/u_ble_sps_extmod.c
@@ -45,6 +45,7 @@
 #include "u_port_event_queue.h"
 #include "u_cfg_os_platform_specific.h"
 
+#include "u_timeout.h"
 #include "u_at_client.h"
 #include "u_ble_sps.h"
 #include "u_ble_private.h"

diff --git a/ble/src/u_ble_sps_intmod.c b/ble/src/u_ble_sps_intmod.c
@@ -39,6 +39,8 @@
 
 #include "u_error_common.h"
 
+#include "u_timeout.h"
+
 #include "u_device_shared.h"
 
 #include "u_cfg_sw.h"
@@ -1232,14 +1234,12 @@ int32_t uBleSpsSend(uDeviceHandle_t devHandle, int32_t channel, const char *pDat
         return (int32_t)U_ERROR_COMMON_INVALID_PARAMETER;
     }
 
-    int64_t startTime = uPortGetTickTimeMs();
     int32_t errorCode = (int32_t)U_ERROR_COMMON_SUCCESS;
     spsConnection_t *pSpsConn = pGetSpsConn(spsConnHandle);
     if (pSpsConn->spsState == SPS_STATE_CONNECTED) {
-        uint32_t timeout = pSpsConn->dataSendTimeoutMs;
-        int64_t time = startTime;
-
-        while ((bytesLeftToSend > 0) && (time - startTime < timeout)) {
+        uint32_t timeoutMs = pSpsConn->dataSendTimeoutMs;
+        uTimeoutStart_t timeoutStart = uTimeoutStart();
+        while ((bytesLeftToSend > 0) && !uTimeoutExpiredMs(timeoutStart, timeoutMs)) {
             int32_t bytesToSendNow = bytesLeftToSend;
             int32_t maxDataLength = pSpsConn->mtu - U_BLE_PDU_HEADER_SIZE;
 
@@ -1253,13 +1253,11 @@ int32_t uBleSpsSend(uDeviceHandle_t devHandle, int32_t channel, const char *pDat
                 // again later if we are out of credits.
                 (void)uPortSemaphoreTryTake(pSpsConn->txCreditsSemaphore, 0);
                 if (pSpsConn->txCredits == 0) {
-                    int32_t timeoutLeft = (int32_t)timeout - (int32_t)(time - startTime);
-                    if (timeoutLeft < 0) {
-                        timeoutLeft = 0;
-                    }
+                    uint32_t elapsedMs = uTimeoutElapsedMs(timeoutStart);
+                    uint32_t timeoutLeftMs = (elapsedMs >= timeoutMs) ? 0 : timeoutMs - elapsedMs;
                     // We are out of credits, wait for more
-                    if (uPortSemaphoreTryTake(pSpsConn->txCreditsSemaphore, timeoutLeft) != 0) {
-                        uPortLog("U_BLE_SPS: SPS Timed out waiting for new TX credits!\n");
+                    if (uPortSemaphoreTryTake(pSpsConn->txCreditsSemaphore, timeoutLeftMs) != 0) {
+                        uPortLog("U_BLE_SPS: SPS timed out waiting for new TX credits!\n");
                         break;
                     }
                 }
@@ -1277,9 +1275,6 @@ int32_t uBleSpsSend(uDeviceHandle_t devHandle, int32_t channel, const char *pDat
                 errorCode = (int32_t)U_ERROR_COMMON_UNKNOWN;
                 break;
             }
-            if (bytesLeftToSend > 0) {
-                time = uPortGetTickTimeMs();
-            }
         }
     }
 

diff --git a/ble/test/u_ble_bond_test.c b/ble/test/u_ble_bond_test.c
@@ -64,6 +64,8 @@
 
 #include "u_test_util_resource_check.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 #include "u_short_range_pbuf.h"
 #include "u_short_range.h"

diff --git a/cell/src/u_cell.c b/cell/src/u_cell.c
@@ -37,11 +37,14 @@
 
 #include "u_error_common.h"
 
+#include "u_port.h"
 #include "u_port_debug.h"
 #include "u_port_os.h"
 #include "u_port_heap.h"
 #include "u_port_gpio.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_ringbuffer.h"
@@ -297,6 +300,8 @@ int32_t uCellAdd(uCellModuleType_t moduleType,
                     pInstance->pinPwrOn = pinPwrOn;
                     pInstance->pinVInt = pinVInt;
                     pInstance->pinDtrPowerSaving = -1;
+                    pInstance->lastCfunFlipTime = uTimeoutStart();
+                    pInstance->lastDtrPinToggleTime = uTimeoutStart();
                     for (size_t x = 0;
                          x < sizeof(pInstance->networkStatus) / sizeof(pInstance->networkStatus[0]);
                          x++) {

diff --git a/cell/src/u_cell_cfg.c b/cell/src/u_cell_cfg.c
@@ -46,6 +46,8 @@
 #include "u_port_os.h"
 #include "u_port_heap.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_cell_module_type.h"

diff --git a/cell/src/u_cell_file.c b/cell/src/u_cell_file.c
@@ -39,6 +39,7 @@
 #include "u_error_common.h"
 #include "u_port.h"
 #include "u_port_os.h"
+#include "u_timeout.h"
 #include "u_at_client.h"
 #include "u_cell_module_type.h"
 #include "u_cell_net.h"

diff --git a/cell/src/u_cell_fota.c b/cell/src/u_cell_fota.c
@@ -41,6 +41,8 @@
 #include "u_port_os.h"
 #include "u_port_heap.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_cell_module_type.h"

diff --git a/cell/src/u_cell_geofence.c b/cell/src/u_cell_geofence.c
@@ -37,6 +37,8 @@
 
 #include "u_error_common.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_linked_list.h"

diff --git a/cell/src/u_cell_gpio.c b/cell/src/u_cell_gpio.c
@@ -42,6 +42,8 @@
 #include "u_port_os.h"
 #include "u_port_heap.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_cell_module_type.h"

diff --git a/cell/src/u_cell_http.c b/cell/src/u_cell_http.c
@@ -51,6 +51,8 @@
 #include "u_port_debug.h"
 #include "u_port_event_queue.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_sock.h"

diff --git a/cell/src/u_cell_info.c b/cell/src/u_cell_info.c
@@ -49,6 +49,8 @@
 #include "u_port_os.h"
 #include "u_port_uart.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_device_shared.h"

diff --git a/cell/src/u_cell_loc.c b/cell/src/u_cell_loc.c
@@ -51,6 +51,8 @@
 #include "u_port_heap.h"
 #include "u_port_debug.h"
 
+#include "u_timeout.h"
+
 #include "u_time.h"
 
 #include "u_at_client.h"
@@ -1748,7 +1750,7 @@ int32_t uCellLocGet(uDeviceHandle_t cellHandle,
     uCellPrivateLocContext_t *pContext;
     uCellLocFixDataStorage_t *pFixDataStorage;
     volatile uCellLocFixDataStorageBlock_t fixDataStorageBlock = {0};
-    int64_t startTime;
+    uTimeoutStart_t timeoutStart;
 
     fixDataStorageBlock.errorCode = (int32_t) U_ERROR_COMMON_TIMEOUT;
 
@@ -1795,10 +1797,10 @@ int32_t uCellLocGet(uDeviceHandle_t cellHandle,
                 uPortLog("U_CELL_LOC: waiting for the answer...\n");
                 // Wait for the callback called by the URC to set
                 // errorCode inside our block to success
-                startTime = uPortGetTickTimeMs();
+                timeoutStart = uTimeoutStart();
                 while ((fixDataStorageBlock.errorCode == (int32_t) U_ERROR_COMMON_TIMEOUT) &&
                        (((pKeepGoingCallback == NULL) &&
-                         (uPortGetTickTimeMs() - startTime) / 1000 < U_CELL_LOC_TIMEOUT_SECONDS) ||
+                         !uTimeoutExpiredSeconds(timeoutStart, U_CELL_LOC_TIMEOUT_SECONDS)) ||
                         ((pKeepGoingCallback != NULL) && pKeepGoingCallback(cellHandle)))) {
                     // Relax a little
                     uPortTaskBlock(1000);

diff --git a/cell/src/u_cell_mno_db.c b/cell/src/u_cell_mno_db.c
@@ -34,6 +34,8 @@
 
 #include "u_error_common.h"
 
+#include "u_timeout.h"
+
 #include "u_at_client.h"
 
 #include "u_port.h"