Merge pull request #76 from pine64/hex-fixup

Adding Hex file support

.github/workflows/build.yml

@@ -26,7 +26,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4
         with:
-          submodules: 'recursive'
+          submodules: "recursive"
       - uses: lukka/get-cmake@latest
       - name: Build blisp tool
         run: |
@@ -47,7 +47,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4
         with:
-          submodules: 'recursive'
+          submodules: "recursive"
       - uses: lukka/get-cmake@latest
       - name: Build blisp tool
         run: |
@@ -68,7 +68,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4
         with:
-          submodules: 'recursive'
+          submodules: "recursive"
       - uses: lukka/get-cmake@latest
       - name: Build blisp tool
         run: |
@@ -101,7 +101,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4
         with:
-          submodules: 'recursive'
+          submodules: "recursive"
       - uses: uraimo/run-on-arch-action@v2
         name: Build artifact
         id: build
@@ -155,3 +155,29 @@ jobs:
           path: |
             artifacts/blisp-*
           if-no-files-found: error
+
+  test-linux:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: "recursive"
+      - uses: lukka/get-cmake@latest
+      - name: Build blisp tool & unit tests
+        run: |
+          mkdir build
+          cd build
+          cmake .. -DBLISP_BUILD_CLI=ON -DCOMPILE_TESTS=ON
+          cmake --build .
+      - name: Run unit tests
+        run: |
+          cd build
+          for f in $(find . -type f -executable -iname "*_test"); do
+            echo "Running test file $f"
+            "$f"
+            status=$?
+            if [ $status -ne 0 ]; then
+              echo "Test $f failed with exit code $status"
+              exit $status
+            fi
+          done

.gitignore

@@ -78,3 +78,4 @@ fabric.properties
 build/
 
 .DS_Store
+.cache/

CMakeLists.txt

@@ -48,13 +48,13 @@ if(BLISP_USE_SYSTEM_LIBRARIES)
     target_link_libraries(libblisp_static PUBLIC Libserialport::Libserialport)
     target_include_directories(libblisp_obj PUBLIC ${Libserialport_INCLUDE_DIRS})
 else()
-	if(NOT ${CMAKE_SYSTEM_NAME} STREQUAL "FreeBSD")
+    if(NOT ${CMAKE_SYSTEM_NAME} STREQUAL "FreeBSD")
-	target_sources(libblisp_obj PRIVATE
+        target_sources(libblisp_obj PRIVATE
         ${CMAKE_SOURCE_DIR}/vendor/libserialport/serialport.c
         ${CMAKE_SOURCE_DIR}/vendor/libserialport/timing.c)
 
-	target_include_directories(libblisp_obj PRIVATE ${CMAKE_SOURCE_DIR}/vendor/libserialport)
+        target_include_directories(libblisp_obj PRIVATE ${CMAKE_SOURCE_DIR}/vendor/libserialport)
-	endif()
+    endif()
 
     if(WIN32)
         target_link_libraries(libblisp PRIVATE Setupapi.lib)
@@ -105,5 +105,21 @@ endif()
 
 
 if(COMPILE_TESTS)
-    add_subdirectory(tools/blisp/src/cmd/dfu/tests)
+    # Bring in googletest & C++
+    enable_language(CXX)
+    enable_testing()
+    include(FetchContent)
+
+
+    FetchContent_Declare(
+        googletest
+        GIT_REPOSITORY https://github.com/google/googletest.git
+        GIT_TAG v1.17.0
+    )
+    FetchContent_MakeAvailable(googletest)
+    add_library(GTest::GTest INTERFACE IMPORTED)
+    target_link_libraries(GTest::GTest INTERFACE gtest_main)
+
+    add_subdirectory(tools/blisp/src/file_parsers/dfu/tests)
+    add_subdirectory(tools/blisp/src/file_parsers/hex/tests)
 endif(COMPILE_TESTS)

README.md

@@ -94,6 +94,15 @@ Because this is done at the lowest level of serial communication, the
 displays aren't packet-aware or know about the chip's command set or such.
 This is really only useful for debugging systems-level issues withing
 the device or blisp itself.
+## Running unit tests
+
+```shell
+mkdir build && cd build
+cmake -DBLISP_BUILD_CLI=ON -DCOMPILE_TESTS=ON ..
+cmake --build .
+# Find all compiled unit test files; you can now run these directly
+find . -type f -executable -iname "*_test" -print
+```
 
 ## Troubleshooting
 

tools/blisp/src/file_parsers/CMakeLists.txt

@@ -1,6 +1,7 @@
 list(APPEND ADD_INCLUDE
 "${CMAKE_CURRENT_SOURCE_DIR}/bin"
 "${CMAKE_CURRENT_SOURCE_DIR}/dfu"
+"${CMAKE_CURRENT_SOURCE_DIR}/hex"
 "${CMAKE_CURRENT_SOURCE_DIR}"
 )
 
@@ -12,6 +13,7 @@ add_library(file_parsers STATIC
 "${CMAKE_CURRENT_SOURCE_DIR}/bin/bin_file.c"
 "${CMAKE_CURRENT_SOURCE_DIR}/dfu/dfu_file.c"
 "${CMAKE_CURRENT_SOURCE_DIR}/dfu/dfu_crc.c"
+"${CMAKE_CURRENT_SOURCE_DIR}/hex/hex_file.c"
 "${CMAKE_CURRENT_SOURCE_DIR}/parse_file.c"
 "${CMAKE_CURRENT_SOURCE_DIR}/get_file_contents.c"
 )
@@ -19,6 +21,7 @@ add_library(file_parsers STATIC
 target_include_directories(file_parsers PUBLIC
 ${CMAKE_CURRENT_SOURCE_DIR}/bin
 ${CMAKE_CURRENT_SOURCE_DIR}/dfu
+${CMAKE_CURRENT_SOURCE_DIR}/hex
 ${CMAKE_CURRENT_SOURCE_DIR}
 
 )

tools/blisp/src/file_parsers/dfu/dfu_file.c

@@ -78,6 +78,7 @@ int dfu_file_parse(const char* file_path_on_disk,
                    size_t* payload_address) {
   uint8_t* dfu_file_contents = NULL;
   ssize_t file_size = get_file_contents(file_path_on_disk, &dfu_file_contents);
+  // Bubble up the result if it was an error instead of size (a negative value)
   if (file_size < 0) {
     return file_size;
   }

tools/blisp/src/file_parsers/dfu/tests/CMakeLists.txt

@@ -1,31 +1,10 @@
-enable_language(CXX)
+add_executable(dfu_file_test test_dfu_file.cpp ../dfu_file.c ../dfu_crc.c ../../get_file_contents.c)
-
-enable_testing()
-include(FetchContent)
-
-
-FetchContent_Declare(
-        googletest
-        GIT_REPOSITORY https://github.com/google/googletest.git
-        GIT_TAG release-1.11.0
-)
-FetchContent_MakeAvailable(googletest)
-add_library(GTest::GTest INTERFACE IMPORTED)
-target_link_libraries(GTest::GTest INTERFACE gtest_main)
-
-
-add_executable(dfu_file_test test_dfu_file.cpp ../dfu_file.c ../dfu_crc.c)
 
 target_link_libraries(dfu_file_test
         PRIVATE
         GTest::GTest
         )
-include_directories(dfu_file_test PRIVATE ../)
+include(GoogleTest)
-add_test(dfu_file_test_gtests dfu_file_test)
+include_directories(dfu_file_test PRIVATE ../ ../../)
-
+target_compile_definitions(dfu_file_test PUBLIC "SOURCE_DIR=\"${CMAKE_SOURCE_DIR}\"")
-configure_file(Config.h.in ${CMAKE_BINARY_DIR}/Config.h)
+gtest_discover_tests(dfu_file_test)
-include_directories(${CMAKE_BINARY_DIR})
-
-set(TEST_APP_NAME dfu_file_tests)
-
-#add_custom_command(TARGET ${TEST_APP_NAME} COMMAND ./${TEST_APP_NAME} POST_BUILD)

tools/blisp/src/file_parsers/dfu/tests/Config.h.in

@@ -1,10 +0,0 @@
-//
-// Created by ralim on 26/09/22.
-//
-
-#ifndef BLISP_CONFIG_H_IN_H
-#define BLISP_CONFIG_H_IN_H
-
-#define SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}"
-
-#endif // BLISP_CONFIG_H_IN_H

tools/blisp/src/file_parsers/dfu/tests/test_dfu_file.cpp

@@ -2,16 +2,16 @@
 // Created by ralim on 26/09/22.
 //
 
-#include "Config.h"
-#include "dfu_file.h"
 #include <gtest/gtest.h>
+#include "dfu_file.h"
 TEST(DFU_FILE_PARSER, ParseTestFile) {
-    uint8_t* payload = nullptr;
+  uint8_t* payload = nullptr;
-    size_t payload_size = 0;
+  size_t payload_size = 0;
-    size_t payload_address = 0;
+  size_t payload_address = 0;
-    int res = dfu_file_parse(SOURCE_DIR "/test.dfu", &payload, &payload_size,
+  int res = dfu_file_parse(SOURCE_DIR
-                             &payload_address);
+                           "/tools/blisp/src/file_parsers/dfu/tests/test.dfu",
-    ASSERT_EQ(res, 1);
+                           &payload, &payload_size, &payload_address);
-    ASSERT_EQ(payload_size, 1337);
+  ASSERT_EQ(res, 1);
-    ASSERT_EQ(payload_address, 0x11223344);
+  ASSERT_EQ(payload_size, 1337);
+  ASSERT_EQ(payload_address, 0x11223344);
 }

tools/blisp/src/file_parsers/hex/hex_file.c

@@ -0,0 +1,261 @@
+#include "hex_file.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include "parse_file.h"
+
+// Convert ASCII hex character to integer
+static int hex_to_int(char c) {
+  if (c >= '0' && c <= '9')
+    return c - '0';
+  if (c >= 'A' && c <= 'F')
+    return c - 'A' + 10;
+  if (c >= 'a' && c <= 'f')
+    return c - 'a' + 10;
+  return -1;
+}
+
+// Convert 2 ASCII hex characters to a byte
+static int hex_byte_to_int(const char* str) {
+  int high = hex_to_int(str[0]);
+  int low = hex_to_int(str[1]);
+  if (high < 0 || low < 0)
+    return -1;
+  return (high << 4) | low;
+}
+
+// Parse a single Intel HEX line into the record type and data
+// Returns: Record Type on success, negative error code on failure
+static int parse_hex_line(const char* line,
+                          uint8_t* data_buffer,
+                          uint32_t* address,
+                          uint32_t* base_address,
+                          uint32_t* max_address,
+                          uint32_t min_address) {
+  size_t len = strlen(line);
+
+  // Line must start with ':' and have at least 11 characters (:BBAAAATTCC)
+  if (line[0] != ':' || len < 11) {
+    return HEX_PARSE_ERROR_INVALID_FORMAT;
+  }
+
+  // Extract record fields
+  int byte_count = hex_byte_to_int(line + 1);
+  if (byte_count < 0)
+    return HEX_PARSE_ERROR_INVALID_FORMAT;
+
+  // Make sure line is long enough
+  if (len < (size_t)(11 + byte_count * 2)) {
+    return HEX_PARSE_ERROR_INVALID_FORMAT;
+  }
+
+  int addr_high = hex_byte_to_int(line + 3);
+  int addr_low = hex_byte_to_int(line + 5);
+  if (addr_high < 0 || addr_low < 0)
+    return HEX_PARSE_ERROR_INVALID_FORMAT;
+
+  uint16_t record_address = (addr_high << 8) | addr_low;
+
+  int record_type = hex_byte_to_int(line + 7);
+  if (record_type < 0)
+    return HEX_PARSE_ERROR_INVALID_FORMAT;
+
+  // Verify checksum
+  uint8_t checksum = 0;
+  for (int i = 1; i < 9 + byte_count * 2; i += 2) {
+    int value = hex_byte_to_int(line + i);
+    if (value < 0) {
+      return HEX_PARSE_ERROR_INVALID_FORMAT;
+    }
+    checksum += value;
+  }
+
+  int file_checksum = hex_byte_to_int(line + 9 + byte_count * 2);
+  if (file_checksum < 0) {
+    return HEX_PARSE_ERROR_INVALID_FORMAT;
+  }
+  checksum = ~checksum + 1;  // Two's complement
+  // Verify checksum
+  if (checksum != file_checksum) {
+    return HEX_PARSE_ERROR_CHECKSUM;
+  }
+
+  // Process the record based on record type
+  switch (record_type) {
+    case HEX_RECORD_DATA: {
+      uint32_t absolute_address = *base_address + record_address;
+      *address = absolute_address;
+
+      // Update max address if this data extends beyond current max
+      if (absolute_address + byte_count > *max_address) {
+        *max_address = absolute_address + byte_count;
+      }
+
+      // Parse data bytes
+      for (int i = 0; i < byte_count; i++) {
+        int value = hex_byte_to_int(line + 9 + i * 2);
+        if (value < 0)
+          return HEX_PARSE_ERROR_INVALID_FORMAT;
+
+        // Make sure we don't write outside our buffer
+        if (data_buffer != NULL) {
+          size_t buf_offset = absolute_address - min_address + i;
+          data_buffer[buf_offset] = value;
+        }
+      }
+      break;
+    }
+
+    case HEX_RECORD_EOF:
+      // End of file, nothing to do
+      break;
+
+    case HEX_RECORD_EXTENDED_SEGMENT:
+      // Set segment base address (offset = value * 16)
+      if (byte_count != 2)
+        return HEX_PARSE_ERROR_INVALID_FORMAT;
+      int value_high = hex_byte_to_int(line + 9);
+      int value_low = hex_byte_to_int(line + 11);
+      if (value_high < 0 || value_low < 0)
+        return HEX_PARSE_ERROR_INVALID_FORMAT;
+      *base_address = ((value_high << 8) | value_low) << 4;
+      break;
+
+    case HEX_RECORD_EXTENDED_LINEAR:
+      // Set high-order 16 bits of address
+      if (byte_count != 2)
+        return HEX_PARSE_ERROR_INVALID_FORMAT;
+      value_high = hex_byte_to_int(line + 9);
+      value_low = hex_byte_to_int(line + 11);
+      if (value_high < 0 || value_low < 0)
+        return HEX_PARSE_ERROR_INVALID_FORMAT;
+      *base_address = ((value_high << 8) | value_low) << 16;
+      break;
+
+    case HEX_RECORD_START_LINEAR:
+      // Start linear address - store as a potential entry point
+      // but not crucial for firmware extraction
+      break;
+
+    case HEX_RECORD_START_SEGMENT:
+      // Start segment address - similar to above
+      break;
+
+    default:
+      return HEX_PARSE_ERROR_UNSUPPORTED_RECORD;
+  }
+
+  return record_type;
+}
+
+int hex_file_parse(const char* file_path_on_disk,
+                   uint8_t** payload,
+                   size_t* payload_length,
+                   size_t* payload_address) {
+  FILE* file = fopen(file_path_on_disk, "r");
+  if (!file) {
+    fprintf(stderr, "Could not open file %s for reading\n", file_path_on_disk);
+    return PARSED_ERROR_CANT_OPEN_FILE;
+  }
+
+  // First pass: Find start and end addresses, and thus size of the payload
+  char line[512];
+  uint32_t base_address = 0;
+  uint32_t address = 0;
+  uint32_t min_address = 0xFFFFFFFF;
+  uint32_t max_address = 0;
+  bool found_data = false;
+
+  // First pass to determine memory range
+  while (fgets(line, sizeof(line), file)) {
+    size_t len = strlen(line);
+    // Trim trailing newline and carriage return
+    while (len > 0 && (line[len - 1] == '\n' || line[len - 1] == '\r')) {
+      line[--len] = 0;
+    }
+    if (len == 0 || line[0] != ':')
+      continue;
+
+    int result =
+        parse_hex_line(line, NULL, &address, &base_address, &max_address, 0);
+    if (result < 0) {
+      fclose(file);
+      return result;
+    }
+
+    // Check if this is a data record (type 0)
+    if (result == HEX_RECORD_DATA) {
+      found_data = true;
+      if (address < min_address) {
+        min_address = address;
+      }
+    }
+
+    // If we hit EOF record, we're done
+    if (result == HEX_RECORD_EOF) {
+      break;
+    }
+  }
+
+  // If no data was found, return error
+  if (!found_data) {
+    fclose(file);
+    return HEX_PARSE_ERROR_INVALID_FORMAT;
+  }
+
+  // Calculate payload size
+  size_t size = max_address - min_address;
+  if (size > (1024 * 1024 * 128)) {  // Limit to 128 MB
+    fclose(file);
+    return HEX_PARSE_ERROR_TOO_LARGE;
+  }
+  // Allocate memory for the payload
+  *payload = (uint8_t*)calloc(size, sizeof(uint8_t));
+  if (!*payload) {
+    fclose(file);
+    return -1;
+  }
+
+  // Clear the memory to ensure all bytes are initialized
+  memset(*payload, 0, size);
+
+  // Second pass: actually parse the data and fill out the buffer with the data
+  rewind(file);
+  base_address = 0;
+
+  while (fgets(line, sizeof(line), file)) {
+    size_t len = strlen(line);
+    // Trim trailing newline and carriage return
+    while (len > 0 && (line[len - 1] == '\n' || line[len - 1] == '\r')) {
+      line[--len] = 0;
+    }
+    if (len == 0 || line[0] != ':')
+      continue;
+
+    // When parsing for real, data is written to the payload buffer
+    // with addresses relative to min_address
+    uint32_t dummy_max = 0;
+
+    int result = parse_hex_line(line, *payload, &address, &base_address,
+                                &dummy_max, min_address);
+    if (result < 0) {
+      free(*payload);
+      *payload = NULL;
+      fclose(file);
+      return result;
+    }
+
+    // If we hit EOF record, we're done
+    if (result == HEX_RECORD_EOF) {
+      break;
+    }
+  }
+
+  fclose(file);
+
+  // Set output parameters
+  *payload_length = size;
+  *payload_address = min_address;
+
+  return 0;
+}

tools/blisp/src/file_parsers/hex/hex_file.h

@@ -0,0 +1,49 @@
+//
+// Created for Intel HEX file parsing
+//
+
+#ifndef BLISP_HEX_FILE_H
+#define BLISP_HEX_FILE_H
+
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+// Error codes specific to hex parsing
+#define HEX_PARSE_ERROR_INVALID_FORMAT -0x2001
+#define HEX_PARSE_ERROR_CHECKSUM -0x2002
+#define HEX_PARSE_ERROR_UNSUPPORTED_RECORD -0x2003
+#define HEX_PARSE_ERROR_TOO_LARGE -0x2004
+
+// Intel HEX record types
+typedef enum {
+  HEX_RECORD_DATA = 0x00,              // Data record
+  HEX_RECORD_EOF = 0x01,               // End of file record
+  HEX_RECORD_EXTENDED_SEGMENT = 0x02,  // Extended segment address record
+  HEX_RECORD_START_SEGMENT = 0x03,     // Start segment address record
+  HEX_RECORD_EXTENDED_LINEAR = 0x04,   // Extended linear address record
+  HEX_RECORD_START_LINEAR = 0x05       // Start linear address record
+} hex_record_type_t;
+
+// Parse an Intel HEX file and return a contiguous memory block
+// Parameters:
+//   file_path_on_disk: Path to the Intel HEX file
+//   payload: Pointer to the buffer that will hold the parsed data
+//   payload_length: Size of the payload
+//   payload_address: Start address of the payload
+// Returns:
+//   0 on success, negative value on error
+int hex_file_parse(const char* file_path_on_disk,
+                   uint8_t** payload,
+                   size_t* payload_length,
+                   size_t* payload_address);
+
+#ifdef __cplusplus
+};
+#endif
+
+#endif  // BLISP_HEX_FILE_H

tools/blisp/src/file_parsers/hex/tests/CMakeLists.txt

@@ -0,0 +1,10 @@
+add_executable(hex_file_test test_hex_file.cpp ../hex_file.c )
+
+target_link_libraries(hex_file_test
+        PRIVATE
+        GTest::GTest
+        )
+include(GoogleTest)
+include_directories(hex_file_test PRIVATE ../ ../../)
+target_compile_definitions(hex_file_test PUBLIC "SOURCE_DIR=\"${CMAKE_SOURCE_DIR}\"")
+gtest_discover_tests(hex_file_test)

tools/blisp/src/file_parsers/hex/tests/test.hex

@@ -0,0 +1,9 @@
+:02000004230FC8
+:10FC0000400196000500000021005A000200070094
+:10FC10000100000000001E000000000000000000C5
+:10FC2000000000000000040001007602A40184032B
+:10FC3000000000000A0001000700000000000000B2
+:10FC4000140000001A000100000000000000040081
+:10FC50005A00010082005A008C001E00A5001E0000
+:10FC60008C001E005A001E00020000000000000070
+:00000001FF

tools/blisp/src/file_parsers/hex/tests/test_hex_file.cpp

@@ -0,0 +1,47 @@
+// Intel hex file parser test
+
+#include <gtest/gtest.h>
+#include "hex_file.h"
+#include "parse_file.h"
+
+TEST(HEX_FILE_PARSER, ParseTestFile) {
+  uint8_t* payload = nullptr;
+  size_t payload_size = 0;
+  size_t payload_address = 0;
+  int res = hex_file_parse(SOURCE_DIR
+                           "/tools/blisp/src/file_parsers/hex/tests/test.hex",
+                           &payload, &payload_size, &payload_address);
+  // Shall return 0 on success
+  ASSERT_EQ(res, 0);
+  // The expected base address is 0x230F0000 + 0xFC00 = 0x230FFC00
+  ASSERT_EQ(payload_address, 0x230FFC00);
+  // There are 7 data records of 16 bytes each, so payload size should be 0x70
+  // (112 bytes)
+  ASSERT_EQ(payload_size, 0x70);
+
+  // Optionally, check the first few bytes for expected values
+  ASSERT_EQ(payload[0], 0x40);
+  ASSERT_EQ(payload[1], 0x01);
+  ASSERT_EQ(payload[2], 0x96);
+  ASSERT_EQ(payload[3], 0x00);
+
+  // Clean up
+  free(payload);
+}
+
+TEST(HEX_FILE_PARSER, ParseNonExistentFile) {
+  uint8_t* payload = nullptr;
+  size_t payload_size = 0;
+  size_t payload_address = 0;
+  int res = hex_file_parse(SOURCE_DIR "/non_existent_file.hex", &payload,
+                           &payload_size, &payload_address);
+
+  ASSERT_EQ(res, PARSED_ERROR_CANT_OPEN_FILE);
+}
+
+TEST(HEX_FILE_PARSER, ParseInvalidFormat) {
+  // This test would require creating an invalid hex file
+  // For simplicity, we'll skip actual implementation
+  // but in a real test suite we would create a file with invalid format
+  // and verify that it returns HEX_PARSE_ERROR_INVALID_FORMAT
+}

tools/blisp/src/file_parsers/parse_file.c

@@ -2,6 +2,7 @@
 #include <string.h>
 #include "bin_file.h"
 #include "dfu_file.h"
+#include "hex_file.h"
 
 const char* get_filename_ext(const char* filename) {
   const char* dot = strrchr(filename, '.');
@@ -27,8 +28,13 @@ int parse_firmware_file(const char* file_path_on_disk,
     res = bin_file_parse(file_path_on_disk, &parsed_results->payload,
                          &parsed_results->payload_length,
                          &parsed_results->payload_address);
+  } else if (strncmp(ext, "hex", 3) == 0 || strncmp(ext, "HEX", 3) == 0) {
+    printf("Input file identified as a .hex file\n");
+    // Intel HEX file
+    res = hex_file_parse(file_path_on_disk, &parsed_results->payload,
+                         &parsed_results->payload_length,
+                         &parsed_results->payload_address);
   }
-  // If we wanted to support hex files, here would be where
 
   // Normalise address, some builds will base the firmware at flash start but
   // for the flasher we use 0 base (i.e. offsets into flash)

vendor/argtable3

@@ -1 +1 @@
-Subproject commit 6f0e40bc44c99af353ced367c6fafca8705f5fca
+Subproject commit b50c6c81f25eef8af51141678b333c55b661414d