[riot-notifications] [RIOT-OS/RIOT] tools/serial.inc.mk: Support miniterm.py. (#11003)

Alexandre Abadie notifications at github.com
Wed Feb 20 09:03:10 CET 2019


aabadie requested changes on this pull request.

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

I have other comments.

> @@ -19,4 +19,9 @@ ifeq ($(RIOT_TERMINAL),pyterm)
 else ifeq ($(RIOT_TERMINAL),picocom)
     export TERMPROG  ?= picocom
     export TERMFLAGS ?= --nolock --imap lfcrlf --echo --baud "$(BAUD)" "$(PORT)"
+else ifeq ($(RIOT_TERMINAL),miniterm.py)
+    export TERMPROG  ?= miniterm.py

```suggestion
    export TERMPROG  ?= $(RIOT_TERMINAL)
```

Not blocking for this

> @@ -12,4 +12,10 @@ BOARD_BLACKLIST += chronos
 
 TEST_ON_CI_WHITELIST += all
 
+# 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

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

> @@ -12,4 +12,10 @@ BOARD_BLACKLIST += chronos
 
 TEST_ON_CI_WHITELIST += all
 
+# In order to properly test and debug the shell it is better to use a terminal

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11003#pullrequestreview-205613342
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190220/57a26643/attachment.html>


More information about the notifications mailing list