fix(exp_modem): DTE should own both command and data terminal

reworks to clear shared_ptr<> of both command and data terminals
and having DTE own it. When we swith to CMUX mode the ownership is transfered to CMux terminal

components/esp_modem/examples/modem_console/main/console_helper.hpp

@@ -10,6 +10,7 @@
 #pragma once
 
 #include <vector>
+#include <string>
 #include <algorithm>
 #include <functional>
 #include <esp_console.h>

components/esp_modem/include/cxx_include/esp_modem_cmux.hpp

@@ -54,7 +54,7 @@ class CMuxInstance;
  */
 class CMux {
 public:
-    explicit CMux(std::unique_ptr<Terminal> t, std::unique_ptr<uint8_t[]> b, size_t buff_size):
+    explicit CMux(std::shared_ptr<Terminal> t, std::unique_ptr<uint8_t[]> b, size_t buff_size):
         term(std::move(t)), payload_start(nullptr), total_payload_size(0), buffer_size(buff_size), buffer(std::move(b))  {}
     ~CMux() = default;
 
@@ -69,7 +69,7 @@ public:
      * @return  nullptr on failure
      *          tuple of the original terminal and buffer on success
      */
-    std::tuple<std::unique_ptr<Terminal>, std::unique_ptr<uint8_t[]>, size_t> deinit_and_eject();
+    std::tuple<std::shared_ptr<Terminal>, std::unique_ptr<uint8_t[]>, size_t> deinit_and_eject();
 
     /**
      * @brief Sets read callback for the appropriate terminal
@@ -110,7 +110,7 @@ private:
     bool on_footer(CMuxFrame &frame);
 
     std::function<bool(uint8_t *data, size_t len)> read_cb[MAX_TERMINALS_NUM];  /*!< Function pointers to read callbacks */
-    std::unique_ptr<Terminal> term;                   /*!< The original terminal */
+    std::shared_ptr<Terminal> term;                   /*!< The original terminal */
     cmux_state state;                                 /*!< CMux protocol state */
 
     /**

components/esp_modem/include/cxx_include/esp_modem_dce.hpp

@@ -40,6 +40,7 @@ public:
     modem_mode get();
 
 private:
+    bool set_unsafe(DTE *dte, ModuleIf *module, Netif &netif, modem_mode m);
     modem_mode mode;
 
 };

components/esp_modem/include/cxx_include/esp_modem_dte.hpp

@@ -94,19 +94,26 @@ public:
      */
     command_result command(const std::string &command, got_line_cb got_line, uint32_t time_ms, char separator) override;
 
+protected:
+    /**
+     * @brief Allows for locking the DTE
+     */
+    void lock()     { internal_lock.lock();   }
+    void unlock()   { internal_lock.unlock(); }
+    friend class Scoped<DTE>;                               /*!< Declaring "Scoped<DTE> lock(dte)" locks this instance */
 private:
     static const size_t GOT_LINE = SignalGroup::bit0;       /*!< Bit indicating response available */
 
     [[nodiscard]] bool setup_cmux();                         /*!< Internal setup of CMUX mode */
     [[nodiscard]] bool exit_cmux();                          /*!< Exit of CMUX mode */
 
-    Lock lock{};                                            /*!< Locks DTE operations */
+    Lock internal_lock{};                                    /*!< Locks DTE operations */
     size_t buffer_size;                                      /*!< Size of available DTE buffer */
     size_t consumed;                                         /*!< Indication of already processed portion in DTE buffer */
     std::unique_ptr<uint8_t[]> buffer;                       /*!< DTE buffer */
-    std::unique_ptr<Terminal> term;                          /*!< Primary terminal for this DTE */
+    std::shared_ptr<CMux> cmux_term;                        /*!< Primary terminal for this DTE */
-    Terminal *command_term;                                  /*!< Reference to the terminal used for sending commands */
+    std::shared_ptr<Terminal> command_term;                 /*!< Reference to the terminal used for sending commands */
-    std::unique_ptr<Terminal> other_term;                    /*!< Secondary terminal for this DTE */
+    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 */
     std::function<bool(uint8_t *data, size_t len)> on_data;  /*!< on data callback for current terminal */

components/esp_modem/src/esp_modem_cmux.cpp

@@ -273,7 +273,7 @@ bool CMux::on_footer(CMuxFrame &frame)
     return true;
 }
 
-bool CMux::on_cmux(uint8_t *data, size_t actual_len)
+bool CMux::on_cmux_data(uint8_t *data, size_t actual_len)
 {
     if (!data) {
 #ifdef DEFRAGMENT_CMUX_PAYLOAD
@@ -354,7 +354,7 @@ bool CMux::init()
     frame_header_offset = 0;
     state = cmux_state::INIT;
     term->set_read_cb([this](uint8_t *data, size_t len) {
-        this->on_cmux(data, len);
+        this->on_cmux_data(data, len);
         return false;
     });
 
@@ -415,7 +415,7 @@ void CMux::set_read_cb(int inst, std::function<bool(uint8_t *, size_t)> f)
     }
 }
 
-std::tuple<std::unique_ptr<Terminal>, std::unique_ptr<uint8_t[]>, size_t> esp_modem::CMux::deinit_and_eject()
+std::tuple<std::shared_ptr<Terminal>, std::unique_ptr<uint8_t[]>, size_t> esp_modem::CMux::deinit_and_eject()
 {
     if (exit_cmux_protocol()) {
         return std::make_tuple(std::move(term), std::move(buffer), buffer_size);

components/esp_modem/src/esp_modem_dce.cpp

@@ -22,8 +22,24 @@
 
 namespace esp_modem {
 
-
+/**
+ * Set mode while the entire DTE is locked
+*/
 bool DCE_Mode::set(DTE *dte, ModuleIf *device, Netif &netif, modem_mode m)
+{
+    Scoped<DTE> lock(*dte);
+    return set_unsafe(dte, device, netif, m);
+}
+
+/**
+ * state machine:
+ *
+ * COMMAND_MODE <----> DATA_MODE
+ * COMMAND_MODE <----> CMUX_MODE
+ *
+ * UNDEF <----> any
+ */
+bool DCE_Mode::set_unsafe(DTE *dte, ModuleIf *device, Netif &netif, modem_mode m)
 {
     switch (m) {
     case modem_mode::UNDEF:
@@ -44,7 +60,7 @@ bool DCE_Mode::set(DTE *dte, ModuleIf *device, Netif &netif, modem_mode m)
             }
             netif.stop();
             SignalGroup signal;
-            dte->set_read_cb([&](uint8_t *data, size_t len) -> bool {
+            dte->set_read_cb([&signal](uint8_t *data, size_t len) -> bool {
                 if (memchr(data, '\n', len)) {
                     ESP_LOG_BUFFER_HEXDUMP("esp-modem: debug_data", data, len, ESP_LOG_DEBUG);
                     const auto pass = std::list<std::string_view>({"NO CARRIER", "DISCONNECTED"});
@@ -74,7 +90,7 @@ bool DCE_Mode::set(DTE *dte, ModuleIf *device, Netif &netif, modem_mode m)
         }
         break;
     case modem_mode::DATA_MODE:
-        if (mode == modem_mode::DATA_MODE) {
+        if (mode == modem_mode::DATA_MODE || mode == modem_mode::CMUX_MODE) {
             return false;
         }
         if (!device->setup_data_mode()) {
@@ -94,7 +110,7 @@ bool DCE_Mode::set(DTE *dte, ModuleIf *device, Netif &netif, modem_mode m)
             return false;
         }
         device->set_mode(modem_mode::CMUX_MODE);    // switch the device into CMUX mode
-        usleep(100'000);                            // some devices need a few ms to switch
+        usleep(100'000);                          // some devices need a few ms to switch
 
         if (!dte->set_mode(modem_mode::CMUX_MODE)) {
             return false;

components/esp_modem/src/esp_modem_dte.cpp

@@ -24,18 +24,18 @@ static const size_t dte_default_buffer_size = 1000;
 DTE::DTE(const esp_modem_dte_config *config, std::unique_ptr<Terminal> terminal):
     buffer_size(config->dte_buffer_size), consumed(0),
     buffer(std::make_unique<uint8_t[]>(buffer_size)),
-    term(std::move(terminal)), command_term(term.get()), other_term(nullptr),
+    cmux_term(nullptr), command_term(std::move(terminal)), data_term(command_term),
     mode(modem_mode::UNDEF) {}
 
 DTE::DTE(std::unique_ptr<Terminal> terminal):
     buffer_size(dte_default_buffer_size), consumed(0),
     buffer(std::make_unique<uint8_t[]>(buffer_size)),
-    term(std::move(terminal)), command_term(term.get()), other_term(nullptr),
+    cmux_term(nullptr), command_term(std::move(terminal)), data_term(command_term),
     mode(modem_mode::UNDEF) {}
 
 command_result DTE::command(const std::string &command, got_line_cb got_line, uint32_t time_ms, const char separator)
 {
-    Scoped<Lock> l(lock);
+    Scoped<Lock> l(internal_lock);
     command_result res = command_result::TIMEOUT;
     command_term->set_read_cb([&](uint8_t *data, size_t len) {
         if (!data) {
@@ -71,28 +71,21 @@ command_result DTE::command(const std::string &cmd, got_line_cb got_line, uint32
 
 bool DTE::exit_cmux()
 {
-    auto cmux_term = static_cast<CMuxInstance*>(term.get())->get_cmux();
     auto ejected = cmux_term->deinit_and_eject();
     if (ejected == std::tuple(nullptr, nullptr, 0)) {
         return false;
     }
     // deinit succeeded -> swap the internal terminals with those ejected from cmux
-    auto term_orig = std::move(term);
+    command_term = std::move(std::get<0>(ejected));
-    auto other_term_orig = std::move(other_term);
-    term = std::move(std::get<0>(ejected));
     buffer = std::move(std::get<1>(ejected));
     buffer_size = std::get<2>(ejected);
-    command_term = term.get(); // use command terminal as previously
+    data_term = command_term;
     return true;
 }
 
 bool DTE::setup_cmux()
 {
-    auto original_term = std::move(term);
+    cmux_term = std::make_shared<CMux>(command_term, std::move(buffer), buffer_size);
-    if (original_term == nullptr) {
-        return false;
-    }
-    auto cmux_term = std::make_shared<CMux>(std::move(original_term), std::move(buffer), buffer_size);
     if (cmux_term == nullptr) {
         return false;
     }
@@ -100,34 +93,52 @@ bool DTE::setup_cmux()
     if (!cmux_term->init()) {
         return false;
     }
-    term = std::make_unique<CMuxInstance>(cmux_term, 0);
+    command_term = std::make_unique<CMuxInstance>(cmux_term, 0);
-    if (term == nullptr) {
+    if (command_term == nullptr) {
         return false;
     }
-    command_term = term.get(); // use command terminal as previously
+    data_term = std::make_unique<CMuxInstance>(cmux_term, 1);
-    other_term = std::make_unique<CMuxInstance>(cmux_term, 1);
     return true;
 }
 
 bool DTE::set_mode(modem_mode m)
 {
-    if (mode == modem_mode::CMUX_MODE && m == modem_mode::COMMAND_MODE) {
+    // transitions (COMMAND|UNDEF) -> CMUX
-        if (exit_cmux()) {
+    if (m == modem_mode::CMUX_MODE) {
+        if (mode == modem_mode::UNDEF || mode == modem_mode::COMMAND_MODE) {
+            if (setup_cmux()) {
+                mode = m;
+                return true;
+            }
+            mode = modem_mode::UNDEF;
+            return false;
+        }
+    }
+    // transitions (COMMAND|CMUX|UNDEF) -> DATA
+    if (m == modem_mode::DATA_MODE) {
+        if (mode == modem_mode::CMUX_MODE) {
+            // mode stays the same, but need to swap terminals (as command has been switch)
+            data_term.swap(command_term);
+        } else {
+            mode = m;
+        }
+        // prepare the data terminal's callback to the configured std::function (used by netif)
+        data_term->set_read_cb(on_data);
+        return true;
+    }
+    // transitions (DATA|CMUX|UNDEF) -> COMMAND
+    if (m == modem_mode::COMMAND_MODE) {
+        if (mode == modem_mode::CMUX_MODE) {
+            if (exit_cmux()) {
+                mode = m;
+                return true;
+            }
+            mode = modem_mode::UNDEF;
+            return false;
+        } else {
             mode = m;
             return true;
         }
-        return false;
-    }
-    if (mode != modem_mode::CMUX_MODE) {    // keep CMUX internally, it's CMD+PPP
-        mode = m;
-    }
-    if (m == modem_mode::DATA_MODE) {
-        term->set_read_cb(on_data);
-        if (other_term) { // if we have the other terminal, let's use it for commands
-            command_term = other_term.get();
-        }
-    } else if (m == modem_mode::CMUX_MODE) {
-        return setup_cmux();
     }
     return true;
 }
@@ -135,10 +146,10 @@ bool DTE::set_mode(modem_mode m)
 void DTE::set_read_cb(std::function<bool(uint8_t *, size_t)> f)
 {
     on_data = std::move(f);
-    term->set_read_cb([this](uint8_t *data, size_t len) {
+    data_term->set_read_cb([this](uint8_t *data, size_t len) {
         if (!data) { // if no data available from terminal callback -> need to explicitly read some
             data = buffer.get();
-            len = term->read(buffer.get(), buffer_size);
+            len = data_term->read(buffer.get(), buffer_size);
         }
         if (on_data) {
             return on_data(data, len);
@@ -151,12 +162,12 @@ int DTE::read(uint8_t **d, size_t len)
 {
     auto data_to_read = std::min(len, buffer_size);
     auto data = buffer.get();
-    auto actual_len = term->read(data, data_to_read);
+    auto actual_len = data_term->read(data, data_to_read);
     *d = data;
     return actual_len;
 }
 
 int DTE::write(uint8_t *data, size_t len)
 {
-    return term->write(data, len);
+    return data_term->write(data, len);
 }