From 6d648092e1c4957a107411ff5fb2b1c28c3506fe Mon Sep 17 00:00:00 2001 From: Leszek Rusinowicz Date: Tue, 12 Feb 2019 17:04:19 +0100 Subject: [PATCH 1/3] Asynchronous Serial API fixes and refactoring, part 1 1. As RX and TX flows are separate on Serial device, read and write functionalities should be completely separate, including any deep sleep locking etc. 2. User may want to use asynchronous API without a callback (especially for write operations), for example in a command-response scheme end of write operation is usually meaningless. The intuitive method is to submit NULL pointer for a callback. For this reason depending on the _callback field in determining whether the operation is in progress seems to be uncertain, so introduced additional flags for this purpose. --- drivers/SerialBase.cpp | 46 +++++++++++++++++++++++++----------------- drivers/SerialBase.h | 2 ++ 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/drivers/SerialBase.cpp b/drivers/SerialBase.cpp index 6557a93e65..0be820359a 100644 --- a/drivers/SerialBase.cpp +++ b/drivers/SerialBase.cpp @@ -27,7 +27,8 @@ SerialBase::SerialBase(PinName tx, PinName rx, int baud) : #if DEVICE_SERIAL_ASYNCH _thunk_irq(this), _tx_usage(DMA_USAGE_NEVER), _rx_usage(DMA_USAGE_NEVER), _tx_callback(NULL), - _rx_callback(NULL), + _rx_callback(NULL), _tx_asynch_set(false), + _rx_asynch_set(false), #endif _serial(), _baud(baud) { @@ -218,6 +219,7 @@ int SerialBase::write(const uint16_t *buffer, int length, const event_callback_t void SerialBase::start_write(const void *buffer, int buffer_size, char buffer_width, const event_callback_t &callback, int event) { + _tx_asynch_set = true; _tx_callback = callback; _thunk_irq.callback(&SerialBase::interrupt_handler_asynch); @@ -227,22 +229,22 @@ void SerialBase::start_write(const void *buffer, int buffer_size, char buffer_wi void SerialBase::abort_write(void) { - // rx might still be active - if (_rx_callback) { + if (_tx_asynch_set) { + _tx_callback = NULL; + _tx_asynch_set = false; + serial_tx_abort_asynch(&_serial); sleep_manager_unlock_deep_sleep(); } - _tx_callback = NULL; - serial_tx_abort_asynch(&_serial); } void SerialBase::abort_read(void) { - // tx might still be active - if (_tx_callback) { + if (_rx_asynch_set) { + _rx_callback = NULL; + _rx_asynch_set = false; + serial_rx_abort_asynch(&_serial); sleep_manager_unlock_deep_sleep(); } - _rx_callback = NULL; - serial_rx_abort_asynch(&_serial); } int SerialBase::set_dma_usage_tx(DMAUsage usage) @@ -285,6 +287,7 @@ int SerialBase::read(uint16_t *buffer, int length, const event_callback_t &callb void SerialBase::start_read(void *buffer, int buffer_size, char buffer_width, const event_callback_t &callback, int event, unsigned char char_match) { + _rx_asynch_set = true; _rx_callback = callback; _thunk_irq.callback(&SerialBase::interrupt_handler_asynch); sleep_manager_lock_deep_sleep(); @@ -295,20 +298,25 @@ void SerialBase::interrupt_handler_asynch(void) { int event = serial_irq_handler_asynch(&_serial); int rx_event = event & SERIAL_EVENT_RX_MASK; - bool unlock_deepsleep = false; - if (_rx_callback && rx_event) { - unlock_deepsleep = true; - _rx_callback.call(rx_event); + if (_rx_asynch_set && rx_event) { + event_callback_t cb = _rx_callback; + _rx_asynch_set = false; + _rx_callback = NULL; + if (cb) { + cb.call(rx_event); + } + sleep_manager_unlock_deep_sleep(); } int tx_event = event & SERIAL_EVENT_TX_MASK; - if (_tx_callback && tx_event) { - unlock_deepsleep = true; - _tx_callback.call(tx_event); - } - // unlock if tx or rx events are generated - if (unlock_deepsleep) { + if (_tx_asynch_set && tx_event) { + event_callback_t cb = _tx_callback; + _tx_asynch_set = false; + _tx_callback = NULL; + if (cb) { + cb.call(tx_event); + } sleep_manager_unlock_deep_sleep(); } } diff --git a/drivers/SerialBase.h b/drivers/SerialBase.h index b00abe5bae..5c61b43e01 100644 --- a/drivers/SerialBase.h +++ b/drivers/SerialBase.h @@ -269,6 +269,8 @@ protected: DMAUsage _rx_usage; event_callback_t _tx_callback; event_callback_t _rx_callback; + bool _tx_asynch_set; + bool _rx_asynch_set; #endif serial_t _serial; From cbb84d8ad35f4c3f4af93aa32d9b9abeff814c33 Mon Sep 17 00:00:00 2001 From: Leszek Rusinowicz Date: Tue, 12 Feb 2019 17:09:48 +0100 Subject: [PATCH 2/3] Asynchronous Serial API fixes and refactoring, part 2 Aborting of asynchronous operation is necessarily hazardous, as operation can end in interrupt anywhere from the point of decision until it is actually aborted down the abort_...() method. Proper deep sleep unlocking requires then use of the critical section and should be contained completely within API implementation. --- drivers/SerialBase.cpp | 56 ++++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/drivers/SerialBase.cpp b/drivers/SerialBase.cpp index 0be820359a..856944fd34 100644 --- a/drivers/SerialBase.cpp +++ b/drivers/SerialBase.cpp @@ -201,20 +201,28 @@ void SerialBase::set_flow_control(Flow type, PinName flow1, PinName flow2) int SerialBase::write(const uint8_t *buffer, int length, const event_callback_t &callback, int event) { - if (serial_tx_active(&_serial)) { - return -1; // transaction ongoing + int result = 0; + lock(); + if (!serial_tx_active(&_serial) && !_tx_asynch_set) { + start_write((void *)buffer, length, 8, callback, event); + } else { + result = -1; // transaction ongoing } - start_write((void *)buffer, length, 8, callback, event); - return 0; + unlock(); + return result; } int SerialBase::write(const uint16_t *buffer, int length, const event_callback_t &callback, int event) { - if (serial_tx_active(&_serial)) { - return -1; // transaction ongoing + int result = 0; + lock(); + if (!serial_tx_active(&_serial) && !_tx_asynch_set) { + start_write((void *)buffer, length, 16, callback, event); + } else { + result = -1; // transaction ongoing } - start_write((void *)buffer, length, 16, callback, event); - return 0; + unlock(); + return result; } void SerialBase::start_write(const void *buffer, int buffer_size, char buffer_width, const event_callback_t &callback, int event) @@ -229,22 +237,30 @@ void SerialBase::start_write(const void *buffer, int buffer_size, char buffer_wi void SerialBase::abort_write(void) { + lock(); + core_util_critical_section_enter(); if (_tx_asynch_set) { _tx_callback = NULL; _tx_asynch_set = false; serial_tx_abort_asynch(&_serial); sleep_manager_unlock_deep_sleep(); } + core_util_critical_section_exit(); + unlock(); } void SerialBase::abort_read(void) { + lock(); + core_util_critical_section_enter(); if (_rx_asynch_set) { _rx_callback = NULL; _rx_asynch_set = false; serial_rx_abort_asynch(&_serial); sleep_manager_unlock_deep_sleep(); } + core_util_critical_section_exit(); + unlock(); } int SerialBase::set_dma_usage_tx(DMAUsage usage) @@ -267,21 +283,29 @@ int SerialBase::set_dma_usage_rx(DMAUsage usage) int SerialBase::read(uint8_t *buffer, int length, const event_callback_t &callback, int event, unsigned char char_match) { - if (serial_rx_active(&_serial)) { - return -1; // transaction ongoing + int result = 0; + lock(); + if (!serial_rx_active(&_serial) && !_rx_asynch_set) { + start_read((void *)buffer, length, 8, callback, event, char_match); + } else { + result = -1; // transaction ongoing } - start_read((void *)buffer, length, 8, callback, event, char_match); - return 0; + unlock(); + return result; } int SerialBase::read(uint16_t *buffer, int length, const event_callback_t &callback, int event, unsigned char char_match) { - if (serial_rx_active(&_serial)) { - return -1; // transaction ongoing + int result = 0; + lock(); + if (!serial_rx_active(&_serial) && !_rx_asynch_set) { + start_read((void *)buffer, length, 16, callback, event, char_match); + } else { + result = -1; // transaction ongoing } - start_read((void *)buffer, length, 16, callback, event, char_match); - return 0; + unlock(); + return result; } From 8586528ff6f7dcb412dd463a4fec9fcd4d11afe1 Mon Sep 17 00:00:00 2001 From: Leszek Rusinowicz Date: Tue, 12 Feb 2019 17:13:02 +0100 Subject: [PATCH 3/3] Asynchronous Serial API fixes and refactoring, part 3 Updated API documentation to better explain API operation and to explicitly point out that any event ends the operation. --- drivers/SerialBase.h | 52 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/drivers/SerialBase.h b/drivers/SerialBase.h index 5c61b43e01..99da7a9b45 100644 --- a/drivers/SerialBase.h +++ b/drivers/SerialBase.h @@ -179,57 +179,85 @@ public: #if DEVICE_SERIAL_ASYNCH - /** Begin asynchronous write using 8bit buffer. The completion invokes registered TX event callback + /** Begin asynchronous write using 8bit buffer. * - * This function locks the deep sleep until any event has occurred + * The write operation ends with any of the enabled events and invokes + * registered callback function (which can be NULL to not receive callback at all). + * Events that are not enabled by event argument are simply ignored. + * Operation has to be ended explicitly by calling abort_write() when + * no events are enabled. + * This function locks the deep sleep until any event has occurred. * * @param buffer The buffer where received data will be stored * @param length The buffer length in bytes * @param callback The event callback function - * @param event The logical OR of TX events + * @param event The logical OR of TX events that should end operation + * @return Zero if new transaction was started, -1 if transaction is already on-going */ int write(const uint8_t *buffer, int length, const event_callback_t &callback, int event = SERIAL_EVENT_TX_COMPLETE); - /** Begin asynchronous write using 16bit buffer. The completion invokes registered TX event callback + /** Begin asynchronous write using 16bit buffer. * - * This function locks the deep sleep until any event has occurred + * The write operation ends with any of the enabled events and invokes + * registered callback function (which can be NULL to not receive callback at all). + * Events that are not enabled by event argument are simply ignored. + * Operation has to be ended explicitly by calling abort_write() when + * no events are enabled. + * This function locks the deep sleep until any event has occurred. * * @param buffer The buffer where received data will be stored * @param length The buffer length in bytes * @param callback The event callback function - * @param event The logical OR of TX events + * @param event The logical OR of TX events that should end operation + * @return Zero if new transaction was started, -1 if transaction is already on-going */ int write(const uint16_t *buffer, int length, const event_callback_t &callback, int event = SERIAL_EVENT_TX_COMPLETE); /** Abort the on-going write transfer + * + * It is safe to call abort_write() when there is no on-going transaction. */ void abort_write(); - /** Begin asynchronous reading using 8bit buffer. The completion invokes registered RX event callback. + /** Begin asynchronous reading using 8bit buffer. * - * This function locks the deep sleep until any event has occurred + * The read operation ends with any of the enabled events and invokes registered + * callback function (which can be NULL to not receive callback at all). + * Events that are not enabled by event argument are simply ignored. + * Operation has to be ended explicitly by calling abort_read() when + * no events are enabled. + * This function locks the deep sleep until any event has occurred. * * @param buffer The buffer where received data will be stored * @param length The buffer length in bytes * @param callback The event callback function - * @param event The logical OR of RX events + * @param event The logical OR of RX events that should end operation * @param char_match The matching character + * @return Zero if new transaction was started, -1 if transaction is already on-going */ int read(uint8_t *buffer, int length, const event_callback_t &callback, int event = SERIAL_EVENT_RX_COMPLETE, unsigned char char_match = SERIAL_RESERVED_CHAR_MATCH); - /** Begin asynchronous reading using 16bit buffer. The completion invokes registered RX event callback. + /** Begin asynchronous reading using 16bit buffer. * - * This function locks the deep sleep until any event has occurred + * The read operation ends with any of the enabled events and invokes registered + * callback function (which can be NULL to not receive callback at all). + * Events that are not enabled by event argument are simply ignored. + * Operation has to be ended explicitly by calling abort_read() when + * no events are enabled. + * This function locks the deep sleep until any event has occurred. * * @param buffer The buffer where received data will be stored * @param length The buffer length in bytes * @param callback The event callback function - * @param event The logical OR of RX events + * @param event The logical OR of RX events that should end operation * @param char_match The matching character + * @return Zero if new transaction was started, -1 if transaction is already on-going */ int read(uint16_t *buffer, int length, const event_callback_t &callback, int event = SERIAL_EVENT_RX_COMPLETE, unsigned char char_match = SERIAL_RESERVED_CHAR_MATCH); /** Abort the on-going read transfer + * + * It is safe to call abort_read() when there is no on-going transaction. */ void abort_read();