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: simplify shell #3402

Merged
merged 4 commits into from
Sep 3, 2015
Merged

sys: simplify shell #3402

merged 4 commits into from
Sep 3, 2015

Conversation

kaspar030
Copy link
Contributor

seeing multiple different types of getchar/putchar given to shell, with no actual effect as all output goes to the default stdio anyways, I've decided to get rid of the parameters.

If we one day want shells with IO other than stdio, we should implement it right and redirect stdio.

This is breaks all platforms not offering getchar/putchar. (WIP because of this)

@kaspar030 kaspar030 added 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 State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 14, 2015
@kaspar030 kaspar030 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 14, 2015
haukepetersen
haukepetersen reviewed Jul 15, 2015
View reviewed changes
examples/rpl_udp/main.c
shell_init(&shell, shell_commands, UART0_BUFSIZE, uart0_readc, uart0_putc);
/* start shell */
char line_buf[SHELL_DEFAULT_BUFSIZE];
shell_run(shell_commands, line_buf, SHELL_DEFAULT_BUFSIZE);
Copy link
Contributor

Choose a reason for hiding this comment

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

where is line_buf defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhm, one line above? :)

@haukepetersen
Copy link
Contributor

very nice, +1!

@kaspar030
Copy link
Contributor Author

(I mark the PR "ready for CI" to see build errors)

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 15, 2015
@haukepetersen
Copy link
Contributor

only some :-)

@kaspar030 kaspar030 mentioned this pull request Jul 16, 2015
Merged
@kaspar030
Copy link
Contributor Author

  • rebased
  • fixed chronos fix

@haukepetersen
Copy link
Contributor

could you pls squash?

@kaspar030
Copy link
Contributor Author

  • rebased
  • squashed

@kaspar030 kaspar030 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jul 31, 2015
@haukepetersen haukepetersen mentioned this pull request Aug 5, 2015
Closed
OlegHahm
OlegHahm reviewed Aug 5, 2015
View reviewed changes
sys/include/shell.h
@@ -29,6 +29,11 @@ extern "C" {
#endif

/**
* @brief Default shell buffer size (maximum line length shell can handle)
*/
#define SHELL_DEFAULT_BUFSIZE (128)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be 64 bytes enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go with 128, its the safer bet...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, if I think of commands like fibroute you're probably right.

@OlegHahm
Copy link
Member

OlegHahm commented Aug 5, 2015

Shell doesn't work on MSB-A2.

@haukepetersen
Copy link
Contributor

#3530 should fix everything for the msba2

@OlegHahm
Copy link
Member

OlegHahm commented Aug 5, 2015

Ok, works on Z1 and pba-d-01-kw2x. Let's wait for #3530 then.

@haukepetersen
Copy link
Contributor

reviewing and testing it as we speak

@kaspar030
Copy link
Contributor Author

breaks msb430.

@haukepetersen
Copy link
Contributor

Will look into the msb430 anyway, so I try to deal with it.

@kaspar030
Copy link
Contributor Author

  • rebased

@OlegHahm OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 10, 2015
@kaspar030 kaspar030 mentioned this pull request Aug 11, 2015
Closed
@haukepetersen
Copy link
Contributor

MSP430 periph driver are under way, hope we can merge this this week...

@OlegHahm OlegHahm mentioned this pull request Aug 14, 2015
Closed
This was referenced Aug 21, 2015
Closed
Merged
@cgundogan
Copy link
Member

needs a rebase, because of the ng_=> gnrc_ renaming

@kaspar030
Copy link
Contributor Author

  • rebased

@PeterKietzmann PeterKietzmann mentioned this pull request Aug 27, 2015
Merged
10 tasks
@kaspar030
Copy link
Contributor Author

-rebased

@kaspar030 kaspar030 added this to the Release 2015.08 milestone Sep 2, 2015
@kaspar030 kaspar030 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 3, 2015
@kaspar030
Copy link
Contributor Author

all dependencies are in!

  • rebased

@haukepetersen
Copy link
Contributor

code looks good, works on every board I tested (i.e msb-430h, nucleo-l1, arduino-due, avsextrem, msba2, yunjia-nrf) -> ACK

@haukepetersen
Copy link
Contributor

just for fun, also successfully tested for the frdm-k64f and the pba-d-01-kw2x

@OlegHahm
Copy link
Member

OlegHahm commented Sep 3, 2015

ACK

@haukepetersen
Copy link
Contributor

and go!

haukepetersen added a commit that referenced this pull request Sep 3, 2015
@haukepetersen
Merge pull request #3402 from kaspar030/simplify_shell
076e9db 
sys: simplify shell
@haukepetersen haukepetersen merged commit 076e9db into RIOT-OS:master Sep 3, 2015
@OlegHahm
Copy link
Member

OlegHahm commented Sep 3, 2015

Nice work, @kaspar030!

@kaspar030
Copy link
Contributor Author

Thanks, and thanks for reviewing!

@kaspar030 kaspar030 deleted the simplify_shell branch September 3, 2015 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@OlegHahm OlegHahm

Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2015.09
Development

Successfully merging this pull request may close these issues.
4 participants
@kaspar030 @haukepetersen @OlegHahm @cgundogan