UART for POSIX_ARCH #10212

aescolar · 2018-09-25T12:40:22Z

Added a new UART driver for posix arch boards. The driver can be configured to either attach to a new
pseudo-terminal, or to connect to the process stdin-out (only meant for automatic testing, not for interactive use)

Fixes #10190
Fixes #10127 by working around #10191

codecov-io · 2018-09-25T13:25:40Z

Codecov Report

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

@@           Coverage Diff           @@
##           master   #10212   +/-   ##
=======================================
  Coverage   53.06%   53.06%           
=======================================
  Files         214      214           
  Lines       26198    26198           
  Branches     5645     5645           
=======================================
  Hits        13901    13901           
  Misses      10025    10025           
  Partials     2272     2272

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 ab8d0c4...705b5e7. Read the comment docs.

aescolar · 2018-09-25T13:29:50Z

One clarification to a question you will probably have:
The default is, purposely, to attach the UART to its own pty.
The option to map it to STDIN/OUT is really not the intended use-case.

The reason for this is that the shell assumes it has access to a terminal thru a raw driver, without any of the standard terminal niceties.

That means that if we wanted to provide some reasonable setup thru stdin/out for interactive use, we would need to reconfigure the terminal into a non-sane mode (no echo, no line input, no Ctrl+X raising signals, etc..).
That can be done easily, but then we have 2 issues:

We need to be sure to leave the terminal as it was on SIGSTP (Ctrl+Z) and bring it back to funny mode in SIGCONT. Otherwise the terminal is not usable if/when people leaves the native_posix executable in the background. This is double with a bit of extra code.
We need to be sure to leave the terminal back how it was on exit. And here is the big problem. When things end gracefully enough, we can bring the terminal back. But given that native_posix is meant as a debugging/development aid, it is likely/normal that things will crash horribly (e.g. segfault). If we don't bring the terminal back, it would be left in an unusable and unrecoverable state for 99.9% of the users.

aescolar · 2018-09-25T14:10:18Z

You can for example try the shell sample app as:

cmake ../samples/subsys/shell/shell_module/  -GNinja -DBOARD=native_posix  -DCONFIG_NO_OPTIMIZATIONS=y
ninja
zephyr/zephyr.exe --attach_uart #assuming you have xterm and screen installed

If you run any of the commands which require options without an option you will trigger #10195

aescolar · 2018-09-25T14:13:27Z

recheck

ulfalizer · 2018-09-25T20:06:55Z

subsys/shell/Kconfig

depends on might be better here. selecting symbols with prompts or dependencies often leads to two common issues:

select ignores dependencies. If someone adds (unsatisfied) dependencies to SERIAL, this will still force it on (but at least you get a warning).

With select, you can't immediately tell when looking at SERIAL that there's other ways for it to get enabled, or that CONFIG_SHELL=y also enables SERIAL. That can lead to twisty and hard-to-follow Kconfig files if there's a lot of selecting going on.

People often overlook the selecting symbols when changing dependencies too, which is how you end up with the first issue.

Usually, it's best to limit select to simple invisible "helper" symbols (select CPU_SUPPORTS_FOO and the like). It's great for that.

I whitelisted the select-without-satisfied-dependencies warning when I was turning warnings into errors, because there was a lot of it going on, and a lot of it seemed to be from CONSOLE-related symbols. Should probably refactor a bunch of those to use depends on instead. :)

This sentiment also appears in kconfig-language.txt btw:

select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over.

Common sense overrides it though, but in practice, select very often leads to those issues.

dbkinder · 2018-09-25T22:10:27Z

boards/posix/native_posix/doc/board.rst

Some examples would be nice here:

This board provides a few peripherals such as an Ethernet driver and UART. See ...

dbkinder · 2018-09-25T22:11:35Z

boards/posix/native_posix/doc/board.rst

Not the new shell, eh? Will the new and legacy shells work together or are they mutually exclusive?

Not the new shell, eh?

Not with that console driver. The new shell is not using CONSOLE , but hooks directly into the UART driver. So the CONSOLE/old shell backends we had are of no use with the new shell.

Will the new and legacy shells work together or are they mutually exclusive?

I guess the question should be for the new shell developers. But my understanding is that, today:

The modules using the legacy shell are being ported to use the new shell. So the legacy shell will be purposeless quite soon (unless some old app uses it).

The new shell does not know or care about the old shell, and does not intend to avoid any conflict with it. If the old shell was driven thru UART they will certainly conflict if compiled together.

In case of native_posix, I think you may still be able to compile the old shell in parallel with the new one by using this native console driver for the old shell, and the new UART driver for the new shell.

AFAIK legacy shell will be removed completely

dbkinder · 2018-09-25T22:19:00Z

boards/posix/native_posix/doc/board.rst

:ref:`shell` will be a reference to the new shell documentation and not to the legacy shell subsystem. The shell documentation is changing with PR #10216, so your PR's references may break when 10216 is merged (or not work until it is). This should probably change to:

When the :ref:`legacy shell` subsystem is compiled ...

Yep.. I saw that PR coming after I sent mine. So I guess my options are
a. to leave the old link which is correct today, and if that PR is merged before this, correct it. If this PR is merged before, the other PR author should correct the references to the old shell page around, OR, I can send a new PR after 10216 is merged.
b. remove all links to Shell all together, and add them back in a new PR after both are merged.
c. Point to the new shell doc and stall this PR.. => don't like this one.

aescolar · 2018-09-26T10:31:45Z

@jarz-nordic: are you ok with the fixes in the shell files?

aescolar · 2018-09-26T10:46:48Z

@nashif , @jukkar , @jhedberg : Please take a quick look at the code, and tell if it there is something obviously horrible or not. This code is adding a new optional feature meant for testing, so I do not think we need an extremely thorough review. (I do not expect anybody to review the particular terminal driver flags combination...).

If any of you can think of a better reviewer for this, just add him.

jukkar

LGTM

jakub-uC · 2018-09-26T13:02:07Z

@aescolar : sorry for late review, I had a busy day.

subsys/shell/shell_fprintf.c

pfalcon · 2018-09-26T17:03:17Z

Is this supposed to fix #5941 ? If no, why? If yes, doc clauses like:

:option:CONFIG_NATIVE_UART_0_ON_OWN_PTY or
:option:CONFIG_NATIVE_UART_0_ON_STDINOUT
Note that for interactive use, the first option should be chosen. The
second option is only intended for automated testing or feeding/piping other
processes output to the UART.

Seem strange/limiting. I wanted to check myself, but hit another obstacle: #10243

pfalcon · 2018-09-26T17:17:21Z

Is this supposed to fix #5941 ?

Well, it doesn't. There's no irq mode support, only busy-polling.

aescolar · 2018-09-26T18:28:24Z

Seem strange/limiting

It just means that for interactive use, you have a dedicated TTY for the UART, instead of taking over the one from where you launch the zephyr app.

pfalcon · 2018-09-26T18:33:10Z

It just means that for interactive use, you have a dedicated TTY for the UART, instead of taking over the one from where you launch the zephyr app.

In a typical embedded situation, there's just one UART for user to connect to and do everything (provide input and get output, see logging, etc.). I'd expect the same functionality to be available on an emulated Zephyr port (at least available, if not default).

aescolar · 2018-09-26T18:39:35Z

I think you missunderstood me. When you configure the native_poxis UART with CONFIG_NATIVE_UART_0_ON_OWN_PTY (default), you get a new linux /dev/pts mapped to the Zephyr UART. If you use the --atach_uart command, you even get a new window with a terminal emulator connected to it pop automagically.

pfalcon · 2018-09-26T18:46:54Z

Right, and if CONFIG_NATIVE_UART_0_ON_STDINOUT is defined, your docs say that it "is only intended for automated testing or feeding/piping other
processes output to the UART." I don't see why it would be intended only for that. Anyway, that can be handled later.

aescolar · 2018-09-26T18:51:21Z

I don't see why it would be ...

If you try to use the shell interactively in that way, you will find yourself with a lot of annoyances, like both the Linux terminal driver and Zephyr shell echoing, the terminal capturing the Ctrl+x combinations (which the shell expects to get raw), the terminal replacing the \r with a \n, while the shell only reacts to \r as end of line, and so forth. That's why I warn people in advance that it is not going to be pleasant, and guide them to the other option which provides what I think most people will expect.

pfalcon · 2018-09-26T19:15:42Z

If you try to use the shell interactively in that way, you will find yourself with a lot of annoyances, like both the Linux terminal driver and Zephyr shell echoing

Ack, I see. As I hinted above, I quickly tried this patch with samples/subsys/console/echo/, but that failed as it required interrupt-driven UART, and I didn't look further so far. Now the situation is clear, thanks. I'd be keen to do something about it, but will look into it later.

ulfalizer

select nit fixed.

aescolar · 2018-09-27T06:54:00Z

@dbkinder : Is your review stale? or do you want to hold merging this one until the shell doc PR is fixed and merged, and this one would point to the new pages?

jukkar · 2018-09-27T06:59:50Z

I would like to see this merged asap, the net-shell conversion at #9825 is currently blocked and needs this fix.

The posix arch does not compile either of Zephyr's libc, so _prf() is not avaliable Signed-off-by: Alberto Escolar Piedras <[email protected]>

The shell subsystem, as it is today, depends on having a UART, therefore let's add the dependency explicitly in its Kconfig Fixes zephyrproject-rtos#10190 Signed-off-by: Alberto Escolar Piedras <[email protected]>

Added a new UART driver for posix arch boards. The driver can be configured to either attach to a new pseudo-terminal, or to connect to the invoking shell stdin-out. Signed-off-by: Alberto Escolar Piedras <[email protected]>

When running a UART test in CI, it is better to configure the nativeposix UART to be mapped to the process STDIN/OUT. Signed-off-by: Alberto Escolar Piedras <[email protected]>

aescolar · 2018-09-27T07:05:42Z

This last push is just a commit message fix to address the neither->either typo. No other change.

Vudentz · 2018-09-27T11:55:59Z

I hope we are getting this merging soon, it is also blocking #10239

dbkinder

I'll get out of the way for this PR and we'll deal with the doc changes after the new shell code and docs are merged.

aescolar · 2018-09-27T15:02:20Z

@nashif / @carlescufi : If either of you does not have anything against, feel free to press the button.

aescolar added area: UART Universal Asynchronous Receiver-Transmitter area: native port Host native arch port (native_sim) labels Sep 25, 2018

aescolar requested review from carlescufi, jakub-uC, jhedberg, jukkar and nashif September 25, 2018 12:40

aescolar requested a review from dbkinder as a code owner September 25, 2018 12:40

aescolar changed the title ~~UART for POSIX_ARCH~~ [DNM] UART for POSIX_ARCH Sep 25, 2018

aescolar force-pushed the native_uart branch from 252b3fb to 7739892 Compare September 25, 2018 12:53

aescolar requested a review from ulfalizer September 25, 2018 12:54

aescolar changed the title ~~[DNM] UART for POSIX_ARCH~~ UART for POSIX_ARCH Sep 25, 2018

aescolar force-pushed the native_uart branch from 7739892 to eebd94c Compare September 25, 2018 13:38

ulfalizer reviewed Sep 25, 2018

View reviewed changes

dbkinder suggested changes Sep 25, 2018

View reviewed changes

aescolar force-pushed the native_uart branch 3 times, most recently from 0b42286 to dd4364f Compare September 26, 2018 10:23

aescolar force-pushed the native_uart branch from dd4364f to 918214a Compare September 26, 2018 10:44

jukkar approved these changes Sep 26, 2018

View reviewed changes

aescolar force-pushed the native_uart branch 2 times, most recently from 2ad14ce to 5714624 Compare September 26, 2018 11:38

jakub-uC approved these changes Sep 26, 2018

View reviewed changes

pfalcon self-requested a review September 26, 2018 16:08

pfalcon reviewed Sep 26, 2018

View reviewed changes

subsys/shell/shell_fprintf.c Outdated Show resolved Hide resolved

ulfalizer approved these changes Sep 26, 2018

View reviewed changes

aescolar force-pushed the native_uart branch from 5714624 to 705b5e7 Compare September 27, 2018 07:05

aescolar added 4 commits September 27, 2018 09:05

subsys: shell: support posix arch

0711131

The posix arch does not compile either of Zephyr's libc, so _prf() is not avaliable Signed-off-by: Alberto Escolar Piedras <[email protected]>

subsys: shell: Add missing dependency to SERIAL

86e4c50

The shell subsystem, as it is today, depends on having a UART, therefore let's add the dependency explicitly in its Kconfig Fixes zephyrproject-rtos#10190 Signed-off-by: Alberto Escolar Piedras <[email protected]>

uart: Add driver for posix arch

701f104

Added a new UART driver for posix arch boards. The driver can be configured to either attach to a new pseudo-terminal, or to connect to the invoking shell stdin-out. Signed-off-by: Alberto Escolar Piedras <[email protected]>

tests: uart: add extra config for native_posix

705b5e7

When running a UART test in CI, it is better to configure the nativeposix UART to be mapped to the process STDIN/OUT. Signed-off-by: Alberto Escolar Piedras <[email protected]>

aescolar mentioned this pull request Sep 27, 2018

native_sim/posix missing features #6044

Open

dbkinder approved these changes Sep 27, 2018

View reviewed changes

carlescufi approved these changes Sep 27, 2018

View reviewed changes

carlescufi merged commit bded528 into zephyrproject-rtos:master Sep 27, 2018

aescolar deleted the native_uart branch September 27, 2018 15:59

jukkar mentioned this pull request Sep 28, 2018

Cannot use new shell with native_posix #10127

Closed

ulfalizer mentioned this pull request Oct 4, 2018

shell: Enable backends via Kconfig file #10379

Merged

UART for POSIX_ARCH #10212

UART for POSIX_ARCH #10212

Uh oh!

Conversation

aescolar commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

aescolar commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aescolar commented Sep 25, 2018

Uh oh!

aescolar commented Sep 25, 2018

Uh oh!

ulfalizer Sep 25, 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!

ulfalizer Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

aescolar Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

dbkinder Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

dbkinder Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

aescolar Sep 26, 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!

jakub-uC Sep 27, 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!

dbkinder Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

aescolar Sep 26, 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!

aescolar commented Sep 26, 2018

Uh oh!

aescolar commented Sep 26, 2018

Uh oh!

jukkar left a comment

Choose a reason for hiding this comment

Uh oh!

jakub-uC commented Sep 26, 2018

Uh oh!

Uh oh!

pfalcon commented Sep 26, 2018

Uh oh!

pfalcon commented Sep 26, 2018

Uh oh!

aescolar commented Sep 26, 2018

Uh oh!

pfalcon commented Sep 26, 2018

Uh oh!

aescolar commented Sep 26, 2018

Uh oh!

pfalcon commented Sep 26, 2018

Uh oh!

aescolar commented Sep 26, 2018

Uh oh!

pfalcon commented Sep 26, 2018

Uh oh!

ulfalizer left a comment

Choose a reason for hiding this comment

Uh oh!

aescolar commented Sep 27, 2018

Uh oh!

aescolar commented Sep 25, 2018 •

edited

Loading

codecov-io commented Sep 25, 2018 •

edited

Loading

aescolar commented Sep 25, 2018 •

edited

Loading

ulfalizer Sep 25, 2018 •

edited

Loading

aescolar Sep 26, 2018 •

edited

Loading

jakub-uC Sep 27, 2018 •

edited

Loading

aescolar Sep 26, 2018 •

edited

Loading

aescolar commented Sep 27, 2018 •

edited

Loading