Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/cli/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
idf_component_register(
INCLUDE_DIRS "include" "detail/cli/include"
SRC_DIRS "src"
REQUIRES driver esp_driver_uart esp_driver_usb_serial_jtag vfs logger)
REQUIRES driver esp_driver_uart esp_driver_usb_serial_jtag vfs esp_vfs_console logger)
3 changes: 3 additions & 0 deletions components/cli/example/sdkconfig.defaults.esp32s2
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# on the ESP32S2, which has native USB, we need to set the console so that the
# CLI can be configured correctly:
CONFIG_ESP_CONSOLE_USB_CDC=y
58 changes: 49 additions & 9 deletions components/cli/include/cli.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,14 @@
#include "driver/usb_serial_jtag_vfs.h"
#include "esp_err.h"
#include "esp_system.h"
#include <fcntl.h>

#include "esp_vfs_cdcacm.h"
#include "esp_vfs_dev.h"
#include "esp_vfs_usb_serial_jtag.h"

#include "line_input.hpp"

#ifdef CONFIG_ESP_CONSOLE_USB_CDC
#error The cli component is currently incompatible with CONFIG ESP_CONSOLE_USB_CDC console.
#endif // CONFIG_ESP_CONSOLE_USB_CDC

#ifndef STRINGIFY
#define STRINGIFY(s) STRINGIFY2(s)
#define STRINGIFY2(s) #s
Expand All @@ -38,9 +36,9 @@ namespace espp {
*
* @note You should call configure_stdin_stdout() before creating a Cli object
* to ensure that std::cin works as needed. If you do not want to use the
* Cli over the ESP CONSOLE (e.g. the ESP's UART, USB Serial/JTAG) and
* instead want to run it over a different UART port, VFS, or some other
* configuration, then you should call one of
* Cli over the ESP CONSOLE (e.g. the ESP's UART, USB Serial/JTAG, USB
* CDC) and instead want to run it over a different UART port, VFS, or
* some other configuration, then you should call one of
* - configure_stdin_stdout_uart()
* - configure_stdin_stdout_vfs()
* - configure_stdin_stdout_custom()
Expand All @@ -58,6 +56,7 @@ class Cli : private cli::CliSession {
* compiled to use. This will only work if the ESP_CONSOLE was
* configured to use one of the following:
* - CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG
* - CONFIG_ESP_CONSOLE_USB_CDC
* - CONFIG_ESP_CONSOLE_UART
*
* If you want to use a different console, you should use one of the
Expand All @@ -77,6 +76,8 @@ class Cli : private cli::CliSession {

#if CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG
configure_stdin_stdout_usb_serial_jtag();
#elif CONFIG_ESP_CONSOLE_USB_CDC
configure_stdin_stdout_usb_cdc();
#elif CONFIG_ESP_CONSOLE_UART
configure_stdin_stdout_uart((uart_port_t)CONFIG_ESP_CONSOLE_UART_NUM,
CONFIG_ESP_CONSOLE_UART_BAUDRATE);
Expand Down Expand Up @@ -182,6 +183,47 @@ class Cli : private cli::CliSession {
configured_ = true;
}

/**
* @brief Configure the USB CDC driver to support blocking input read, so
* that std::cin (which assumes a blocking read) will function. This
* should be primarily used when you want to use the std::cin/std::getline
* and other std input functions or you want to use the cli library.
*/
static void configure_stdin_stdout_usb_cdc(void) {
if (configured_) {
return;
}

// drain stdout before reconfiguring it
fflush(stdout);
fsync(fileno(stdout));

const std::string_view dev_name = "/dev/cdcacm";

// redirect stdin, stdout, stderr to the USB CDC interface
console_.in = freopen(dev_name.data(), "r", stdin);
console_.out = freopen(dev_name.data(), "w", stdout);
console_.err = freopen(dev_name.data(), "w", stderr);
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The freopen calls can return NULL on failure, but these return values are not checked. While configure_stdin_stdout_vfs() also doesn't check these (lines 264-266), it would be good practice to verify that the operations succeeded before proceeding. Consider adding checks like:

if (!console_.in || !console_.out || !console_.err) {
  // Handle error
}
Suggested change
console_.err = freopen(dev_name.data(), "w", stderr);
console_.err = freopen(dev_name.data(), "w", stderr);
// Check for errors in freopen calls
if (!console_.in || !console_.out || !console_.err) {
// Handle error: could not redirect stdio to USB CDC
return;
}

Copilot uses AI. Check for mistakes.

esp_vfs_dev_cdcacm_register();

Comment on lines +203 to +209
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VFS device should be registered before calling freopen on it. Currently, freopen is called on lines 204-206 before esp_vfs_dev_cdcacm_register() on line 208. This is inconsistent with the configure_stdin_stdout_vfs() function (lines 261 then 264-266) which registers the VFS first, then calls freopen. The order should be:

  1. Register the VFS device with esp_vfs_dev_cdcacm_register()
  2. Then call freopen to redirect stdin/stdout/stderr
Suggested change
// redirect stdin, stdout, stderr to the USB CDC interface
console_.in = freopen(dev_name.data(), "r", stdin);
console_.out = freopen(dev_name.data(), "w", stdout);
console_.err = freopen(dev_name.data(), "w", stderr);
esp_vfs_dev_cdcacm_register();
// Register the VFS device before redirecting streams
esp_vfs_dev_cdcacm_register();
// redirect stdin, stdout, stderr to the USB CDC interface
console_.in = freopen(dev_name.data(), "r", stdin);
console_.out = freopen(dev_name.data(), "w", stdout);
console_.err = freopen(dev_name.data(), "w", stderr);

Copilot uses AI. Check for mistakes.
esp_vfs_dev_cdcacm_set_rx_line_endings(ESP_LINE_ENDINGS_CR);
esp_vfs_dev_cdcacm_set_tx_line_endings(ESP_LINE_ENDINGS_CRLF);

// Enable blocking mode on stdin and stdout
fcntl(fileno(stdout), F_SETFL, 0);
fcntl(fileno(stdin), F_SETFL, 0);

// Initialize VFS & UART so we can use std::cout/cin
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment "Initialize VFS & UART" is misleading as this function configures USB CDC, not UART. It should say "Initialize VFS & USB CDC" to be accurate.

Suggested change
// Initialize VFS & UART so we can use std::cout/cin
// Initialize VFS & USB CDC so we can use std::cout/cin

Copilot uses AI. Check for mistakes.
// _IOFBF = full buffering
// _IOLBF = line buffering
// _IONBF = no buffering
// disable buffering on stdin
setvbuf(stdin, nullptr, _IONBF, 0);
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setvbuf call should be placed before the VFS registration and freopen calls, similar to the pattern in configure_stdin_stdout_vfs() (line 258) and configure_stdin_stdout_uart() (line 121). The current placement after freopen may not properly configure stdin's buffering mode. Move this call to after the fflush/fsync operations (around line 199-200).

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Missing a final fflush/fsync sequence before setting configured_ = true. The configure_stdin_stdout_uart() (lines 146-147) and configure_stdin_stdout_usb_serial_jtag() (lines 180-181) functions both call fflush(stdout) and fsync(fileno(stdout)) at the end to ensure all output is written before completing configuration. Consider adding this for consistency:

fflush(stdout);
fsync(fileno(stdout));

configured_ = true;
Suggested change
fflush(stdout);
fsync(fileno(stdout));

Copilot uses AI. Check for mistakes.
configured_ = true;
}

/**
* @brief Configure stdin/stdout to use a custom VFS driver. This should be
* used when you have a custom VFS driver that you want to use for
Expand Down Expand Up @@ -218,8 +260,6 @@ class Cli : private cli::CliSession {
// Register the USB CDC interface
[[maybe_unused]] auto err = esp_vfs_register(dev_name.data(), &vfs, NULL);

// TODO: this function is mostly untested, so we should probably add some
// error handling here and store the resultant pointers for later use
// redirect stdin, stdout, stderr to the USB CDC interface
console_.in = freopen(dev_name.data(), "r", stdin);
console_.out = freopen(dev_name.data(), "w", stdout);
Expand Down
1 change: 1 addition & 0 deletions components/cli/src/cli.cpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "cli.hpp"

bool espp::Cli::configured_ = false;
espp::Cli::console_handle_t espp::Cli::console_ = {nullptr, nullptr, nullptr};
Loading