From 6cc093fea213da49c4dadc2e4c614eb383b624b2 Mon Sep 17 00:00:00 2001 From: folkert van heusden Date: Thu, 9 May 2024 22:03:46 +0200 Subject: [PATCH] arduino Serial is not thread safe --- ESP32/main.ino | 27 +++---------------- dc11.cpp | 71 ++++++++++++++++++++++++++++++++++---------------- dc11.h | 6 ++--- debugger.cpp | 20 -------------- 4 files changed, 53 insertions(+), 71 deletions(-) diff --git a/ESP32/main.ino b/ESP32/main.ino index cbf04bc..36e475d 100644 --- a/ESP32/main.ino +++ b/ESP32/main.ino @@ -53,12 +53,6 @@ constexpr const char SERIAL_CFG_FILE[] = "/serial.json"; -#if defined(BUILD_FOR_RP2040) -#define Serial_RS232 Serial1 -#elif defined(TTY_SERIAL_RX) -HardwareSerial Serial_RS232(2); -#endif - bus *b = nullptr; cpu *c = nullptr; tty *tty_ = nullptr; @@ -78,7 +72,7 @@ bool trace_output { false }; ntp *ntp_ { nullptr }; -void console_thread_wrapper_panel(void *const c) +static void console_thread_wrapper_panel(void *const c) { console *const cnsl = reinterpret_cast(c); @@ -213,17 +207,12 @@ void start_network(console *const c) dc11 *dc11_ = new dc11(1100, b); #if !defined(BUILD_FOR_RP2040) && defined(TTY_SERIAL_RX) - constexpr uint32_t hwSerialConfig = SERIAL_8N1; - uint32_t bitrate = load_serial_speed_configuration(); + uint32_t bitrate = load_serial_speed_configuration(); Serial.printf("* Init TTY (on DC11), baudrate: %d bps, RX: %d, TX: %d", bitrate, TTY_SERIAL_RX, TTY_SERIAL_TX); Serial.println(F("")); - Serial_RS232.begin(bitrate, hwSerialConfig, TTY_SERIAL_RX, TTY_SERIAL_TX); - Serial_RS232.setHwFlowCtrlMode(0); - Serial_RS232.println(F("Go!")); - - dc11_->set_serial(&Serial_RS232); + dc11_->set_serial(bitrate, TTY_SERIAL_RX, TTY_SERIAL_TX); #endif b->add_DC11(dc11_); @@ -244,16 +233,6 @@ void recall_configuration(console *const cnsl) } #endif -#if defined(TTY_SERIAL_RX) -void set_tty_serial_speed(console *const c, const uint32_t bps) -{ - Serial_RS232.begin(bps); - - if (save_serial_speed_configuration(bps) == false) - c->put_string_lf("Failed to store configuration file with serial settings"); -} -#endif - #if defined(ESP32) void heap_caps_alloc_failed_hook(size_t requested_size, uint32_t caps, const char *function_name) { diff --git a/dc11.cpp b/dc11.cpp index 087dbe0..a088082 100644 --- a/dc11.cpp +++ b/dc11.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #elif defined(_WIN32) #include #include @@ -26,6 +27,8 @@ #include "utils.h" +#define ESP32_UART UART_NUM_1 + const char *const dc11_register_names[] { "RCSR", "RBUF", "TSCR", "TBUF" }; bool setup_telnet_session(const int fd) @@ -89,10 +92,7 @@ dc11::~dc11() delete [] pfds; #if defined(ESP32) - if (serial_th) { - serial_th->join(); - delete serial_th; - } + // won't work due to freertos thread #endif } @@ -241,18 +241,47 @@ void dc11::operator()() } #if defined(ESP32) -void dc11::set_serial(Stream *const s) +void dc11_thread_wrapper_serial_handler(void *const c) { - if (serial_th) { + dc11 *const d = reinterpret_cast(c); + + d->serial_handler(); + + vTaskSuspend(nullptr); +} + +void dc11::set_serial(const int bitrate, const int rx, const int tx) +{ + if (serial_thread_running) { DOLOG(info, true, "DC11: serial port already configured"); return; } - std::unique_lock lck(s_lock); - this->s = s; - s->write("Press enter to connect"); + serial_thread_running = true; - serial_th = new std::thread(&dc11::serial_handler, this); + // Configure UART parameters + static uart_config_t uart_config = { + .baud_rate = bitrate, + .data_bits = UART_DATA_8_BITS, + .parity = UART_PARITY_DISABLE, + .stop_bits = UART_STOP_BITS_1, + .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, + .rx_flow_ctrl_thresh = 122, + }; + ESP_ERROR_CHECK(uart_param_config(ESP32_UART, &uart_config)); + + ESP_ERROR_CHECK(uart_set_pin(ESP32_UART, tx, rx, UART_PIN_NO_CHANGE, UART_PIN_NO_CHANGE)); + + // Setup UART buffered IO with event queue + const int uart_buffer_size = 1024 * 2; + static QueueHandle_t uart_queue; + // Install UART driver using an event queue here + ESP_ERROR_CHECK(uart_driver_install(ESP32_UART, uart_buffer_size, uart_buffer_size, 10, &uart_queue, 0)); + + const char msg[] = "Press enter to connect\r\n"; + uart_write_bytes(ESP32_UART, msg, sizeof(msg) - 1); + + xTaskCreate(&dc11_thread_wrapper_serial_handler, "dc11_tty", 3072, this, 1, nullptr); } void dc11::serial_handler() @@ -260,15 +289,14 @@ void dc11::serial_handler() while(!stop_flag) { yield(); + size_t n_available = 0; + ESP_ERROR_CHECK(uart_get_buffered_data_len(ESP32_UART, &n_available)); + if (n_available == 0) + continue; + char c = 0; - - { - std::unique_lock lck(s_lock); - if (s->available() == 0) - continue; - - c = s->read(); - } + if (uart_read_bytes(ESP32_UART, &c, 1, 100) == 0) + continue; // 3 is reserved for a serial port constexpr const int serial_line = 3; @@ -416,11 +444,8 @@ void dc11::write_word(const uint16_t addr, const uint16_t v) #if defined(ESP32) if (line_nr == 3) { - if (s != nullptr) { - std::unique_lock lck(s_lock); - - s->write(c); - } + if (serial_thread_running) + uart_write_bytes(ESP32_UART, &c, 1); if (is_tx_interrupt_enabled(line_nr)) trigger_interrupt(line_nr, true); diff --git a/dc11.h b/dc11.h index 4801878..c9cc3be 100644 --- a/dc11.h +++ b/dc11.h @@ -49,9 +49,7 @@ private: std::vector recv_buffers[dc11_n_lines]; std::mutex input_lock[dc11_n_lines]; #if defined(ESP32) - std::mutex s_lock; - Stream *s { nullptr }; - std::thread *serial_th { nullptr }; + std::atomic_bool serial_thread_running { false }; bool serial_enabled { false }; #endif @@ -70,7 +68,7 @@ public: void reset(); #if defined(ESP32) - void set_serial(Stream *const s); + void set_serial(const int bitrate, const int rx, const int tx); void serial_handler(); #endif diff --git a/debugger.cpp b/debugger.cpp index d751945..90992c0 100644 --- a/debugger.cpp +++ b/debugger.cpp @@ -43,8 +43,6 @@ void configure_network(console *const cnsl); void check_network(console *const cnsl); void start_network(console *const cnsl); - -void set_tty_serial_speed(console *const c, const uint32_t bps); #endif #if !defined(BUILD_FOR_RP2040) && !defined(linux) && !defined(_WIN32) @@ -891,21 +889,6 @@ void debugger(console *const cnsl, bus *const b, std::atomic_uint32_t *const sto continue; } -#if defined(CONSOLE_SERIAL_RX) - else if (parts.at(0) == "serspd") { - if (parts.size() == 2) { - uint32_t speed = std::stoi(parts.at(1), nullptr, 10); - set_tty_serial_speed(cnsl, speed); - - cnsl->put_string_lf(format("Set baudrate to %d", speed)); - } - else { - cnsl->put_string_lf("serspd requires an (decimal) parameter"); - } - - continue; - } -#endif #endif else if (cmd == "stats") { show_run_statistics(cnsl, c); @@ -1112,9 +1095,6 @@ void debugger(console *const cnsl, bus *const b, std::atomic_uint32_t *const sto "cfgnet - configure network (e.g. WiFi)", "startnet - start network", "chknet - check network status", -#if defined(CONSOLE_SERIAL_RX) - "serspd x - set serial speed in bps (8N1 are default)", -#endif #endif "cfgdisk - configure disk", "log ... - log a message to the logfile",