pyterm: read space after prompt into prompt by miri64 · Pull Request #15962 · RIOT-OS/RIOT

miri64 · 2021-02-09T14:11:15Z

Contribution description

Currently, when the prompt is read in pyterm the space after it is ignored for the prompt and the output command just adds its own prompt. This leads to the next output always having a leading space, see e.g. this output from tests/shell using RIOT_TERMINAL=pyterm:

make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT2/tests/shell'
/home/mlenders/Repositories/RIOT-OS/RIOT2/dist/tools/pyterm/pyterm -p "/dev/ttyUSB1" -b "500000"
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2021-02-09 14:47:15,071 # Connect to serial port /dev/ttyUSB1
Welcome to pyterm!
Type '/exit' to exit.
bufsize
2021-02-09 14:47:19,712 # bufsize
2021-02-09 14:47:19,712 # 128
> bufsize
2021-02-09 14:47:21,535 #  bufsize
2021-02-09 14:47:21,536 # 128
>

While this isn't necessarily a problem in most cases, it becomes a problem when the prompt is expected and the output of a command is empty. In that case, the space is added to the empty output, making it " ", so the prompt output command is never triggered and the prompt is added to the next command in the log output. To demonstrate I added a command empty to tests/shell that just does nothing and deactivated the command echoing using CFLAGS=-DCONFIG_SHELL_NO_ECHO=1:

empty
> empty
empty
bufsize
2021-02-09 14:54:33,753 #  > > 128
>

This fixes that problem by also reading the assumed space (we already assume the prompt, so I don't see no harm in that) and if it is not a space to skip the reading of the next char in the next iteration of the reader loop.

Testing procedure

Using the provided empty command in tests/shell now yields the expected output:

Details

$ BOARD=iotlab-m3 RIOT_TERMINAL=pyterm CFLAGS=-DCONFIG_SHELL_NO_ECHO=1 RIOT_CI_BUILD=1 make -j --no-print-directory flash term
Building application "tests_shell" for "iotlab-m3" with MCU "stm32".

   text	   data	    bss	    dec	    hex	filename
  12388	    132	   2368	  14888	   3a28	/home/mlenders/Repositories/RIOT-OS/RIOT/tests/shell/bin/iotlab-m3/tests_shell.elf
/home/mlenders/Repositories/RIOT-OS/RIOT/dist/tools/openocd/openocd.sh flash /home/mlenders/Repositories/RIOT-OS/RIOT/tests/shell/bin/iotlab-m3/tests_shell.elf
### Flashing Target ###
Open On-Chip Debugger 0.10.0+dev-01089-g3bfe49266 (2020-02-26-14:18)
Licensed under GNU GPL v2
For bug reports, read
	http://openocd.org/doc/doxygen/bugs.html
jtag
Warn : Interface already configured, ignoring
trst_and_srst separate srst_nogate trst_push_pull srst_open_drain connect_assert_srst

Info : clock speed 1000 kHz
Info : JTAG tap: stm32f1x.cpu tap/device found: 0x3ba00477 (mfg: 0x23b (ARM Ltd.), part: 0xba00, ver: 0x3)
Info : JTAG tap: stm32f1x.bs tap/device found: 0x06414041 (mfg: 0x020 (STMicroelectronics), part: 0x6414, ver: 0x0)
Info : stm32f1x.cpu: hardware has 6 breakpoints, 4 watchpoints
Info : stm32f1x.cpu: external reset detected
Info : Listening on port 34845 for gdb connections
    TargetName         Type       Endian TapName            State       
--  ------------------ ---------- ------ ------------------ ------------
 0* stm32f1x.cpu       cortex_m   little stm32f1x.cpu       reset

Info : JTAG tap: stm32f1x.cpu tap/device found: 0x3ba00477 (mfg: 0x23b (ARM Ltd.), part: 0xba00, ver: 0x3)
Info : JTAG tap: stm32f1x.bs tap/device found: 0x06414041 (mfg: 0x020 (STMicroelectronics), part: 0x6414, ver: 0x0)
target halted due to debug-request, current mode: Thread 
xPSR: 0x01000000 pc: 0x080007d0 msp: 0x20000200
Info : device id = 0x10016414
Info : flash size = 512kbytes
auto erase enabled
wrote 14336 bytes from file /home/mlenders/Repositories/RIOT-OS/RIOT/tests/shell/bin/iotlab-m3/tests_shell.elf in 0.641664s (21.818 KiB/s)

verified 12520 bytes in 0.210252s (58.152 KiB/s)

Info : JTAG tap: stm32f1x.cpu tap/device found: 0x3ba00477 (mfg: 0x23b (ARM Ltd.), part: 0xba00, ver: 0x3)
Info : JTAG tap: stm32f1x.bs tap/device found: 0x06414041 (mfg: 0x020 (STMicroelectronics), part: 0x6414, ver: 0x0)
shutdown command invoked
Done flashing
/home/mlenders/Repositories/RIOT-OS/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyUSB1" -b "500000"  
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2021-02-09 15:08:12,883 # Connect to serial port /dev/ttyUSB1
Welcome to pyterm!
Type '/exit' to exit.
empty
> empty
> empty
> bufsize
2021-02-09 15:08:25,954 # 128

Naturally, tests/shell should still pass, when run as test

make -j -C tests/shell flash test

Issues/PRs references

Found in #15951, where a command with empty output lead to problems in the output parsing.

fjmolinas · 2021-02-09T14:46:30Z

@miri64 do you think the first commit b663ab0 could be split?

fjmolinas · 2021-02-09T14:58:54Z

While this isn't necessarily a problem in most cases,

I've definitely found this annoying, had no idea where it was coming from, just thought of it as a pyterm quirk hahaha :)

fjmolinas

Looks good, I would only split up some changes.

dist/tools/pyterm/pyterm

fjmolinas · 2021-02-09T15:06:55Z

dist/tools/pyterm/pyterm

            elif c == self.serprompt and output == "":
                sys.stdout.write('%c ' % self.serprompt)
                sys.stdout.flush()
+                # we now need to read away the space we printed


Maybe add a comment above on what is ignored for this whole section?

So the > + ' '

The > is not ignored. It is checked in the c == self.serprompt.

I updated the comment in the latest force push. Is that what you had in mind?

I don't really understand the wording now TBH

Better now?

It's a bit rambling now, but at least it should be easy to understand.

fjmolinas

ACK, please squash

fjmolinas · 2021-02-09T16:08:09Z

I set up tests to be ran as well :)

Currently, when the prompt is read in `pyterm` the space after it is ignored for the prompt and the output command just adds its own prompt. This leads to the next output always having a leading space, see e.g. this output from `tests/shell` using `RIOT_TERMINAL=pyterm`: ``` make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT2/tests/shell' /home/mlenders/Repositories/RIOT-OS/RIOT2/dist/tools/pyterm/pyterm -p "/dev/ttyUSB1" -b "500000" Twisted not available, please install it if you want to use pyterm's JSON capabilities 2021-02-09 14:47:15,071 # Connect to serial port /dev/ttyUSB1 Welcome to pyterm! Type '/exit' to exit. bufsize 2021-02-09 14:47:19,712 # bufsize 2021-02-09 14:47:19,712 # 128 > bufsize 2021-02-09 14:47:21,535 # bufsize 2021-02-09 14:47:21,536 # 128 > ``` While this isn't necessarily a problem in most cases, it becomes a problem when the prompt is expected and the output of a command is empty. In that case, the space is added to the empty output, making it " ", so the prompt output command is never triggered and the prompt is added to the next command in the log output. To demonstrate I added a command `empty` to `tests/shell` that just does nothing and deactivated the command echoing using `CFLAGS=-DCONFIG_SHELL_NO_ECHO=1`: ``` empty > empty empty bufsize 2021-02-09 14:54:33,753 # > > 128 > ``` This fixes that problem by also reading the assumed space (we already assume the prompt, so I don't see no harm in that) and if it is not a space to skip the reading of the next char in the next iteration of the reader loop.

miri64 · 2021-02-09T16:18:30Z

Squashed.

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: tools Area: Supplementary tools labels Feb 9, 2021

miri64 requested review from MrKevinWeiss and maribu February 9, 2021 14:11

miri64 requested review from MichelRottleuthner, aabadie, fjmolinas, leandrolanzieri and smlng as code owners February 9, 2021 14:11

fjmolinas reviewed Feb 9, 2021

View reviewed changes

pyterm: move char reading and reconnect to their own private functions

40da800

miri64 force-pushed the pyterm/fix/space-after-prompt branch from b72f8c0 to b11bc71 Compare February 9, 2021 15:54

fjmolinas approved these changes Feb 9, 2021

View reviewed changes

miri64 added 2 commits February 9, 2021 17:17

tests/shell: add command with empty output

bd96e4b

miri64 force-pushed the pyterm/fix/space-after-prompt branch from 533401c to bd96e4b Compare February 9, 2021 16:17

benpicco mentioned this pull request Feb 9, 2021

tests/riotboot/flashwrite: add automatic test #15559

Merged

miri64 merged commit 7bb7063 into RIOT-OS:master Feb 9, 2021

miri64 deleted the pyterm/fix/space-after-prompt branch February 9, 2021 19:34

miri64 mentioned this pull request Feb 9, 2021

congure: initial import of a congestion control framework #15951

Merged

kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021

Conversation

miri64 commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

fjmolinas commented Feb 9, 2021

Uh oh!

fjmolinas commented Feb 9, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjmolinas Feb 9, 2021 • 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!

fjmolinas Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 9, 2021

Uh oh!

miri64 commented Feb 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miri64 commented Feb 9, 2021 •

edited

Loading

fjmolinas Feb 9, 2021 •

edited

Loading