Skip to content

[TRACKING] buildsystem: introduce "make rawterm" #11099

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jcarrano wants to merge 9 commits into from
Closed

[TRACKING] buildsystem: introduce "make rawterm" #11099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9679d05
makefiles: introduce rawterm recipe and variables.
jcarrano Feb 28, 2019
53800ba
makefiles/serial: Add raw terminal programs
jcarrano Feb 28, 2019
3849f8a
makefiles/serial: add rlwrap support.
jcarrano Feb 28, 2019
990020e
boards/lobaro-lorabox: force RIOT_TERMINAL=pyterm
jcarrano Mar 1, 2019
7a2e825
boards/msba2: force RIOT_TERMINAL=pyterm
jcarrano Mar 1, 2019
086480a
boards/native: Use socat as a termprog.
jcarrano Mar 4, 2019
0ad9540
examples,tests: replace TERMFLAGS by NATIVE_FLAGS for native.
jcarrano Mar 4, 2019
88e14f6
testrunner: Use `make rawterm`
jcarrano Mar 6, 2019
8d74a1b
fixup! connect native to a pty in socat.
jcarrano May 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Makefile.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,10 @@ flash-only: $(FLASHDEPS)
preflash: $(BUILD_BEFORE_FLASH)
$(PREFLASHER) $(PREFFLAGS)

rawterm: $(filter flash, $(MAKECMDGOALS)) $(TERMDEPS)
$(RAWTERMPROG) $(RAWTERMFLAGS)

term: $(filter flash, $(MAKECMDGOALS)) $(TERMDEPS)
$(call check_cmd,$(TERMPROG),Terminal program)
$(TERMPROG) $(TERMFLAGS)

list-ttys:
Expand Down
2 changes: 2 additions & 0 deletions boards/common/msba2/Makefile.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ PORT_LINUX ?= /dev/ttyUSB0
# This does not make a lot of sense, but it has the same value as the previous code
PORT_DARWIN ?= /dev/tty.usbserial-ARM

# The -tg option is specific to pyterm so we are forced to use it.
RIOT_TERMINAL = pyterm
Copy link
Member

@cgundogan cgundogan Jun 3, 2019

Choose a reason for hiding this comment

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

Can't you also use simply expanded variables := here and in all following cases below? There's no need for recursions as it appears to me.

Copy link
Contributor Author

@jcarrano jcarrano Jun 3, 2019

Choose a reason for hiding this comment

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

At the moment we are trying to do the opposite: remove instances of := where it is not needed.

Copy link
Member

@cgundogan cgundogan Jun 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting .. as far as I know you usually remove instances of =, where it is not needed (: maintaining recursively expanded variables is complex and can lead to hard-to-debug problems. Imagine someone replaces pyterm with a function or a variable in the future, but keeps the = ..

What's the motivation for choosing recursive assignments over simple assignments?

Copy link
Contributor Author

@jcarrano jcarrano Jun 3, 2019

Choose a reason for hiding this comment

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

:= has some disadvantages:

  • The value of variables defined with := depends on the order they are defined.
  • Once introduced, it is not that easy to tell if := can be replaced by =
  • := will be evaluated even if not needed. This will force evaluation of all called functions, in particular shell calls. This causes unnecessary overhead at minimum, or a failure that should not occur at most (think what happens if a tool is defined with := but you don't have the tool and you don't need it either)

I believe immediate assignments cause harder-to-debug problems because the order of definition starts to matter.

With respect to performance, if some variable is too expensive to evaluate more than once, there is a trick to memoize it (I think there is a PR by @cladmi ).

Finally, recursive variables fit better into the declarative style that good makefiles should follow.

Copy link
Member

@cgundogan cgundogan Jun 3, 2019

Choose a reason for hiding this comment

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

a quote from the docs (:

To avoid all the problems and inconveniences of recursively expanded variables,
there is another flavor: simply expanded variables.

Kidding aside, I am not very involved in the build system development and I don't know what the desired end result will look like. I was merely stating that I had problems with recursive evaluations in earlier projects and most of the time they lead to performance drops. If you think that recursive evaluations fit better in the picture, then I trust your judgement.

Copy link
Contributor Author

@jcarrano jcarrano Jun 3, 2019

Choose a reason for hiding this comment

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

The reality is that we (and by that I mean @cladmi and me) should probably spend some time writing a proper guide to writing makefiles in RIOT.

That part of the docs is true, but it refers mainly to things like wildcards and shell calls with side-efects. Essentially, I/O. In our case := is most of the times workaround for this. The real solution is to more cleanly separate the functional and I/O parts of our makefiles.

Performance drop in RIOT builds is mainly due to:

  • FORCE and PHONY targets (for example, we are relinking every time).
    • these are ugly hacks because people could not figure out how to do dependencies properly.
  • Recursive make (or more precisely, the fact that the submakes run always)
  • Checking tools all the time / evaluating tool-related variables, even when those tools are not used (:= is involved here).
    • Here is where a memoization function will help, but only after we refactor.

This is probably more info than you asked for, but I feel this should be stated somewhere.

Copy link
Contributor

@cladmi cladmi Jun 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If you are doing recursive definitions you are not defining but doing evaluation.

Here I would say there is no need for evaluation so no :=.

Interesting .. as far as I know you usually remove instances of =, where it is not needed (: maintaining recursively expanded variables is complex and can lead to hard-to-debug problems. Imagine someone replaces pyterm with a function or a variable in the future, but keeps the = ..

usually I would say no, its only in RIOT that I saw this pattern.
If you have INCLUDES that depends on INCLUDES you should introduce a new name and do INCLUDES = $(INCLUDES_TOOLCHAIN) $(INCLUDES_C_LIBRARY) and define INCLUDES_TOOLCHAIN later instead of prepending to the previous value.

If you replace pyterm with a function, it does nothing as you are not evaluating the name with $(pyterm).

I was merely stating that I had problems with recursive evaluations in earlier projects and most of the time they lead to performance drops.

If a shell call is evaluated 100 times it can indeed slow down the build, but then only this one should be optimized. Also, with correct file targets you do not rebuild the target if not neheaderseded so do not re-evaluate the variable for nothing. It may make the clean build slower but makes incremental build faster.
Also, by doing := everywhere you are making make clean slow as it checks the warnings supported for your gcc version even if you do not use it.

Copy link
Contributor

@fjmolinas fjmolinas Sep 27, 2019

Choose a reason for hiding this comment

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

@cladmi @jcarrano This discussion was quite insightful for me, I think it would be great to have this documented somewhere, kind ok like the advanced-build-system-tricks.md but a build-system-basics.md page :)

Copy link
Contributor

@cladmi cladmi Sep 30, 2019

Choose a reason for hiding this comment

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

It is somehow "gnumake" basics, but how it should be applied to RIOT could indeed be good.
I never had any issues before as was not using recursive makefiles.

We still need to export things and so how to counter the issues with it with memoized somehow and target-export-variables.

TERMFLAGS += -tg -p "$(PORT)"
include $(RIOTMAKE)/tools/serial.inc.mk

Expand Down
2 changes: 2 additions & 0 deletions boards/lobaro-lorabox/Makefile.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ PORT_LINUX ?= /dev/ttyUSB0
PORT_DARWIN ?= $(firstword $(sort $(wildcard /dev/tty.usbmodem*)))

# setup serial terminal
# The --set-rts option is specific to pyterm so we are forced to use it.
RIOT_TERMINAL = pyterm
include $(RIOTMAKE)/tools/serial.inc.mk

FLASHER = $(RIOTTOOLS)/stm32loader/stm32loader.py
Expand Down
35 changes: 25 additions & 10 deletions boards/native/Makefile.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,30 @@ else
export DEBUGGER ?= gdb
endif

TERMPROG ?= $(ELFFILE)
# set the tap interface for term/valgrind
# FIXME: this is not a port. It should be renames as "NET_INTERFACE" or
# something else that is more honest.
ifneq (,$(filter netdev_default gnrc_netdev_default,$(USEMODULE)))
export PORT ?= tap0
else
export PORT =
endif

RIOT_TERMINAL ?= rlwrap
# Silence the "Warning: no PORT set!" message
# PORT ?= not-applicable
# Wrapping native in socat disables line buffering so the behavior of native
# is more similar to that of bare-metal RIOT.
RIOT_RAWTERMINAL ?= socat
NATIVE_EXEC ?= $(ELFFILE)
NATIVE_FLAGS += $(PORT) # Flags to the native executable

# checkout the "sigint" flags. Without it ctrl-c just exits socat, which breaks
# the pipe on the RIOT executable causing SIGPIPE and a dirty exit.
SOCAT_REMOTE = exec:"$(NATIVE_EXEC) $(NATIVE_FLAGS)",sigint,pty $(PORT)

include $(RIOTMAKE)/tools/serial.inc.mk

export FLASHER = true
export VALGRIND ?= valgrind
export CGANNOTATE ?= cg_annotate
Expand Down Expand Up @@ -93,14 +116,6 @@ else
endif
export LINKFLAGS += -ffunction-sections

# set the tap interface for term/valgrind
ifneq (,$(filter netdev_default gnrc_netdev_default,$(USEMODULE)))
export PORT ?= tap0
else
export PORT =
endif

TERMFLAGS := $(PORT) $(TERMFLAGS)

export ASFLAGS =
ifeq ($(shell basename $(DEBUGGER)),lldb)
Expand All @@ -117,7 +132,7 @@ debug-valgrind-server: export VALGRIND_FLAGS ?= --vgdb=yes --vgdb-error=0 -v \
--leak-check=full --track-origins=yes --fullpath-after=$(RIOTBASE) \
--read-var-info=yes
term-cachegrind: export CACHEGRIND_FLAGS += --tool=cachegrind
term-gprof: TERMPROG = GMON_OUT_PREFIX=gmon.out $(ELFFILE)
term-gprof: export GMON_OUT_PREFIX=gmon.out
all-valgrind: export CFLAGS += -DHAVE_VALGRIND_H -g3
all-valgrind: export NATIVEINCLUDES += $(shell pkg-config valgrind --cflags)
all-debug: export CFLAGS += -g3
Expand Down
3 changes: 1 addition & 2 deletions boards/ruuvitag/Makefile.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ CPU_MODEL = nrf52832xxaa

# for this board, we are using Segger's RTT as default terminal interface
USEMODULE += stdio_rtt
TERMPROG = $(RIOTTOOLS)/jlink/jlink.sh
TERMFLAGS = term_rtt
RIOT_RAWTERMINAL = jlink

# use shared Makefile.include
include $(RIOTBOARD)/common/nrf52xxxdk/Makefile.include
3 changes: 1 addition & 2 deletions boards/thingy52/Makefile.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ CPU_MODEL = nrf52832xxaa

# for this board, we are using Segger's RTT as default terminal interface
USEMODULE += stdio_rtt
TERMPROG = $(RIOTTOOLS)/jlink/jlink.sh
TERMFLAGS = term_rtt
RIOT_RAWTERMINAL = jlink

# use shared Makefile.include
include $(RIOTBOARD)/common/nrf52/Makefile.include
2 changes: 1 addition & 1 deletion dist/pythonlibs/testrunner/spawn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def find_exc_origin(exc_info):


def setup_child(timeout=10, spawnclass=pexpect.spawnu, env=None, logfile=None):
child = spawnclass("make term", env=env, timeout=timeout,
child = spawnclass("make rawterm", env=env, timeout=timeout,
codec_errors='replace', echo=False)

# on many platforms, the termprog needs a short while to be ready...
Expand Down
2 changes: 1 addition & 1 deletion examples/gnrc_border_router/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ ifeq (,$(filter native,$(BOARD)))
CFLAGS += -DETHOS_BAUDRATE=$(ETHOS_BAUDRATE) -DUSE_ETHOS_FOR_STDIO
else
GNRC_NETIF_NUMOF := 2
TERMFLAGS += -z [::1]:17754
NATIVE_FLAGS += -z [::1]:17754
USEMODULE += socket_zep
endif

Expand Down
2 changes: 2 additions & 0 deletions makefiles/info.inc.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ info-build:
@echo ''
@echo 'TERMPROG: $(TERMPROG)'
@echo 'TERMFLAGS: $(TERMFLAGS)'
@echo 'RAWTERMPROG: $(RAWTERMPROG)'
@echo 'RAWTERMFLAGS: $(RAWTERMFLAGS)'
@echo 'PORT: $(PORT)'
@echo ''
@echo 'DEBUGGER: $(DEBUGGER)'
Expand Down
5 changes: 5 additions & 0 deletions makefiles/tools/jlink.inc.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,8 @@ export FFLAGS ?= flash $(FLASHFILE)
export DEBUGGER_FLAGS ?= debug $(ELFFILE)
export DEBUGSERVER_FLAGS ?= debug-server
export RESET_FLAGS ?= reset

ifeq ($(RIOT_RAWTERMINAL),jlink)
RAWTERMPROG ?= $(RIOTTOOLS)/jlink/jlink.sh
RAWTERMFLAGS ?= term_rtt
endif
26 changes: 22 additions & 4 deletions makefiles/tools/serial.inc.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,29 @@ RIOT_TERMINAL ?= pyterm
ifeq ($(RIOT_TERMINAL),pyterm)
TERMPROG ?= $(RIOTTOOLS)/pyterm/pyterm
TERMFLAGS ?= -p "$(PORT)" -b "$(BAUD)"
else ifeq ($(RIOT_TERMINAL),socat)
SOCAT_OUTPUT ?= -
TERMPROG ?= $(RIOT_TERMINAL)
TERMFLAGS ?= $(SOCAT_OUTPUT) open:$(PORT),b$(BAUD),echo=0,raw
else ifeq ($(RIOT_TERMINAL),rlwrap)
Copy link
Contributor

@cladmi cladmi Jun 3, 2019

Choose a reason for hiding this comment

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

This is unrelated to supporting rawterm.

TERMPROG ?= $(RIOT_TERMINAL)
RLWRAP_PROMPT ?= -pPurple -S 'RIOT $$ '
Copy link
Contributor

@cladmi cladmi Jun 3, 2019

Choose a reason for hiding this comment

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

Why even adding a prompt ?

RLWRAP_FLAGS ?= -a -C RIOT-$(BOARD)
TERMFLAGS ?= $(RLWRAP_PROMPT) $(RLWRAP_FLAGS) $(RAWTERMPROG) $(RAWTERMFLAGS)
else ifeq ($(RIOT_TERMINAL),picocom)
TERMPROG ?= picocom
TERMFLAGS ?= --nolock --imap lfcrlf --baud "$(BAUD)" "$(PORT)"
else ifeq ($(RIOT_TERMINAL),custom)
# Do nothing. This name is reserved so that TERMFLAGS does not get
# set if the user wants to override TERMPROG.
endif

RIOT_RAWTERMINAL ?= socat
ifeq ($(RIOT_RAWTERMINAL),socat)
SOCAT_LOCAL ?= STDIO,icanon=0,echo=0,escape=0x04
Copy link
Contributor Author

@jcarrano jcarrano Aug 27, 2019

Choose a reason for hiding this comment

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

With these options I'm still getting line buffering.

SOCAT_REMOTE ?= open:$(PORT),b$(BAUD),echo=0,raw
RAWTERMPROG ?= $(RIOT_RAWTERMINAL)
RAWTERMFLAGS ?= $(SOCAT_LOCAL) $(SOCAT_REMOTE)
else ifeq ($(RIOT_RAWTERMINAL),picocom)
RAWTERMPROG ?= picocom
RAWTERMFLAGS ?= --nolock -q --imap lfcrlf --baud "$(BAUD)" "$(PORT)"
else ifeq ($(RIOT_RAWTERMINAL),custom)
# Do nothing. This name is reserved so that TERMFLAGS does not get
# set if the user wants to override TERMPROG.
endif
22 changes: 21 additions & 1 deletion makefiles/vars.inc.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,28 @@ export GIT_CACHE_DIR # path to git-cache cache directory
export FLASHER # The command to call on "make flash".
export FFLAGS # The parameters to supply to FLASHER.
export FLASH_ADDR # Define an offset to flash code into ROM memory.
# TERMPROG # The command to call on "make term".

# --- Terminal Access --- #

# RIOT_TERMINAL # Preset to use for the "user friendly" terminal.
# TERMPROG and TERMFLAGS will be set according to
# RIOT_TERMINAL. The special name "custom" can be
# used to leave these variables undefined.
# TERMPROG # The command to call on "make term". This should be
# "user friendly" terminal, with line editing-history, etc.
# By default, rlwrap is used around the RAWTERM program.
# TERMFLAGS # Additional parameters to supply to TERMPROG.

# RIOT_RAWTERMINAL # Like RIOT_TERMINAL but for the raw/testing terminal.
# RAWTERMPROG # The command to call on "make rawterm". This terminal
# is used for testing. It should have no buffering,
# and no translation or modification of input or output.
# RAWTERMFLAGS # Additional parameters to supply to RAWTERMPROG.

# --- Native-specific --- #
# NATIVE_EXEC # Executable for native builds.
# NATIVE_FLAGS # Command line arguments for the native executable.

export PORT # The port to connect the TERMPROG to.
export ELFFILE # The unstripped result of the compilation.
export HEXFILE # The stripped result of the compilation.
Expand Down
2 changes: 1 addition & 1 deletion tests/gnrc_ipv6_ext/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export TAP ?= tap0
ifeq (native,$(BOARD))
USEMODULE += netdev_tap

TERMFLAGS ?= $(TAP)
NATIVE_FLAGS ?= $(TAP)
else
USEMODULE += ethos

Expand Down
2 changes: 1 addition & 1 deletion tests/gnrc_rpl_srh/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ CFLAGS += -DOUTPUT=TEXT
ifeq (native,$(BOARD))
USEMODULE += netdev_tap

TERMFLAGS ?= $(TAP)
NATIVE_FLAGS ?= $(TAP)
else
USEMODULE += ethos

Expand Down
2 changes: 1 addition & 1 deletion tests/socket_zep/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ USEMODULE += socket_zep

CFLAGS += -DDEVELHELP

TERMFLAGS ?= -z [::1]:17754
NATIVE_FLAGS ?= -z [::1]:17754

include $(RIOTBASE)/Makefile.include
2 changes: 1 addition & 1 deletion tests/socket_zep/tests/01-run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def testfunc(child):


if __name__ == "__main__":
os.environ['TERMFLAGS'] = "-z [%s]:%d,[%s]:%d" % (
os.environ['NATIVE_FLAGS'] = "-z [%s]:%d,[%s]:%d" % (
zep_params['local_addr'], zep_params['local_port'],
zep_params['remote_addr'], zep_params['remote_port'])
s = socket.socket(family=socket.AF_INET6, type=socket.SOCK_DGRAM)
Expand Down