From 1e3e8a29a92cb3cfa4444e474b9f7a8bd093bcd5 Mon Sep 17 00:00:00 2001 From: Philippe Renon Date: Tue, 4 Mar 2014 09:49:34 +0100 Subject: [PATCH 1/7] OP-1246 fixed clumsy error handling of UAVTalkSendObject calls from RadioComBridge --- flight/modules/RadioComBridge/RadioComBridge.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/flight/modules/RadioComBridge/RadioComBridge.c b/flight/modules/RadioComBridge/RadioComBridge.c index 2be3764cb..c0d17593b 100644 --- a/flight/modules/RadioComBridge/RadioComBridge.c +++ b/flight/modules/RadioComBridge/RadioComBridge.c @@ -342,11 +342,11 @@ static void telemetryTxTask(__attribute__((unused)) void *parameters) updateRadioComBridgeStats(); } // Send update (with retries) + int32_t ret = -1; uint32_t retries = 0; - int32_t success = -1; - while (retries < MAX_RETRIES && success == -1) { - success = UAVTalkSendObject(data->telemUAVTalkCon, ev.obj, ev.instId, 0, RETRY_TIMEOUT_MS) == 0; - if (success == -1) { + while (retries <= MAX_RETRIES && ret == -1) { + ret = UAVTalkSendObject(data->telemUAVTalkCon, ev.obj, ev.instId, 0, RETRY_TIMEOUT_MS); + if (ret == -1) { ++retries; } } @@ -376,11 +376,11 @@ static void radioTxTask(__attribute__((unused)) void *parameters) if (xQueueReceive(data->radioEventQueue, &ev, MAX_PORT_DELAY) == pdTRUE) { if ((ev.event == EV_UPDATED) || (ev.event == EV_UPDATE_REQ)) { // Send update (with retries) + int32_t ret = -1; uint32_t retries = 0; - int32_t success = -1; - while (retries < MAX_RETRIES && success == -1) { - success = UAVTalkSendObject(data->radioUAVTalkCon, ev.obj, ev.instId, 0, RETRY_TIMEOUT_MS) == 0; - if (success == -1) { + while (retries <= MAX_RETRIES && ret == -1) { + ret = UAVTalkSendObject(data->radioUAVTalkCon, ev.obj, ev.instId, 0, RETRY_TIMEOUT_MS); + if (ret == -1) { ++retries; } } From 29198e17cffc9b373fea69714f7ca4c6717cfc8a Mon Sep 17 00:00:00 2001 From: Philippe Renon Date: Tue, 4 Mar 2014 09:51:45 +0100 Subject: [PATCH 2/7] OP-1246 fixed wrong logic for telemtryTxRetries and radioTxRetries : was wrongly accumulated --- flight/modules/RadioComBridge/RadioComBridge.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/flight/modules/RadioComBridge/RadioComBridge.c b/flight/modules/RadioComBridge/RadioComBridge.c index c0d17593b..3aa6297b9 100644 --- a/flight/modules/RadioComBridge/RadioComBridge.c +++ b/flight/modules/RadioComBridge/RadioComBridge.c @@ -299,10 +299,12 @@ static void updateRadioComBridgeStats() // Get stats object data RadioComBridgeStatsGet(&radioComBridgeStats); + radioComBridgeStats.TelemetryTxRetries = data->telemetryTxRetries; + radioComBridgeStats.RadioTxRetries = data->radioTxRetries; + // Update stats object radioComBridgeStats.TelemetryTxBytes += telemetryUAVTalkStats.txBytes; radioComBridgeStats.TelemetryTxFailures += telemetryUAVTalkStats.txErrors; - radioComBridgeStats.TelemetryTxRetries += data->telemetryTxRetries; radioComBridgeStats.TelemetryRxBytes += telemetryUAVTalkStats.rxBytes; radioComBridgeStats.TelemetryRxFailures += telemetryUAVTalkStats.rxErrors; @@ -311,7 +313,6 @@ static void updateRadioComBridgeStats() radioComBridgeStats.RadioTxBytes += radioUAVTalkStats.txBytes; radioComBridgeStats.RadioTxFailures += radioUAVTalkStats.txErrors; - radioComBridgeStats.RadioTxRetries += data->radioTxRetries; radioComBridgeStats.RadioRxBytes += radioUAVTalkStats.rxBytes; radioComBridgeStats.RadioRxFailures += radioUAVTalkStats.rxErrors; From 067cb6f098537482ed76b398b52978a1cd30a026 Mon Sep 17 00:00:00 2001 From: Philippe Renon Date: Tue, 4 Mar 2014 09:56:42 +0100 Subject: [PATCH 3/7] OP-1246 made updating of RadioComBridgeStats thread safe --- flight/modules/RadioComBridge/RadioComBridge.c | 6 ++---- flight/modules/Telemetry/telemetry.c | 6 ++---- flight/uavtalk/inc/uavtalk.h | 4 ++-- flight/uavtalk/uavtalk.c | 14 ++++++++++++-- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/flight/modules/RadioComBridge/RadioComBridge.c b/flight/modules/RadioComBridge/RadioComBridge.c index 3aa6297b9..7e10557ea 100644 --- a/flight/modules/RadioComBridge/RadioComBridge.c +++ b/flight/modules/RadioComBridge/RadioComBridge.c @@ -289,12 +289,10 @@ static void updateRadioComBridgeStats() RadioComBridgeStatsData radioComBridgeStats; // Get telemetry stats - UAVTalkGetStats(data->telemUAVTalkCon, &telemetryUAVTalkStats); - UAVTalkResetStats(data->telemUAVTalkCon); + UAVTalkGetStats(data->telemUAVTalkCon, &telemetryUAVTalkStats, true); // Get radio stats - UAVTalkGetStats(data->radioUAVTalkCon, &radioUAVTalkStats); - UAVTalkResetStats(data->radioUAVTalkCon); + UAVTalkGetStats(data->radioUAVTalkCon, &radioUAVTalkStats, true); // Get stats object data RadioComBridgeStatsGet(&radioComBridgeStats); diff --git a/flight/modules/Telemetry/telemetry.c b/flight/modules/Telemetry/telemetry.c index b07bb85a5..7a6844e8b 100644 --- a/flight/modules/Telemetry/telemetry.c +++ b/flight/modules/Telemetry/telemetry.c @@ -542,12 +542,10 @@ static void updateTelemetryStats() uint32_t timeNow; // Get stats - UAVTalkGetStats(uavTalkCon, &utalkStats); + UAVTalkGetStats(uavTalkCon, &utalkStats, true); #ifdef PIOS_INCLUDE_RFM22B - UAVTalkAddStats(radioUavTalkCon, &utalkStats); - UAVTalkResetStats(radioUavTalkCon); + UAVTalkAddStats(radioUavTalkCon, &utalkStats, true); #endif - UAVTalkResetStats(uavTalkCon); // Get object data FlightTelemetryStatsGet(&flightStats); diff --git a/flight/uavtalk/inc/uavtalk.h b/flight/uavtalk/inc/uavtalk.h index e54231735..9dc92187c 100644 --- a/flight/uavtalk/inc/uavtalk.h +++ b/flight/uavtalk/inc/uavtalk.h @@ -61,8 +61,8 @@ UAVTalkRxState UAVTalkProcessInputStream(UAVTalkConnection connection, uint8_t r UAVTalkRxState UAVTalkProcessInputStreamQuiet(UAVTalkConnection connection, uint8_t rxbyte); UAVTalkRxState UAVTalkRelayPacket(UAVTalkConnection inConnectionHandle, UAVTalkConnection outConnectionHandle); int32_t UAVTalkReceiveObject(UAVTalkConnection connectionHandle); -void UAVTalkGetStats(UAVTalkConnection connection, UAVTalkStats *stats); -void UAVTalkAddStats(UAVTalkConnection connection, UAVTalkStats *stats); +void UAVTalkGetStats(UAVTalkConnection connection, UAVTalkStats *stats, bool reset); +void UAVTalkAddStats(UAVTalkConnection connection, UAVTalkStats *stats, bool reset); void UAVTalkResetStats(UAVTalkConnection connection); void UAVTalkGetLastTimestamp(UAVTalkConnection connection, uint16_t *timestamp); uint32_t UAVTalkGetPacketObjId(UAVTalkConnection connection); diff --git a/flight/uavtalk/uavtalk.c b/flight/uavtalk/uavtalk.c index 0f9dd3d0e..47c668461 100644 --- a/flight/uavtalk/uavtalk.c +++ b/flight/uavtalk/uavtalk.c @@ -133,7 +133,7 @@ UAVTalkOutputStream UAVTalkGetOutputStream(UAVTalkConnection connectionHandle) * \param[in] connection UAVTalkConnection to be used * @param[out] statsOut Statistics counters */ -void UAVTalkGetStats(UAVTalkConnection connectionHandle, UAVTalkStats *statsOut) +void UAVTalkGetStats(UAVTalkConnection connectionHandle, UAVTalkStats *statsOut, bool reset) { UAVTalkConnectionData *connection; @@ -145,6 +145,11 @@ void UAVTalkGetStats(UAVTalkConnection connectionHandle, UAVTalkStats *statsOut) // Copy stats memcpy(statsOut, &connection->stats, sizeof(UAVTalkStats)); + if (reset) { + // Clear stats + memset(&connection->stats, 0, sizeof(UAVTalkStats)); + } + // Release lock xSemaphoreGiveRecursive(connection->lock); } @@ -154,7 +159,7 @@ void UAVTalkGetStats(UAVTalkConnection connectionHandle, UAVTalkStats *statsOut) * \param[in] connection UAVTalkConnection to be used * @param[out] statsOut Statistics counters */ -void UAVTalkAddStats(UAVTalkConnection connectionHandle, UAVTalkStats *statsOut) +void UAVTalkAddStats(UAVTalkConnection connectionHandle, UAVTalkStats *statsOut, bool reset) { UAVTalkConnectionData *connection; @@ -175,6 +180,11 @@ void UAVTalkAddStats(UAVTalkConnection connectionHandle, UAVTalkStats *statsOut) statsOut->rxSyncErrors += connection->stats.rxSyncErrors; statsOut->rxCrcErrors += connection->stats.rxCrcErrors; + if (reset) { + // Clear stats + memset(&connection->stats, 0, sizeof(UAVTalkStats)); + } + // Release lock xSemaphoreGiveRecursive(connection->lock); } From d37854dceb0d533401c6d36a83468d67c1974e0c Mon Sep 17 00:00:00 2001 From: Philippe Renon Date: Tue, 4 Mar 2014 10:01:13 +0100 Subject: [PATCH 4/7] OP-1246 fixed UAVTalkRelayPacket return type to match documentation --- flight/uavtalk/inc/uavtalk.h | 2 +- flight/uavtalk/uavtalk.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/flight/uavtalk/inc/uavtalk.h b/flight/uavtalk/inc/uavtalk.h index 9dc92187c..f66eb4a00 100644 --- a/flight/uavtalk/inc/uavtalk.h +++ b/flight/uavtalk/inc/uavtalk.h @@ -59,7 +59,7 @@ int32_t UAVTalkSendObjectTimestamped(UAVTalkConnection connectionHandle, UAVObjH int32_t UAVTalkSendObjectRequest(UAVTalkConnection connection, UAVObjHandle obj, uint16_t instId, int32_t timeoutMs); UAVTalkRxState UAVTalkProcessInputStream(UAVTalkConnection connection, uint8_t rxbyte); UAVTalkRxState UAVTalkProcessInputStreamQuiet(UAVTalkConnection connection, uint8_t rxbyte); -UAVTalkRxState UAVTalkRelayPacket(UAVTalkConnection inConnectionHandle, UAVTalkConnection outConnectionHandle); +int32_t UAVTalkRelayPacket(UAVTalkConnection inConnectionHandle, UAVTalkConnection outConnectionHandle); int32_t UAVTalkReceiveObject(UAVTalkConnection connectionHandle); void UAVTalkGetStats(UAVTalkConnection connection, UAVTalkStats *stats, bool reset); void UAVTalkAddStats(UAVTalkConnection connection, UAVTalkStats *stats, bool reset); diff --git a/flight/uavtalk/uavtalk.c b/flight/uavtalk/uavtalk.c index 47c668461..4de5a32d5 100644 --- a/flight/uavtalk/uavtalk.c +++ b/flight/uavtalk/uavtalk.c @@ -599,7 +599,7 @@ UAVTalkRxState UAVTalkProcessInputStream(UAVTalkConnection connectionHandle, uin * \return 0 Success * \return -1 Failure */ -UAVTalkRxState UAVTalkRelayPacket(UAVTalkConnection inConnectionHandle, UAVTalkConnection outConnectionHandle) +int32_t UAVTalkRelayPacket(UAVTalkConnection inConnectionHandle, UAVTalkConnection outConnectionHandle) { UAVTalkConnectionData *inConnection; @@ -666,17 +666,17 @@ UAVTalkRxState UAVTalkRelayPacket(UAVTalkConnection inConnectionHandle, UAVTalkC outConnection->stats.txBytes += (rc > 0) ? rc : 0; // evaluate return value before releasing the lock - UAVTalkRxState rxState = 0; + int32_t ret = 0; if (rc != (int32_t)(headerLength + inIproc->length + UAVTALK_CHECKSUM_LENGTH)) { outConnection->stats.txErrors++; - rxState = -1; + ret = -1; } // Release lock xSemaphoreGiveRecursive(outConnection->lock); // Done - return rxState; + return ret; } /** From 7ab38eacb6ad438d9887d09727a766b5637f138a Mon Sep 17 00:00:00 2001 From: Philippe Renon Date: Tue, 4 Mar 2014 10:02:43 +0100 Subject: [PATCH 5/7] OP-1246 removed now uneeded FIXME tag in RadioComBridge --- flight/modules/RadioComBridge/RadioComBridge.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/flight/modules/RadioComBridge/RadioComBridge.c b/flight/modules/RadioComBridge/RadioComBridge.c index 7e10557ea..66cb22a19 100644 --- a/flight/modules/RadioComBridge/RadioComBridge.c +++ b/flight/modules/RadioComBridge/RadioComBridge.c @@ -412,8 +412,8 @@ static void radioRxTask(__attribute__((unused)) void *parameters) } } else if (PIOS_COM_TELEMETRY) { // Send the data straight to the telemetry port. - // FIXME following call can fail (with -2 error code) if buffer is full - // it is the caller responsibility to retry in such cases... + // Following call can fail with -2 error code (buffer full) or -3 error code (could not acquire send mutex) + // It is the caller responsibility to retry in such cases... int32_t ret = -2; uint8_t count = 5; while (count-- > 0 && ret < -1) { @@ -512,8 +512,8 @@ static void serialRxTask(__attribute__((unused)) void *parameters) if (bytes_to_process > 0) { // Send the data over the radio link. - // FIXME following call can fail (with -2 error code) if buffer is full - // it is the caller responsibility to retry in such cases... + // Following call can fail with -2 error code (buffer full) or -3 error code (could not acquire send mutex) + // It is the caller responsibility to retry in such cases... int32_t ret = -2; uint8_t count = 5; while (count-- > 0 && ret < -1) { @@ -546,8 +546,8 @@ static int32_t UAVTalkSendHandler(uint8_t *buf, int32_t length) } #endif /* PIOS_INCLUDE_USB */ if (outputPort) { - // FIXME following call can fail (with -2 error code) if buffer is full - // it is the caller responsibility to retry in such cases... + // Following call can fail with -2 error code (buffer full) or -3 error code (could not acquire send mutex) + // It is the caller responsibility to retry in such cases... ret = -2; uint8_t count = 5; while (count-- > 0 && ret < -1) { @@ -576,8 +576,8 @@ static int32_t RadioSendHandler(uint8_t *buf, int32_t length) // Don't send any data unless the radio port is available. if (outputPort && PIOS_COM_Available(outputPort)) { - // FIXME following call can fail (with -2 error code) if buffer is full - // it is the caller responsibility to retry in such cases... + // Following call can fail with -2 error code (buffer full) or -3 error code (could not acquire send mutex) + // It is the caller responsibility to retry in such cases... int32_t ret = -2; uint8_t count = 5; while (count-- > 0 && ret < -1) { From 5d7e37f6b85b306c53312b991e97b67d1a429ab0 Mon Sep 17 00:00:00 2001 From: Philippe Renon Date: Tue, 4 Mar 2014 10:03:45 +0100 Subject: [PATCH 6/7] OP-1246 removed left over crc_table member from ground uavtalk --- ground/openpilotgcs/src/plugins/uavtalk/uavtalk.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/ground/openpilotgcs/src/plugins/uavtalk/uavtalk.h b/ground/openpilotgcs/src/plugins/uavtalk/uavtalk.h index 4d27399f9..ecda5e3f5 100644 --- a/ground/openpilotgcs/src/plugins/uavtalk/uavtalk.h +++ b/ground/openpilotgcs/src/plugins/uavtalk/uavtalk.h @@ -105,8 +105,6 @@ private: static const int TX_BUFFER_SIZE = 2 * 1024; - static const quint8 crc_table[256]; - // Types typedef enum { STATE_SYNC, STATE_TYPE, STATE_SIZE, STATE_OBJID, STATE_INSTID, STATE_DATA, STATE_CS, STATE_COMPLETE, STATE_ERROR From 37a09bbd349c33f9de2825d83247d2c2dcc750b5 Mon Sep 17 00:00:00 2001 From: Philippe Renon Date: Tue, 4 Mar 2014 10:05:17 +0100 Subject: [PATCH 7/7] OP-1246 minor cleanups in flight uavtalk --- flight/uavtalk/uavtalk.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/flight/uavtalk/uavtalk.c b/flight/uavtalk/uavtalk.c index 4de5a32d5..35e4361ef 100644 --- a/flight/uavtalk/uavtalk.c +++ b/flight/uavtalk/uavtalk.c @@ -872,7 +872,7 @@ static int32_t sendObject(UAVTalkConnectionData *connection, uint8_t type, uint3 { uint32_t numInst; uint32_t n; - uint32_t ret = -1; + int32_t ret = -1; // Important note : obj can be null (when type is NACK for example) so protect all obj dereferences. @@ -890,8 +890,8 @@ static int32_t sendObject(UAVTalkConnectionData *connection, uint8_t type, uint3 // This allows the receiver to detect when the last object has been received (i.e. when instance 0 is received) ret = 0; for (n = 0; n < numInst; ++n) { - if (sendSingleObject(connection, type, objId, numInst - n - 1, obj) == -1) { - ret = -1; + ret = sendSingleObject(connection, type, objId, numInst - n - 1, obj); + if (ret == -1) { break; } } @@ -914,8 +914,7 @@ static int32_t sendObject(UAVTalkConnectionData *connection, uint8_t type, uint3 * \param[in] connection UAVTalkConnection to be used * \param[in] type Transaction type * \param[in] objId The object ID - * \param[in] instId The instance ID (can NOT be UAVOBJ_ALL_INSTANCES, use - () instead) + * \param[in] instId The instance ID (can NOT be UAVOBJ_ALL_INSTANCES, use () instead) * \param[in] obj Object handle to send (null when type is NACK) * \return 0 Success * \return -1 Failure @@ -992,7 +991,7 @@ static int32_t sendSingleObject(UAVTalkConnectionData *connection, uint8_t type, connection->stats.txBytes += tx_msg_len; } else { connection->stats.txErrors++; - // TDOD rc == -1 connection not open, -2 buffer full should retry + // TODO rc == -1 connection not open, -2 buffer full should retry connection->stats.txBytes += (rc > 0) ? rc : 0; return -1; }