Skip to content

subsys: console/tty: Refactor to introduce POSIX-like read/write API #10765

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 4, 2018
Merged

subsys: console/tty: Refactor to introduce POSIX-like read/write API #10765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9316d91
subsys: console: tty: Allow to specify receive timeout
pfalcon Oct 22, 2018
224f1fe
subsys: console: tty: Allow to specify transmit timeout
pfalcon Oct 23, 2018
7433a40
subsys: console: Switch to POSIX-like read/write interface
pfalcon Oct 23, 2018
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
86 changes: 76 additions & 10 deletions include/console.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#ifndef ZEPHYR_INCLUDE_CONSOLE_H_
#define ZEPHYR_INCLUDE_CONSOLE_H_

#include <sys/types.h>
#include <zephyr/types.h>
#include <kernel.h>

Expand All @@ -21,10 +22,13 @@ struct tty_serial {
u8_t *rx_ringbuf;
u32_t rx_ringbuf_sz;
u16_t rx_get, rx_put;
s32_t rx_timeout;

struct k_sem tx_sem;
u8_t *tx_ringbuf;
u32_t tx_ringbuf_sz;
u16_t tx_get, tx_put;
s32_t tx_timeout;
};

/**
Expand All @@ -47,24 +51,60 @@ void tty_init(struct tty_serial *tty, struct device *uart_dev,
u8_t *rxbuf, u16_t rxbuf_sz,
u8_t *txbuf, u16_t txbuf_sz);

/**
* @brief Set receive timeout for tty device.
*
* Set timeout for getchar() operation. Default timeout after
* device initialization is K_FOREVER.
*
* @param tty tty device structure
* @param timeout timeout in milliseconds, or K_FOREVER, or K_NO_WAIT
*/
static inline void tty_set_rx_timeout(struct tty_serial *tty, s32_t timeout)
{
tty->rx_timeout = timeout;
}

/**
* @brief Input a character from a tty device.
* @brief Set transmit timeout for tty device.
*
* Set timeout for putchar() operation, for a case when output buffer is full.
* Default timeout after device initialization is K_FOREVER.
*
* @param tty tty device structure
* @param timeout timeout in milliseconds, or K_FOREVER, or K_NO_WAIT
*/
u8_t tty_getchar(struct tty_serial *tty);
static inline void tty_set_tx_timeout(struct tty_serial *tty, s32_t timeout)
{
tty->tx_timeout = timeout;
}

/**
* @brief Output a character from to tty device.
* @brief Read data from a tty device.
*
* @param tty tty device structure
* @param c character to output
* @return 0 if ok, <0 if error
* @param buf buffer to read data to
* @param size maximum number of bytes to read
* @return >0, number of actually read bytes (can be less than size param)
* =0, for EOF-like condition (e.g., break signalled)
* <0, in case of error (e.g. -EAGAIN if timeout expired). errno
* variable is also set.
*/
int tty_putchar(struct tty_serial *tty, u8_t c);
ssize_t tty_read(struct tty_serial *tty, void *buf, size_t size);

/** @brief Initialize console_getchar()/putchar() calls.
/**
* @brief Write data to tty device.
*
* @param tty tty device structure
* @param buf buffer containg data
* @param size maximum number of bytes to write
* @return =>0, number of actually written bytes (can be less than size param)
* <0, in case of error (e.g. -EAGAIN if timeout expired). errno
* variable is also set.
*/
ssize_t tty_write(struct tty_serial *tty, const void *buf, size_t size);

/** @brief Initialize console device.
*
* This function should be called once to initialize pull-style
* access to console via console_getchar() function and buffered
Expand All @@ -76,6 +116,31 @@ int tty_putchar(struct tty_serial *tty, u8_t c);
*/
void console_init(void);

/**
* @brief Read data from console.
*
* @param dummy ignored, present to follow read() prototype
* @param buf buffer to read data to
* @param size maximum number of bytes to read
* @return >0, number of actually read bytes (can be less than size param)
* =0, in case of EOF
* <0, in case of error (e.g. -EAGAIN if timeout expired). errno
* variable is also set.
*/
ssize_t console_read(void *dummy, void *buf, size_t size);

/**
* @brief Write data to console.
*
* @param dummy ignored, present to follow write() prototype
* @param buf buffer to write data to
* @param size maximum number of bytes to write
* @return =>0, number of actually written bytes (can be less than size param)
* <0, in case of error (e.g. -EAGAIN if timeout expired). errno
* variable is also set.
*/
ssize_t console_write(void *dummy, const void *buf, size_t size);

/** @brief Get next char from console input buffer.
*
* Return next input character from console. If no characters available,
Expand All @@ -86,16 +151,17 @@ void console_init(void);
* Zephyr callback-based console input processing, shell subsystem,
* or console_getline().
*
* @return A character read, including control characters.
* @return 0-255: a character read, including control characters.
* <0: error occured.
*/
u8_t console_getchar(void);
int console_getchar(void);

/** @brief Output a char to console (buffered).
*
* Puts a character into console output buffer. It will be sent
* to a console asynchronously, e.g. using an IRQ handler.
*
* @return -1 on output buffer overflow, otherwise 0.
* @return <0 on error, otherwise 0.
*/
int console_putchar(char c);

Expand Down
28 changes: 25 additions & 3 deletions subsys/console/getchar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,36 @@ static struct tty_serial console_serial;
static u8_t console_rxbuf[CONFIG_CONSOLE_GETCHAR_BUFSIZE];
static u8_t console_txbuf[CONFIG_CONSOLE_PUTCHAR_BUFSIZE];

ssize_t console_write(void *dummy, const void *buf, size_t size)
{
ARG_UNUSED(dummy);

return tty_write(&console_serial, buf, size);
}

ssize_t console_read(void *dummy, void *buf, size_t size)
{
ARG_UNUSED(dummy);

return tty_read(&console_serial, buf, size);
}

int console_putchar(char c)
Copy link
Contributor

@jakub-uC jakub-uC Oct 24, 2018

Choose a reason for hiding this comment

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

I suggest to update the description of a function return values in a header file.

Copy link
Contributor Author

@pfalcon pfalcon Oct 26, 2018

Choose a reason for hiding this comment

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

Done

{
return tty_putchar(&console_serial, (u8_t)c);
return tty_write(&console_serial, &c, 1);
}

u8_t console_getchar(void)
int console_getchar(void)
Copy link
Contributor

@jakub-uC jakub-uC Oct 24, 2018

Choose a reason for hiding this comment

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

I suggest to update the description of a function return values in a header file.

Copy link
Contributor Author

@pfalcon pfalcon Oct 26, 2018

Choose a reason for hiding this comment

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

Done

{
return tty_getchar(&console_serial);
u8_t c;
int res;

res = tty_read(&console_serial, &c, 1);
if (res < 0) {
return res;
}

return c;
}

void console_init(void)
Expand Down
84 changes: 80 additions & 4 deletions subsys/console/tty.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ static void tty_uart_isr(void *user_data)
if (tty->tx_get >= tty->tx_ringbuf_sz) {
tty->tx_get = 0;
}
k_sem_give(&tty->tx_sem);
}
}
}
Expand All @@ -68,10 +69,16 @@ static int tty_irq_input_hook(struct tty_serial *tty, u8_t c)
return 1;
}

int tty_putchar(struct tty_serial *tty, u8_t c)
static int tty_putchar(struct tty_serial *tty, u8_t c)
{
unsigned int key;
int tx_next;
int res;

res = k_sem_take(&tty->tx_sem, tty->tx_timeout);
Copy link
Contributor

@nordic-krch nordic-krch Oct 24, 2018

Choose a reason for hiding this comment

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

this doesn't look right. There might be room in the ring buffer but thread will get blocked until irq is handled. I would assume that this happens only when ringbuffer gets full. Irq_locking is already protecting the ring buffer.

Copy link
Contributor Author

@pfalcon pfalcon Oct 24, 2018

Choose a reason for hiding this comment

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

this doesn't look right. There might be room in the ring buffer but thread will get blocked until irq is handled.

Did you see how sema's are initialized? rx_sema starts with zero, because rx buffer starts as empty. tx_sema start as tx_buf_sz-1, because that's how much free space we have there. If user asked us to block on TX if it's not possible to process (buffer in our case) tx data, we'll do that until ISR finds some free space for us, indeed.

Copy link
Contributor

@nordic-krch nordic-krch Oct 24, 2018

Choose a reason for hiding this comment

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

ok, didn't noticed that.

Copy link
Contributor Author

@pfalcon pfalcon Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

is this buffer full case? shouldn't 0 be returned then?

Well, I don't know. We could start with saying that adding semaphore would cover that case, but there can still be a race, which we should handle. And return 0 is a special thing. For read(), it means EOF. For write, there's no such connotation, but 0 is still special. Consider for example that some write() is stuck at 0, but a caller is not prepared to handle it, but has a usual looping pattern of handling short write. Then it'll go into infinite loop.

So, I think returning error is safer for now. We can adjust that based on the actual usecase we hit. If you have one already which leans towards a particular solution, feel free to tell it. Otherwise, I described ones I had in mind to lean to this way of doing it.

if (res < 0) {
return res;
}

key = irq_lock();
tx_next = tty->tx_put + 1;
Expand All @@ -80,7 +87,7 @@ int tty_putchar(struct tty_serial *tty, u8_t c)
}
if (tx_next == tty->tx_get) {
Copy link
Contributor

@nordic-krch nordic-krch Oct 24, 2018

Choose a reason for hiding this comment

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

is this buffer full case? shouldn't 0 be returned then? btw, any plans to transform it to use ring_buffer?

Copy link
Contributor Author

@pfalcon pfalcon Oct 24, 2018

Choose a reason for hiding this comment

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

btw, any plans to transform it to use ring_buffer?

You know that yes (I mentioned that on a couple of occasions). But please my latest comment (in the main comment thread) - the main thing now is to get API right, internals can be refactored then as needed.

irq_unlock(key);
return -1;
return -ENOSPC;
}

tty->tx_ringbuf[tty->tx_put] = c;
Expand All @@ -91,12 +98,46 @@ int tty_putchar(struct tty_serial *tty, u8_t c)
return 0;
}

u8_t tty_getchar(struct tty_serial *tty)
ssize_t tty_write(struct tty_serial *tty, const void *buf, size_t size)
{
const u8_t *p = buf;
size_t out_size = 0;
int res = 0;

while (size--) {
res = tty_putchar(tty, *p++);
if (res < 0) {
/* If we didn't transmit anything, return the error. */
if (out_size == 0) {
errno = -res;
return res;
}

/*
* Otherwise, return how much we transmitted. If error
* was transient (like EAGAIN), on next call user might
* not even get it. And if it's non-transient, they'll
* get it on the next call.
*/
return out_size;
}

out_size++;
}

return out_size;
}

static int tty_getchar(struct tty_serial *tty)
{
Copy link
Contributor

@jakub-uC jakub-uC Oct 23, 2018

Choose a reason for hiding this comment

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

don't you check against NULL pointer?

Copy link
Contributor Author

@pfalcon pfalcon Oct 23, 2018

Choose a reason for hiding this comment

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

don't you check against NULL pointer?

I don't, just the same as I don't test for a pointer with value of 1, 2, 3, and unlimited (well, asymptotically 2^32 on 32-bit machine) number of other invalid pointers. That's my position when writing small systems like Zephyr. We can discuss requirements re: that separately, as it's implementation detail, not part of API (well, largely: as long as there's ability to communicate back errors - and that should be part of the API, more errors can be added later as needed).

unsigned int key;
u8_t c;
int res;

k_sem_take(&tty->rx_sem, K_FOREVER);
res = k_sem_take(&tty->rx_sem, tty->rx_timeout);
if (res < 0) {
return res;
}

key = irq_lock();
c = tty->rx_ringbuf[tty->rx_get++];
Expand All @@ -108,6 +149,37 @@ u8_t tty_getchar(struct tty_serial *tty)
return c;
}

ssize_t tty_read(struct tty_serial *tty, void *buf, size_t size)
{
u8_t *p = buf;
size_t out_size = 0;
int res = 0;

while (size--) {
res = tty_getchar(tty);
if (res < 0) {
/* If we didn't transmit anything, return the error. */
if (out_size == 0) {
errno = -res;
return res;
}

/*
* Otherwise, return how much we transmitted. If error
* was transient (like EAGAIN), on next call user might
* not even get it. And if it's non-transient, they'll
* get it on the next call.
*/
return out_size;
}

*p++ = (u8_t)res;
out_size++;
}

return out_size;
}

void tty_init(struct tty_serial *tty, struct device *uart_dev,
u8_t *rxbuf, u16_t rxbuf_sz,
u8_t *txbuf, u16_t txbuf_sz)
Expand All @@ -119,6 +191,10 @@ void tty_init(struct tty_serial *tty, struct device *uart_dev,
tty->tx_ringbuf_sz = txbuf_sz;
tty->rx_get = tty->rx_put = tty->tx_get = tty->tx_put = 0;
k_sem_init(&tty->rx_sem, 0, UINT_MAX);
k_sem_init(&tty->tx_sem, txbuf_sz - 1, UINT_MAX);

tty->rx_timeout = K_FOREVER;
tty->tx_timeout = K_FOREVER;

uart_irq_callback_user_data_set(uart_dev, tty_uart_isr, tty);
uart_irq_rx_enable(uart_dev);
Expand Down