Skip to content

Commit

Permalink
fix(modem): Fixed AT commands to copy strings to prevent overrides
Browse files Browse the repository at this point in the history 
Previously we used std::string_view, which pointed to the lower-layers
buffer which might have been reused for other asynchronous operations
before processing it, thus causing corruption of replies.

Closes #463
  • Loading branch information
@david-cermak
david-cermak committed Jan 15, 2024
1 parent 7d0eb5c commit e57691b
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 29 deletions.
8 changes: 3 additions & 5 deletions components/esp_modem/include/cxx_include/esp_modem_command_library_utils.hpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD
* SPDX-FileCopyrightText: 2021-2024 Espressif Systems (Shanghai) CO LTD
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -25,18 +25,16 @@ command_result generic_command(CommandableIf *t, const std::string &command,
* @brief Utility command to send command and return reply (after DCE says OK)
* @param t Anything that is "command-able"
* @param command Command to issue
* @param output String to return
* @param timeout_ms
* @param output String to return (could be either std::string& or std::string_view&)
* @param timeout_ms Command timeout in ms
* @return Generic command return type (OK, FAIL, TIMEOUT)
*/
command_result generic_get_string(CommandableIf *t, const std::string &command, std::string &output, uint32_t timeout_ms = 500);
template <typename T> command_result generic_get_string(CommandableIf *t, const std::string &command, T &output, uint32_t timeout_ms = 500);

/**
* @brief Generic command that passes on "OK" and fails on "ERROR"
* @param t Anything that is "command-able"
* @param command Command to issue
* @param timeout
* @param timeout_ms Command timeout in ms
* @return Generic command return type (OK, FAIL, TIMEOUT)
*/
Expand Down
65 changes: 41 additions & 24 deletions components/esp_modem/src/esp_modem_command_library.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* SPDX-FileCopyrightText: 2021-2023 Espressif Systems (Shanghai) CO LTD
* SPDX-FileCopyrightText: 2021-2024 Espressif Systems (Shanghai) CO LTD
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -51,7 +51,34 @@ command_result generic_command(CommandableIf *t, const std::string &command,
return generic_command(t, command, pass, fail, timeout_ms);
}

static command_result generic_get_string(CommandableIf *t, const std::string &command, std::string_view &output, uint32_t timeout_ms = 500)
/*
* Purpose of this namespace is to provide different means of assigning the result to a string-like parameter.
* By default we assign strings, which comes with an allocation. Alternatively we could take `std::span`
* with user's buffer and directly copy the result, thus avoiding allocations (this is not used as of now)
*/
namespace str_copy {

bool set(std::string &dest, std::string_view &src)
{
dest = src;
return true;
}

/* This is an example of using std::span output in generic_get_string()
bool set(std::span<char> &dest, std::string_view &src)
{
if (dest.size() >= src.size()) {
std::copy(src.begin(), src.end(), dest.data());
return true;
}
ESP_LOGE(TAG, "Cannot set result of size %d (to span of size %d)", dest.size(), src.size());
return false;
}
*/

} // str_copy

template <typename T> command_result generic_get_string(CommandableIf *t, const std::string &command, T &output, uint32_t timeout_ms)
{
ESP_LOGV(TAG, "%s", __func__ );
return t->command(command, [&](uint8_t *data, size_t len) {
Expand All @@ -70,26 +97,16 @@ static command_result generic_get_string(CommandableIf *t, const std::string &co
} else if (token.find("ERROR") != std::string::npos) {
return command_result::FAIL;
} else if (token.size() > 2) {
output = token;
if (!str_copy::set(output, token)) {
return command_result::FAIL;
}
}
response = response.substr(pos + 1);
}
return command_result::TIMEOUT;
}, timeout_ms);
}

command_result generic_get_string(CommandableIf *t, const std::string &command, std::string &output, uint32_t timeout_ms)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
auto ret = generic_get_string(t, command, out, timeout_ms);
if (ret == command_result::OK) {
output = out;
}
return ret;
}


command_result generic_command_common(CommandableIf *t, const std::string &command, uint32_t timeout_ms)
{
ESP_LOGV(TAG, "%s", __func__ );
Expand Down Expand Up @@ -153,7 +170,7 @@ command_result hang_up(CommandableIf *t)
command_result get_battery_status(CommandableIf *t, int &voltage, int &bcs, int &bcl)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+CBC\r", out);
if (ret != command_result::OK) {
return ret;
Expand Down Expand Up @@ -189,7 +206,7 @@ command_result get_battery_status(CommandableIf *t, int &voltage, int &bcs, int
command_result get_battery_status_sim7xxx(CommandableIf *t, int &voltage, int &bcs, int &bcl)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+CBC\r", out);
if (ret != command_result::OK) {
return ret;
Expand Down Expand Up @@ -224,7 +241,7 @@ command_result set_flow_control(CommandableIf *t, int dce_flow, int dte_flow)
command_result get_operator_name(CommandableIf *t, std::string &operator_name, int &act)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+COPS?\r", out, 75000);
if (ret != command_result::OK) {
return ret;
Expand Down Expand Up @@ -361,7 +378,7 @@ command_result set_cmux(CommandableIf *t)
command_result read_pin(CommandableIf *t, bool &pin_ok)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+CPIN?\r", out);
if (ret != command_result::OK) {
return ret;
Expand Down Expand Up @@ -397,7 +414,7 @@ command_result at(CommandableIf *t, const std::string &cmd, std::string &out, in
command_result get_signal_quality(CommandableIf *t, int &rssi, int &ber)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+CSQ\r", out);
if (ret != command_result::OK) {
return ret;
Expand Down Expand Up @@ -435,7 +452,7 @@ command_result set_network_attachment_state(CommandableIf *t, int state)
command_result get_network_attachment_state(CommandableIf *t, int &state)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+CGATT?\r", out);
if (ret != command_result::OK) {
return ret;
Expand All @@ -462,7 +479,7 @@ command_result set_radio_state(CommandableIf *t, int state)
command_result get_radio_state(CommandableIf *t, int &state)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+CFUN?\r", out);
if (ret != command_result::OK) {
return ret;
Expand Down Expand Up @@ -527,7 +544,7 @@ command_result set_network_bands_sim76xx(CommandableIf *t, const std::string &mo
command_result get_network_system_mode(CommandableIf *t, int &mode)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+CNSMOD?\r", out);
if (ret != command_result::OK) {
return ret;
Expand Down Expand Up @@ -555,7 +572,7 @@ command_result set_gnss_power_mode(CommandableIf *t, int mode)
command_result get_gnss_power_mode(CommandableIf *t, int &mode)
{
ESP_LOGV(TAG, "%s", __func__ );
std::string_view out;
std::string out;
auto ret = generic_get_string(t, "AT+CGNSPWR?\r", out);
if (ret != command_result::OK) {
return ret;
Expand Down

0 comments on commit e57691b

Please sign in to comment.