From a02bf05eed7c96f703958a3a3f52c6234e386ffb Mon Sep 17 00:00:00 2001
From: David Cermak <cermak@espressif.com>
Date: Mon, 10 Oct 2022 15:39:09 +0200
Subject: [PATCH] fix(esp-modem): Uart Terminal read_cb race

---
 .../include/cxx_include/esp_modem_dte.hpp         |  1 +
 components/esp_modem/src/esp_modem_dte.cpp        | 12 ++++++------
 components/esp_modem/src/esp_modem_uart.cpp       | 15 ++++++---------
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/components/esp_modem/include/cxx_include/esp_modem_dte.hpp b/components/esp_modem/include/cxx_include/esp_modem_dte.hpp
index 2b8ab5f5a..9c220171b 100644
--- a/components/esp_modem/include/cxx_include/esp_modem_dte.hpp
+++ b/components/esp_modem/include/cxx_include/esp_modem_dte.hpp
@@ -122,6 +122,7 @@ private:
     std::shared_ptr<Terminal> data_term;                    /*!< Secondary terminal for this DTE */
     modem_mode mode;                                        /*!< DTE operation mode */
     SignalGroup signal;                                     /*!< Event group used to signal request-response operations */
+    command_result result;                                  /*!< Command result of the currently exectuted command */
     std::function<bool(uint8_t *data, size_t len)> on_data; /*!< on data callback for current terminal */
 };
 
diff --git a/components/esp_modem/src/esp_modem_dte.cpp b/components/esp_modem/src/esp_modem_dte.cpp
index e20418a2c..48eba2ec5 100644
--- a/components/esp_modem/src/esp_modem_dte.cpp
+++ b/components/esp_modem/src/esp_modem_dte.cpp
@@ -35,9 +35,9 @@ DTE::DTE(std::unique_ptr<Terminal> terminal):
 command_result DTE::command(const std::string &command, got_line_cb got_line, uint32_t time_ms, const char separator)
 {
     Scoped<Lock> l(internal_lock);
-    command_result res = command_result::TIMEOUT;
+    result = command_result::TIMEOUT;
     signal.clear(GOT_LINE);
-    command_term->set_read_cb([&](uint8_t *data, size_t len) {
+    command_term->set_read_cb([this, got_line, separator](uint8_t *data, size_t len) {
         if (!data) {
             data = buffer.get();
             len = command_term->read(data + buffer.consumed, buffer.size - buffer.consumed);
@@ -45,8 +45,8 @@ command_result DTE::command(const std::string &command, got_line_cb got_line, ui
             buffer.consumed = 0; // if the underlying terminal contains data, we cannot fragment
         }
         if (memchr(data + buffer.consumed, separator, len)) {
-            res = got_line(data, buffer.consumed + len);
-            if (res == command_result::OK || res == command_result::FAIL) {
+            result = got_line(data, buffer.consumed + len);
+            if (result == command_result::OK || result == command_result::FAIL) {
                 signal.set(GOT_LINE);
                 return true;
             }
@@ -56,12 +56,12 @@ command_result DTE::command(const std::string &command, got_line_cb got_line, ui
     });
     command_term->write((uint8_t *)command.c_str(), command.length());
     auto got_lf = signal.wait(GOT_LINE, time_ms);
-    if (got_lf && res == command_result::TIMEOUT) {
+    if (got_lf && result == command_result::TIMEOUT) {
         ESP_MODEM_THROW_IF_ERROR(ESP_ERR_INVALID_STATE);
     }
     buffer.consumed = 0;
     command_term->set_read_cb(nullptr);
-    return res;
+    return result;
 }
 
 command_result DTE::command(const std::string &cmd, got_line_cb got_line, uint32_t time_ms)
diff --git a/components/esp_modem/src/esp_modem_uart.cpp b/components/esp_modem/src/esp_modem_uart.cpp
index 407a8b7e1..6c78399d1 100644
--- a/components/esp_modem/src/esp_modem_uart.cpp
+++ b/components/esp_modem/src/esp_modem_uart.cpp
@@ -72,8 +72,8 @@ public:
 
     void set_read_cb(std::function<bool(uint8_t *data, size_t len)> f) override
     {
+        ESP_MODEM_THROW_IF_FALSE(signal.wait(TASK_PARAMS, 1000), "Failed to set UART task params");
         on_read = std::move(f);
-        signal.set(TASK_PARAMS);
     }
 
 private:
@@ -118,7 +118,6 @@ std::unique_ptr<Terminal> create_uart_terminal(const esp_modem_dte_config *confi
 
 void UartTerminal::task()
 {
-    std::function<bool(uint8_t *data, size_t len)> on_read_priv = nullptr;
     uart_event_t event;
     size_t len;
     signal.set(TASK_INIT);
@@ -127,17 +126,15 @@ void UartTerminal::task()
         return; // exits to the static method where the task gets deleted
     }
     while (signal.is_any(TASK_START)) {
+        signal.set(TASK_PARAMS);
         if (get_event(event, 100)) {
-            if (signal.is_any(TASK_PARAMS)) {
-                on_read_priv = on_read;
-                signal.clear(TASK_PARAMS);
-            }
+            signal.clear(TASK_PARAMS);
             switch (event.type) {
             case UART_DATA:
                 uart_get_buffered_data_len(uart.port, &len);
-                if (len && on_read_priv) {
-                    if (on_read_priv(nullptr, len)) {
-                        on_read_priv = nullptr;
+                if (len && on_read) {
+                    if (on_read(nullptr, len)) {
+                        on_read = nullptr;
                     }
                 }
                 break;