Skip to content

tools/serial.inc.mk: Support miniterm.py.#11003

Closed
jcarrano wants to merge 2 commits intoRIOT-OS:masterfrom
jcarrano:support-miniterm.py
Closed

tools/serial.inc.mk: Support miniterm.py.#11003
jcarrano wants to merge 2 commits intoRIOT-OS:masterfrom
jcarrano:support-miniterm.py

Conversation

@jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Feb 12, 2019 •
edited
Loading

Contribution description

miniterm.py is a simple terminal program that is included with pyserial. This means that it is available wherever pyterm can work. It allows raw access, does line translation and passes through special characters.

For test scripts, a terminal that does not modify the input and output streams, configured without local echo, is preferred as it ensures the test setup is introducing as few noise as possible.

Testing procedure

With this change, tests/shell uses miniterm.py. Just run the tests. Use a real board, native has no real TTY.

$ cd tests/shell
$ BOARD=samr21-xpro make all
$ BOARD=samr21-xpro make flash
$ BOARD=samr21-xpro make test

Issues/PRs references

This PR is part of #10994 and it's purpose is to ease shell automation.
Split from #10788 .

@jcarrano jcarrano added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: tools Area: Supplementary tools labels Feb 12, 2019
This was referenced Feb 12, 2019
Merged
Closed
@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 12, 2019
aabadie
aabadie requested changes Feb 14, 2019
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

It's indeed useful to provide support for miniterm even if it's not the default.
I haven't tried this PR but I think local echo should also be enabled by default. There are also a couple minor issues.

tests/shell/Makefile Outdated
@@ -1,4 +1,5 @@
DEVELHELP=0

Copy link
Contributor

@aabadie aabadie Feb 14, 2019

Choose a reason for hiding this comment

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

unrelated

Copy link
Contributor Author

@jcarrano jcarrano Feb 19, 2019

Choose a reason for hiding this comment

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

Oops, that slipped through when I splitted the change.

tests/shell/Makefile Outdated
TEST_ON_CI_WHITELIST += all

ifneq ($(BOARD), native)
RIOT_TERMINAL=miniterm.py
Copy link
Contributor

@aabadie aabadie Feb 14, 2019

Choose a reason for hiding this comment

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

Looks like there's tab here. The indent should be 2 spaces.

makefiles/tools/serial.inc.mk Outdated
export TERMFLAGS ?= --nolock --imap lfcrlf --echo --baud "$(BAUD)" "$(PORT)"
else ifeq ($(RIOT_TERMINAL),miniterm.py)
export TERMPROG ?= miniterm.py
export TERMFLAGS ?= --eol CRLF "$(PORT)" "$(BAUD)"
Copy link
Contributor

@aabadie aabadie Feb 14, 2019

Choose a reason for hiding this comment

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

Maybe also enable --echo ? It's the kind of useful things when using the shell.

Copy link
Contributor Author

@jcarrano jcarrano Feb 19, 2019

Choose a reason for hiding this comment

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

With local echo you end up having double echo, as the shell echoes too, and miniterm does not do line buffering, eg.:

> hheellpp

Copy link
Contributor

@cladmi cladmi Feb 20, 2019

Choose a reason for hiding this comment

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

Having local echo messes with the tests also as the sent string is matched by pexpect.

@jcarrano jcarrano force-pushed the support-miniterm.py branch from 5d1197c to 7478f60 Compare February 19, 2019 11:10
This was referenced Feb 20, 2019
Merged
Merged
aabadie
aabadie requested changes Feb 20, 2019
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

I tested tests/shell with miniterm and confirm that it also works.

I have other comments.

makefiles/tools/serial.inc.mk Outdated
export TERMPROG ?= picocom
export TERMFLAGS ?= --nolock --imap lfcrlf --echo --baud "$(BAUD)" "$(PORT)"
else ifeq ($(RIOT_TERMINAL),miniterm.py)
export TERMPROG ?= miniterm.py
Copy link
Contributor

@aabadie aabadie Feb 20, 2019

Choose a reason for hiding this comment

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

Suggested change
export TERMPROG ?= miniterm.py
export TERMPROG ?= $(RIOT_TERMINAL)

Not blocking for this

tests/shell/Makefile Outdated
# In order to properly test and debug the shell it is better to use a terminal
# program that does not modify the input and output streams and has no buffering.
ifneq ($(BOARD), native)
RIOT_TERMINAL?=miniterm.py
Copy link
Contributor

@aabadie aabadie Feb 20, 2019

Choose a reason for hiding this comment

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

The tab is still there. I would put space around ?= like it's done in other places.

tests/shell/Makefile

TEST_ON_CI_WHITELIST += all

# In order to properly test and debug the shell it is better to use a terminal
Copy link
Contributor

@aabadie aabadie Feb 20, 2019

Choose a reason for hiding this comment

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

Why just set this in tests/shell only ? I would prefer to have this set globally in Makefile.tests_common.

@jcarrano jcarrano force-pushed the support-miniterm.py branch from 7478f60 to 0c27453 Compare February 20, 2019 10:38
@jcarrano
Copy link
Contributor Author

jcarrano commented Feb 20, 2019

@aabadie I rebased this on top of your indentation fix.

@jcarrano
tools/serial.inc.mk: Support miniterm.py.
0c27453 
miniterm.py is a simple terminal program that is included with pyserial.
This means that it is available wherever pyterm can work. It allows raw
access, does line translation and passes through special characters.

For test scripts, a terminal that does not modify the input and output
streams, configured without local echo, is preferred as it ensures the
test setup is introducing as few noise as possible.
@cladmi
Copy link
Contributor

cladmi commented Feb 20, 2019

When testing #10952 I found that having local echo may even be hurting for testing as it introduces more text in the tested text.

@cladmi
Copy link
Contributor

cladmi commented Feb 20, 2019

I would prefer to move changing the test configuration in a different PR as it relates to the main tracking issue and not to adding the support.

Adding support should be done alone and it does not hurt anything.
Changing the test configuration is a different thing that may have consequences.

smlng
smlng reviewed Feb 25, 2019
View reviewed changes
@jcarrano
Copy link
Contributor Author

jcarrano commented Feb 25, 2019

#11034 makes this PR irrelevant. Socat is a better tool. I'm closing this.

@jcarrano jcarrano closed this Feb 25, 2019
This was referenced Nov 19, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie requested changes

+2 more reviewers

@cladmi cladmi cladmi left review comments

@smlng smlng smlng left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jcarrano @cladmi @aabadie @smlng