laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35171?usp=email )
Change subject: soft_uart: check Rx/Tx state once in osmo_soft_uart_{rx,tx}_ubits() ......................................................................
soft_uart: check Rx/Tx state once in osmo_soft_uart_{rx,tx}_ubits()
Check it once rather than doing this in a loop. Return -EAGAIN if Rx or Tx is not enabled when calling osmo_soft_uart_{rx,tx}_ubits().
This [theoretically] improves performance by reducing the number of conditional statements in loops. In the Tx path, this also prevents calling the .tx_cb() when the transmitter is disabled, so that we don't loose the application data.
Change-Id: I70f93b3655eb21c2323e451052c40cd305c016c8 Related: OS#4396 --- M src/core/soft_uart.c M tests/soft_uart/soft_uart_test.c 2 files changed, 36 insertions(+), 8 deletions(-)
Approvals: osmith: Looks good to me, but someone else must approve laforge: Looks good to me, approved neels: Looks good to me, but someone else must approve pespin: Looks good to me, but someone else must approve Jenkins Builder: Verified
diff --git a/src/core/soft_uart.c b/src/core/soft_uart.c index 8d9142b..8cf45e1 100644 --- a/src/core/soft_uart.c +++ b/src/core/soft_uart.c @@ -111,9 +111,6 @@ /* receive a single bit */ static inline void osmo_uart_rx_bit(struct osmo_soft_uart *suart, const ubit_t bit) { - if (!suart->rx.running) - return; - switch (suart->rx.flow_state) { case SUART_FLOW_ST_IDLE: if (bit == 0) { /* start bit condition */ @@ -196,9 +193,12 @@ * \param[in] suart soft-UART instance to feed bits into. * \param[in] ubits pointer to the unpacked bits. * \param[in] n_ubits number of unpacked bits to be fed. - * \returns 0 on success; negative on error. */ + * \returns 0 on success; negative on error. + * -EAGAIN indicates that the receiver is disabled. */ int osmo_soft_uart_rx_ubits(struct osmo_soft_uart *suart, const ubit_t *ubits, size_t n_ubits) { + if (!suart->rx.running) + return -EAGAIN; for (size_t i = 0; i < n_ubits; i++) osmo_uart_rx_bit(suart, ubits[i]); return 0; @@ -213,9 +213,6 @@ { ubit_t tx_bit = 1;
- if (!suart->tx.running) - return tx_bit; - switch (suart->tx.flow_state) { case SUART_FLOW_ST_IDLE: if (msg && msgb_length(msg) > 0) { /* if we have pending data */ @@ -280,7 +277,8 @@ * \param[in] suart soft-UART instance to pull the bits from. * \param[out] ubits pointer to a buffer where to store pulled bits. * \param[in] n_ubits number of unpacked bits to be pulled. - * \returns 0 on success; negative on error. */ + * \returns 0 on success; negative on error. + * -EAGAIN indicates that the transmitter is disabled. */ int osmo_soft_uart_tx_ubits(struct osmo_soft_uart *suart, ubit_t *ubits, size_t n_ubits) { const struct osmo_soft_uart_cfg *cfg = &suart->cfg; @@ -290,6 +288,9 @@ if (OSMO_UNLIKELY(n_ubits == 0)) return -EINVAL;
+ if (!suart->tx.running) + return -EAGAIN; + /* calculate UART frame size for the effective config */ n_frame_bits = 1 + cfg->num_data_bits + cfg->num_stop_bits; if (cfg->parity_mode != OSMO_SUART_PARITY_NONE) diff --git a/tests/soft_uart/soft_uart_test.c b/tests/soft_uart/soft_uart_test.c index 522739a..276ffe1 100644 --- a/tests/soft_uart/soft_uart_test.c +++ b/tests/soft_uart/soft_uart_test.c @@ -16,6 +16,7 @@ * */
+#include <errno.h> #include <stdio.h> #include <stdint.h>
@@ -256,10 +257,18 @@ { struct osmo_soft_uart_cfg cfg; struct osmo_soft_uart *suart; + int rc;
suart = osmo_soft_uart_alloc(NULL, __func__, &suart_test_default_cfg); OSMO_ASSERT(suart != NULL);
+ /* expect -EAGAIN when the transmitter is not enabled */ + rc = osmo_soft_uart_tx_ubits(suart, NULL, 42); + OSMO_ASSERT(rc == -EAGAIN); + /* expect -EAGAIN when the receiver is not enabled */ + rc = osmo_soft_uart_rx_ubits(suart, NULL, 42); + OSMO_ASSERT(rc == -EAGAIN); + osmo_soft_uart_set_tx(suart, true); osmo_soft_uart_set_rx(suart, true);