shell: shell_uart: add ring_buffers and interrupt support #10923

nordic-krch · 2018-10-30T07:23:13Z

Improved RX path to use ring buffer for incoming data instead of single
byte buffer. Improved TX path to use ring buffer. Added support for
asynchronous UART API (interrupts).

Signed-off-by: Krzysztof Chruscinski [email protected]

nordic-krch · 2018-10-30T07:26:36Z

note that #10924 are needed to make uart with interrupts work.

codecov-io · 2018-10-30T07:53:28Z

Codecov Report

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

@@           Coverage Diff           @@
##           master   #10923   +/-   ##
=======================================
  Coverage    53.1%    53.1%           
=======================================
  Files         218      218           
  Lines       26853    26853           
  Branches     5950     5950           
=======================================
  Hits        14259    14259           
  Misses      10162    10162           
  Partials     2432     2432

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 7a18efe...9d72806. Read the comment docs.

nordic-krch · 2018-10-30T08:12:48Z

It is rebased on #10924, added DNM label which i will remove one #10924 is merged.

nordic-krch · 2018-10-30T13:23:36Z

sample net/echo_server with netusb with log and shell enable is near its ROM limits (~99% of ROM used) on quark_se_c1000 so any increase is causing compilation failure. I already hit that in #10794 and now here. Here i solved it by disabling built-in commands in shell to save some bytes (~1k). On this branch it is now at 98,05% but this approach will not fly for long. @jukkar what would you suggest for long term?

jukkar · 2018-10-30T14:51:41Z

One easy way to lower the memory consumption is to lower network buffer count, but there is also a limit there after which the device is not very usable from networking point of view. What this limit is depends lot of use cases.
@finikorg has been tweaking the netusb stuff lately with this board, do you have any suggestions for lowering memory?

pfalcon · 2018-10-30T15:23:08Z

subsys/shell/shell.c

Commit message:

Shell log backend was enabled to early

"too early"

pfalcon · 2018-10-30T15:30:46Z

subsys/shell/Kconfig.backends

That looks too low for me. I still didn't try to do the usual smoke test of pasting a decent length of line into the new shell - afraid of unpleasant surprises ;-). But with such a receive buffer size surprises are guaranteed. I'd suggest to set to 64 or something. On the contrary, tx buffer can decreased (if really needed.

yes, if you paste a line it will loose bytes. i will increase to 64. On the other hand if user is only typing then 8 is more then enough and 56 bytes of ram on some platforms means another feature.

On the other hand if user is only typing then 8 is more then enough and 56 bytes of ram on some platforms means another feature.

Well, we need to think about UX and common usecases. But granted, that can be seen as requiring separate consideration (and testing), so can be done later.

pfalcon · 2018-10-30T15:36:53Z

So, as usual, this just duplicates effort of subsys/console, instead of reusing and improving it. Well, let there be parallel pathways, until they intersect. I'll be happy to give +1 when same is done to my piece, #10765 ;-).

And please specify which boards were tested with this patch - we can expect it's not working with some (or more). (That's also the reason why I don't want to make too much changes at once in #10765 - currently refactor leaves alone (mostly) known-good ISR handling).

jakub-uC · 2018-10-30T19:47:02Z

samples/net/echo_server/overlay-netusb.conf

Isn't it enough to switch off only resize command?

jakub-uC · 2018-10-30T19:48:45Z

subsys/shell/shell.c

what happened with todo?

todo was outdated and should have been removed when k_poll was added

jakub-uC · 2018-10-30T19:53:33Z

subsys/shell/shell.c

I do not understand this conversion. log_level is a pointer, you are converting it to bool and passing to function expecting u32_t ?

fixed. I also refactored a bit to cast early those void * to local variables.

jakub-uC · 2018-10-30T19:59:42Z

subsys/shell/shell_uart.c

We shall use __ASSERT_NO_MSG

Don't you get any compiler warning about unused variable err when assert is off?

jakub-uC

Please clarify some questions.

nordic-krch · 2018-10-31T07:30:23Z

@pfalcon

So, as usual, this just duplicates effort of subsys/console, instead of reusing and improving it. Well, let there be parallel pathways, until they intersect. I'll be happy to give +1 when same is done to my piece, #10765 ;-).

Yes, it was started weeks before #10765 was public. I expect they will intersect at some point. I accept you offer: you got +1 for #10765, i'm waiting for mine :).

And please specify which boards were tested with this patch - we can expect it's not working with some (or more). (That's also the reason why I don't want to make too much changes at once in #10765 - currently refactor leaves alone (mostly) known-good ISR handling).

Tested on nrf52. ISR UART backend is use by default if platform supports uart with interrupts. Will see how many uarts implements interrupts correctly.

finikorg · 2018-10-31T08:11:33Z

One easy way to lower the memory consumption is to lower network buffer count, but there is also a limit there after which the device is not very usable from networking point of view. What this limit is depends lot of use cases.
@finikorg has been tweaking the netusb stuff lately with this board, do you have any suggestions for lowering memory?

I am afraid I took too rough approach disabling IPv6 ;)

pfalcon

Yes, it was started weeks before #10765 was public. I expect they will intersect at some point.

Yep, let's look at the bright side of it: because we follow different by close footpaths it'll allow to find the best route to place a highway, likewise, exploring different by close code will allow to explore problem space in more detail and know problem spots/find ways to address them quicker.

UART handling looks OK, approving from that side.

pfalcon · 2018-10-31T09:17:40Z

@nordic-krch:

And please specify which boards were tested with this patch

Tested on nrf52. ISR UART backend is use by default if platform supports uart with interrupts.

For reference, subsys/console when was written was tested at least on arduino_101 (that's a "different" beast), frdm_k64f, 96b_carbon (these are about the same). Testing included being able to paste more than the rx buffer. That was one of the prompting point and requirement for subsys/console - it was developed for MicroPython porting, and it has "paste mode", which allows you to copy and paste code from a host file, manual, etc. That's the most practical way small-scale repeated interaction, and before subsys/console, nothing in Zephyr allowed pastes to work reliably.

Will see how many uarts implements interrupts correctly.

We now have report (#10726) that subsys/console glitches on a particular STM32 chip. And as subsys/console is just a generic produces/consumer queues + ISR, then the problem would be either in UART driver, or that this particular chip has IRQ process not fitting into the model (and I spent enough time to chart what would be the appropriate IRQ handling model for UART drivers, again, it work for as diverse cases as 16550 (arduino_101) and common single-byte RX/TX regs as a baselines for ARM chips). My current mental solution for that is console/subsys (to be upgraded to ring_buffer, like here) is a reference, baseline implementation. It should move to drivers/serial, and a particular driver can either use it, or can override completely. All that are just draft thoughts, should be considered for interaction with #10820, etc.

nordic-krch · 2018-11-02T10:36:16Z

@jarz-nordic please take another look, i've added blocking support to the backend (to handle panic case). Also moved rx polling period to kconfig.

jakub-uC · 2018-11-02T10:41:28Z

subsys/shell/Kconfig.backends

Maybe rename it to SHELL_BACKEND_SERIAL_USE_INTERRUPTS

Improved RX path to use ring buffer for incoming data instead of single byte buffer. Improved TX path to use ring buffer. Added support for asynchronous UART API (interrupts). Signed-off-by: Krzysztof Chruscinski <[email protected]>

pfalcon · 2018-11-08T15:09:10Z

@nordic-krch: So, how well did you test this patch with various qemu targets? Are you aware of #8869 for example? In summary, interrupt-driven UART is great boon for real hardware, and curse for QEMU emulations, which don't have faithful IRQ emulation, sometimes even remotely.

All in all, I see obvious shell behavior regressions with this patch for BOARD=mps2_an385 in qemu (and that's about to become the default qemu target for arm).

jakub-uC · 2018-11-08T15:18:30Z

@pfalcon : Old shell was also interrupt driven for Rx.

pfalcon · 2018-11-08T15:24:04Z

@pfalcon : Old shell was also interrupt driven for Rx.

Old shell as in "old shell", or as in "new shell before this PR"? The old shell had a pretty fun design, which didn't work well even on real hardware (e.g. for pastes). Here we discuss the problem with QEMU and this particular PR.

pfalcon · 2018-11-08T15:26:13Z

subsys/shell/Kconfig.backends

+	bool
+	default y
+	depends on SERIAL_SUPPORT_INTERRUPT
+	imply UART_INTERRUPT_DRIVEN


Oh god, "imply"! That's first time I see it in Kconfig, I swear. I've got so much to learn...

It is nice difference. With imply you are able to unselect an option with menuconfig.

jakub-uC · 2018-11-08T15:57:39Z

@pfalcon : old shell as an old shell. So this is not a regression but something we need to improve :)

pfalcon · 2018-11-08T17:02:29Z

@jarz-nordic :

old shell as an old shell. So this is not a regression but something we need to improve :)

Yeah, we mostly left old shell behind (#10769 is again mostly to prove the point that if we replace "old" with "new", we should port over the functionality by default, or someone will ask why we didn't, and we'll need to have an answer, preferably good).

So, this is not regression re: old shell. This is qemu-specific regression before and after this patch. You're right that we need to deal with it. Let's move discussion to a more suitable place, I'll see if we can repurpose some existing ticket or it's better to create new.

nordic-krch · 2018-11-09T06:41:05Z

@pfalcon

So, how well did you test this patch with various qemu targets?

Well, i think that we have uart.h api that i should be able to rely on. If specific driver implements interrupt version then it's not my fault if it is faulty :). If qemu interrupt driven uart is lousy then it should be improved or disabled or we should have big banner somewhere do not trust drivers, test all before any change

pfalcon · 2018-11-09T07:47:30Z

@nordic-krch, @jarz-nordic: Let's move discussion to #8869 please.

nordic-krch requested a review from jakub-uC as a code owner October 30, 2018 07:23

nordic-krch force-pushed the shell_uart_irq branch from 4c71928 to 532f966 Compare October 30, 2018 08:10

nordic-krch added the DNM This PR should not be merged (Do Not Merge) label Oct 30, 2018

nordic-krch changed the title ~~shell: shell_uart: add ring_buffers and interrupt support~~ [DNM] shell: shell_uart: add ring_buffers and interrupt support Oct 30, 2018

nordic-krch force-pushed the shell_uart_irq branch 2 times, most recently from 6d289c6 to a169deb Compare October 30, 2018 13:17

nordic-krch requested review from jukkar, pfalcon and tbursztyka as code owners October 30, 2018 13:17

pfalcon reviewed Oct 30, 2018

View reviewed changes

subsys/shell/shell.c Outdated

Copy link

Contributor

pfalcon Oct 30, 2018

Choose a reason for hiding this comment

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

Commit message:

Shell log backend was enabled to early

"too early"

pfalcon reviewed Oct 30, 2018

View reviewed changes

jakub-uC reviewed Oct 30, 2018

View reviewed changes

samples/net/echo_server/overlay-netusb.conf Outdated

Copy link

Contributor

jakub-uC Oct 30, 2018

Choose a reason for hiding this comment

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

Isn't it enough to switch off only resize command?

jakub-uC reviewed Oct 30, 2018

View reviewed changes

jakub-uC requested changes Oct 30, 2018

View reviewed changes

pfalcon approved these changes Oct 31, 2018

View reviewed changes

nordic-krch force-pushed the shell_uart_irq branch from a169deb to 4f04b31 Compare November 2, 2018 07:57

jakub-uC mentioned this pull request Nov 2, 2018

Shell: trying to browse history freezes shell on disco_l475_iot1 #10413

Closed

jakub-uC approved these changes Nov 2, 2018

View reviewed changes

jakub-uC added the Enhancement Changes/Updates/Additions to existing features label Nov 2, 2018

jakub-uC added the area: Shell Shell subsystem label Nov 2, 2018

nordic-krch force-pushed the shell_uart_irq branch 2 times, most recently from c4e050a to 94d5850 Compare November 2, 2018 10:35

jakub-uC reviewed Nov 2, 2018

View reviewed changes

subsys/shell/Kconfig.backends Outdated

Copy link

Contributor

jakub-uC Nov 2, 2018

Choose a reason for hiding this comment

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

Maybe rename it to SHELL_BACKEND_SERIAL_USE_INTERRUPTS

jakub-uC approved these changes Nov 2, 2018

View reviewed changes

nordic-krch force-pushed the shell_uart_irq branch 2 times, most recently from 0dff87b to 5078243 Compare November 5, 2018 08:31

nordic-krch force-pushed the shell_uart_irq branch from 5078243 to 9d72806 Compare November 5, 2018 12:51

nordic-krch changed the title ~~[DNM] shell: shell_uart: add ring_buffers and interrupt support~~ shell: shell_uart: add ring_buffers and interrupt support Nov 5, 2018

nordic-krch removed the DNM This PR should not be merged (Do Not Merge) label Nov 5, 2018

nashif approved these changes Nov 6, 2018

View reviewed changes

nashif merged commit 4962618 into zephyrproject-rtos:master Nov 6, 2018

pfalcon reviewed Nov 8, 2018

View reviewed changes

pfalcon mentioned this pull request Nov 8, 2018

uart: Problems with interrupt-driven UART in QEMU and some hw boards #8869

Closed

shell: shell_uart: add ring_buffers and interrupt support #10923

shell: shell_uart: add ring_buffers and interrupt support #10923

Uh oh!

Conversation

nordic-krch commented Oct 30, 2018

Uh oh!

nordic-krch commented Oct 30, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

nordic-krch commented Oct 30, 2018

Uh oh!

nordic-krch commented Oct 30, 2018

Uh oh!

jukkar commented Oct 30, 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!

pfalcon commented Oct 30, 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!

jakub-uC left a comment

Choose a reason for hiding this comment

Uh oh!

nordic-krch commented Oct 31, 2018

Uh oh!

finikorg commented Oct 31, 2018

Uh oh!

pfalcon left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

nordic-krch commented Nov 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Nov 8, 2018

Uh oh!

jakub-uC commented Nov 8, 2018

Uh oh!

pfalcon commented Nov 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakub-uC Nov 8, 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 commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfalcon commented Nov 8, 2018

Uh oh!

nordic-krch commented Nov 9, 2018

Uh oh!

pfalcon commented Nov 9, 2018

Uh oh!

Reviewers

Assignees

codecov-io commented Oct 30, 2018 •

edited

Loading

pfalcon commented Oct 31, 2018 •

edited

Loading

jakub-uC Nov 8, 2018 •

edited

Loading

jakub-uC commented Nov 8, 2018 •

edited

Loading