Skip to content

boards/msba2/lobaro-lorabox: change TERMFLAGS to PYTERMFLAGS#12095

Merged
maribu merged 3 commits intoRIOT-OS:masterfrom
cladmi:pr/pyterm/pytermflags
Aug 28, 2019
Merged

boards/msba2/lobaro-lorabox: change TERMFLAGS to PYTERMFLAGS#12095
maribu merged 3 commits intoRIOT-OS:masterfrom
cladmi:pr/pyterm/pytermflags

Conversation

@cladmi
Copy link
Copy Markdown
Contributor

@cladmi cladmi commented Aug 27, 2019

Contribution description

The boards are using pyterm specific options that do not work on any
other RIOT_TERMINAL. It is a shame this is required but at least do
not pass arbitrary arguments to the other RIOT_TERMINAL.
So use the new PYTERMFLAGS for this.

This addresses setting the TERMFLAGS in an incompatible way to the other RIOT_TERMINAL in #12089 #12090

However, term would still not work with socat or picocom.

Testing procedure

Testing still works on msba2. For example tests/bloom_bytes

BOARD=msba2 RIOT_CI_BUILD=1 make --no-print-directory -C tests/bloom_bytes/ flash >/dev/null; BOARD=msba2 RIOT_CI_BUILD=1 make --no-print-directory -C tests/bloom_bytes/  test
/home/harter/work/git/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyUSB0" -b "115200" -tg
Twisted not available, please install it if you want to use pyterm's JSON capabilities
Connect to serial port /dev/ttyUSB0
Welcome to pyterm!
Type '/exit' to exit.
main(): This is RIOT! (Version: buildtest)
Testing Bloom filter.

m: 4096 k: 8

adding 512 elements took 188ms
checking 10000 elements took 1801ms

267 elements probably in the filter.
9733 elements not in the filter.
0.026700 false positive rate.

All done!
tests/shell using socat

It does not fail on invalid arguments

Reset CPU (into user code)
Programming done.
socat - open:/dev/ttyUSB0,b115200,echo=0,raw
Timeout in expect script at "child.expect('test_shell.')" (tests/shell/tests/01-run.py:67)

/home/harter/work/git/RIOT/tests/shell/../../Makefile.include:605: recipe for target 'test' failed
make: *** [test] Error 1
make: Leaving directory '/home/harter/work/git/RIOT/tests/shell'

Where in master, socat was called with incompatible arguments

BOARD=msba2 make -C tests/shell/ test
make: Entering directory '/home/harter/work/git/RIOT/tests/shell'
socat -tg -p "/dev/ttyUSB0"
2019/08/27 14:23:55 socat[7360] E unknown option "-p"; use option "-h" for help
/home/harter/work/git/RIOT/Makefile.include:566: recipe for target 'term' failed
make[1]: *** [term] Error 1
Unexpected end of file in expect script at "child.expect('test_shell.')" (tests/shell/tests/01-run.py:67)

lobaro-lorabox

The term command is still the same for lobaro-lorabox as in master
I do not have a board to do the real test

BOARD=lobaro-lorabox RIOT_CI_BUILD=1 make --no-print-directory -C tests/bloom_bytes/ info-debug-variable-TERMFLAGS
-p /dev/ttyUSB0 -b 115200 --set-rts 0

Which gives this execution

BOARD=lobaro-lorabox RIOT_CI_BUILD=1 make --no-print-directory -C tests/bloom_bytes/ term | head -n 3
/home/harter/work/git/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyUSB0" -b "115200" --set-rts 0
Twisted not available, please install it if you want to use pyterm's JSON capabilities
Connect to serial port /dev/ttyUSB0
Cannot connect to serial port /dev/ttyUSB0: could not open port /dev/ttyUSB0: [Errno 2] No such file or directory: '/dev/ttyUSB0'

Issues/PRs references

cladmi added 3 commits August 27, 2019 12:06
@cladmi
boards/common/msba2: remove duplicate PORT
42d6584 
PORT is already given in pyterm TERMFLAGS.
@cladmi
makefiles/serial.inc.mk: add PYTERMFLAGS variable
62fe786 
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.
@cladmi
boards/pyterm: change TERMFLAGS to PYTERMFLAGS
367aa84 
The boards are using `pyterm` specific options that do not work on any
other `RIOT_TERMINAL`. It is a shame this is required but at least do
not pass arbitrary arguments to the other RIOT_TERMINAL.
So use the new PYTERMFLAGS for this.
This was referenced Aug 27, 2019
Closed
Closed
Closed
@jcarrano jcarrano self-requested a review August 27, 2019 13:11
@jcarrano jcarrano added Area: build system Area: Build system 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: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 27, 2019
@jcarrano jcarrano requested a review from maribu August 27, 2019 13:11
@maribu
Copy link
Copy Markdown
Member

maribu commented Aug 27, 2019

I can confirm that the terminal still works on the MSB-A2.

The flags for picocom for the MSB-A2 are --lower-rts --lower-dtr btw.

@maribu
Copy link
Copy Markdown
Member

maribu commented Aug 27, 2019

I have no lobaro-lorabox to test. If someone would do this, I'd ACK

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Aug 27, 2019
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 27, 2019

I think the lobaro-lorabox is a bit more challenging. It seems like it needs the rts cleared to actually work. It does compile and connected but the bloombytes test doesn't actually execute... This is for both socat and picocom, it does work with pyterm.

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 27, 2019

If this is the case then maybe we just throw an error or say RIOT_TERMINAL=pyterm for that board?

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 27, 2019

I confirmed that you must toggle the rts to get the board running at all. Just plugging in will not allow it to execute (though battery power is fine).

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 27, 2019

We could maybe change the flash script to leave the RTS cleared after so it would work if just flashed (meaning if could work for running all the tests).

@maribu
Copy link
Copy Markdown
Member

maribu commented Aug 27, 2019 •
edited
Loading

If special requirements for the terminal interface are more commen than exclusively these two boards, how about adding special variables like TERM_CLEAR_RTS. The build system could translate these into the correct flags for the supported terminals. That way support for new terminals could be added without having to touch every board's Makefile again.

@maribu
Copy link
Copy Markdown
Member

maribu commented Aug 27, 2019

We could maybe change the flash script to leave the RTS cleared after so it would work if just flashed (meaning if could work for running all the tests).

picocom seems to set RTS on start unless a flag says different. So that would not work for all terminal programs

@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Aug 28, 2019

I think the lobaro-lorabox is a bit more challenging. It seems like it needs the rts cleared to actually work. It does compile and connected but the bloombytes test doesn't actually execute... This is for both socat and picocom, it does work with pyterm.

This PR does not address any compatibility with the other terminals, it just prevents sending arguments that do not match the program.

We could maybe change the flash script to leave the RTS cleared after so it would work if just flashed (meaning if could work for running all the tests).

This would also not work after unplugging the board I guess.

@cladmi cladmi mentioned this pull request Aug 28, 2019
Closed
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 28, 2019

Yup, if it is out of scope then that is fine. Consider the lobaro-lorabox tested and expected to fail and another PR can take care of updating that. Probably by one of the people that have it!
ping @leandrolanzieri @jia200x @me

@maribu maribu added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Aug 28, 2019
maribu
maribu approved these changes Aug 28, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

OK, now with testing done for both boards: ACK

@maribu maribu merged commit 5019480 into RIOT-OS:master Aug 28, 2019
@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Aug 28, 2019

Thank you for the review.

@cladmi cladmi deleted the pr/pyterm/pytermflags branch August 28, 2019 13:57
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu approved these changes

@jcarrano jcarrano Awaiting requested review from jcarrano

Assignees

No one assigned

Labels

Area: build system Area: Build system 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 Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

5 participants

@cladmi @maribu @MrKevinWeiss @kb2ma @jcarrano