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

@@ -151,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

components/esp_modem/idf_component.yml

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

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/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();

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

@@ -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);
                     }
                 }