From 3d111965cc6a6629e8ac2ee6ee7b6f85f807ee5c Mon Sep 17 00:00:00 2001 From: Rashmi Vaidya Date: Thu, 9 May 2024 12:03:34 -0700 Subject: [PATCH] Error handling with retries in blockwise upload Signed-off-by: Rashmi Vaidya --- include/golioth/config.h | 8 ++ include/golioth/golioth_status.h | 3 +- src/Kconfig | 14 ++++ src/coap_blockwise.c | 138 ++++++++++++++++++++++--------- 4 files changed, 125 insertions(+), 38 deletions(-) diff --git a/include/golioth/config.h b/include/golioth/config.h index fe66ee6fa..e5640aad5 100644 --- a/include/golioth/config.h +++ b/include/golioth/config.h @@ -57,6 +57,14 @@ #define CONFIG_GOLIOTH_BLOCKWISE_DOWNLOAD_BUFFER_SIZE 1024 #endif +#ifndef CONFIG_GOLIOTH_BLOCKWISE_RETRY_COUNT +#define CONFIG_GOLIOTH_BLOCKWISE_RETRY_COUNT 3 +#endif + +#ifndef CONFIG_GOLIOTH_BLOCKWISE_RETRY_SLEEP_TIME_MS +#define CONFIG_GOLIOTH_BLOCKWISE_RETRY_SLEEP_TIME_MS 5000 +#endif + #ifndef CONFIG_GOLIOTH_OTA_THREAD_STACK_SIZE #define CONFIG_GOLIOTH_OTA_THREAD_STACK_SIZE 4096 #endif diff --git a/include/golioth/golioth_status.h b/include/golioth/golioth_status.h index 69ef21f00..27aa2c295 100644 --- a/include/golioth/golioth_status.h +++ b/include/golioth/golioth_status.h @@ -22,7 +22,8 @@ STATUS(GOLIOTH_ERR_INVALID_STATE) \ STATUS(GOLIOTH_ERR_NO_MORE_DATA) \ STATUS(GOLIOTH_ERR_NACK) \ - STATUS(GOLIOTH_ERR_BAD_REQUEST) + STATUS(GOLIOTH_ERR_BAD_REQUEST) \ + STATUS(GOLIOTH_ERR_INVALID_BLOCK_SIZE) #define GENERATE_GOLIOTH_STATUS_ENUM(code) code, enum golioth_status diff --git a/src/Kconfig b/src/Kconfig index b49a04320..4c8574b61 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -75,6 +75,20 @@ config GOLIOTH_BLOCKWISE_DOWNLOAD_BUFFER_SIZE Buffer size used in Blockwise Downloads. This size will also determine the max size of each block. +config GOLIOTH_BLOCKWISE_RETRY_COUNT + int "Retry count in blockwise transfer failures" + default 3 + help + Number of times the blockwise transfer module will retry a transfer + in case of failures. Set to -1 to retry until Success + +config GOLIOTH_BLOCKWISE_RETRY_SLEEP_TIME_MS + int "Time in ms between blockwise transfer retries" + default 5000 + help + Time in milliseconds between blockwise transfer retries in case of + failures. + config GOLIOTH_OTA_THREAD_STACK_SIZE int "Golioth OTA thread stack size" default 4096 diff --git a/src/coap_blockwise.c b/src/coap_blockwise.c index fb80cfea0..719dac129 100644 --- a/src/coap_blockwise.c +++ b/src/coap_blockwise.c @@ -10,6 +10,15 @@ #include "coap_client.h" #include "coap_blockwise.h" +#define IS_POWER_OF_TWO(x) (((x) & ((x) - 1)) == 0) + +#define IS_VALID_BLOCK_SIZE(x) \ +( \ + (x) >= 16 && (x) <= 1024 && IS_POWER_OF_TWO(x) \ +) + +LOG_TAG_DEFINE(blockwise_module); + struct blockwise_transfer { bool is_last; @@ -63,65 +72,114 @@ static void on_block_sent(struct golioth_client *client, ctx->status = response->status; } -// Function to call the application's read block callback after a successful -// blockwise upload -static void call_read_block_callback(struct blockwise_transfer *ctx) +// Function to call the application's read block callback for obtaining +// blockwise upload data +static enum golioth_status call_read_block_callback(struct blockwise_transfer *ctx) { - int err = ctx->callback.read_cb(ctx->offset, - ctx->block_buffer, - &ctx->block_size, - &ctx->is_last, - ctx->callback_arg); - if (err != GOLIOTH_OK) + size_t block_size = 0; + enum golioth_status status = ctx->callback.read_cb(ctx->offset, + ctx->block_buffer, + &block_size, + &ctx->is_last, + ctx->callback_arg); + if (status != GOLIOTH_OK) { - // TODO: handle application callback error + return status; } + + if (block_size == 0 || block_size > CONFIG_GOLIOTH_BLOCKWISE_UPLOAD_BLOCK_SIZE) + { + return GOLIOTH_ERR_INVALID_BLOCK_SIZE; + } + + if (ctx->is_last) + { + ctx->block_size = block_size; + } + else if (ctx->block_size == 0) + { + // This is the first time we called ctx->callback.read_cb() + // Make sure the block size is a valid size for blockwise uploads + if (!IS_VALID_BLOCK_SIZE(block_size)) + { + return GOLIOTH_ERR_INVALID_BLOCK_SIZE; + } + ctx->block_size = block_size; + } + else if (block_size != ctx->block_size) + { + // Size of all blocks must be the same + return GOLIOTH_ERR_INVALID_BLOCK_SIZE; + } + + return GOLIOTH_OK; } // Function to upload a single block -static enum golioth_status upload_single_block(struct golioth_client *client, - struct blockwise_transfer *ctx) +static void upload_single_block(struct golioth_client *client, + struct blockwise_transfer *ctx) { - assert(ctx->block_size > 0 && ctx->block_size <= CONFIG_GOLIOTH_BLOCKWISE_UPLOAD_BLOCK_SIZE); - return golioth_coap_client_set_block(client, - ctx->path_prefix, - ctx->path, - ctx->is_last, - GOLIOTH_CONTENT_TYPE_JSON, - ctx->offset, - ctx->block_buffer, - ctx->block_size, - on_block_sent, - ctx, - true, - GOLIOTH_SYS_WAIT_FOREVER); + enum golioth_status status = golioth_coap_client_set_block(client, + ctx->path_prefix, + ctx->path, + ctx->is_last, + ctx->content_type, + ctx->offset, + ctx->block_buffer, + ctx->block_size, + on_block_sent, + ctx, + true, + GOLIOTH_SYS_WAIT_FOREVER); + ctx->status = status; } // Function to handle blockwise upload errors -static enum golioth_status handle_upload_error() +// This function retries for all errors like invalid state, failed mem alloc, +// queue full, disconnects, etc returned by golioth_coap_client_set_block +static enum golioth_status handle_upload_error(struct golioth_client *client, + struct blockwise_transfer *ctx) { - // TODO: handle errors like disconnects etc - return GOLIOTH_OK; + while (1) + { + GLTH_LOGE(TAG, "Blockwise upload failed with error %d. Retrying.", ctx->status); + golioth_sys_msleep(CONFIG_GOLIOTH_BLOCKWISE_RETRY_SLEEP_TIME_MS); + upload_single_block(client, ctx); + if (ctx->status == GOLIOTH_OK) + { + ctx->retry_count = 0; + break; + } + else + { + if (CONFIG_GOLIOTH_BLOCKWISE_RETRY_COUNT != -1) + { + ctx->retry_count++; + if (ctx->retry_count > CONFIG_GOLIOTH_BLOCKWISE_RETRY_COUNT) + { + break; + } + } + } + } + return ctx->status; } // Function to manage blockwise upload and handle errors static enum golioth_status process_blockwise_uploads(struct golioth_client *client, struct blockwise_transfer *ctx) { - enum golioth_status status = GOLIOTH_ERR_FAIL; - ctx->block_size = 0; - call_read_block_callback(ctx); - if (ctx->block_size) + enum golioth_status status = call_read_block_callback(ctx); + if (status == GOLIOTH_OK) { - status = upload_single_block(client, ctx); + upload_single_block(client, ctx); if (ctx->status == GOLIOTH_OK) { ctx->offset++; } else { - // TODO: handle_upload_error - status = handle_upload_error(); + status = handle_upload_error(client, ctx); } } return status; @@ -134,10 +192,16 @@ enum golioth_status golioth_blockwise_post(struct golioth_client *client, read_block_cb cb, void *callback_arg) { - enum golioth_status status = GOLIOTH_ERR_FAIL; + enum golioth_status status; + if (!client || !path || !cb) { - return status; + return GOLIOTH_ERR_NULL; + } + + if (!path_prefix) + { + path_prefix = ""; } struct blockwise_transfer *ctx = malloc(sizeof(struct blockwise_transfer));