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.

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

Jump to bottom

sys/shell: refactor readline function #13196

Merged
merged 5 commits into from
May 14, 2020
Merged

sys/shell: refactor readline function #13196

merged 5 commits into from
May 14, 2020

Conversation

HendrikVE
Copy link
Contributor

@HendrikVE HendrikVE commented Jan 24, 2020 •
edited
Loading

Contribution description

With this PR I simply copied PR #12099 authored by @jcarrano. Since he left the team, I hereby offer to take care of rebasing and implement required changes.

This makes the code of readline() clearer and shorter. It also fixes a minor artifact of the long line handling.

Previously, it was not possible to recover from a long line. That is, if too many characters were sent, the line would be invalidated and pressing backspace would not fix it. The only option was to discard the line. It is now possible to bring the line back to size. Note that visual effects when deleting characters will still depend on the host's terminal.

The new code is written in a way that all writes to memory are guarded by bounds check, so an assertion was removed.

Testing procedure

Test vectors were added to tests/shell. The test is not really testing a lot right now because the host is still line buffering.

Issues/PRs references

Copy of #12099

Dependencies:

@HendrikVE HendrikVE mentioned this pull request Jan 24, 2020
Closed
2 tasks
@benpicco benpicco added Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation and removed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jan 25, 2020
@jcarrano
Copy link
Contributor

@HendrikVE thank you very much for taking over

@HendrikVE HendrikVE mentioned this pull request Feb 7, 2020
Merged
@HendrikVE HendrikVE added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 7, 2020
@HendrikVE HendrikVE force-pushed the shell-readline-refactor branch 2 times, most recently from 758547c to a3c1fca Compare February 9, 2020 23:23
@HendrikVE HendrikVE added Area: tests Area: tests and testing framework CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Feb 10, 2020
@fjmolinas
Copy link
Contributor

This one needs a rebase now!

@HendrikVE
Copy link
Contributor Author

I'm already on it ;) The same with #13197

@HendrikVE HendrikVE force-pushed the shell-readline-refactor branch from a3c1fca to 886bcd3 Compare March 30, 2020 12:44
@HendrikVE HendrikVE force-pushed the shell-readline-refactor branch from 886bcd3 to cb12b5d Compare March 30, 2020 12:45
@HendrikVE HendrikVE added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 30, 2020
@HendrikVE
Copy link
Contributor Author

Rebased and ready for review :)

@kaspar030
Copy link
Contributor

I tried this on particle-xenon and nucleo-f072rb. Just typing a long line (hammering the keyboard) causes a failed assertio. Please take a look.

Welcome to RIOT!
> 
> 
> 
> 
> kjdföjasdfkljsdöljfölsdjföljsdöfjksdöaklfjlökasdjfölkasjdföljasdölfjasdlöjfölsdajfölasdjflöasdjölfjasdlökfjölas0x800110b
*** RIOT kernel panic:
FAILED ASSERTION.

        pid | name                 | state    Q | pri | stack  ( used) | base addr  | current     
          - | isr_stack            | -        - |   - |    512 (  132) | 0x20000000 | 0x200001c0
          1 | idle                 | pending  Q |  15 |    256 (  156) | 0x200003c0 | 0x20000424 
          2 | main                 | running  Q |   7 |   1536 (  672) | 0x200004c0 | 0x200008f4 
            | SUM                  |            |     |   2304 (  960)

*** halted.

@gschorcht gschorcht added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 7, 2020
@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 7, 2020
@fjmolinas
Copy link
Contributor

@HendrikVE can you test if the failing test fails in master as well? I saw it fail in another PR

@gschorcht
Copy link
Contributor

gschorcht commented May 8, 2020 •
edited
Loading

@HendrikVE can you test if the failing test fails in master as well? I saw it fail in another PR

This problem definitely has nothing to do with this PR, it only occurs sporadically in Murdock. tests/pkg_littlefs works perfectly on my local system. Maybe it is a hardware problem with the flash memory on certain workers. I'm trying to figure out whether the problem occurs on certain workers. Til now, I have observed the problem on pi-6356c652, pi-67e8efb7 and pi-e4c1d860.

Let's rerun the test and see what happens.

@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 8, 2020
@fjmolinas fjmolinas added CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 12, 2020
@fjmolinas
Copy link
Contributor

There is something wrong with esp32-wroom32 in master, its failing multiple tests, and has been failing multiple tests in nightly for the last couple of days.

@kaspar030
Copy link
Contributor

There is something wrong with esp32-wroom32 in master, its failing multiple tests, and has been failing multiple tests in nightly for the last couple of days.

It seems like not only esp32-wroom32 is failing tests though. :(

@kaspar030
Copy link
Contributor

It seems like not only esp32-wroom32 is failing tests though. :(

sorry, I think I misread the output, you're right.

@gschorcht
Copy link
Contributor

gschorcht commented May 12, 2020 •
edited
Loading

Yeah, I already saw it in the nightlies. According to the nightlies the tests fail since May 10th. So it seems to be related to the fix in PR #14041 that was merged on May, 9th. This fix sets BUILD_BEFORE_FLASH to ensure that the ELF file is build before the flash image is generated. Is this variable used by the CI?

@kaspar030
Copy link
Contributor

Is this variable used by the CI?

Probably now it is used too much ;) CI splits building and testing. One (fast) worker compiles the elf and sends it to a queue as "test job", then a RasPi picks that, copies the .elf to its place and calls "make test". it seems that due to that change, it tries to rebuild the .elf, or something in that direction.

@kaspar030
Copy link
Contributor

Let's continue in #14041!

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 14, 2020
@fjmolinas
Copy link
Contributor

Hopefully this build will the the last..

@HendrikVE
Copy link
Contributor Author

Looks good now!

fjmolinas
fjmolinas approved these changes May 14, 2020
View reviewed changes
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!

@fjmolinas fjmolinas merged commit 1f9d299 into RIOT-OS:master May 14, 2020
@HendrikVE
Copy link
Contributor Author

Cool! Thanks a lot @kaspar030, @fjmolinas and @gschorcht for your reviews! #13197 needs a rebase now, I'll do that soon.

@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
@HendrikVE HendrikVE deleted the shell-readline-refactor branch June 26, 2020 09:35
bergzand pushed a commit to bergzand/RIOT that referenced this pull request Jul 15, 2020
@fjmolinas @bergzand
cpu/esp32: switch from O2 to Os
3fa9552 
In RIOT-OS#12955 optimization was switched to O2 because with the '-Os'
option, the ESP32 hangs sporadically in 'tests/bench*' if
interrupts where disabled too early by benchmark tests.

Since it hasn't been reproduced since and in RIOT-OS#13196 O2 was causing
un-explained hardfaults, since the aforementioned issue could not
be reproduced we switch back to Os by removing O2, as Os will be
used by default.
jia200x pushed a commit to jia200x/RIOT that referenced this pull request Mar 9, 2021
@fjmolinas @jia200x
cpu/esp32: switch from O2 to Os
db53264 
In RIOT-OS#12955 optimization was switched to O2 because with the '-Os'
option, the ESP32 hangs sporadically in 'tests/bench*' if
interrupts where disabled too early by benchmark tests.

Since it hasn't been reproduced since and in RIOT-OS#13196 O2 was causing
un-explained hardfaults, since the aforementioned issue could not
be reproduced we switch back to Os by removing O2, as Os will be
used by default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@gschorcht gschorcht gschorcht approved these changes

@aabadie aabadie Awaiting requested review from aabadie

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@smlng smlng Awaiting requested review from smlng

Assignees

@kaspar030 kaspar030

Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.07
Development

Successfully merging this pull request may close these issues.
7 participants
@HendrikVE @jcarrano @fjmolinas @kaspar030 @gschorcht @miri64 @benpicco