From f460ae736382593c52b568b40e258e6c4da5facd Mon Sep 17 00:00:00 2001 From: Carlos Pereira Atencio Date: Fri, 20 Dec 2024 21:54:32 +0000 Subject: [PATCH 1/2] flash_decoder: Pass address to validate_target_image function. When blocks are received out of order the data passed to board_detect_incompatible_image() via flash_decoder_validate_target_image() might not be the first block of flash data, as expected by the microbitv2 imcompatibility detection, so pass the address to the function and perform the check. --- source/board/microbitv2/microbitv2.c | 9 ++++++--- source/daplink/drag-n-drop/flash_decoder.c | 8 ++++---- source/daplink/drag-n-drop/flash_decoder.h | 1 - 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/source/board/microbitv2/microbitv2.c b/source/board/microbitv2/microbitv2.c index 8c61bc5197..9ebfc3b196 100644 --- a/source/board/microbitv2/microbitv2.c +++ b/source/board/microbitv2/microbitv2.c @@ -446,13 +446,16 @@ static uint32_t read_file_data_txt(uint32_t sector_offset, uint8_t *data, uint32 return VFS_SECTOR_SIZE; } -uint8_t board_detect_incompatible_image(const uint8_t *data, uint32_t size) +uint8_t board_detect_incompatible_image(uint32_t addr, const uint8_t *data, uint32_t size) { - uint8_t result = 0; + // We can only check image compatibility on the first block of flash data + if (addr != 0) { + return 0; + } // Check difference in vectors (mem fault, bus fault, usage fault) // If these vectors are 0, we assume it's an M0 image (not compatible) - result = memcmp(data + M0_RESERVED_VECT_OFFSET, (uint8_t[M0_RESERVED_VECT_SIZE]){0}, M0_RESERVED_VECT_SIZE); + uint8_t result = memcmp(data + M0_RESERVED_VECT_OFFSET, (uint8_t[M0_RESERVED_VECT_SIZE]){0}, M0_RESERVED_VECT_SIZE); return result == 0; } diff --git a/source/daplink/drag-n-drop/flash_decoder.c b/source/daplink/drag-n-drop/flash_decoder.c index 46b4bc7ae3..b8bfa798c2 100644 --- a/source/daplink/drag-n-drop/flash_decoder.c +++ b/source/daplink/drag-n-drop/flash_decoder.c @@ -60,7 +60,7 @@ static bool flash_type_target_bin; static bool flash_decoder_is_at_end(uint32_t addr, const uint8_t *data, uint32_t size); -__WEAK uint8_t board_detect_incompatible_image(const uint8_t *data, uint32_t size) +__WEAK uint8_t board_detect_incompatible_image(uint32_t addr, const uint8_t *data, uint32_t size) { return 0; // Return 0 if image is compatible } @@ -190,14 +190,14 @@ error_t flash_decoder_get_flash(flash_decoder_type_t type, uint32_t addr, bool a return status; } -error_t flash_decoder_validate_target_image(flash_decoder_type_t type, const uint8_t *data, uint32_t size) +error_t flash_decoder_validate_target_image(flash_decoder_type_t type, uint32_t addr, const uint8_t *data, uint32_t size) { error_t status = ERROR_SUCCESS; if (daplink_is_interface()) { if (FLASH_DECODER_TYPE_TARGET == type) { if (g_board_info.target_cfg) { - if (board_detect_incompatible_image(data, size)){ + if (board_detect_incompatible_image(addr, data, size)){ status = ERROR_FD_INCOMPATIBLE_IMAGE; } else { status = ERROR_SUCCESS; @@ -295,7 +295,7 @@ error_t flash_decoder_write(uint32_t addr, const uint8_t *data, uint32_t size) // Validate incompatible target image file if (config_get_detect_incompatible_target()){ - status = flash_decoder_validate_target_image(flash_type, flash_buf, flash_buf_pos); + status = flash_decoder_validate_target_image(flash_type, addr, flash_buf, flash_buf_pos); if (ERROR_SUCCESS != status) { state = DECODER_STATE_ERROR; diff --git a/source/daplink/drag-n-drop/flash_decoder.h b/source/daplink/drag-n-drop/flash_decoder.h index bf6b0bd8cb..2895c93d01 100644 --- a/source/daplink/drag-n-drop/flash_decoder.h +++ b/source/daplink/drag-n-drop/flash_decoder.h @@ -45,7 +45,6 @@ typedef enum { flash_decoder_type_t flash_decoder_detect_type(const uint8_t *data, uint32_t size, uint32_t addr, bool addr_valid); error_t flash_decoder_get_flash(flash_decoder_type_t type, uint32_t addr, bool addr_valid, uint32_t *start_addr, const flash_intf_t **flash_intf); -error_t flash_decoder_validate_target_image(flash_decoder_type_t type, const uint8_t *data, uint32_t size); error_t flash_decoder_open(void); error_t flash_decoder_write(uint32_t addr, const uint8_t *data, uint32_t size); error_t flash_decoder_close(void); From f083cac4449cb6b36c475d794d54dba5e915aabd Mon Sep 17 00:00:00 2001 From: Carlos Pereira Atencio Date: Tue, 7 Jan 2025 23:06:30 +0000 Subject: [PATCH 2/2] Update vfs_manager & file_stream for out-of-order Universal Hex blocks. When Universalh Hex was created two formats where specified, the segment format, which is currently used due to faster parsing, and the block format. The block format was design so that each 512-bytes of hex characters contain enough info to be self contained, however it was never tested in a systen that sent blocks out-of-order. The original vfs_manager expects all file blocks to arrive sequentially in order. This update adds a check of "self contained blocks" and a sticky flag that process these blocks whatever order they arrive and ignores all other non-self-contained vfs sectors received. --- source/daplink/drag-n-drop/file_stream.c | 100 ++++++++++++++++++++- source/daplink/drag-n-drop/file_stream.h | 10 +++ source/daplink/drag-n-drop/flash_decoder.c | 3 + source/daplink/drag-n-drop/vfs_manager.c | 48 ++++++---- source/daplink/validation.c | 9 ++ source/daplink/validation.h | 1 + 6 files changed, 150 insertions(+), 21 deletions(-) diff --git a/source/daplink/drag-n-drop/file_stream.c b/source/daplink/drag-n-drop/file_stream.c index 284ccc9f82..062ce80cc0 100644 --- a/source/daplink/drag-n-drop/file_stream.c +++ b/source/daplink/drag-n-drop/file_stream.c @@ -30,6 +30,16 @@ #include "compiler.h" #include "validation.h" +// Set to 1 to enable debugging +#define DEBUG_FILE_STREAM 0 + +#if DEBUG_FILE_STREAM +#include "daplink_debug.h" +#define stream_printf debug_msg +#else +#define stream_printf(...) +#endif + typedef enum { STREAM_STATE_CLOSED, STREAM_STATE_OPEN, @@ -75,9 +85,15 @@ static error_t open_hex(void *state); static error_t write_hex(void *state, const uint8_t *data, uint32_t size); static error_t close_hex(void *state); -stream_t stream[] = { +static bool detect_uhex_blocks(const uint8_t *data, uint32_t size); +static error_t open_uhex_blocks(void *state); +static error_t write_uhex_blocks(void *state, const uint8_t *data, uint32_t size); +static error_t close_uhex_blocks(void *state); + +static stream_t stream[] = { {detect_bin, open_bin, write_bin, close_bin}, // STREAM_TYPE_BIN {detect_hex, open_hex, write_hex, close_hex}, // STREAM_TYPE_HEX + {detect_uhex_blocks, open_uhex_blocks, write_uhex_blocks, close_uhex_blocks}, // STREAM_TYPE_UHEX_BLOCKS }; COMPILER_ASSERT(ARRAY_SIZE(stream) == STREAM_TYPE_COUNT); // STREAM_TYPE_NONE must not be included in count @@ -104,6 +120,7 @@ stream_type_t stream_start_identify(const uint8_t *data, uint32_t size) for (i = STREAM_TYPE_START; i < STREAM_TYPE_COUNT; i++) { if (stream[i].detect(data, size)) { + stream_printf("file_stream start_identify stream=%i\r\n", i); return i; } } @@ -118,12 +135,48 @@ stream_type_t stream_type_from_name(const vfs_filename_t filename) if (0 == strncmp("BIN", &filename[8], 3)) { return STREAM_TYPE_BIN; } else if (0 == strncmp("HEX", &filename[8], 3)) { - return STREAM_TYPE_HEX; + return STREAM_TYPE_HEX_OR_UHEX; } else { return STREAM_TYPE_NONE; } } +bool stream_compatible(stream_type_t type_left, stream_type_t type_right) +{ + if (type_left == type_right) { + return true; + } + + if ((type_left == STREAM_TYPE_HEX_OR_UHEX && + (type_right == STREAM_TYPE_HEX || type_right == STREAM_TYPE_UHEX_BLOCKS)) || + (type_right == STREAM_TYPE_HEX_OR_UHEX && + (type_left == STREAM_TYPE_HEX || type_left == STREAM_TYPE_UHEX_BLOCKS))) { + return true; + } + + return false; +} + +bool stream_self_contained_block(stream_type_t type, const uint8_t *data, uint32_t size) +{ + switch (type) { + case STREAM_TYPE_BIN: + return false; + + case STREAM_TYPE_HEX: + // A hex stream can also be a Universal Hex stream + return validate_uhex_block(data, size) ? true : false; + + case STREAM_TYPE_UHEX_BLOCKS: + // The Universal Hex stream can be ordered (sectors) or unordered (blocks) + return validate_uhex_block(data, size) ? true : false; + + default: + util_assert(0); + return false; + } +} + error_t stream_open(stream_type_t stream_type) { error_t status; @@ -147,6 +200,7 @@ error_t stream_open(stream_type_t stream_type) current_stream = &stream[stream_type]; // Initialize the specified stream status = current_stream->open(&shared_state); + stream_printf("file_stream stream_open(type=%d); open ret=%d\r\n", stream_type, status); if (ERROR_SUCCESS != status) { state = STREAM_STATE_ERROR; @@ -170,6 +224,7 @@ error_t stream_write(const uint8_t *data, uint32_t size) stream_thread_assert(); // Write to stream status = current_stream->write(&shared_state, data, size); + stream_printf("file_stream stream_write(size=%d); write ret=%d\r\n", size, status); if (ERROR_SUCCESS_DONE == status) { state = STREAM_STATE_END; @@ -198,6 +253,7 @@ error_t stream_close(void) stream_thread_assert(); // Close stream status = current_stream->close(&shared_state); + stream_printf("file_stream stream_close; close ret=%d\r\n", status); state = STREAM_STATE_CLOSED; return status; } @@ -289,6 +345,13 @@ static error_t close_bin(void *state) static bool detect_hex(const uint8_t *data, uint32_t size) { + // Both Universal Hex formats will pass the normal hex file validation, + // but a Universal Hex in block format needs to be processed with the + // STREAM_TYPE_UHEX_BLOCKS stream. + // A Universal Hex in segment format can be be processed as a normal hex. + if (1 == validate_uhex_block(data, size)) { + return false; + } return 1 == validate_hexfile(data); } @@ -315,6 +378,7 @@ static error_t write_hex(void *state, const uint8_t *data, uint32_t size) while (1) { // try to decode a block of hex data into bin data parse_status = parse_hex_blob(data, size, &block_amt_parsed, hex_state->bin_buffer, sizeof(hex_state->bin_buffer), &bin_start_address, &bin_buf_written); + stream_printf("file_stream write_hex; parse_hex_blob ret=%d, bin_buf_written=%d\r\n", parse_status, bin_buf_written); // the entire block of hex was decoded. This is a simple state if (HEX_PARSE_OK == parse_status) { @@ -364,3 +428,35 @@ static error_t close_hex(void *state) status = flash_decoder_close(); return status; } + +/* Universal Hex, block format, file processing */ +/* https://tech.microbit.org/software/spec-universal-hex/ */ +/* The Universal Hex segment format is processed by the Intel Hex parser. */ +/* This stream is for the Universal Hex block format only. */ + +static bool detect_uhex_blocks(const uint8_t *data, uint32_t size) +{ + return 1 == validate_uhex_block(data, size); +} + +static inline error_t open_uhex_blocks(void *state) +{ + return open_hex(state); +} + +static inline error_t write_uhex_blocks(void *state, const uint8_t *data, uint32_t size) +{ + error_t status = write_hex(state, data, size); + + // The block containing the EoF record could arrive at any point + if (ERROR_SUCCESS_DONE == status || ERROR_SUCCESS == status) { + status = ERROR_SUCCESS_DONE_OR_CONTINUE; + } + + return status; +} + +static inline error_t close_uhex_blocks(void *state) +{ + return close_hex(state); +} diff --git a/source/daplink/drag-n-drop/file_stream.h b/source/daplink/drag-n-drop/file_stream.h index 50bf38e08b..605ca01b47 100644 --- a/source/daplink/drag-n-drop/file_stream.h +++ b/source/daplink/drag-n-drop/file_stream.h @@ -36,11 +36,15 @@ typedef enum { STREAM_TYPE_BIN = STREAM_TYPE_START, STREAM_TYPE_HEX, + STREAM_TYPE_UHEX_BLOCKS, // Add new stream types here STREAM_TYPE_COUNT, + // Needed as both formats share the same .hex extension + STREAM_TYPE_HEX_OR_UHEX, + STREAM_TYPE_NONE } stream_type_t; @@ -50,6 +54,12 @@ stream_type_t stream_start_identify(const uint8_t *data, uint32_t size); // Stateless function to identify a filestream by its name stream_type_t stream_type_from_name(const vfs_filename_t filename); +// Stateless function to identify if two streams are compatible +bool stream_compatible(stream_type_t type_left, stream_type_t type_right); + +// Stateless function to identify if a stream can take blocks out of order +bool stream_self_contained_block(stream_type_t type, const uint8_t *data, uint32_t size); + error_t stream_open(stream_type_t stream_type); error_t stream_write(const uint8_t *data, uint32_t size); diff --git a/source/daplink/drag-n-drop/flash_decoder.c b/source/daplink/drag-n-drop/flash_decoder.c index b8bfa798c2..7551a303a6 100644 --- a/source/daplink/drag-n-drop/flash_decoder.c +++ b/source/daplink/drag-n-drop/flash_decoder.c @@ -374,6 +374,9 @@ error_t flash_decoder_close(void) if ((DECODER_STATE_DONE != prev_state) && (flash_type != FLASH_DECODER_TYPE_TARGET) && (status == ERROR_SUCCESS)) { + flash_decoder_printf(" error: update incomplete\r\n"); + flash_decoder_printf(" prev_state=%d; flash_type=%d; status=%d\r\n", + prev_state, flash_type, status); status = ERROR_IAP_UPDT_INCOMPLETE; } diff --git a/source/daplink/drag-n-drop/vfs_manager.c b/source/daplink/drag-n-drop/vfs_manager.c index 22f9dde336..87648e50cf 100644 --- a/source/daplink/drag-n-drop/vfs_manager.c +++ b/source/daplink/drag-n-drop/vfs_manager.c @@ -85,6 +85,7 @@ typedef struct { bool stream_started; // Stream processing started. This only gets reset remount bool stream_finished; // Stream processing is done. This only gets reset remount bool stream_optional_finish; // True if the stream processing can be considered done + bool stream_ooo_blocks; // True if the stream data blocks can be processed out of order bool file_info_optional_finish; // True if the file transfer can be considered done bool transfer_timeout; // Set if the transfer was finished because of a timeout. This only gets reset remount stream_type_t stream; // Current stream or STREAM_TYPE_NONE is stream is closed. This only gets reset remount @@ -112,6 +113,7 @@ static const file_transfer_state_t default_transfer_state = { false, false, false, + false, STREAM_TYPE_NONE, }; @@ -439,13 +441,13 @@ static void file_change_handler(const vfs_filename_t filename, vfs_file_change_t static void file_data_handler(uint32_t sector, const uint8_t *buf, uint32_t num_of_sectors) { stream_type_t stream; - uint32_t size; + uint32_t size = VFS_SECTOR_SIZE * num_of_sectors; // this is the key for starting a file write - we dont care what file types are sent // just look for something unique (NVIC table, hex, srec, etc) until root dir is updated if (!file_transfer_state.stream_started) { // look for file types we can program - stream = stream_start_identify((uint8_t *)buf, VFS_SECTOR_SIZE * num_of_sectors); + stream = stream_start_identify((uint8_t *)buf, size); if (STREAM_TYPE_NONE != stream) { transfer_stream_open(stream, sector); @@ -453,13 +455,25 @@ static void file_data_handler(uint32_t sector, const uint8_t *buf, uint32_t num_ } if (file_transfer_state.stream_started) { - // Ignore sectors coming before this file - if (sector < file_transfer_state.start_sector) { - return; - } + bool self_contained_block = stream_self_contained_block(file_transfer_state.stream, buf, size); - // sectors must be in order - if (sector != file_transfer_state.file_next_sector) { + if (self_contained_block) { + vfs_mngr_printf("vfs_manager file_data_handler sector=%i\r\n", sector); + vfs_mngr_printf(" sector self-contained\r\n"); + file_transfer_state.stream_ooo_blocks = true; + } else if (file_transfer_state.stream_ooo_blocks) { + // Invalid block for a self-contained stream, can be ignored + vfs_mngr_printf("vfs_manager file_data_handler sector=%i\r\n", sector); + vfs_mngr_printf(" invalid data for self-contained stream\r\n"); + vfs_mngr_printf(" size=%d data=%x,%x,%x,%x,...,%x,%x,%x,%x\r\n", + size, buf[0], buf[1], buf[2], buf[3], buf[size - 4], + buf[size - 3], buf[size - 2], buf[size - 1]); + return; + } else if (sector < file_transfer_state.start_sector) { + // For ordered streams, ignore sectors coming before this file + return; + } else if (sector != file_transfer_state.file_next_sector) { + // For ordered streams, sector must be the next in the sequence vfs_mngr_printf("vfs_manager file_data_handler sector=%i\r\n", sector); if (sector < file_transfer_state.file_next_sector) { @@ -482,7 +496,6 @@ static void file_data_handler(uint32_t sector, const uint8_t *buf, uint32_t num_ } // This sector could be part of the file so record it - size = VFS_SECTOR_SIZE * num_of_sectors; file_transfer_state.size_transferred += size; file_transfer_state.file_next_sector = sector + num_of_sectors; @@ -584,15 +597,6 @@ static void transfer_update_file_info(vfs_file_t file, uint32_t start_sector, ui } } - // Initialize the stream if it has not been set - if (STREAM_TYPE_NONE == file_transfer_state.stream) { - file_transfer_state.stream = stream; - - if (stream != STREAM_TYPE_NONE) { - vfs_mngr_printf(" stream=%i\r\n", stream); - } - } - // Check - File size must either grow or be smaller than the size already transferred if ((size < file_transfer_state.file_size) && (size < file_transfer_state.size_transferred) && (size > 0)) { vfs_mngr_printf(" error: file size changed from %i to %i\r\n", file_transfer_state.file_size, size); @@ -608,7 +612,7 @@ static void transfer_update_file_info(vfs_file_t file, uint32_t start_sector, ui } // Check - stream must be the same - if ((stream != STREAM_TYPE_NONE) && (stream != file_transfer_state.stream)) { + if (stream != STREAM_TYPE_NONE && file_transfer_state.stream != STREAM_TYPE_NONE && !stream_compatible(file_transfer_state.stream, stream)) { vfs_mngr_printf(" error: changed types during transfer from %i to %i\r\n", file_transfer_state.stream, stream); transfer_update_state(ERROR_ERROR_DURING_TRANSFER); return; @@ -761,6 +765,12 @@ static void transfer_update_state(error_t status) (file_transfer_state.size_transferred >= file_transfer_state.file_size) && (file_transfer_state.file_size > 0) && (file_transfer_state.start_sector == file_transfer_state.file_start_sector); + if (file_transfer_state.stream_ooo_blocks && + file_transfer_state.file_size > 0 && + file_transfer_state.size_transferred >= file_transfer_state.file_size) { + vfs_mngr_printf(" OoO file_info_optional_finish=%d\r\n", file_transfer_state.file_info_optional_finish); + file_transfer_state.file_info_optional_finish = true; + } transfer_timeout = file_transfer_state.transfer_timeout; transfer_started = (VFS_FILE_INVALID != file_transfer_state.file_to_program) || (STREAM_TYPE_NONE != file_transfer_state.stream); diff --git a/source/daplink/validation.c b/source/daplink/validation.c index 0a3134f890..ba719b6d00 100644 --- a/source/daplink/validation.c +++ b/source/daplink/validation.c @@ -94,3 +94,12 @@ uint8_t validate_hexfile(const uint8_t *buf) return ((buf[0] == ':') && ((buf[8] == '0') || (buf[8] == '2') || (buf[8] == '3') || (buf[8] == '4') || (buf[8] == '5'))) ? 1 : 0; } } + +uint8_t validate_uhex_block(const uint8_t *buf, uint32_t size) { + if (size != 512) { + return 0; + } + return (memcmp(buf, (const void *)":02000004", 9) == 0) && + (memcmp(buf + 16, (const void *)":0400000A", 9) == 0) && + (buf[511] == '\n'); +} diff --git a/source/daplink/validation.h b/source/daplink/validation.h index 929d827fb6..19e4ea60b8 100644 --- a/source/daplink/validation.h +++ b/source/daplink/validation.h @@ -30,6 +30,7 @@ extern "C" { uint8_t validate_bin_nvic(const uint8_t *buf); uint8_t validate_hexfile(const uint8_t *buf); +uint8_t validate_uhex_block(const uint8_t *buf, uint32_t size); /*! * @brief Baseline implementation of NVIC validator.