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

pfalcon · 2018-10-22T18:06:40Z

Allow to specify receive/transmit timeouts.
Switch to API based on POSIX-like read()/write() functions.

(The underlying implementation stays the same for now, subject to follow-up refactors.)

pfalcon · 2018-10-22T18:12:08Z

This is related to #10512. Please follow with me thru the transformation of initial subsys/console/ "prototype" into POSIXish subsystem, with reusable intermediate layers.

As I mentioned already, this is background work for me, so I'd prefer to do that in gradual, easy to follow/decide on steps. In return I hope for a quick review turnaround.

I've added people mentioned in #10512 as reviewers.

codecov-io · 2018-10-23T09:09:18Z

Codecov Report

Merging #10765 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #10765   +/-   ##
=======================================
  Coverage   53.32%   53.32%           
=======================================
  Files         215      215           
  Lines       25887    25887           
  Branches     5705     5705           
=======================================
  Hits        13804    13804           
  Misses       9763     9763           
  Partials     2320     2320

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74318e6...7433a40. Read the comment docs.

jakub-uC · 2018-10-23T09:37:12Z

subsys/console/tty.c

don't you check against NULL pointer?

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).

pfalcon · 2018-10-23T09:43:30Z

Ok, so my original idea was that it might be OK to use the same timeout for both RX and TX (the latter yet needs to be implemented). But right on spot is this ticket: #10726 , with a common "magic" usecase of non-blocking RX and blocking TX. So, going to make it tty_set_rx_timeout() after all.

@gon1332: FYI.

And the reason I prefer to do it step by step is that each of us was able to trace how more and more functionality is being added, and how the feature bloats up, based on good intentions (so anybody had a chance to influence it).

pfalcon · 2018-10-23T10:03:09Z

So, going to make it tty_set_rx_timeout() after all.

Done.

pfalcon · 2018-10-23T18:23:26Z

Ok, I figured that doing it piecemeal would be too long after all. So, pushing a couple more commits here, which lead this console/tty subsys into largely POSIX API, as was intended for a long time. The underlying implementation remains the same, where only single chars are processed per call. Addressing that will require separate refactor, but the point is that API is now (largely) there.

@jarz-nordic: Thanks for quick review, to be fair, let me dismiss it (and hope you'd find time for another look still).

PR changed, please re-review.

pfalcon · 2018-10-24T14:23:46Z

@jarz-nordic: Thanks for quick review, to be fair, let me dismiss it (and hope you'd find time for another look still).

Oh, and forgot to do that, did now.

nordic-krch · 2018-10-24T15:54:42Z

include/console.h

copy paste? ... follow write() prototype

nordic-krch · 2018-10-24T16:05:32Z

subsys/console/tty.c

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

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.

nordic-krch · 2018-10-24T16:15:59Z

subsys/console/tty.c

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.

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.

ok, didn't noticed that.

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.

nordic-krch · 2018-10-24T16:17:51Z

one more thing: getchar.c doesn't look right. Any chance to rename it?

pfalcon · 2018-10-24T17:08:21Z

one more thing: getchar.c doesn't look right. Any chance to rename it?

Not only that doesn't look right, the whole "console" vs "tty" being in subsys/console doesn't look right. "tty" is a moniker for "buffered uart", and should be somewhere closer to UART (but not clear where - own "micro-subsys" or straight in drivers/?). Console shouldn't be tied to just UART. As mentioned, I'd prefer to do it step by step. If we rename it now, that unlikely will be the right and final name, so better to just avoid noise associated with rename for now.

But if you don't agree and have good suggestion for a name, feel free to say it. (I don't know if you know or now, but getchar.c comes from contrasting it with getline.c lying nearby).

jakub-uC

Just minor update needed.

jakub-uC · 2018-10-24T22:33:26Z

subsys/console/getchar.c

why -1 and not res?

why -1 and not res?

Kinda wanted it to mimic behavior of ANSI C, which guarantees that return value has type of set {EOF, 0-255} (not an arbitrary negative value). But ok, let's just propagate res for now.

jakub-uC · 2018-10-24T22:45:39Z

subsys/console/getchar.c

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

jakub-uC · 2018-10-24T22:45:54Z

subsys/console/getchar.c

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

This allows to specify receive timeout, instead of previously hardcoded K_FOREVER value. K_FOREVER is still the default, and can be changes after tty initialization using tty_set_rx_timeout() call, and timeout is stored as a property of tty. (Instead of e.g. being a param of each receive call. Handling like that is required for POSIX-like behavior of tty). Signed-off-by: Paul Sokolovsky <[email protected]>

Previously, transmit was effectively non-blocking - a character either went into buffer, or -1 was returned. Now it's possible to block if buffer is full. Timeout is K_FOREVER by default, can be adjusted with tty_set_tx_timeout() (similar to receive timeout). Signed-off-by: Paul Sokolovsky <[email protected]>

Tty device gets only read/write calls, but console retains getchar/putchar for convenience. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon · 2018-10-26T16:46:15Z

Ok, as it spends more time in the queue, I went ahead and made the final change - now read() and write() calls actually can transfer buffers, not just a single char of it. So, now both API and behavior and there. What would be left is optimizing that, and finding a final place for it. But those are definitely subjects of separate work.

Also, addressed @jarz-nordic's comments. Waiting for responses from @nordic-krch .

pfalcon · 2018-10-30T18:20:22Z

@jarz-nordic, @nordic-krch : Ping.

nordic-krch · 2018-10-31T07:22:50Z

@pfalcon as console will be logger/shell backend it should be possible to reconfigure it to synchronous mode in case of panic when data is flushed. In that mode console must not use interrupts or kernel primitives. I only see use case for writing data so it may be that write_sync() would be sufficient. Another option would be to add dedicated function for re-enabling in blocking mode.

pfalcon · 2018-10-31T12:22:12Z

@nordic-krch: Thanks, captured as #10512 (comment)

nordic-krch · 2018-11-02T06:32:57Z

@pfalcon How do you see reporting, of RX buffer overflow? It is not hw error but rather application not reading data on time. In that case tty has valid data buffered but tty_read returns error?

pfalcon · 2018-11-02T08:14:00Z

@nordic-krch: Why do you post here, in this transient PR? Please repost it to #10512, so it's not lost, let's discuss it there.

nashif · 2018-11-02T14:49:58Z

@pfalcon I do not see any tests being added for all the API changes. Can you please add tests?

pfalcon · 2018-11-02T16:31:30Z

@nashif : This is effectively driver-level code (it would end up in drivers/serial/ if everything goes right), what kind of tests do you expect? Plus, this API is just shaping up, as well as actual implementation. Once those are stable enough (subsumed requirements of different parties), can think of some mock tests (because those are the only ones I can think of right away). The API is marked experimental just in case.

pfalcon · 2018-11-02T17:26:12Z

And just to clarify, we have a few samples for the "top-level" functions console_getchar(), console_puchar(), which cover all that code for buildability, and can be used for manual testing.

carlescufi

I'm happy with this. Long term we might be able to use the console as a backend for the shell and logging subsystems, and as @pfalcon says, that makes sense

pfalcon added area: UART Universal Asynchronous Receiver-Transmitter area: Console labels Oct 22, 2018

pfalcon requested review from carlescufi, jakub-uC, mike-scott and nordic-krch October 22, 2018 18:07

pfalcon force-pushed the console-set-timeout branch from 8202da0 to 6ac3107 Compare October 23, 2018 08:34

jakub-uC reviewed Oct 23, 2018

View reviewed changes

jakub-uC previously approved these changes Oct 23, 2018

View reviewed changes

pfalcon force-pushed the console-set-timeout branch from 6ac3107 to 21318fb Compare October 23, 2018 09:59

pfalcon changed the title ~~subsys: console: Allow to specify (receive) timeout~~ subsys: console: Allow to specify receive timeout Oct 23, 2018

pfalcon force-pushed the console-set-timeout branch from 21318fb to c470e3a Compare October 23, 2018 18:18

pfalcon requested a review from andrewboie October 23, 2018 18:23

pfalcon changed the title ~~subsys: console: Allow to specify receive timeout~~ subsys: console: Refactor to introduce POSIX-like read/write API Oct 23, 2018

nordic-krch reviewed Oct 24, 2018

View reviewed changes

nordic-krch requested changes Oct 24, 2018

View reviewed changes

pfalcon force-pushed the console-set-timeout branch from c470e3a to e43993b Compare October 24, 2018 17:01

pfalcon mentioned this pull request Oct 24, 2018

lib: posix: Implement generic file descriptor table #10429

Merged

jakub-uC requested changes Oct 24, 2018

View reviewed changes

pfalcon mentioned this pull request Oct 25, 2018

api: uart: Add new asynchronous UART API (Transfer sub-API) #10820

Merged

pfalcon force-pushed the console-set-timeout branch from e43993b to 53ac41e Compare October 26, 2018 16:24

subsys: console: Switch to POSIX-like read/write interface

7433a40

Tty device gets only read/write calls, but console retains getchar/putchar for convenience. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon force-pushed the console-set-timeout branch from 53ac41e to 7433a40 Compare October 26, 2018 16:37

This was referenced Oct 30, 2018

shell: shell_uart: add ring_buffers and interrupt support #10923

Merged

[RFC] logging: Default to redirecting printk() through it #10715

Closed

jakub-uC approved these changes Oct 30, 2018

View reviewed changes

nordic-krch approved these changes Oct 31, 2018

View reviewed changes

pfalcon mentioned this pull request Oct 31, 2018

Console, logger, shell architecure #10512

Closed

pfalcon changed the title ~~subsys: console: Refactor to introduce POSIX-like read/write API~~ subsys: console/tty: Refactor to introduce POSIX-like read/write API Oct 31, 2018

carlescufi approved these changes Nov 4, 2018

View reviewed changes

nashif merged commit 6203020 into zephyrproject-rtos:master Nov 4, 2018

pfalcon mentioned this pull request Nov 7, 2018

RFC: UART API Enhancements #8243

Closed

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

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

Uh oh!

Conversation

pfalcon commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfalcon commented Oct 22, 2018

Uh oh!

codecov-io commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Oct 23, 2018

Uh oh!

pfalcon commented Oct 23, 2018

Uh oh!

pfalcon commented Oct 23, 2018

Uh oh!

pfalcon commented Oct 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordic-krch commented Oct 24, 2018

Uh oh!

pfalcon commented Oct 24, 2018

Uh oh!

jakub-uC left a comment

Choose a reason for hiding this comment

Uh oh!

jakub-uC Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Oct 26, 2018

Uh oh!

pfalcon commented Oct 30, 2018

Uh oh!

nordic-krch commented Oct 31, 2018

Uh oh!

pfalcon commented Oct 31, 2018

Uh oh!

nordic-krch commented Nov 2, 2018

Uh oh!

pfalcon commented Nov 2, 2018

Uh oh!

nashif commented Nov 2, 2018

Uh oh!

pfalcon commented Nov 2, 2018

Uh oh!

pfalcon commented Oct 22, 2018 •

edited

Loading

codecov-io commented Oct 23, 2018 •

edited

Loading

pfalcon Oct 24, 2018 •

edited

Loading

jakub-uC Oct 24, 2018 •

edited

Loading