fix(esp_modem): Implement movable unique_buffer to bundle data, size, ptr

Also improves and fixes tests

components/esp_modem/include/cxx_include/esp_modem_buffer.hpp

@@ -0,0 +1,50 @@
+// Copyright 2022 Espressif Systems (Shanghai) PTE LTD
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#pragma once
+
+namespace esp_modem {
+
+/**
+ * Common unique buffer, which is transferable between DTE and CMUX
+ *
+ */
+struct unique_buffer {
+    explicit unique_buffer(size_t size);
+    unique_buffer (unique_buffer const&) = delete;
+    unique_buffer& operator=(unique_buffer const&) = delete;
+    unique_buffer(unique_buffer&& other) noexcept
+    {
+        data = std::move(other.data);
+        size = other.size;
+        consumed = 0;
+    }
+    unique_buffer& operator=(unique_buffer&& other) noexcept
+    {
+        if (&other == this) {
+            return *this;
+        }
+        data = std::move(other.data);
+        size = other.size;
+        consumed = 0;
+        return *this;
+    }
+    [[nodiscard]] uint8_t *get() const { return data.get(); }
+
+    std::unique_ptr<uint8_t[]> data;
+    size_t size{};
+    size_t consumed{};
+};
+
+}

components/esp_modem/include/cxx_include/esp_modem_cmux.hpp

@@ -15,6 +15,7 @@
 #pragma once
 
 #include "esp_modem_terminal.hpp"
+#include "cxx_include/esp_modem_buffer.hpp"
 
 namespace esp_modem {
 
@@ -54,8 +55,8 @@ class CMuxInstance;
  */
 class CMux {
 public:
-    explicit CMux(std::shared_ptr<Terminal> t, std::unique_ptr<uint8_t[]> b, size_t buff_size):
+    explicit CMux(std::shared_ptr<Terminal> t, unique_buffer&& b):
-        term(std::move(t)), payload_start(nullptr), total_payload_size(0), buffer_size(buff_size), buffer(std::move(b))  {}
+        term(std::move(t)), payload_start(nullptr), total_payload_size(0), buffer(std::move(b))  {}
     ~CMux() = default;
 
     /**
@@ -65,11 +66,17 @@ public:
     [[nodiscard]] bool init();
 
     /**
-     * @brief   Closes CMux protocol and ejects attached terminal and buffer
+     * @brief Closes and deinits CMux protocol
-     * @return  nullptr on failure
+     * @return true on success
-     *          tuple of the original terminal and buffer on success
      */
-    std::tuple<std::shared_ptr<Terminal>, std::unique_ptr<uint8_t[]>, size_t> deinit_and_eject();
+    [[nodiscard]] bool deinit();
+
+    /**
+     * @brief   Ejects the attached terminal and buffer,
+     * so they could be used as traditional command/data DTE's
+     * @return  pair of the original terminal and buffer
+     */
+    std::pair<std::shared_ptr<Terminal>, unique_buffer> detach();
 
     /**
      * @brief Sets read callback for the appropriate terminal
@@ -91,10 +98,8 @@ private:
     static uint8_t fcs_crc(const uint8_t frame[6]);     /*!< Utility to calculate FCS CRC */
     void data_available(uint8_t *data, size_t len);     /*!< Called when valid data available */
     void send_sabm(size_t i);                           /*!< Sending initial SABM */
-    void send_disc(size_t i);                           /*!< Sending closing request for each virtual terminal */
+    void send_disconnect(size_t i);                     /*!< Sending closing request for each virtual or control terminal */
-    void close_down();                                  /*!< Close up the control terminla (term=0) */
+    bool on_cmux_data(uint8_t *data, size_t len);       /*!< Called from terminal layer when raw CMUX protocol data available */
-    bool exit_cmux_protocol();                          /*!< Sequence of exit of all virtual terms and control term */
-    bool on_cmux_data(uint8_t *data, size_t len);            /*!< Called from terminal layer when raw CMUX protocol data available */
 
     struct CMuxFrame;                                   /*!< Forward declare the Frame struct, used in protocol decoders */
     /**
@@ -127,10 +132,9 @@ private:
     int sabm_ack;
 
     /**
-     * Processing buffer size and pointer
+     * Processing unique buffer (reused and transferred from it's parent DTE)
      */
-    size_t buffer_size;
+    unique_buffer buffer;
-    std::unique_ptr<uint8_t[]> buffer;
 
     Lock lock;
 };
@@ -157,7 +161,6 @@ public:
     }
     void start() override { }
     void stop() override { }
-    CMux* get_cmux() { return cmux.get(); }
 private:
     std::shared_ptr<CMux> cmux;
     size_t instance;

components/esp_modem/include/cxx_include/esp_modem_dte.hpp

@@ -15,18 +15,20 @@
 #pragma once
 
 #include <memory>
+#include <utility>
 #include <cstddef>
 #include <cstdint>
-#include <utility>
 #include "cxx_include/esp_modem_primitives.hpp"
 #include "cxx_include/esp_modem_terminal.hpp"
-#include "cxx_include/esp_modem_cmux.hpp"
 #include "cxx_include/esp_modem_types.hpp"
+#include "cxx_include/esp_modem_buffer.hpp"
 
 struct esp_modem_dte_config;
 
 namespace esp_modem {
 
+class CMux;
+
 /**
  * @defgroup ESP_MODEM_DTE
  * @brief Definition of DTE and related classes
@@ -104,19 +106,17 @@ protected:
 private:
     static const size_t GOT_LINE = SignalGroup::bit0;       /*!< Bit indicating response available */
 
-    [[nodiscard]] bool setup_cmux();                         /*!< Internal setup of CMUX mode */
+    [[nodiscard]] bool setup_cmux();                        /*!< Internal setup of CMUX mode */
-    [[nodiscard]] bool exit_cmux();                          /*!< Exit of CMUX mode */
+    [[nodiscard]] bool exit_cmux();                         /*!< Exit of CMUX mode */
 
-    Lock internal_lock{};                                    /*!< Locks DTE operations */
+    Lock internal_lock{};                                   /*!< Locks DTE operations */
-    size_t buffer_size;                                      /*!< Size of available DTE buffer */
+    unique_buffer buffer;                                   /*!< DTE buffer */
-    size_t consumed;                                         /*!< Indication of already processed portion in DTE buffer */
-    std::unique_ptr<uint8_t[]> buffer;                       /*!< DTE buffer */
     std::shared_ptr<CMux> cmux_term;                        /*!< Primary terminal for this DTE */
     std::shared_ptr<Terminal> command_term;                 /*!< Reference to the terminal used for sending commands */
     std::shared_ptr<Terminal> data_term;                    /*!< Secondary terminal for this DTE */
-    modem_mode mode;                                         /*!< DTE operation mode */
+    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 */
+    std::function<bool(uint8_t *data, size_t len)> on_data; /*!< on data callback for current terminal */
 };
 
 /**

components/esp_modem/include/cxx_include/esp_modem_primitives.hpp

@@ -46,7 +46,7 @@ private:
 using TaskT = TaskHandle_t;
 using SignalT = EventGroupHandle_t;
 #else
-using Lock = std::mutex;
+using Lock = std::recursive_mutex;
 struct SignalGroupInternal;
 using SignalT = std::unique_ptr<SignalGroupInternal>;
 using TaskT = std::thread;

components/esp_modem/src/esp_modem_cmux.cpp

@@ -78,23 +78,19 @@ uint8_t CMux::fcs_crc(const uint8_t frame[6])
     return crc;
 }
 
-void CMux::close_down()
+void CMux::send_disconnect(size_t i)
 {
-    uint8_t frame[] = {
+    if (i == 0) {   // control terminal
-        SOF_MARKER, 0x3, 0xFF, 0x5, 0xC3, 0x1, 0xE7, SOF_MARKER };
+        uint8_t frame[] = {
-    term->write(frame, 8);
+                SOF_MARKER, 0x3, 0xFF, 0x5, 0xC3, 0x1, 0xE7, SOF_MARKER };
-}
+        term->write(frame, 8);
-
+    } else {        // separate virtual terminal
-void CMux::send_disc(size_t i)
+        uint8_t frame[] = {
-{
+                SOF_MARKER, 0x3, FT_DISC | PF, 0x1, 0, SOF_MARKER };
-    uint8_t frame[6];
+        frame[1] |= i << 2;
-    frame[0] = SOF_MARKER;
+        frame[4] = 0xFF - fcs_crc(frame);
-    frame[1] = (i << 2) | 0x3;
+        term->write(frame, sizeof(frame));
-    frame[2] = FT_DISC | PF;
+    }
-    frame[3] = 1;
-    frame[4] = 0xFF - fcs_crc(frame);
-    frame[5] = SOF_MARKER;
-    term->write(frame, 6);
 }
 
 void CMux::send_sabm(size_t i)
@@ -146,6 +142,9 @@ void CMux::data_available(uint8_t *data, size_t len)
             read_cb[virtual_term](payload_start, total_payload_size);
 #endif
         }
+    } else if (type == 0xFF && dlci == 0) { // notify the internal DISC command
+        Scoped<Lock> l(lock);
+        sabm_ack = dlci;
     }
 }
 
@@ -277,7 +276,7 @@ bool CMux::on_cmux_data(uint8_t *data, size_t actual_len)
 {
     if (!data) {
 #ifdef DEFRAGMENT_CMUX_PAYLOAD
-        auto data_to_read = buffer_size - 128; // keep 128 (max CMUX payload) backup buffer)
+        auto data_to_read = buffer.size - 128; // keep 128 (max CMUX payload) backup buffer)
         if (payload_start) {
             data = payload_start + total_payload_size;
             data_to_read = payload_len + 2;
@@ -324,12 +323,13 @@ bool CMux::on_cmux_data(uint8_t *data, size_t actual_len)
     return true;
 }
 
-bool CMux::exit_cmux_protocol()
+bool CMux::deinit()
 {
+    int timeout = 0;
     sabm_ack = -1;
+    // First disconnect all (2) virtual terminals
     for (size_t i = 1; i < 3; i++) {
-        int timeout = 0;
+        send_disconnect(i);
-        send_disc(i);
         while (true) {
             usleep(10'000);
             Scoped<Lock> l(lock);
@@ -342,11 +342,21 @@ bool CMux::exit_cmux_protocol()
             }
         }
     }
-    close_down();
+    sabm_ack = -1;
-    usleep(100'000);
+    // Then disconnect the control terminal
+    send_disconnect(0);
+    while (true) {
+        usleep(10'000);
+        Scoped<Lock> l(lock);
+        if (sabm_ack == 0) {
+            break;
+        }
+        if (timeout++ > 100) {
+            return false;
+        }
+    }
     term->set_read_cb(nullptr);
     return true;
-
 }
 
 bool CMux::init()
@@ -415,10 +425,7 @@ void CMux::set_read_cb(int inst, std::function<bool(uint8_t *, size_t)> f)
     }
 }
 
-std::tuple<std::shared_ptr<Terminal>, std::unique_ptr<uint8_t[]>, size_t> esp_modem::CMux::deinit_and_eject()
+std::pair<std::shared_ptr<Terminal>, unique_buffer> CMux::detach()
 {
-    if (exit_cmux_protocol()) {
+    return std::make_pair(std::move(term), std::move(buffer));
-        return std::make_tuple(std::move(term), std::move(buffer), buffer_size);
-    }
-    return std::tuple(nullptr, nullptr, 0);
 }

components/esp_modem/src/esp_modem_dce.cpp

@@ -59,22 +59,24 @@ bool DCE_Mode::set_unsafe(DTE *dte, ModuleIf *device, Netif &netif, modem_mode m
                 return true;
             }
             netif.stop();
-            SignalGroup signal;
+            auto signal = std::make_shared<SignalGroup>();
-            dte->set_read_cb([&signal](uint8_t *data, size_t len) -> bool {
+            std::weak_ptr<SignalGroup> weak_signal = signal;
+            dte->set_read_cb([weak_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"});
                     std::string_view response((char *) data, len);
                     for (auto &it : pass)
                         if (response.find(it) != std::string::npos) {
-                            signal.set(1);
+                            if (auto signal = weak_signal.lock())
+                                signal->set(1);
                             return true;
                         }
                 }
                 return false;
             });
             netif.wait_until_ppp_exits();
-            if (!signal.wait(1, 2000)) {
+            if (!signal->wait(1, 2000)) {
                 if (!device->set_mode(modem_mode::COMMAND_MODE)) {
                     mode = modem_mode::UNDEF;
                     return false;
@@ -110,7 +112,7 @@ bool DCE_Mode::set_unsafe(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

@@ -15,6 +15,7 @@
 #include <cstring>
 #include "esp_log.h"
 #include "cxx_include/esp_modem_dte.hpp"
+#include "cxx_include/esp_modem_cmux.hpp"
 #include "esp_modem_config.h"
 
 using namespace esp_modem;
@@ -22,14 +23,12 @@ using namespace esp_modem;
 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(config->dte_buffer_size),
-    buffer(std::make_unique<uint8_t[]>(buffer_size)),
     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(dte_default_buffer_size),
-    buffer(std::make_unique<uint8_t[]>(buffer_size)),
     cmux_term(nullptr), command_term(std::move(terminal)), data_term(command_term),
     mode(modem_mode::UNDEF) {}
 
@@ -40,18 +39,18 @@ command_result DTE::command(const std::string &command, got_line_cb got_line, ui
     command_term->set_read_cb([&](uint8_t *data, size_t len) {
         if (!data) {
             data = buffer.get();
-            len = command_term->read(data + consumed, buffer_size - consumed);
+            len = command_term->read(data + buffer.consumed, buffer.size - buffer.consumed);
         } else {
-            consumed = 0; // if the underlying terminal contains data, we cannot fragment
+            buffer.consumed = 0; // if the underlying terminal contains data, we cannot fragment
         }
-        if (memchr(data + consumed, separator, len)) {
+        if (memchr(data + buffer.consumed, separator, len)) {
-            res = got_line(data, consumed + len);
+            res = got_line(data, buffer.consumed + len);
             if (res == command_result::OK || res == command_result::FAIL) {
                 signal.set(GOT_LINE);
                 return true;
             }
         }
-        consumed += len;
+        buffer.consumed += len;
         return false;
     });
     command_term->write((uint8_t *)command.c_str(), command.length());
@@ -59,7 +58,7 @@ command_result DTE::command(const std::string &command, got_line_cb got_line, ui
     if (got_lf && res == command_result::TIMEOUT) {
         throw_if_esp_fail(ESP_ERR_INVALID_STATE);
     }
-    consumed = 0;
+    buffer.consumed = 0;
     command_term->set_read_cb(nullptr);
     return res;
 }
@@ -71,25 +70,23 @@ command_result DTE::command(const std::string &cmd, got_line_cb got_line, uint32
 
 bool DTE::exit_cmux()
 {
-    auto ejected = cmux_term->deinit_and_eject();
+    if (!cmux_term->deinit()) {
-    if (ejected == std::tuple(nullptr, nullptr, 0)) {
         return false;
     }
-    // deinit succeeded -> swap the internal terminals with those ejected from cmux
+    auto ejected = cmux_term->detach();
-    command_term = std::move(std::get<0>(ejected));
+    // return the ejected terminal and buffer back to this DTE
-    buffer = std::move(std::get<1>(ejected));
+    command_term = std::move(ejected.first);
-    buffer_size = std::get<2>(ejected);
+    buffer = std::move(ejected.second);
     data_term = command_term;
     return true;
 }
 
 bool DTE::setup_cmux()
 {
-    cmux_term = std::make_shared<CMux>(command_term, std::move(buffer), buffer_size);
+    cmux_term = std::make_shared<CMux>(command_term, std::move(buffer));
     if (cmux_term == nullptr) {
         return false;
     }
-    buffer_size = 0;
     if (!cmux_term->init()) {
         return false;
     }
@@ -149,7 +146,7 @@ void DTE::set_read_cb(std::function<bool(uint8_t *, size_t)> f)
     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 = data_term->read(buffer.get(), buffer_size);
+            len = data_term->read(buffer.get(), buffer.size);
         }
         if (on_data) {
             return on_data(data, len);
@@ -160,7 +157,7 @@ void DTE::set_read_cb(std::function<bool(uint8_t *, size_t)> f)
 
 int DTE::read(uint8_t **d, size_t len)
 {
-    auto data_to_read = std::min(len, buffer_size);
+    auto data_to_read = std::min(len, buffer.size);
     auto data = buffer.get();
     auto actual_len = data_term->read(data, data_to_read);
     *d = data;
@@ -170,4 +167,10 @@ int DTE::read(uint8_t **d, size_t len)
 int DTE::write(uint8_t *data, size_t len)
 {
     return data_term->write(data, len);
-}
+}
+
+/**
+ * Implemented here to keep all headers C++11 compliant
+ */
+unique_buffer::unique_buffer(size_t size):
+    data(std::make_unique<uint8_t[]>(size)), size(size), consumed(0) {}

components/esp_modem/src/esp_modem_netif_linux.cpp

@@ -54,13 +54,16 @@ void Netif::start()
     signal.set(PPP_STARTED);
 }
 
-void Netif::stop() {}
+void Netif::stop()
+{
+    ppp_dte->set_read_cb(nullptr);
+    signal.clear(PPP_STARTED);
+}
 
 Netif::~Netif() = default;
 
 void Netif::wait_until_ppp_exits()
 {
-
 }
 
 } // namespace esp_modem

components/esp_modem/test/host_test/main/LoopbackTerm.cpp

@@ -25,7 +25,7 @@ int LoopbackTerm::write(uint8_t *data, size_t len)
         if (command == "+++") {
             response = "NO CARRIER\r\n";
         } else if (command == "ATE1\r" || command == "ATE0\r") {
-            response = "OK\r\n";
+            response = "OK\r\n ";
         } else if (command == "ATO\r") {
             response = "ERROR\r\n";
         } else if (command.find("ATD") != std::string::npos) {
@@ -43,7 +43,16 @@ int LoopbackTerm::write(uint8_t *data, size_t len)
         } else if (command.find("AT+CPIN?\r") != std::string::npos) {
             response = pin_ok ? "+CPIN: READY\r\nOK\r\n" : "+CPIN: SIM PIN\r\nOK\r\n";
         } else if (command.find("AT") != std::string::npos) {
-            response = "OK\r\n";
+            if (command.length() > 4) {
+                response = command;
+                response[0] = 'O';
+                response[1] = 'K';
+                response[2] = '\r';
+                response[3] = '\n';
+            } else {
+                response = "OK\r\n";
+            }
+
         }
         if (!response.empty()) {
             data_len = response.length();
@@ -55,7 +64,7 @@ int LoopbackTerm::write(uint8_t *data, size_t len)
     }
     if (len > 2 && data[0] == 0xf9) { // Simple CMUX responder
         // turn the request into a reply -> implements CMUX loopback
-        if (data[2] == 0x3f) {  // SABM command
+        if (data[2] == 0x3f || data[2] == 0x53) {  // SABM command
             data[2] = 0x73;
         } else if (data[2] == 0xef) { // Generic request
             data[2] = 0xff;         // generic reply

components/esp_modem/test/host_test/main/test_modem.cpp

@@ -77,6 +77,7 @@ TEST_CASE("DTE send/receive command", "[esp_modem]")
     CHECK(ret == command_result::OK);
 }
 
+
 TEST_CASE("DCE commands", "[esp_modem]")
 {
     auto term = std::make_unique<LoopbackTerm>();
@@ -96,7 +97,6 @@ TEST_CASE("DCE commands", "[esp_modem]")
     }, 1000);
     CHECK(ret == command_result::OK);
 }
-
 TEST_CASE("DCE AT commands", "[esp_modem]")
 {
     auto term = std::make_unique<LoopbackTerm>();
@@ -128,9 +128,21 @@ TEST_CASE("DCE modes", "[esp_modem]")
     auto dce = create_SIM7600_dce(&dce_config, dte, &netif);
     CHECK(dce != nullptr);
 
+    // UNDER -> CMD (OK)
     CHECK(dce->set_mode(esp_modem::modem_mode::COMMAND_MODE) == true);
+    // CMD -> CMD (Fail)
     CHECK(dce->set_mode(esp_modem::modem_mode::COMMAND_MODE) == false);
+    // CMD -> DATA (OK)
     CHECK(dce->set_mode(esp_modem::modem_mode::DATA_MODE) == true);
+    // DATA -> CMUX (Fail)
+    CHECK(dce->set_mode(esp_modem::modem_mode::CMUX_MODE) == false);
+    // DATA back -> CMD (OK)
+    CHECK(dce->set_mode(esp_modem::modem_mode::COMMAND_MODE) == true);
+    // CMD -> CMUX (OK)
+    CHECK(dce->set_mode(esp_modem::modem_mode::CMUX_MODE) == true);
+    // CMUX -> DATA (Fail)
+    CHECK(dce->set_mode(esp_modem::modem_mode::DATA_MODE) == false);
+    // CMUX back -> CMD (OK)
     CHECK(dce->set_mode(esp_modem::modem_mode::COMMAND_MODE) == true);
 }
 
@@ -171,7 +183,7 @@ TEST_CASE("Test CMUX protocol by injecting payloads", "[esp_modem]")
     CHECK(dce->set_mode(esp_modem::modem_mode::CMUX_MODE) == true);
     const auto test_command = "Test\n";
     // 1 byte payload size
-    uint8_t test_payload[] = {0xf9, 0x05, 0xff, 0x0b, 0x54, 0x65, 0x73, 0x74, 0x0a, 0xbb, 0xf9 };
+    uint8_t test_payload[] = {0xf9, 0x09, 0xff, 0x0b, 0x54, 0x65, 0x73, 0x74, 0x0a, 0xbb, 0xf9 };
     loopback->inject(&test_payload[0], sizeof(test_payload), 1);
     auto ret = dce->command(test_command, [&](uint8_t *data, size_t len) {
         std::string response((char *) data, len);
@@ -181,7 +193,7 @@ TEST_CASE("Test CMUX protocol by injecting payloads", "[esp_modem]")
     CHECK(ret == command_result::OK);
 
     // 2 byte payload size
-    uint8_t long_payload[453] = { 0xf9, 0x05, 0xef, 0x7c, 0x03, 0x7e }; // header
+    uint8_t long_payload[453] = { 0xf9, 0x09, 0xef, 0x7c, 0x03, 0x7e }; // header
     long_payload[5]   = 0x7e;   // payload to validate
     long_payload[449] = 0x7e;
     long_payload[450] = '\n';