pkg/ubasic: add support for BASIC's "shell" statement. by jcarrano · Pull Request #11349 · RIOT-OS/RIOT

jcarrano · 2019-04-05T16:18:12Z

Contribution description

A joke can go wrong when people don't understand it, and it can wronger if they take it seriously...

This commit is based on #11319 and adds support for the "shell" statement. In BASIC, this is supposed to call a system command:

10 shell "ls $HOME"

I used "system()" to implement it. For more information regarding shell in other BASIC dialects, see:

FreeBASIC: https://freebasic.net/wiki/wikka.php?wakka=KeyPgShell
Gambas: http://gambaswiki.org/wiki/lang/shell
Other: https://rosettacode.org/wiki/Execute_a_system_command#BASIC

Testing procedure

Run the test. Test case no.3 takes ages in a board (I tested samr21) so you can use the following patch to disable it (place it under pkg/ubasic/patches).
0006-remove-me-relax-test-2.patch

From 83837232900bbdd2e8827046a428ecdb0655eb3c Mon Sep 17 00:00:00 2001
From: Juan Carrano <j.carrano@fu-berlin.de>
Date: Fri, 5 Apr 2019 16:29:48 +0200
Subject: [PATCH 6/6] remove me - relax test 2

---
 tests.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests.c b/tests.c
index b4ac79a..7c0575a 100644
--- a/tests.c
+++ b/tests.c
@@ -48,8 +48,8 @@ static const char program_goto[] =
 70 end\n";
 
 static const char program_loop[] =
-"10 for i = 0 to 126\n\
-20 for j = 0 to 126\n\
+"10 for i = 0 to 1\n\
+20 for j = 0 to 1\n\
 30 for k = 0 to 10\n\
 40 let a = i * j * k\n\
 rem 45 print a, i, j, k\n\
@@ -128,7 +128,7 @@ main(void)
   assert(ubasic_get_variable(2) == 108);
 
   run(program_loop);
-  assert(ubasic_get_variable(0) == (VARIABLE_TYPE)(126 * 126 * 10));
+   /* assert(ubasic_get_variable(0) == (VARIABLE_TYPE)(126 * 126 * 10)); */
 
   run(program_shell);
 
-- 
2.21.0

Side notes.

The modifications to ubasic were unexpectedly easy. I spent most of the time figuring out how to get system() running in RIOT. One of the biggest issues I had was that at some point I added a main.c to the test application and afterwards remove it, bit the main.o was still there messing up the symbols!!!

Issues/PRs references

Depends (and extends) #11319 .

Make procedure more reusable

Add ubasic_tests module

This is a quick and dirty solution to have something to test the system() standard function.

The system() function is standard in C99. It should invoke a system command. BUGS: this implementation ignores the return value.

"shell" invokes C's system() function wich in riot (if shell_commands is enabled) invokes a shell command.

miri64 · 2019-04-05T18:53:10Z

This commit is based on #11319 and adds support for the "shell" statement. In BASIC, this is supposed to call a system command.

Can you give a ref for that? "basic shell statement" is a bit frustrating to google...

jcarrano · 2019-04-08T09:24:37Z

Can you give a ref for that?

Well, to be honest, I'm not very sure of what I did. There are many BASIC dialects (uBASIC being one of them), but several seem to have a shell command that takes a single string as an argument and executes it in a sub-process using the system shell.

jcarrano · 2019-04-08T09:25:41Z

I updated the description with reference to other dialects' shell.

miri64 · 2019-04-08T09:45:55Z

I updated the description with reference to other dialects' shell.

Thank you!

miri64

There are some dependency issues in this PR, which we should rather discuss seperately.

miri64 · 2019-04-08T09:46:53Z

sys/shell/commands/sc_sys.c

    return 0;
 }
+
+int _echo_handler(int argc, char **argv)


This belongs into its own PR.

Also, I noticed today, that this somewhat clashes with a task in our tutorials

miri64 · 2019-04-08T09:48:42Z

sys/shell/shell.c

+
+#ifdef MODULE_SHELL_COMMANDS
+/* Provide standard system() function */
+int system(const char *command)


This as well.

Also I think this rather belongs into the POSIX module and shell_commands shouldn't be a dependency.

This should have its own PR. I would not put it under POSIX, since system() is C99/C89 and placing it is the easiest way to make sure system() is tied to shell_commands.

since system() is C99/C89 and placing it is the easiest way to make sure system() is tied to shell_commands.

You can have shell commands without having the module shell_commands (which provides a number of default shell commands), so I don't understand the requirement for shell_commands.

I you don't have shell_commands, then you don't have access to any command from within system(), Because you have no way for your application to communicate a custom command list:

RIOT/sys/shell/shell.c

Lines 54 to 62 in 200c465

static shell_command_handler_t find_handler(const shell_command_t *command_list, char *command)

{

const shell_command_t *command_lists[] = {

command_list,

#ifdef MODULE_SHELL_COMMANDS

_shell_command_list,

#endif

};

Nothing will break, but no commands will be available.

In 2038, when we finally get XFAs, this won't be an issue (if we survive the UNIX timestamp overflow)

I you don't have shell_commands, then you don't have access to any command from within system()

Well, that seems to be more of an issue of the current implementation. So I see it as premature to establish a dependency that in reality should not be there (I would expect e.g. system("udp …") to work if I call it with gnrc_networking.

In 2038, when we finally get XFAs, this won't be an issue (if we survive the UNIX timestamp overflow)

ztimer will handle the latter. ;-)

jcarrano · 2019-04-08T15:42:30Z

sys/shell/shell.c

+
+    strncpy(tmpstring, command, len+1);
+
+    handle_input_line(_shell_command_list, tmpstring);


I should have used NULL here, since the _shell_command_list is going to be searched anyways.

Suggested change

handle_input_line(_shell_command_list, tmpstring);

handle_input_line(NULL, tmpstring);

jcarrano · 2019-04-08T15:47:46Z

sys/shell/shell.c

+
+#ifdef MODULE_SHELL_COMMANDS
+/* Provide standard system() function */
+int system(const char *command)


I you don't have shell_commands, then you don't have access to any command from within system(), Because you have no way for your application to communicate a custom command list:

RIOT/sys/shell/shell.c

Lines 54 to 62 in 200c465

static shell_command_handler_t find_handler(const shell_command_t *command_list, char *command)

{

const shell_command_t *command_lists[] = {

command_list,

#ifdef MODULE_SHELL_COMMANDS

_shell_command_list,

#endif

};

Nothing will break, but no commands will be available.

In 2038, when we finally get XFAs, this won't be an issue (if we survive the UNIX timestamp overflow)

miri64 · 2019-04-09T07:09:17Z

Needs rebase and split-up.

jcarrano · 2019-04-24T09:10:15Z

@miri64 Maybe we can leave the echo commit out (i.e. not even PR it). The only reason I put it there was to be able to have at least one command to test (otherwise the only command available by default is "reboot").

The thing is, because of the way the shell works it not easy or practical to make the application specific commands available to system(). And because of how the shell is compiled, there is no [easy] way to add custom system commands without going into the shell module.

The result of all this is that it makes testing this feature harder.

miri64 · 2019-07-30T14:23:54Z

Couldn't the echo command be included via a pseudo-module? This way it is a "system" command, but still optional.

jcarrano · 2019-07-30T15:00:47Z

@miri64 yes, that goes along the lines of what I was proposing in #9258, which is to allow choosing which system functionality (I will try to avoid calling them "system calls" because in RIOT they are not) are provided and to allow choosing an implementation.

The first question, then, is: do we want 'system()'. I think yes because it is in the standard and because it is useful.

miri64 · 2019-07-30T15:03:47Z

The first question, then, is: do we want 'system()'. I think yes because it is in the standard and because it is useful.

I'm not against it.

jcarrano · 2019-07-30T15:17:30Z

I'm not against it.

Then I'll make a PR just for that.

stale · 2020-01-31T16:05:48Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

miri64 and others added 14 commits April 1, 2019 09:00

pkg: Initial introduction of ubasic package

637c6a3

tests: provide tests for ubasic package

675d27d

fixup! tests: provide tests for ubasic package

f915d93

fixup! tests: provide tests for ubasic package

2f62fa1

fixup! fixup! tests: provide tests for ubasic package

8bf59c4

Make procedure more reusable

fixup! fixup! fixup! tests: provide tests for ubasic package

590c987

fixup! tests: provide tests for ubasic package

eb0dc03

fixup! pkg: Initial introduction of ubasic package

318709b

Add ubasic_tests module

fixup! tests: provide tests for ubasic package

700a739

fixup! fixup! pkg: Initial introduction of ubasic package

2972ef9

fixup! fixup! tests: provide tests for ubasic package

0e1816a

sys/shell: Add echo to the default system commands.

e8646b5

This is a quick and dirty solution to have something to test the system() standard function.

sys/shell: Make the shell provide the system() function.

81bf703

The system() function is standard in C99. It should invoke a system command. BUGS: this implementation ignores the return value.

pkg/ubasic: add support for the "shell" BASIC statement.

de2ba62

"shell" invokes C's system() function wich in riot (if shell_commands is enabled) invokes a shell command.

jcarrano added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pkg Area: External package ports State: waiting for other PR State: The PR requires another PR to be merged first labels Apr 5, 2019

jcarrano requested a review from miri64 April 5, 2019 16:18

fixup! update test script.

200c465

miri64 requested changes Apr 8, 2019

View reviewed changes

jcarrano commented Apr 8, 2019

View reviewed changes

miri64 self-assigned this Jul 30, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jan 31, 2020

stale bot closed this Mar 2, 2020

	static shell_command_handler_t find_handler(const shell_command_t command_list, char command)
	{
	const shell_command_t *command_lists[] = {
	command_list,
	#ifdef MODULE_SHELL_COMMANDS
	_shell_command_list,
	#endif
	};


		strncpy(tmpstring, command, len+1);

		handle_input_line(_shell_command_list, tmpstring);

	handle_input_line(_shell_command_list, tmpstring);
	handle_input_line(NULL, tmpstring);

Conversation

jcarrano commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Side notes.

Issues/PRs references

Uh oh!

miri64 commented Apr 5, 2019

Uh oh!

jcarrano commented Apr 8, 2019

Uh oh!

jcarrano commented Apr 8, 2019

Uh oh!

miri64 commented Apr 8, 2019 via email

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Apr 9, 2019

Uh oh!

jcarrano commented Apr 24, 2019

Uh oh!

miri64 commented Jul 30, 2019

Uh oh!

jcarrano commented Jul 30, 2019

Uh oh!

miri64 commented Jul 30, 2019

Uh oh!

jcarrano commented Jul 30, 2019

Uh oh!

stale bot commented Jan 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jcarrano commented Apr 5, 2019 •

edited

Loading