update(esp_modem): Bump component version

Race condtion:
* First command timeouted, but the reply came just after evaluation and
set signal variable to GOT_LINE
* Second command should timeout too, but a consistency check validates
that it timeouted and at the same time GOT_LINE (from previous step) and
throws an exception

STR:
* Revert change in esp_modem_dte.cpp
* Run TEST_CASE("DTE command races", "[esp_modem]")

Closes https://github.com/espressif/esp-protocols/issues/110

.github/workflows/publish-docs-component.yml

@@ -9,6 +9,8 @@ jobs:
   docs_build:
     name: Docs-Build-And-Upload
     runs-on: ubuntu-latest
+    # Skip running on forks since it won't have access to secrets
+    if: github.repository == 'espressif/esp-protocols'
 
     steps:
       - name: Checkout esp-protocols

.github/workflows/target-test.yml

@@ -56,16 +56,23 @@ jobs:
         run: | 
           . ${IDF_PATH}/export.sh 
           cd $GITHUB_WORKSPACE/esp-protocols/components/mdns/examples/ 
-          idf.py set-target ${{ matrix.idf_target }}  
+          idf.py set-target ${{ matrix.idf_target }}
+          echo "Building for default configuration"
+          idf.py build
+          rm sdkconfig
+          echo "Building for sdkconfig.ci.eth_def"
           cat sdkconfig.ci.eth_def >> sdkconfig.defaults 
           idf.py build 
-          rm sdkconfig.defaults 
+          rm sdkconfig sdkconfig.defaults
+          echo "Building for sdkconfig.ci.eth_custom_netif"
           cat sdkconfig.ci.eth_custom_netif >> sdkconfig.defaults 
           idf.py build 
-          rm sdkconfig.defaults 
+          rm sdkconfig sdkconfig.defaults
+          echo "Building for sdkconfig.ci.eth_socket"
           cat sdkconfig.ci.eth_socket >> sdkconfig.defaults 
           idf.py build 
-          rm sdkconfig.defaults
+          rm sdkconfig sdkconfig.defaults
+          echo "Building for sdkconfig.ci.eth_no_ipv6"
           cat sdkconfig.ci.eth_no_ipv6 >> sdkconfig.defaults
           idf.py build
           cd $GITHUB_WORKSPACE/esp-protocols/components/mdns/tests/test_apps/ 
@@ -144,6 +151,8 @@ jobs:
     runs-on:
       - self-hosted
       - ESP32-ETHERNET-KIT
+    # Skip running on forks since it won't have access to secrets
+    if: github.repository == 'espressif/esp-protocols'
     container:
       image: python:3.7-buster
       options: --privileged # Privileged mode has access to serial ports
@@ -154,10 +163,8 @@ jobs:
           name: examples_app_bin_${{ matrix.idf_target }}_${{ matrix.idf_ver }}
           path: components/esp_websocket_client/examples/build/
       - name: Install Python packages
-        env:
-          PIP_EXTRA_INDEX_URL: "https://www.piwheels.org/simple"
         run: |
-          pip install -r $GITHUB_WORKSPACE/components/esp_websocket_client/examples/requirements.txt
+          pip install --only-binary cryptography --extra-index-url https://dl.espressif.com/pypi/ -r $GITHUB_WORKSPACE/components/esp_websocket_client/examples/requirements.txt
       - name: Download Example Test to target
         run: python -m esptool --chip ${{ matrix.idf_target }} write_flash 0x0 components/esp_websocket_client/examples/build/flash_image.bin
       - name: Run Example Test on target

common_components/protocol_examples_common/connect.c

@@ -396,7 +396,9 @@ static esp_netif_t *eth_start(void)
 #endif
 #elif CONFIG_EXAMPLE_USE_SPI_ETHERNET
     gpio_install_isr_service(0);
+#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)
     spi_device_handle_t spi_handle = NULL;
+#endif
     spi_bus_config_t buscfg = {
         .miso_io_num = CONFIG_EXAMPLE_ETH_SPI_MISO_GPIO,
         .mosi_io_num = CONFIG_EXAMPLE_ETH_SPI_MOSI_GPIO,
@@ -414,9 +416,14 @@ static esp_netif_t *eth_start(void)
         .spics_io_num = CONFIG_EXAMPLE_ETH_SPI_CS_GPIO,
         .queue_size = 20
     };
+#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)
     ESP_ERROR_CHECK(spi_bus_add_device(CONFIG_EXAMPLE_ETH_SPI_HOST, &devcfg, &spi_handle));
     /* dm9051 ethernet driver is based on spi driver */
     eth_dm9051_config_t dm9051_config = ETH_DM9051_DEFAULT_CONFIG(spi_handle);
+#else
+    eth_dm9051_config_t dm9051_config = ETH_DM9051_DEFAULT_CONFIG(CONFIG_EXAMPLE_ETH_SPI_HOST, &devcfg);
+#endif // ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)
+
     dm9051_config.int_gpio_num = CONFIG_EXAMPLE_ETH_SPI_INT_GPIO;
     s_mac = esp_eth_mac_new_dm9051(&dm9051_config, &mac_config);
     s_phy = esp_eth_phy_new_dm9051(&phy_config);
@@ -429,9 +436,13 @@ static esp_netif_t *eth_start(void)
         .spics_io_num = CONFIG_EXAMPLE_ETH_SPI_CS_GPIO,
         .queue_size = 20
     };
+#if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)
     ESP_ERROR_CHECK(spi_bus_add_device(CONFIG_EXAMPLE_ETH_SPI_HOST, &devcfg, &spi_handle));
     /* w5500 ethernet driver is based on spi driver */
     eth_w5500_config_t w5500_config = ETH_W5500_DEFAULT_CONFIG(spi_handle);
+#else
+    eth_w5500_config_t w5500_config = ETH_W5500_DEFAULT_CONFIG(CONFIG_EXAMPLE_ETH_SPI_HOST, &devcfg);
+#endif // ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)
     w5500_config.int_gpio_num = CONFIG_EXAMPLE_ETH_SPI_INT_GPIO;
     s_mac = esp_eth_mac_new_w5500(&w5500_config, &mac_config);
     s_phy = esp_eth_phy_new_w5500(&phy_config);

components/esp_modem/CMakeLists.txt

@@ -47,5 +47,3 @@ if(${target} STREQUAL "linux")
     # This is needed for ESP_LOGx() macros, as integer formats differ on ESP32(..) and x64
     set_target_properties(${COMPONENT_LIB} PROPERTIES COMPILE_FLAGS -Wno-format)
 endif()
-
-target_compile_options(${COMPONENT_LIB} PRIVATE "-Wno-format")

components/esp_modem/examples/ap_to_pppos/main/ap_to_pppos.c

@@ -11,12 +11,13 @@
 #include "esp_wifi.h"
 #include "esp_event.h"
 #include "esp_log.h"
+#include "esp_idf_version.h"
 #include "nvs_flash.h"
 #include "lwip/lwip_napt.h"
 #include "freertos/FreeRTOS.h"
 #include "freertos/event_groups.h"
 #include "network_dce.h"
-#if ESP_IDF_VERSION_MAJOR >= 5
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 0, 0)
     #include "esp_mac.h"
     #include "dhcpserver/dhcpserver.h"
 #endif

components/esp_modem/idf_component.yml

@@ -1,4 +1,4 @@
-version: "0.1.19"
+version: "0.1.22"
 description: esp modem
 url: https://github.com/espressif/esp-protocols/tree/master/components/esp_modem
 dependencies:

components/esp_modem/include/cxx_include/esp_modem_dte.hpp

@@ -75,6 +75,12 @@ public:
      */
     void set_read_cb(std::function<bool(uint8_t *data, size_t len)> f);
 
+    /**
+     * @brief Sets DTE error callback
+     * @param f Function to be called on DTE error
+     */
+    void set_error_cb(std::function<void(terminal_error err)> f);
+
     /**
      * @brief Sets the DTE to desired mode (Command/Data/Cmux)
      * @param m Desired operation mode

components/esp_modem/include/cxx_include/esp_modem_terminal.hpp

@@ -41,6 +41,7 @@ enum class terminal_error {
     BUFFER_OVERFLOW,
     CHECKSUM_ERROR,
     UNEXPECTED_CONTROL_FLOW,
+    DEVICE_GONE,
 };
 
 /**

components/esp_modem/include/generate/esp_modem_command_declare.inc

@@ -72,9 +72,10 @@ ESP_MODEM_DECLARE_DCE_COMMAND(set_pin, command_result, 1, STRING_IN(p1, pin)) \
  * @brief Execute the supplied AT command
  * @param[in] at AT command
  * @param[out] out Command output string
+ * @param[in] timeout AT command timeout in milliseconds
  * @return OK, FAIL or TIMEOUT
  */\
-ESP_MODEM_DECLARE_DCE_COMMAND(at, command_result, 2, STRING_IN(p1, cmd), STRING_OUT(p2, out)) \
+ESP_MODEM_DECLARE_DCE_COMMAND(at, command_result, 3, STRING_IN(p1, cmd), STRING_OUT(p2, out), INT_IN(p3, timeout)) \
     \
 /**
  * @brief Checks if the SIM needs a PIN

components/esp_modem/private_include/uart_compat.h

@@ -15,6 +15,7 @@
 #pragma once
 
 #include "esp_log.h"
+#include "esp_idf_version.h"
 #include "driver/uart.h"
 
 /**
@@ -23,7 +24,7 @@
  */
 static inline int uart_write_bytes_compat(uart_port_t uart_num, const void* src, size_t size)
 {
-#if ESP_IDF_VERSION_MAJOR >= 4 && ESP_IDF_VERSION_MINOR >= 3
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 3, 0)
     const void *data = src;
 #else
     auto *data = reinterpret_cast<const char*>(src);

components/esp_modem/src/esp_modem_c_api.cpp

@@ -179,14 +179,14 @@ extern "C" esp_err_t esp_modem_set_pin(esp_modem_dce_t *dce_wrap, const char *pi
     return command_response_to_esp_err(dce_wrap->dce->set_pin(pin_str));
 }
 
-extern "C" esp_err_t esp_modem_at(esp_modem_dce_t *dce_wrap, const char *at, char *p_out)
+extern "C" esp_err_t esp_modem_at(esp_modem_dce_t *dce_wrap, const char *at, char *p_out, int timeout)
 {
     if (dce_wrap == nullptr || dce_wrap->dce == nullptr) {
         return ESP_ERR_INVALID_ARG;
     }
     std::string out;
     std::string at_str(at);
-    auto ret = command_response_to_esp_err(dce_wrap->dce->at(at_str, out));
+    auto ret = command_response_to_esp_err(dce_wrap->dce->at(at_str, out, timeout));
     if ((p_out != NULL) && (!out.empty())) {
         strlcpy(p_out, out.c_str(), ESP_MODEM_C_API_STR_MAX);
     }
@@ -243,15 +243,17 @@ extern "C" esp_err_t esp_modem_get_imei(esp_modem_dce_t *dce_wrap, char *p_imei)
     return ret;
 }
 
-extern "C" esp_err_t esp_modem_get_operator_name(esp_modem_dce_t *dce_wrap, char *p_name)
+extern "C" esp_err_t esp_modem_get_operator_name(esp_modem_dce_t *dce_wrap, char *p_name, int *p_act)
 {
-    if (dce_wrap == nullptr || dce_wrap->dce == nullptr) {
+    if (dce_wrap == nullptr || dce_wrap->dce == nullptr || p_name == nullptr || p_act == nullptr) {
         return ESP_ERR_INVALID_ARG;
     }
     std::string name;
-    auto ret = command_response_to_esp_err(dce_wrap->dce->get_operator_name(name));
+    int act;
+    auto ret = command_response_to_esp_err(dce_wrap->dce->get_operator_name(name, act));
     if (ret == ESP_OK && !name.empty()) {
         strlcpy(p_name, name.c_str(), ESP_MODEM_C_API_STR_MAX);
+        *p_act = act;
     }
     return ret;
 }

components/esp_modem/src/esp_modem_command_library.cpp

@@ -273,7 +273,7 @@ command_result set_pdp_context(CommandableIf *t, PdpContext &pdp)
     ESP_LOGV(TAG, "%s", __func__ );
     std::string pdp_command = "AT+CGDCONT=" + std::to_string(pdp.context_id) +
                               ",\"" + pdp.protocol_type + "\",\"" + pdp.apn + "\"\r";
-    return generic_command_common(t, pdp_command);
+    return generic_command_common(t, pdp_command, 150000);
 }
 
 command_result set_data_mode(CommandableIf *t)
@@ -389,11 +389,11 @@ command_result set_pin(CommandableIf *t, const std::string &pin)
     return generic_command_common(t, set_pin_command);
 }
 
-command_result at(CommandableIf *t, const std::string &cmd, std::string &out)
+command_result at(CommandableIf *t, const std::string &cmd, std::string &out, int timeout = 500)
 {
     ESP_LOGV(TAG, "%s", __func__ );
     std::string at_command = cmd + "\r";
-    return generic_get_string(t, at_command, out);
+    return generic_get_string(t, at_command, out, timeout);
 }
 
 command_result get_signal_quality(CommandableIf *t, int &rssi, int &ber)
@@ -458,7 +458,7 @@ command_result get_network_attachment_state(CommandableIf *t, int &state)
 command_result set_radio_state(CommandableIf *t, int state)
 {
     ESP_LOGV(TAG, "%s", __func__ );
-    return generic_command_common(t, "AT+CFUN=" + std::to_string(state) + "\r");
+    return generic_command_common(t, "AT+CFUN=" + std::to_string(state) + "\r", 15000);
 }
 
 command_result get_radio_state(CommandableIf *t, int &state)

components/esp_modem/src/esp_modem_dte.cpp

@@ -36,6 +36,7 @@ command_result DTE::command(const std::string &command, got_line_cb got_line, ui
 {
     Scoped<Lock> l(internal_lock);
     command_result res = command_result::TIMEOUT;
+    signal.clear(GOT_LINE);
     command_term->set_read_cb([&](uint8_t *data, size_t len) {
         if (!data) {
             data = buffer.get();
@@ -155,6 +156,12 @@ void DTE::set_read_cb(std::function<bool(uint8_t *, size_t)> f)
     });
 }
 
+void DTE::set_error_cb(std::function<void(terminal_error err)> f)
+{
+    data_term->set_error_cb(f);
+    command_term->set_error_cb(f);
+}
+
 int DTE::read(uint8_t **d, size_t len)
 {
     auto data_to_read = std::min(len, buffer.size);

components/esp_modem/src/esp_modem_netif.cpp

@@ -14,8 +14,10 @@
 
 #include <memory>
 #include <utility>
+#include <inttypes.h>
 #include <esp_log.h>
 #include <esp_event.h>
+#include "esp_idf_version.h"
 #include "cxx_include/esp_modem_netif.hpp"
 #include "cxx_include/esp_modem_dte.hpp"
 #include "esp_netif_ppp.h"
@@ -28,7 +30,7 @@ void Netif::on_ppp_changed(void *arg, esp_event_base_t event_base,
 {
     auto *ppp = static_cast<Netif *>(arg);
     if (event_id < NETIF_PP_PHASE_OFFSET) {
-        ESP_LOGI("esp_modem_netif", "PPP state changed event %d", event_id);
+        ESP_LOGI("esp_modem_netif", "PPP state changed event %" PRId32, event_id);
         // only notify the modem on state/error events, ignoring phase transitions
         ppp->signal.set(PPP_EXIT);
     }
@@ -58,7 +60,7 @@ esp_err_t Netif::esp_modem_post_attach(esp_netif_t *esp_netif, void *args)
     esp_netif_ppp_config_t ppp_config = { .ppp_phase_event_enabled = true,    // assuming phase enabled, as earlier IDFs
                                           .ppp_error_event_enabled = false
                                         }; // don't provide cfg getters so we enable both events
-#if ESP_IDF_VERSION_MAJOR >= 4 && ESP_IDF_VERSION_MINOR >= 4
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
     esp_netif_ppp_get_params(esp_netif, &ppp_config);
 #endif // ESP-IDF >= v4.4
     if (!ppp_config.ppp_error_event_enabled) {

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

@@ -17,6 +17,7 @@ int LoopbackTerm::write(uint8_t *data, size_t len)
 {
     if (inject_by) {    // injection test: ignore what we write, but respond with injected data
         auto ret = std::async(&LoopbackTerm::batch_read, this);
+        async_results.push_back(std::move(ret));
         return len;
     }
     if (len > 2 && (data[len - 1] == '\r' || data[len - 1] == '+') ) { // Simple AT responder
@@ -99,7 +100,7 @@ LoopbackTerm::LoopbackTerm(bool is_bg96): loopback_data(), data_len(0), pin_ok(f
 
 LoopbackTerm::LoopbackTerm(): loopback_data(), data_len(0), pin_ok(false), is_bg96(false), inject_by(0) {}
 
-int LoopbackTerm::inject(uint8_t *data, size_t len, size_t injected_by)
+int LoopbackTerm::inject(uint8_t *data, size_t len, size_t injected_by, size_t delay_before, size_t delay_after)
 {
     if (data == nullptr) {
         inject_by = 0;
@@ -110,14 +111,20 @@ int LoopbackTerm::inject(uint8_t *data, size_t len, size_t injected_by)
     memcpy(&loopback_data[0], data, len);
     data_len = len;
     inject_by = injected_by;
+    delay_after_inject = delay_after;
+    delay_before_inject = delay_before;
     return len;
 }
 
 void LoopbackTerm::batch_read()
 {
     while (data_len > 0) {
-        on_read(nullptr, std::min(inject_by, data_len));
-        Task::Delay(1);
+        Task::Delay(delay_before_inject);
+        {
+            Scoped<Lock> lock(on_read_guard);
+            on_read(nullptr, std::min(inject_by, data_len));
+        }
+        Task::Delay(delay_after_inject);
     }
 }
 

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

@@ -17,7 +17,7 @@ public:
      * inject_by defines batch sizes: the read callback is called multiple times
      * with partial data of `inject_by` size
      */
-    int inject(uint8_t *data, size_t len, size_t inject_by);
+    int inject(uint8_t *data, size_t len, size_t inject_by,size_t delay_before=0, size_t delay_after=1);
 
     void start() override;
     void stop() override;
@@ -26,6 +26,12 @@ public:
 
     int read(uint8_t *data, size_t len) override;
 
+    void set_read_cb(std::function<bool(uint8_t *data, size_t len)> f) override
+    {
+        Scoped<Lock> lock(on_read_guard);
+        on_read = std::move(f);
+    }
+
 private:
     enum class status_t {
         STARTED,
@@ -39,4 +45,9 @@ private:
     bool pin_ok;
     bool is_bg96;
     size_t inject_by;
+    size_t delay_before_inject;
+    size_t delay_after_inject;
+    std::vector<std::future<void>> async_results;
+    Lock on_read_guard;
+
 };

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

@@ -7,6 +7,28 @@
 
 using namespace esp_modem;
 
+TEST_CASE("DTE command races", "[esp_modem]") {
+    auto term = std::make_unique<LoopbackTerm>(true);
+    auto loopback = term.get();
+    auto dte = std::make_shared<DTE>(std::move(term));
+    CHECK(term == nullptr);
+    esp_modem_dce_config_t dce_config = ESP_MODEM_DCE_DEFAULT_CONFIG("APN");
+    esp_netif_t netif{};
+    auto dce = create_BG96_dce(&dce_config, dte, &netif);
+    CHECK(dce != nullptr);
+    uint8_t resp[] = {'O', 'K', '\n'};
+    // run many commands in succession with the timeout set exactly to the timespan of injected reply
+    // (checks for potential exception, data races, recycled local variables, etc.)
+    for (int i=0; i<1000; ++i) {
+        loopback->inject(&resp[0], sizeof(resp), sizeof(resp), /* 1ms before injecting reply */1, 0);
+        auto ret = dce->command("AT\n", [&](uint8_t *data, size_t len) {
+            return command_result::OK;
+        }, 1);
+        // this command should either timeout or finish successfully
+        CHECK((ret == command_result::TIMEOUT || ret == command_result::OK));
+    }
+}
+
 TEST_CASE("Test polymorphic delete for custom device/dte", "[esp_modem]")
 {
     auto term = std::make_unique<LoopbackTerm>(true);

components/mdns/mdns.c

@@ -232,7 +232,7 @@ static char * _mdns_mangle_name(char* in) {
 static bool _mdns_service_match(const mdns_service_t * srv, const char * service, const char * proto,
                                 const char * hostname)
 {
-    if (!service || !proto) {
+    if (!service || !proto || !srv->hostname) {
         return false;
     }
     return !strcasecmp(srv->service, service) && !strcasecmp(srv->proto, proto) &&
@@ -3520,7 +3520,7 @@ void mdns_parse_packet(mdns_rx_packet_t * packet)
                         _mdns_remove_parsed_question(parsed_packet, type, service);
                     } else if (service) {
                         //check if TTL is more than half of the full TTL value (4500)
-                        if (ttl > 2250) {
+                        if (ttl > (MDNS_ANSWER_PTR_TTL/2)) {
                             _mdns_remove_scheduled_answer(packet->tcpip_if, packet->ip_protocol, type, service);
                         }
                     }
@@ -3675,7 +3675,7 @@ void mdns_parse_packet(mdns_rx_packet_t * packet)
                     if (col && !_mdns_server->interfaces[packet->tcpip_if].pcbs[packet->ip_protocol].probe_running && service) {
                         do_not_reply = true;
                         _mdns_init_pcb_probe(packet->tcpip_if, packet->ip_protocol, &service, 1, true);
-                    } else if (ttl > 2250 && !col && !parsed_packet->authoritative && !parsed_packet->probe && !parsed_packet->questions && !_mdns_server->interfaces[packet->tcpip_if].pcbs[packet->ip_protocol].probe_running) {
+                    } else if (ttl > (MDNS_ANSWER_TXT_TTL/2) && !col && !parsed_packet->authoritative && !parsed_packet->probe && !parsed_packet->questions && !_mdns_server->interfaces[packet->tcpip_if].pcbs[packet->ip_protocol].probe_running) {
                         _mdns_remove_scheduled_answer(packet->tcpip_if, packet->ip_protocol, type, service);
                     }
                 }
@@ -4558,7 +4558,8 @@ static void _mdns_remap_self_service_hostname(const char * old_hostname, const c
     mdns_srv_item_t * service = _mdns_server->services;
 
     while (service) {
-        if (strcmp(service->service->hostname, old_hostname) == 0) {
+        if (service->service->hostname &&
+            strcmp(service->service->hostname, old_hostname) == 0) {
             free((char *)service->service->hostname);
             service->service->hostname = strdup(new_hostname);
         }
@@ -5430,7 +5431,7 @@ esp_err_t mdns_instance_name_set(const char * instance)
 esp_err_t mdns_service_add_for_host(const char * instance, const char * service, const char * proto, const char * hostname,
                                     uint16_t port, mdns_txt_item_t txt[], size_t num_items)
 {
-    if (!_mdns_server || _str_null_or_empty(service) || _str_null_or_empty(proto) || !port) {
+    if (!_mdns_server || _str_null_or_empty(service) || _str_null_or_empty(proto) || !port || !hostname) {
         return ESP_ERR_INVALID_ARG;
     }