Skip to content
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.

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

riotctrl.shell: flush pexpect before prompt #21

Merged
merged 2 commits into from
Mar 3, 2021

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Mar 2, 2021

For some boards there is a weird syncing error happening: The output of a command issued via a ShellInteraction comes after the next command command issued. Examples of this can be seen in this action or in this comment. With flushing pexpected completely before the prompt, this does not happen again. Thanks @fjmolinas for the hint!

For testing the tests here should pass, as well as tests/congure_test in RIOT on the boards we found the issue for; namely nucleo-f411re, b-l072z-lrwan1, and b-l475e-iot01a:

$ git fetch upstream refs/pull/21/head # assuming you are in the riotctrl repo and RIOT-OS/riotctrl remote is called upstream
$ git checkout FETCH_HEAD
$ pip install .[rapidjson] --upgrade
$ cd ../RIOT     # assuming this is where your RIOT repo is
$ git checkout master
$ git pull
$ BOARD="<one of the above>" make -C tests/congure/test -f flash test

@miri64 miri64 added the bug Something isn't working label Mar 2, 2021
@miri64 miri64 requested a review from fjmolinas March 2, 2021 18:46
@miri64
Copy link
Member Author

miri64 commented Mar 2, 2021

I also made some sample tests with the release tests. They also seem to still work.

# it is not shown
self.riotctrl.term.expect_exact([self.prompt, pexpect.TIMEOUT],
timeout=self.PROMPT_TIMEOUT)
# flush pexpect up to 10000 characters to fix syncing issues with
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# flush pexpect up to 10000 characters to fix syncing issues with
# flush pexpect up to 10000(arbitrarily large value) characters to start with an empty buffer.
# This fixes issues where REPLWrapper would capture previous commands output.

Maybe the comment can be a little more detailed? Feel free to modify of course

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

fjmolinas
fjmolinas previously approved these changes Mar 3, 2021
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK, please squash

@miri64
Copy link
Member Author

miri64 commented Mar 3, 2021

Squashed.

@miri64 miri64 force-pushed the riotctrl.shell/fix/sync branch from 14e2947 to 9149ce4 Compare March 3, 2021 08:01
@miri64
Copy link
Member Author

miri64 commented Mar 3, 2021

Squashed and fixed issue pointed out by the CI.

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

RE-ack

@fjmolinas fjmolinas merged commit 904778c into RIOT-OS:master Mar 3, 2021
@miri64 miri64 deleted the riotctrl.shell/fix/sync branch March 3, 2021 08:11
@miri64
Copy link
Member Author

miri64 commented Mar 3, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants