From 3254f8deaec692ac80c5ce052254f155ffca65f1 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 18 Jan 2022 20:49:58 +0100 Subject: [PATCH 1/2] esp_system: usb_console: fix restart when Wi-Fi is working MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, reset over USB CDC was done by calling esp_restart from an interrupt handler. This works only until some restart hook function is registered using esp_register_shutdown_handler, and the hook function tries to do something that isn’t allowed in an interrupt handler. One such case is with Wi-Fi. When Wi-Fi driver is installed, it registers esp_wifi_stop as a shutdown handler function. However esp_wifi_stop cannot be called from an ISR, and hence we shouldn’t call esp_restart from an ISR either. This commit modifies USB CDC driver to call esp_restart by posting it to esp_timer task. Closes https://github.com/espressif/esp-idf/issues/7404 --- components/esp_system/linker.lf | 1 + .../esp_system/port/soc/esp32s2/usb_console.c | 55 ++++++++++++++++++- components/esp_system/system_init_fn.txt | 4 ++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/components/esp_system/linker.lf b/components/esp_system/linker.lf index b7639db8da..581a886631 100644 --- a/components/esp_system/linker.lf +++ b/components/esp_system/linker.lf @@ -22,6 +22,7 @@ entries: usb_console:esp_usb_console_cdc_acm_cb (noflash) usb_console:esp_usb_console_dfu_detach_cb (noflash) usb_console:esp_usb_console_before_restart (noflash) + usb_console:esp_usb_console_on_restart_timeout (noflash) [mapping:vfs_cdcacm] archive: libvfs.a diff --git a/components/esp_system/port/soc/esp32s2/usb_console.c b/components/esp_system/port/soc/esp32s2/usb_console.c index 4750207208..a27308e3a7 100644 --- a/components/esp_system/port/soc/esp32s2/usb_console.c +++ b/components/esp_system/port/soc/esp32s2/usb_console.c @@ -13,8 +13,13 @@ #include "freertos/task.h" #include "freertos/semphr.h" #include "esp_system.h" +#include "esp_log.h" +#include "esp_timer.h" +#include "esp_check.h" #include "esp_intr_alloc.h" #include "esp_private/usb_console.h" +#include "esp_private/system_internal.h" +#include "esp_private/startup_internal.h" #include "soc/periph_defs.h" #include "soc/rtc_cntl_reg.h" #include "soc/usb_struct.h" @@ -51,6 +56,9 @@ static uint8_t cdcmem[CDC_WORK_BUF_SIZE]; static esp_usb_console_cb_t s_rx_cb; static esp_usb_console_cb_t s_tx_cb; static void *s_cb_arg; +static esp_timer_handle_t s_restart_timer; + +static const char* TAG = "usb_console"; #ifdef CONFIG_ESP_CONSOLE_USB_CDC_SUPPORT_ETS_PRINTF static portMUX_TYPE s_write_lock = portMUX_INITIALIZER_UNLOCKED; @@ -68,6 +76,9 @@ static inline void write_lock_release(void); /* The two functions below need to be revisited in the multicore case */ _Static_assert(SOC_CPU_CORES_NUM == 1, "usb_osglue_*_int is not multicore capable"); +/* Other forward declarations */ +void esp_usb_console_before_restart(void); + /* Called by ROM to disable the interrupts * Non-static to allow placement into IRAM by ldgen. */ @@ -150,10 +161,37 @@ void esp_usb_console_interrupt(void *arg) usb_dc_check_poll_for_interrupts(); /* Restart can be requested from esp_usb_console_cdc_acm_cb or esp_usb_console_dfu_detach_cb */ if (s_queue_reboot != REBOOT_NONE) { - esp_restart(); + /* We can't call esp_restart here directly, since this function is called from an ISR. + * Instead, start an esp_timer and call esp_restart from the callback. + */ + esp_err_t err = ESP_FAIL; + if (s_restart_timer) { + /* In case the timer is already running, stop it. No error check since this will fail if + * the timer is not running. + */ + esp_timer_stop(s_restart_timer); + /* Start the timer again. 50ms seems to be not too long for the user to notice, but + * enough for the USB console output to be flushed. + */ + const int restart_timeout_us = 50 * 1000; + err = esp_timer_start_once(s_restart_timer, restart_timeout_us); + } + if (err != ESP_OK) { + /* Can't schedule a restart for some reason? Call the "no-OS" restart function directly. */ + esp_usb_console_before_restart(); + esp_restart_noos(); + } } } +/* Called as esp_timer callback when the restart timeout expires. + * Non-static to allow placement into IRAM by ldgen. + */ +void esp_usb_console_on_restart_timeout(void *arg) +{ + esp_restart(); +} + /* Call the USB interrupt handler while any interrupts are pending, * but not more than a few times. * Non-static to allow placement into IRAM by ldgen. @@ -256,6 +294,21 @@ esp_err_t esp_usb_console_init(void) return ESP_OK; } +/* This function runs as part of the startup code to initialize the restart timer. + * This is not done as part of esp_usb_console_init since that function is called + * too early, before esp_timer is fully initialized. + * This gets called a bit later in the process when we can already register a timer. + */ +ESP_SYSTEM_INIT_FN(esp_usb_console_init_restart_timer, BIT(0), 220) +{ + esp_timer_create_args_t timer_create_args = { + .callback = &esp_usb_console_on_restart_timeout, + .name = "usb_console_restart" + }; + ESP_RETURN_ON_ERROR(esp_timer_create(&timer_create_args, &s_restart_timer), TAG, "failed to create the restart timer"); + return ESP_OK; +} + /* Non-static to allow placement into IRAM by ldgen. * Must be called with the write lock held. */ diff --git a/components/esp_system/system_init_fn.txt b/components/esp_system/system_init_fn.txt index 7f67937c60..3fb5a7a216 100644 --- a/components/esp_system/system_init_fn.txt +++ b/components/esp_system/system_init_fn.txt @@ -26,3 +26,7 @@ # the rest of the components which are initialized from startup.c # [refactor-todo]: move init calls into respective components 200: init_components0 in components/esp_system/startup.c on BIT(0) + +# usb_console needs to create an esp_timer at startup. +# This can be done only after esp_timer initialization, which is now in init_components0. +220: esp_usb_console_init_restart_timer in components/esp_system/port/soc/esp32s2/usb_console.c on BIT(0) From bf10146a157b93edc41f4076f32be378d0c43f7b Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 17 May 2022 18:52:49 +0200 Subject: [PATCH 2/2] esp_system, vfs: fix incomplete blocking reads in vfs_cdcacm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocking read from cdcacm VFS could return less bytes than requested. This didn’t match the behaviour of other VFS drivers, and higher level code could misbehave. --- .../include/esp_private/usb_console.h | 20 ++++++------------- .../esp_system/port/soc/esp32s2/usb_console.c | 8 ++++---- components/vfs/vfs_cdcacm.c | 15 +++++++------- tools/ci/check_copyright_ignore.txt | 1 - 4 files changed, 18 insertions(+), 26 deletions(-) diff --git a/components/esp_system/include/esp_private/usb_console.h b/components/esp_system/include/esp_private/usb_console.h index 1e9536f458..1243b1325c 100644 --- a/components/esp_system/include/esp_private/usb_console.h +++ b/components/esp_system/include/esp_private/usb_console.h @@ -1,16 +1,8 @@ -// Copyright 2019-2020 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. +/* + * SPDX-FileCopyrightText: 2019-2022 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #pragma once @@ -62,7 +54,7 @@ ssize_t esp_usb_console_flush(void); ssize_t esp_usb_console_read_buf(char* buf, size_t buf_size); -bool esp_usb_console_read_available(void); +ssize_t esp_usb_console_available_for_read(void); bool esp_usb_console_write_available(void); diff --git a/components/esp_system/port/soc/esp32s2/usb_console.c b/components/esp_system/port/soc/esp32s2/usb_console.c index a27308e3a7..16a6360762 100644 --- a/components/esp_system/port/soc/esp32s2/usb_console.c +++ b/components/esp_system/port/soc/esp32s2/usb_console.c @@ -386,7 +386,7 @@ ssize_t esp_usb_console_read_buf(char *buf, size_t buf_size) if (s_cdc_acm_device == NULL) { return -1; } - if (!esp_usb_console_read_available()) { + if (esp_usb_console_available_for_read() == 0) { return 0; } int bytes_read = cdc_acm_fifo_read(s_cdc_acm_device, (uint8_t*) buf, buf_size); @@ -414,12 +414,12 @@ esp_err_t esp_usb_console_set_cb(esp_usb_console_cb_t rx_cb, esp_usb_console_cb_ return ESP_OK; } -bool esp_usb_console_read_available(void) +ssize_t esp_usb_console_available_for_read(void) { if (s_cdc_acm_device == NULL) { - return false; + return -1; } - return cdc_acm_rx_fifo_cnt(s_cdc_acm_device) > 0; + return cdc_acm_rx_fifo_cnt(s_cdc_acm_device); } bool esp_usb_console_write_available(void) diff --git a/components/vfs/vfs_cdcacm.c b/components/vfs/vfs_cdcacm.c index 357e67277f..874c3af317 100644 --- a/components/vfs/vfs_cdcacm.c +++ b/components/vfs/vfs_cdcacm.c @@ -125,15 +125,16 @@ static int cdcacm_read_char(void) } } -static bool cdcacm_data_in_buffer(void) +static ssize_t cdcacm_data_length_in_buffer(void) { + ssize_t len = esp_usb_console_available_for_read(); + if (len < 0) { + len = 0; + } if (s_peek_char != NONE) { - return true; + len += 1; } - if (esp_usb_console_read_available()) { - return true; - } - return false; + return len; } /* Push back a character; it will be returned by next call to cdcacm_read_char */ @@ -150,7 +151,7 @@ static ssize_t cdcacm_read(int fd, void *data, size_t size) ssize_t received = 0; _lock_acquire_recursive(&s_read_lock); - while (!cdcacm_data_in_buffer()) { + while (cdcacm_data_length_in_buffer() < size) { if (!s_blocking) { errno = EWOULDBLOCK; _lock_release_recursive(&s_read_lock); diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index 631ba90633..41337e0617 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -644,7 +644,6 @@ components/esp_system/include/esp_int_wdt.h components/esp_system/include/esp_private/dbg_stubs.h components/esp_system/include/esp_private/panic_internal.h components/esp_system/include/esp_private/system_internal.h -components/esp_system/include/esp_private/usb_console.h components/esp_system/include/esp_task.h components/esp_system/port/arch/riscv/expression_with_stack.c components/esp_system/port/arch/xtensa/expression_with_stack.c