Close #28: Refactor message parsing: Use single-threaded state machin…

…e in place of two-threaded implementation (#29)

* [#28] Add googletest

* [#28] Fix bug in macro print_dbgmsg

* [#28] Log ACK confirmation only if DEBUG mode enabled

* [#28] api_report_callback: print dbgmsg if message decoding failed

* [#28] Fix incorrect callback ptr checking in 'parse'

* Close #28: Refactor messaging parsing: Use single-threaded state machine in place of two-threaded implementation

* [#28] Add unit-tests

* Add GitHub actions: code-style and googe-tests

* [#28] Add GitHub action 'build'

* [#28] Fix GitHub actions

* [#28] Fix GitHub action 'build'

* [#28] Fix GitHub action 'build'

* [#28] Increase the stack size for "uart_rx_task" from 2 to 4kB

.github/workflows/build.yml

@@ -0,0 +1,31 @@
+name: Build
+
+on:
+  push:
+    branches: [ master ]
+  pull_request:
+    branches: [ master ]
+
+jobs:
+  build:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v3    # Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it
+        with:
+          fetch-depth: 0  # Shallow clones should be disabled for a better relevancy of analysis
+          submodules: true
+
+      - name: Checkout submodules
+        run: git submodule update --init --recursive
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y gcc g++ cmake make ninja-build
+          sudo apt-get install -y libdbus-1-dev
+
+      - name: Run build.sh
+        run: ./build.sh
+

.github/workflows/code-style.yml

@@ -0,0 +1,28 @@
+name: Clang-Format
+
+on:
+  push:
+    branches: [ master ]
+  pull_request:
+    branches: [ master ]
+
+jobs:
+  clang-format:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v3    # Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it
+
+      - name: Install Clang-Format 14
+        run: |
+          wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add -
+          sudo add-apt-repository "deb http://apt.llvm.org/focal/ llvm-toolchain-focal-14 main"
+          sudo apt-get update
+          sudo apt-get install -y clang-format-14
+
+      - name: Run clang-format
+        run: ./clang_format_all.sh
+
+      - name: Check formatting
+        run: git diff --exit-code --diff-filter=d --color

.github/workflows/google-tests.yml

@@ -0,0 +1,37 @@
+name: Google Test
+
+on:
+  push:
+    branches: [ master ]
+  pull_request:
+    branches: [ master ]
+
+jobs:
+  google-test:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v3
+
+      - name: Checkout submodules
+        run: git submodule update --init --recursive
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get install -y gcc g++ cmake make ninja-build
+          sudo apt-get install -y locales
+          sudo locale-gen de_DE.UTF-8
+
+      - name: Build
+        run: |
+          cd tests
+          mkdir cmake-build-unit-tests
+          cd cmake-build-unit-tests
+          cmake -G "Ninja" ..
+          ninja
+        
+      - name: Run tests
+        run: |
+          cd tests/cmake-build-unit-tests
+          ctest

.gitmodules

@@ -1,3 +1,6 @@
 [submodule "components/ruuvi.endpoints.c"]
 	path = components/ruuvi.endpoints.c
 	url = https://github.com/ruuvi/ruuvi.endpoints.c.git
+[submodule "tests/googletest"]
+	path = tests/googletest
+	url = https://github.com/google/googletest.git

src/debug.h

@@ -27,7 +27,7 @@
 #define print_dbgmsgnofunc(str, ...) fprintf(stderr, "" str, ##__VA_ARGS__)
 #define print_dbgmsgnofuncnoarg(str) fprintf(stderr, "%s", str)
 #else
-#define print_dbgmsg(str, args...)          ESP_LOGI("terminal_dbg", str, __func__, ##args)
+#define print_dbgmsg(str, args...)          ESP_LOGI("terminal_dbg", "%s: " str, __func__, ##args)
 #define print_dbgmsgnoarg(str)              ESP_LOGI("terminal_dbg", "%s in %s: ", str, __func__)
 #define print_dbgmsgnofunc(str, args...)    ESP_LOGI("terminal_dbg", str, ##args)
 #define print_dbgmsgnofuncnoarg(str)        ESP_LOGI("terminal_dbg", str)

src/drv/terminal.c

@@ -42,10 +42,7 @@ typedef struct __terminal_struct
     TaskHandle_t rx_parse_task_manager;
     TaskHandle_t rx_task_manager;
 #endif
-    int     size;
-    uint8_t rx_buffer[RX_BUFFER_MAX_SIZE];
-    uint8_t rx_buffer_error[RX_BUFFER_MAX_SIZE << 1];
-    uint8_t rx_buffer_error_index;
+    re_ca_uart_parser_obj_t parser_obj;
 } terminal_struct_t;
 #pragma pack(pop)
 /*end*/
@@ -55,8 +52,12 @@ typedef struct __terminal_struct
 terminal_struct_t terminal;
 /*end*/
 
-static void
-wait(terminal_struct_t *p_terminal, uint32_t timeout);
+static re_ca_uart_parser_message_len_t
+wait_message_from_uart(
+    terminal_struct_t *const    p_terminal,
+    const uint32_t              timeout,
+    re_ca_uart_message_t *const p_message);
+
 static int
 send_msg(uint8_t *data, uint8_t size);
 
@@ -87,51 +88,31 @@ send_msg(uint8_t *data, uint8_t size)
     return res;
 }
 
-static void
-wait(terminal_struct_t *p_terminal, uint32_t timeout)
+static re_ca_uart_parser_message_len_t
+wait_message_from_uart(
+    terminal_struct_t *const    p_terminal,
+    const uint32_t              timeout,
+    re_ca_uart_message_t *const p_message)
 {
-    int rx_size = 0;
     int rx_size_it;
 
-    static uint8_t rcv_data[RX_BUFFER_MAX_SIZE];
-    memset(rcv_data, 0, RX_BUFFER_MAX_SIZE);
-    while (p_terminal->size != 0)
-    {
-#ifndef RUUVI_ESP
-#else
-        vTaskDelay(1);
-#endif
-    }
     while (1)
     {
+        uint8_t byte = 0;
 #ifndef RUUVI_ESP
-        rx_size_it = read(p_terminal->fd, &rcv_data[rx_size], 1);
+        rx_size_it = read(p_terminal->fd, &byte, 1);
 #else
-        rx_size_it = uart_read_bytes(UART_NUM_1, &rcv_data[rx_size], 1, timeout / portTICK_RATE_MS);
+        rx_size_it = uart_read_bytes(UART_NUM_1, &byte, 1, timeout / portTICK_RATE_MS);
 #endif
-
-        if ((rx_size + rx_size_it) > RX_BUFFER_MAX_SIZE)
+        if (1 != rx_size_it)
         {
-            break;
+            parser_handle_timeout(&p_terminal->parser_obj);
+            continue;
         }
 
-        if (rx_size_it > 0)
+        if (parser_handle_byte(&p_terminal->parser_obj, byte))
         {
-            rx_size += rx_size_it;
-            for (int i = (rx_size - rx_size_it); i < rx_size; i++)
-            {
-                if ((*(uint8_t *)&rcv_data[i]) == RE_CA_UART_ETX)
-                {
-                    memcpy(p_terminal->rx_buffer, rcv_data, rx_size);
-                    p_terminal->size = rx_size;
-                    break;
-                }
-            }
-
-            if (p_terminal->size)
-            {
-                break;
-            }
+            return parser_get_message(&p_terminal->parser_obj, p_message);
         }
     }
 }
@@ -149,57 +130,6 @@ terminal_send_msg(uint8_t *data, uint8_t size)
     return res;
 }
 
-#ifndef RUUVI_ESP
-void *
-th_ctrl_call(void *vargp)
-#else
-static void
-rx_parse_task(void *arg)
-#endif
-{
-#ifndef RUUVI_ESP
-    while (terminal.fd < 0)
-        ;
-#endif
-    while (1)
-    {
-        if (terminal.size)
-        {
-            print_dbgmsgnofuncnoarg("RX: ");
-
-#ifndef RUUVI_ESP
-            for (int i = 0; i < terminal.size; i++)
-            {
-                print_dbgmsgnofunc("0x%02x ", (*(uint8_t *)&terminal.rx_buffer[i]));
-            }
-#else
-            print_dbgmsghexdump((char *)&terminal.rx_buffer[0], terminal.size);
-#endif
-
-            print_dbgmsgnofuncnoarg("\n");
-            if ((-1) == parse((uint8_t *)&terminal.rx_buffer[0]))
-            {
-                memcpy((terminal.rx_buffer_error + terminal.rx_buffer_error_index), terminal.rx_buffer, terminal.size);
-                terminal.rx_buffer_error_index += terminal.size;
-                if (0 == parse((uint8_t *)&terminal.rx_buffer_error[0]))
-                {
-                    memset(terminal.rx_buffer_error, 0, RX_BUFFER_MAX_SIZE << 1);
-                    terminal.rx_buffer_error_index = 0;
-                }
-            }
-            else
-            {
-                memset(terminal.rx_buffer_error, 0, RX_BUFFER_MAX_SIZE << 1);
-                terminal.rx_buffer_error_index = 0;
-            }
-            terminal.size = 0;
-        }
-#ifdef RUUVI_ESP
-        vTaskDelay(1);
-#endif
-    }
-}
-
 #ifndef RUUVI_ESP
 void *
 th_ctrl(void *vargp)
@@ -208,28 +138,41 @@ static void
 rx_task(void *arg)
 #endif
 {
+    parser_init(&terminal.parser_obj);
+
 #ifndef RUUVI_ESP
     while (terminal.fd < 0)
         ;
 #endif
+    re_ca_uart_message_t message;
     while (1)
     {
-        wait(&terminal, RX_ASK_TIMEOUT);
+        re_ca_uart_parser_message_len_t len = wait_message_from_uart(&terminal, RX_ASK_TIMEOUT, &message);
+        (void)len;
+        print_dbgmsgnofuncnoarg("RX: ");
+
 #ifndef RUUVI_ESP
-        // usleep(100000);
+        for (int i = 0; i < len; i++)
+        {
+            print_dbgmsgnofunc("0x%02x ", (*(uint8_t *)&message.buffer[0]));
+        }
 #else
-        vTaskDelay(1);
+        print_dbgmsghexdump((char *)&message.buffer[0], len);
 #endif
+        if ((-1) == parse((uint8_t *)&message.buffer[0]))
+        {
+            print_dbgmsgnofuncnoarg("RX: parsing failed");
+        }
     }
 }
 
 int
 terminal_open(char *device_address, bool rx_enable, int task_priority)
 {
-
     print_dbgmsgnoarg("Enter\n");
     memset(&terminal, 0, sizeof(terminal_struct_t));
 #ifndef RUUVI_ESP
+    print_dbgmsg("Open UART: %s\n", device_address);
     terminal.fd = open(device_address, O_RDWR);
     if (terminal.fd < 0)
     {
@@ -256,13 +199,10 @@ terminal_open(char *device_address, bool rx_enable, int task_priority)
             settings.c_cflag &= ~(CSIZE | PARENB);
             settings.c_cflag |= CS8;
 
-            terminal.size                  = 0;
-            terminal.rx_buffer_error_index = 0;
             tcsetattr(terminal.fd, TCSANOW, &settings); /* apply the settings */
             tcflush(terminal.fd, TCOFLUSH);
 
             pthread_create(&terminal.thread_id, NULL, th_ctrl, NULL);
-            pthread_create(&terminal.thread_id_call, NULL, th_ctrl_call, NULL);
         }
     }
 #else
@@ -281,8 +221,7 @@ terminal_open(char *device_address, bool rx_enable, int task_priority)
     // We won't use a buffer for sending data.
     uart_driver_install(UART_NUM_1, UART_RX_BUF_SIZE * 2, 0, 0, NULL, 0);
 
-    xTaskCreate(rx_task, "uart_rx_task", 1024 * 2, NULL, task_priority, &terminal.rx_task_manager);
-    xTaskCreate(rx_parse_task, "rx_parse_task", 1024 * 4, NULL, task_priority, &terminal.rx_parse_task_manager);
+    xTaskCreate(rx_task, "uart_rx_task", 1024 * 4, NULL, task_priority, &terminal.rx_task_manager);
 #endif
     print_dbgmsgnoarg("End\n");
     return 0;

src/drv/terminal.h

@@ -31,7 +31,6 @@
 #define RX_ASK_TIMEOUT     1000   // in 1000us
 #define DEFAULT_BAUDRATE   115200 // in 1000us
 #define UART_RX_BUF_SIZE   1024
-#define BUF_MAX            1000
 #endif
 /*end*/
 

src/lib/api.c

@@ -325,9 +325,10 @@ api_id_callback(const uint8_t *const buffer)
 static int
 api_report_callback(const uint8_t *const buffer)
 {
-    int                  res          = -1;
-    re_ca_uart_payload_t uart_payload = { 0 };
-    if (RE_SUCCESS == re_ca_uart_decode((uint8_t *)buffer, &uart_payload))
+    int                  res           = -1;
+    re_ca_uart_payload_t uart_payload  = { 0 };
+    const re_status_t    decode_status = re_ca_uart_decode((uint8_t *)buffer, &uart_payload);
+    if (RE_SUCCESS == decode_status)
     {
         res = 0;
         if (NULL != p_adv_callback_func_tbl->AdvReportCallback)
@@ -339,6 +340,10 @@ api_report_callback(const uint8_t *const buffer)
             formated_output_report((void *)&uart_payload);
         }
     }
+    else
+    {
+        print_dbgmsg("re_ca_uart_decode failed, status=%d\n", decode_status);
+    }
     return res;
 }
 

src/lib/formated_output.c

@@ -24,6 +24,7 @@
 void
 formated_output_ack(void *p_uart_payload)
 {
+#if UPDATE_DEBUG_MODE
     re_ca_uart_payload_t *uart_payload          = (re_ca_uart_payload_t *)p_uart_payload;
     char                  ack_state_name[2][32] = {
                          "RE_CA_ACK_OK",
@@ -64,9 +65,10 @@ formated_output_ack(void *p_uart_payload)
         "",
         "RE_CA_UART_ACK",
     };
-    print_logmsgnofuncnoarg("-----ACK-----\n");
-    print_logmsgnofunc("CMD: %s\n", ack_cmd_name[(uint8_t)uart_payload->params.ack.cmd]);
-    print_logmsgnofunc("ACK state: %s\n", ack_state_name[(uint8_t)uart_payload->params.ack.ack_state.state]);
+    print_dbgmsgnofuncnoarg("-----ACK-----\n");
+    print_dbgmsgnofunc("CMD: %s\n", ack_cmd_name[(uint8_t)uart_payload->params.ack.cmd]);
+    print_dbgmsgnofunc("ACK state: %s\n", ack_state_name[(uint8_t)uart_payload->params.ack.ack_state.state]);
+#endif
 }
 
 void