pyterm: Disable repeating command on empty line #12096

cladmi · 2019-08-27T13:19:35Z

Contribution description

Just send a empty newline when it receives a newline.
This is the behavior of all the other terminals.

This also works when applied in conjunction with #12094

Testing procedure

Run BOARD=your_board make -C tests/test_tools/ flash test with different RIOT_TERMINAL, currently supported (not on all boards) pyterm, socat, picocom. If possible one with term_rtt. But as it is internally using pyterm it will benefit from the changes automatically.

RIOT_TERMINAL=pyterm samr21-xpro

RIOT_TERMINAL=pyterm  BOARD=samr21-xpro RIOT_CI_BUILD=1 make --no-print-directory -C tests/test_tools/ flash test
Building application "tests_test_tools" for "samr21-xpro" with MCU "samd21".

   text    data     bss     dec     hex filename
   9244     140    2612   11996    2edc /home/harter/work/git/RIOT/tests/test_tools/bin/samr21-xpro/tests_test_tools.elf
/home/harter/work/git/RIOT/dist/tools/edbg/edbg  -t atmel_cm0p -b -v -p -f /home/harter/work/git/RIOT/tests/test_tools/bin/samr21-xpro/tests_test_tools.bin
Debugger: ATMEL EDBG CMSIS-DAP ATML2127031800004678 01.1A.00FB (S)
Clock frequency: 16.0 MHz
Target: SAM R21G18 (Rev C)
Programming........................................ done.
Verification........................................ done.
/home/harter/work/git/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "115200"
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2019-08-27 15:16:55,257 - INFO # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2019-08-27 15:16:58,394 - INFO # main(): This is RIOT! (Version: buildtest)
2019-08-27 15:16:58,398 - INFO # Running 'tests_tools' application
shellping
2019-08-27 15:16:58,452 - INFO # shellpong
true this should not be echoed
shellping
2019-08-27 15:16:59,557 - INFO # shellpong
getchar

2019-08-27 15:16:59,660 - INFO # getchar 0x0a

RIOT_TERMINAL=picocom samr21-xpro

RIOT_TERMINAL=picocom  BOARD=samr21-xpro RIOT_CI_BUILD=1 make --no-print-directory -C tests/test_tools/ flash test

RIOT_TERMINAL=socat samr21-xpro

RIOT_TERMINAL=picocom  BOARD=samr21-xpro RIOT_CI_BUILD=1 make --no-print-directory -C tests/test_tools/ flash test
Building application "tests_test_tools" for "samr21-xpro" with MCU "samd21".

   text    data     bss     dec     hex filename
   9244     140    2612   11996    2edc /home/harter/work/git/RIOT/tests/test_tools/bin/samr21-xpro/tests_test_tools.elf
/home/harter/work/git/RIOT/dist/tools/edbg/edbg  -t atmel_cm0p -b -v -p -f /home/harter/work/git/RIOT/tests/test_tools/bin/samr21-xpro/tests_test_tools.bin
Debugger: ATMEL EDBG CMSIS-DAP ATML2127031800004678 01.1A.00FB (S)
Clock frequency: 16.0 MHz
Target: SAM R21G18 (Rev C)
Programming........................................ done.
Verification........................................ done.
picocom --nolock --imap lfcrlf --baud "115200" "/dev/ttyACM0"
picocom v2.2

port is        : /dev/ttyACM0
flowcontrol    : none
baudrate is    : 115200
parity is      : none
databits are   : 8
stopbits are   : 1
escape is      : C-a
local echo is  : no
noinit is      : no
noreset is     : no
nolock is      : yes
send_cmd is    : sz -vv
receive_cmd is : rz -vv -E
imap is        : lfcrlf,
omap is        : 
emap is        : crcrlf,delbs,

Type [C-a] [C-h] to see available commands

Terminal ready
main(): This is RIOT! (Version: buildtest)
Running 'tests_tools' application
shellping
shellpong
true this should not be echoed
shellping
shellpong
getchar

getchar 0x0a

native

RIOT_CI_BUILD=1 BOARD=native make --no-print-directory -C tests/test_tools/ flash test
Building application "tests_test_tools" for "native" with MCU "native".

   text    data     bss     dec     hex filename
  22682     640   47652   70974   1153e /home/harter/work/git/RIOT/tests/test_tools/bin/native/tests_test_tools.elf
true
/home/harter/work/git/RIOT/tests/test_tools/bin/native/tests_test_tools.elf
RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: buildtest)
Running 'tests_tools' application
shellping
shellpong
true this should not be echoed
shellping
shellpong
getchar

getchar 0x0a

On IoT-LAB it works too

IOTLAB_NODE=auto-ssh RIOT_CI_BUILD=1 BOARD=iotlab-m3 make --no-print-directory -C tests/test_tools/ flash test
Building application "tests_test_tools" for "iotlab-m3" with MCU "stm32f1".

   text    data     bss     dec     hex filename
   8896     140    2620   11656    2d88 /home/harter/work/git/RIOT/tests/test_tools/bin/iotlab-m3/tests_test_tools.elf
iotlab-node --jmespath='keys(@)[0]' --format='int'  --list grenoble,m3,58 --update /home/harter/work/git/RIOT/tests/test_tools/bin/iotlab-m3/tests_test_tools.elf | grep 0
0
ssh -t harter@grenoble.iot-lab.info 'socat - tcp:m3-58.grenoble.iot-lab.info:20000'
�main(): This is RIOT! (Version: buildtest)
Running 'tests_tools' application
�main(): This is RIOT! (Version: buildtest)
Running 'tests_tools' application
shellping
shellpong
true this should not be echoed
shellping
shellpong
getchar

getchar 0x0a

Issues/PRs references

Different behavior between testing locally and in CI.
Also repeating the last command was always a pain when testing.

Just send a empty newline when it receives a newline. This is the behavior of all the other terminals.

Test receiving an empty line. It was before not possible with `pyterm` but is fixed by previous commit.

miri64 · 2019-08-27T13:22:35Z

I think this was intentionally implemented this way, right @OlegHahm? What is the motivation in removing this quite nice feature that allows to rapidly re-execute commands?

miri64 · 2019-08-27T13:23:46Z

What is the motivation in removing this quite nice feature that allows to rapidly re-execute commands?

The reasoning "because other terminals do so as well" seems quite a weak argument to remove a feature.

cladmi · 2019-08-27T13:26:58Z

It is the default cmd behavior, that's why it is there.
Re-executing a command can be done with Up+Enter like in any terminal as it is using GNUreadline.

cladmi · 2019-08-27T13:28:58Z

Being able to send an empty line is something I would need instead of relying on sending a useless character to workaround pyterm specific behavior.

Also, when typing enter it does not re-print the command, was quite confusing at usage.
In master with the test, you do not see the shellping being repeated.

shellping
2019-08-27 15:25:25,351 - INFO # shellpong

2019-08-27 15:25:26,047 - INFO # shellpong

2019-08-27 15:25:26,567 - INFO # shellpong

2019-08-27 15:25:26,815 - INFO # shellpong

2019-08-27 15:25:26,982 - INFO # shellpong

cladmi · 2019-08-27T13:35:17Z

native does not re-execute the command in master.

BOARD=native RIOT_CI_BUILD=1 make --no-print-directory -C tests/test_tools/ flash term
Building application "tests_test_tools" for "native" with MCU "native".

   text    data     bss     dec     hex filename
  22506     628   47652   70786   11482 /home/harter/work/git/RIOT/tests/test_tools/bin/native/tests_test_tools.elf
true 
/home/harter/work/git/RIOT/tests/test_tools/bin/native/tests_test_tools.elf  
RIOT native interrupts/signals initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: buildtest)
Running 'tests_tools' application
shellping
shellpong




^C
native: exiting

miri64 · 2019-08-27T13:41:27Z

native does not re-execute the command in master.

I know, it's quite annoying ;-P

miri64 · 2019-08-27T13:41:38Z

How about making it configurable then?

cladmi · 2019-08-27T13:49:40Z

Having worked with uboot and some network equipment that repeat the last command, it was one of the most annoying thing to interact with.
Also, nobody has this in his own local terminal.

For me it was just a side-effect of the default behavior of cmd that is not worth my time for keeping support. "up + enter" does the same.

miri64 · 2019-08-27T14:27:59Z

For me it was just a side-effect of the default behavior of cmd that is not worth my time for keeping support. "up + enter" does the same.

Hitting two keys quickly in an alternating fashion is far more far more error prone and exhausting than just hitting or even just holding one button (enter)

cladmi · 2019-08-28T09:58:37Z

To precise what happens here:

pyterm does repeat the command on enter
- However, from what I know, you cannot send an empty line.
  If there is a way that works on all our term programs, please tell me.
native does not repeat the command
ethos does not repeat the command
picocom does not repeat the command
- so this is the behavior in murdock during tests
socat does not repeat the command
IoT-LAB does not repeat the command
miniterm.py does not repeat the command
Graphical terminal emulators do not repeat the command by default
The linux TTY do not repeat the command by default

Guess who has a standard behavior ?

miri64 · 2019-08-28T11:48:24Z

Guess who has a standard behavior ?

I am not aware that there is a standard on how a terminal should behave...

If there is a way that works on all our term programs, please tell me.

This is why I suggested that, instead of killing this feature (at least I consider it as such), we make it configurable...

Add a variable for `pyterm` specific flags that are not handled by other terminals. This will prevent issues with boards that have options only supported by `pyterm` and setting `pyterm` options from the environment.

pyterm: Disable repeating command on empty line by default This can be enabled again by giving '--repeat-command-on-empty-line' and with 'PYTERMFLAGS' in the environment. PYTERMFLAGS='--repeat-command-on-empty-line' BOARD=samr21-xpro make -C examples/default/ term

cladmi · 2019-08-28T12:14:24Z

Done with an argument as it is the way pyterm does its configuration.

To allow configuring it in a build, I cherry-picked the PYTERMFLAGS that are added in #12095 and #12094

aabadie · 2019-08-28T12:20:05Z

dist/tools/pyterm/pyterm

@@ -791,6 +803,9 @@ if __name__ == "__main__":
                        "of CR and LF. Examples: -nl=LF, -nl=CRLF. "
                        "(Default is %s)" % defaultnewline,
                        default=defaultnewline)
+    parser.add_argument("--repeat-command-on-empty-line",
+                        help="Enable repeating command on empty line",
+                        action="store_true", default=False)


To keep the existing behaviour, the default should be True. IIUC, you can change this via PYTERMFLAGS in your test if you want.

It is done on purpose, as I want the behavior to be the same when you do make test, make term, and use any other RIOT_TERMINAL.

And to do the opposite, it cannot just be flipped, but the option would need to be renamed to a "do not repeat command on empty line" as it is an option without argument.

Indeed, but in this case, there's no need for default=False since it's set automatically when using action="store_true".

Anyway, as some others, I wouldn't invert this behaviour in this PR just because it's more convenient for tests. The goal of the tool is to be convenient for users and if users tend to prefer the "repeat command" on empty line, this should be the default. The test can be adapted.

It is done on purpose, as I want the behavior to be the same when you do make test, make term, and use any other RIOT_TERMINAL.

And to do the opposite, it cannot just be flipped, but the option would need to be renamed to a "do not repeat command on empty line" as it is an option without argument.

Please remember, that the testing experience is not the primary user experience, but the normal terminal is.

Yes, then having different behavior depending on the board, example (ethos), or RIOT_TERMINAL is for me bad as user experience.

Yes, then having different behavior depending on the board, example (ethos), or RIOT_TERMINAL is for me bad as user experience.

So does just silently deactivating a feature many people rely on for their local testing.

I learned something.

so did I :)

Yes, then having different behavior depending on the board, example (ethos), or RIOT_TERMINAL is for me bad as user experience.

So does just silently deactivating a feature many people rely on for their local testing.

Those are different tools BTW. Especially ethos and native could also use pyterm by wrapping around with TCP, however this would just add unnecessary overhead when doing this in the default case.

"unnecessary" agreed.

What happened to me, was more when typing a "ping" command, I would type "enter" to add some spacing, and oh, it would be repeated many times.
And it cannot be interrupted with ctrl+c.
I usually do not call it a feature when it can be done explicitly with just "up + enter".

I usually do not call it a feature when it can be done explicitly with just "up + enter".

Again, "up + enter" is very unreliable if you are doing flood testing with commands that are only doing one thing at a time...

cladmi · 2019-08-28T16:02:16Z

I will re-open on top of #12107 and put the inconsistency with other platforms in an issue.

miri64

Alternatively, we could slowly phase over to False being the default, but not in one PR and not if --repeat-command-on-empty-line remains the only option to set the flag (too long to use it on a regular basis with TERMFLAGS)

miri64 · 2019-08-28T17:16:14Z

dist/tools/pyterm/pyterm

@@ -105,7 +105,8 @@ class SerCmd(cmd.Cmd):
    def __init__(self, port=None, baudrate=None, toggle=None, tcp_serial=None,
                 confdir=None, conffile=None, host=None, run_name=None,
                 log_dir_name=None, newline=None, formatter=None,
-                 set_rts=None, set_dtr=None, serprompt=None):
+                 set_rts=None, set_dtr=None, serprompt=None,
+                 repeat_command_on_empty_line=False):


Just a reminder, that True should be the default ;-).

cladmi · 2019-08-29T13:24:11Z

I added the configuration alone in #12120
It keep the current behavior by default.

cladmi · 2019-08-29T13:37:59Z

Closed in favor of

pyterm: configuring repeating command on empty line pyterm: configuring repeating command on empty line #12120
Makefile.include: add cleanterm target and use it for tests Makefile.include: add cleanterm target and use it for tests #12107

miri64 · 2019-09-10T11:57:29Z

Thank you!

cladmi added 2 commits August 27, 2019 15:09

pyterm: Disable repeating command on empty line

75cc2c5

Just send a empty newline when it receives a newline. This is the behavior of all the other terminals.

tests/test_tools: test receiving an empty line

15f20c2

Test receiving an empty line. It was before not possible with `pyterm` but is fixed by previous commit.

miri64 self-requested a review August 27, 2019 13:22

miri64 added Area: tools Area: Supplementary tools Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Aug 27, 2019

cladmi added 2 commits August 28, 2019 14:08

makefiles/serial.inc.mk: add PYTERMFLAGS variable

77f8e81

Add a variable for `pyterm` specific flags that are not handled by other terminals. This will prevent issues with boards that have options only supported by `pyterm` and setting `pyterm` options from the environment.

aabadie reviewed Aug 28, 2019

View reviewed changes

fixup! squash! pyterm: Disable repeating command on empty line

7a45e26

miri64 requested changes Aug 28, 2019

View reviewed changes

cladmi added 2 commits August 29, 2019 15:09

squash! fixup! squash! pyterm: Disable repeating command on empty line

cd10de7

squash! pyterm: configuring repeating command on empty line

9986a9e

cladmi mentioned this pull request Aug 29, 2019

pyterm: configuring repeating command on empty line #12120

Merged

cladmi mentioned this pull request Aug 29, 2019

Makefile.include: add cleanterm target and use it for tests #12107

Merged

cladmi closed this Aug 29, 2019

pyterm: Disable repeating command on empty line #12096

pyterm: Disable repeating command on empty line #12096

Uh oh!

Conversation

cladmi commented Aug 27, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Aug 27, 2019

Uh oh!

miri64 commented Aug 27, 2019

Uh oh!

cladmi commented Aug 27, 2019

Uh oh!

cladmi commented Aug 27, 2019

Uh oh!

cladmi commented Aug 27, 2019

Uh oh!

miri64 commented Aug 27, 2019

Uh oh!

miri64 commented Aug 27, 2019

Uh oh!

cladmi commented Aug 27, 2019

Uh oh!

miri64 commented Aug 27, 2019

Uh oh!

cladmi commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 28, 2019

Uh oh!

cladmi commented Aug 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi Aug 28, 2019 • 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!

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!

cladmi commented Aug 28, 2019

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Aug 29, 2019

Uh oh!

cladmi commented Aug 29, 2019

Uh oh!

miri64 commented Sep 10, 2019

Uh oh!

Uh oh!

cladmi commented Aug 28, 2019 •

edited

Loading

cladmi Aug 28, 2019 •

edited

Loading