[U-Boot] [PATCH 0/4] Improve/fix tests for commands and trace
A recent bug reported showed that the built-in CLI was not correctly handling repeatable commands. A recent series fixes this.
http://patchwork.ozlabs.org/patch/356568/ http://patchwork.ozlabs.org/patch/356569/ http://patchwork.ozlabs.org/patch/356507/
Add a simple test for this. At present we require sandbox's config to be changed to use the hush or the simple parser. In the fullness of time when the parser code allows both to be compiled in, we may be able to adjust the test to do both tests.
In order to make this work, we need to skip Ctrl-C in sandbox, so add a CONFIG option and environment variable to deal with this.
The trace test is broken due a change done as part of the Kbuild work, so rename that also.
Simon Glass (4): Reactivate the tracing feature Add ctrlc_ignore environment variable to ignore Ctrl-C test: Remove tabs from trace test test: Add a test for command repeat
README | 3 +++ common/console.c | 28 ++++++++++++++++++++++------ config.mk | 4 ++++ include/configs/sandbox.h | 1 + include/env_callback.h | 7 +++++++ test/cmd_repeat.sh | 29 +++++++++++++++++++++++++++++ test/common.sh | 20 ++++++++++++++++++++ test/trace/test-trace.sh | 42 ++++++++++++++---------------------------- 8 files changed, 100 insertions(+), 34 deletions(-) create mode 100755 test/cmd_repeat.sh create mode 100644 test/common.sh
This was lost sometime in the Kbuild conversion. Add it back.
Check that the trace test now passes:
$ ./test/trace/test-trace.sh Simple trace test / sanity check using sandbox
/tmp/filemHKPGw Build sandbox O=sandbox FTRACE=1 GEN /home/sjg/c/src/third_party/u-boot/files/sandbox/Makefile Configuring for sandbox board... Check results Test passed
Signed-off-by: Simon Glass sjg@chromium.org ---
config.mk | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/config.mk b/config.mk index 05864aa..0c45c09 100644 --- a/config.mk +++ b/config.mk @@ -46,6 +46,10 @@ ifdef BOARD sinclude $(srctree)/board/$(BOARDDIR)/config.mk # include board specific rules endif
+ifdef FTRACE +PLATFORM_CPPFLAGS += -finstrument-functions -DFTRACE +endif + #########################################################################
RELFLAGS := $(PLATFORM_RELFLAGS)
Hi Simon,
On Thu, 5 Jun 2014 12:27:49 -0600 Simon Glass sjg@chromium.org wrote:
This was lost sometime in the Kbuild conversion. Add it back.
Not lost. It was moved to examples/Makefile.
Prior to Kbuild conversion, config.mk was like this:
------------------>8---------------------- BCURDIR = $(subst $(SRCTREE)/,,$(CURDIR:$(obj)%=%))
ifeq ($(findstring examples/,$(BCURDIR)),) ifeq ($(CONFIG_SPL_BUILD),) ifdef FTRACE CFLAGS += -finstrument-functions -DFTRACE endif endif endif --------------------8<-------------------------
"-finstrument-functions -DFTRACE" was enabled only under examples/ directory. (Do you remember why?)
That's why I moved it to examples/Makefile to keep the equivalent behavior.
diff --git a/config.mk b/config.mk index 05864aa..0c45c09 100644 --- a/config.mk +++ b/config.mk @@ -46,6 +46,10 @@ ifdef BOARD sinclude $(srctree)/board/$(BOARDDIR)/config.mk # include board specific rules endif
+ifdef FTRACE +PLATFORM_CPPFLAGS += -finstrument-functions -DFTRACE +endif
#########################################################################
RELFLAGS := $(PLATFORM_RELFLAGS)
OK. If you want to enable this flag over the whole source tree, please remove it from examples/Makefile.
BTW, In the description of commit 5c2aeac5ae, you mentioned tracing feature is not supported by SPL.
But you are enabling FTRACE also on SPL in this patch. Are you sure there is no bad impact on SPL?
Best Regards Masahiro Yamada
Hi Masahiro,
On 10 June 2014 00:47, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
Hi Simon,
On Thu, 5 Jun 2014 12:27:49 -0600 Simon Glass sjg@chromium.org wrote:
This was lost sometime in the Kbuild conversion. Add it back.
Not lost. It was moved to examples/Makefile.
Prior to Kbuild conversion, config.mk was like this:
------------------>8---------------------- BCURDIR = $(subst $(SRCTREE)/,,$(CURDIR:$(obj)%=%))
ifeq ($(findstring examples/,$(BCURDIR)),) ifeq ($(CONFIG_SPL_BUILD),) ifdef FTRACE CFLAGS += -finstrument-functions -DFTRACE endif endif endif --------------------8<-------------------------
"-finstrument-functions -DFTRACE" was enabled only under examples/ directory. (Do you remember why?)
That's why I moved it to examples/Makefile to keep the equivalent behavior.
I don't think it is the same. In my code I was trying to make sure there was NO tracing in example directory, and SPL. I think I should go the same way, so will update my patch.
diff --git a/config.mk b/config.mk index 05864aa..0c45c09 100644 --- a/config.mk +++ b/config.mk @@ -46,6 +46,10 @@ ifdef BOARD sinclude $(srctree)/board/$(BOARDDIR)/config.mk # include board specific rules endif
+ifdef FTRACE +PLATFORM_CPPFLAGS += -finstrument-functions -DFTRACE +endif
#########################################################################
RELFLAGS := $(PLATFORM_RELFLAGS)
OK. If you want to enable this flag over the whole source tree, please remove it from examples/Makefile.
OK
BTW, In the description of commit 5c2aeac5ae, you mentioned tracing feature is not supported by SPL.
But you are enabling FTRACE also on SPL in this patch. Are you sure there is no bad impact on SPL?
Yes I should remove this otherwise it will at best bloat the code for SPL. I think it is probably best just to revert that part of the Makefile.
Thanks for looking at this, saves me another patch...
Regards, Simon
Hi Masahiro,
On 11 June 2014 23:42, Simon Glass sjg@chromium.org wrote:
Hi Masahiro,
Yes I should remove this otherwise it will at best bloat the code for SPL. I think it is probably best just to revert that part of the Makefile.
Although actually I'm not sure how to have different flags for everything except SPL and examples/ - any clues?
Regards, Simon
Hi Simon,
On Wed, 11 Jun 2014 23:50:48 -0400 Simon Glass sjg@chromium.org wrote:
Hi Masahiro,
On 11 June 2014 23:42, Simon Glass sjg@chromium.org wrote:
Hi Masahiro,
Yes I should remove this otherwise it will at best bloat the code for SPL. I think it is probably best just to revert that part of the Makefile.
Although actually I'm not sure how to have different flags for everything except SPL and examples/ - any clues?
Me neither.
But one possible solution is:
Add the CONFIG_SPL_BUILD conditional to config.mk to disable it for SPL.
ifeq ($(CONFIG_SPL_BUILD),) ifdef FTRACE CFLAGS += -finstrument-functions -DFTRACE endif endif
exmpales/ seems having troubles with FTRACE. So, disable it when FTRACE=1 is defined.
examples/Makefile is like this:
ifndef FTRACE ifndef CONFIG_SANDBOX subdir-y += standalone subdir-$(CONFIG_API) += api endif endif
BTW, can FTRACE build for all boards?
I checked out v2014.01 (pre-kbuild)
- make sandbox_config; make FTRACE=1 - make snow_config; make CROSS_COMPILE=arm-linux-gnueabi- FRACE=1
succeeded. But,
- make omap3_beagle_config; make CROSS_COMPILE=arm-linux-gnueabi- FRACE=1
faied with lots of undefined reference errors to __cyg_profile_func_enter/_exit.
I don't know why.
Best Regards Masahiro Yamada
Hi Masahiro,
On 12 June 2014 20:56, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
Hi Simon,
On Wed, 11 Jun 2014 23:50:48 -0400 Simon Glass sjg@chromium.org wrote:
Hi Masahiro,
On 11 June 2014 23:42, Simon Glass sjg@chromium.org wrote:
Hi Masahiro,
Yes I should remove this otherwise it will at best bloat the code for SPL. I think it is probably best just to revert that part of the Makefile.
Although actually I'm not sure how to have different flags for everything except SPL and examples/ - any clues?
Me neither.
But one possible solution is:
[snip]
That seems to work for me, thank you. Please don't worry about breaking it. If I was worried about the tests I would have come up with a way of automatically running them all by now. U-Boot needs a 'make test' but the test coverage is still pretty poor, so it is not high on my priority list.
BTW, can FTRACE build for all boards?
I checked out v2014.01 (pre-kbuild)
- make sandbox_config; make FTRACE=1
- make snow_config; make CROSS_COMPILE=arm-linux-gnueabi- FRACE=1
succeeded. But,
- make omap3_beagle_config; make CROSS_COMPILE=arm-linux-gnueabi- FRACE=1
faied with lots of undefined reference errors to __cyg_profile_func_enter/_exit.
I don't know why.
You need to enable it - see exynos5-dt.h which has:
/* Allow tracing to be enabled */ #define CONFIG_TRACE #define CONFIG_CMD_TRACE #define CONFIG_TRACE_BUFFER_SIZE (16 << 20) #define CONFIG_TRACE_EARLY_SIZE (8 << 20) #define CONFIG_TRACE_EARLY #define CONFIG_TRACE_EARLY_ADDR 0x50000000
It would be nice to automate these settings, but for now you must add these settings for the board you are using. Actually I think it would be easy except for the 'early trace' feature, which needs to have a pre-relocation memory area to use.
Regards, Simon
Hi Simon
On Wed, 11 Jun 2014 23:42:25 -0400 Simon Glass sjg@chromium.org wrote:
Hi Masahiro,
On 10 June 2014 00:47, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
Hi Simon,
On Thu, 5 Jun 2014 12:27:49 -0600 Simon Glass sjg@chromium.org wrote:
This was lost sometime in the Kbuild conversion. Add it back.
Not lost. It was moved to examples/Makefile.
Prior to Kbuild conversion, config.mk was like this:
------------------>8---------------------- BCURDIR = $(subst $(SRCTREE)/,,$(CURDIR:$(obj)%=%))
ifeq ($(findstring examples/,$(BCURDIR)),) ifeq ($(CONFIG_SPL_BUILD),) ifdef FTRACE CFLAGS += -finstrument-functions -DFTRACE endif endif endif --------------------8<-------------------------
"-finstrument-functions -DFTRACE" was enabled only under examples/ directory. (Do you remember why?)
That's why I moved it to examples/Makefile to keep the equivalent behavior.
I don't think it is the same. In my code I was trying to make sure there was NO tracing in example directory, and SPL. I think I should go the same way, so will update my patch.
Oops. I totally screwed up. So sorry. I'm crazy.
"-finstrument-functions -DFTRACE" must be __disabled__ under examples/ and spl/.
I broke FTRACE feature. My apologies.
Best Regards Masahiro Yamada
Sometimes it is useful to ignore Ctrl-C, because checking for it causes the CLI to drop characters. In particular for tests involving sandbox, where input commands are piped in, some commands will call ctrlc() which will drop characters from the test script.
Add a CONFIG_SYS_CTRLC_IGNORE option which enables this variable. If the variable is present (e.g. "setenv ctrlc_ignore ignore") then no checking for Ctrl-C will be performed.
Signed-off-by: Simon Glass sjg@chromium.org ---
README | 3 +++ common/console.c | 28 ++++++++++++++++++++++------ include/configs/sandbox.h | 1 + include/env_callback.h | 7 +++++++ 4 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/README b/README index 6edb2e7..e25780e 100644 --- a/README +++ b/README @@ -3584,6 +3584,9 @@ Configuration Settings: - CONFIG_SYS_PROMPT: This is what U-Boot prints on the console to prompt for user input.
+- CONFIG_SYS_CTRLC_IGNORE: If enabled, then the console will only check for + Ctrl-C if the 'ctrlc_ignore" environment variable is unset. + - CONFIG_SYS_CBSIZE: Buffer size for input from the Console
- CONFIG_SYS_PBSIZE: Buffer size for Console output diff --git a/common/console.c b/common/console.c index 5453726..607b96f 100644 --- a/common/console.c +++ b/common/console.c @@ -520,15 +520,31 @@ int vprintf(const char *fmt, va_list args) }
/* test if ctrl-c was pressed */ -static int ctrlc_disabled = 0; /* see disable_ctrl() */ -static int ctrlc_was_pressed = 0; +static bool ctrlc_ignore; /* ctrlc detection always disabled */ +static bool ctrlc_disabled; /* see disable_ctrl() */ +static bool ctrlc_was_pressed; + +#ifdef CONFIG_SYS_CTRLC_IGNORE +static int on_ctrlc_ignore(const char *name, const char *value, + enum env_op op, int flags) +{ + if (value != NULL) + ctrlc_ignore = true; + else + ctrlc_ignore = false; + + return 0; +} +U_BOOT_ENV_CALLBACK(ctrlc_ignore, on_ctrlc_ignore); +#endif /* CONFIG_SYS_CTRLC_IGNORE */ + int ctrlc(void) { - if (!ctrlc_disabled && gd->have_console) { + if (!ctrlc_ignore && !ctrlc_disabled && gd->have_console) { if (tstc()) { switch (getc()) { case 0x03: /* ^C - Control C */ - ctrlc_was_pressed = 1; + ctrlc_was_pressed = true; return 1; default: break; @@ -571,7 +587,7 @@ int disable_ctrlc(int disable) { int prev = ctrlc_disabled; /* save previous state */
- ctrlc_disabled = disable; + ctrlc_disabled = disable != 0; return prev; }
@@ -582,7 +598,7 @@ int had_ctrlc (void)
void clear_ctrlc(void) { - ctrlc_was_pressed = 0; + ctrlc_was_pressed = false; }
#ifdef CONFIG_MODEM_SUPPORT_DEBUG diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 6bb2546..36e92ff 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -54,6 +54,7 @@ #define CONFIG_CMD_FS_GENERIC
#define CONFIG_SYS_VSNPRINTF +#define CONFIG_SYS_CTRLC_IGNORE
#define CONFIG_CMD_GPIO #define CONFIG_SANDBOX_GPIO diff --git a/include/env_callback.h b/include/env_callback.h index f90a7fa..cdd6b1e 100644 --- a/include/env_callback.h +++ b/include/env_callback.h @@ -31,6 +31,12 @@ #define SPLASHIMAGE_CALLBACK #endif
+#ifdef CONFIG_SYS_CTRLC_IGNORE +#define CTRLC_IGNORE "ctrlc_ignore:ctrlc_ignore," +#else +#define CTRLC_IGNORE +#endif + /* * This list of callback bindings is static, but may be overridden by defining * a new association in the ".callbacks" environment variable. @@ -40,6 +46,7 @@ "baudrate:baudrate," \ "bootfile:bootfile," \ "loadaddr:loadaddr," \ + CTRLC_IGNORE \ SILENT_CALLBACK \ SPLASHIMAGE_CALLBACK \ "stdin:console,stdout:console,stderr:console," \
Dear Simon Glass,
In message 1401992872-31985-3-git-send-email-sjg@chromium.org you wrote:
Sometimes it is useful to ignore Ctrl-C, because checking for it causes the CLI to drop characters. In particular for tests involving sandbox, where input commands are piped in, some commands will call ctrlc() which will drop characters from the test script.
Why would that be the case?
If this happens, I consider it a bug that should be fixed, and not papered over.
Add a CONFIG_SYS_CTRLC_IGNORE option which enables this variable. If the variable is present (e.g. "setenv ctrlc_ignore ignore") then no checking for Ctrl-C will be performed.
I dislike this idea. It looks wrong to me. Can we not fix the problem at the root cause?
Best regards,
Wolfgang Denk
Hi Wolfgang,
On 5 June 2014 15:43, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message 1401992872-31985-3-git-send-email-sjg@chromium.org you wrote:
Sometimes it is useful to ignore Ctrl-C, because checking for it causes the CLI to drop characters. In particular for tests involving sandbox, where input commands are piped in, some commands will call ctrlc() which will drop characters from the test script.
Why would that be the case?
If this happens, I consider it a bug that should be fixed, and not papered over.
If you look at the code for the 'md' command it calls ctrlc() every now and then. Each call results in a getc() which reads a character from the console. So we lose characters.
Add a CONFIG_SYS_CTRLC_IGNORE option which enables this variable. If the variable is present (e.g. "setenv ctrlc_ignore ignore") then no checking for Ctrl-C will be performed.
I dislike this idea. It looks wrong to me. Can we not fix the problem at the root cause?
I certainly thought about this. I even though maybe we might change the serial module to scan ahead and buffer characters, in case there is a Ctrl-C in the future. But that itself seems like something for the future.
Regards, Simon
Dear Simon,
In message CAPnjgZ15jrh=-tEj-0tFjwBSZyq+0BihKJLEjG3d43wQOowMFg@mail.gmail.com you wrote:
If this happens, I consider it a bug that should be fixed, and not papered over.
If you look at the code for the 'md' command it calls ctrlc() every now and then. Each call results in a getc() which reads a character from the console. So we lose characters.
Well, as I mentioned: it looks like a bug that needs fixing.
I dislike this idea. It looks wrong to me. Can we not fix the problem at the root cause?
I certainly thought about this. I even though maybe we might change the serial module to scan ahead and buffer characters, in case there is a Ctrl-C in the future. But that itself seems like something for the future.
well, I see two sides of this:
1) Looking at sandbox only, we should provide an implementation of ctrlc() that does not consume characters from stdin - I thinkwe should rather be able to react on signals?
2) With a general point of view, consuming characters for no good is always wrong and needs to be fixed.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On 6 June 2014 16:10, Wolfgang Denk wd@denx.de wrote:
Dear Simon,
In message CAPnjgZ15jrh=-tEj-0tFjwBSZyq+0BihKJLEjG3d43wQOowMFg@mail.gmail.com you wrote:
If this happens, I consider it a bug that should be fixed, and not papered over.
If you look at the code for the 'md' command it calls ctrlc() every now and then. Each call results in a getc() which reads a character from the console. So we lose characters.
Well, as I mentioned: it looks like a bug that needs fixing.
I dislike this idea. It looks wrong to me. Can we not fix the problem at the root cause?
I certainly thought about this. I even though maybe we might change the serial module to scan ahead and buffer characters, in case there is a Ctrl-C in the future. But that itself seems like something for the future.
well, I see two sides of this:
- Looking at sandbox only, we should provide an implementation of ctrlc() that does not consume characters from stdin - I thinkwe should rather be able to react on signals?
Currently there is no signal handling, but we set up the terminal so that Cltr-C terminates U-Boot (default) or does not (and it appears as a character in the input). It's probably a bit more complicated but it should be doable. We can probably just ignore Cltl-C on sandbox for now.
- With a general point of view, consuming characters for no good is always wrong and needs to be fixed.
I'm not sure if you recall the serial driver buffer patch I sent for ns16550 which corrected this problem and also dealt with serial input overflow while outputting to the LCD. We might consider resurrecting this and doing it at a higher level. Without buffering I don't see any way to fix this.
Regards, Simon
Dear Simon,
In message CAPnjgZ2BC0MD6NyoOJRmhYeabm3EYMBs8nQA3hcyD71BB+jqDw@mail.gmail.com you wrote:
- Looking at sandbox only, we should provide an implementation of ctrlc() that does not consume characters from stdin - I thinkwe should rather be able to react on signals?
Currently there is no signal handling, but we set up the terminal so that Cltr-C terminates U-Boot (default) or does not (and it appears as a character in the input). It's probably a bit more complicated but it should be doable. We can probably just ignore Cltl-C on sandbox for now.
Hm... ignoring it would mean there is no way to interrupt long running commands. I'm not sure if this is actually an improvement. Eventually we should try to define the wanted behaviour first. My initial feelingis that ^C should terminate a running command nd return us to the shell, but not terminate U-Boot. Outside of sandbox, the only regular way to terminate U-Boot is the "reboot" command. Maybe we should do the same in sandbox?
- With a general point of view, consuming characters for no good is always wrong and needs to be fixed.
I'm not sure if you recall the serial driver buffer patch I sent for
I'm afraid I don't.
ns16550 which corrected this problem and also dealt with serial input overflow while outputting to the LCD. We might consider resurrecting this and doing it at a higher level. Without buffering I don't see any way to fix this.
The 16550 is on the high end side in terms of capabilities. I don;t thinkwe should "buffer" more than a single character - otherwise it would be just a tiny step to implementing thingslike line disciplines and stuff, and I don;t think we need nor want this.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On 10 June 2014 01:08, Wolfgang Denk wd@denx.de wrote:
Dear Simon,
In message CAPnjgZ2BC0MD6NyoOJRmhYeabm3EYMBs8nQA3hcyD71BB+jqDw@mail.gmail.com you wrote:
- Looking at sandbox only, we should provide an implementation of ctrlc() that does not consume characters from stdin - I thinkwe should rather be able to react on signals?
Currently there is no signal handling, but we set up the terminal so that Cltr-C terminates U-Boot (default) or does not (and it appears as a character in the input). It's probably a bit more complicated but it should be doable. We can probably just ignore Cltl-C on sandbox for now.
Hm... ignoring it would mean there is no way to interrupt long running commands. I'm not sure if this is actually an improvement. Eventually we should try to define the wanted behaviour first. My initial feelingis that ^C should terminate a running command nd return us to the shell, but not terminate U-Boot. Outside of sandbox, the only regular way to terminate U-Boot is the "reboot" command. Maybe we should do the same in sandbox?
It is very convenient to terminate U-Boot with Ctrl-C - it makes it work like a 'normal' program, and you can still terminate a long-running command - it just quits U-Boot just like any other command-line utility. When quickly running tests it is helpful. Also it is less confusing I think for people who don't know how to exit.
You can use '-t raw' to get the behaviour you want. Is that good enough?
U-Boot sandbox does not yet support 'reboot', but 'reset' does quit U-Boot.
- With a general point of view, consuming characters for no good is always wrong and needs to be fixed.
I'm not sure if you recall the serial driver buffer patch I sent for
I'm afraid I don't.
Actually I think I was thinking of Scott Wood's patch:
http://patchwork.ozlabs.org/patch/90066/
ns16550 which corrected this problem and also dealt with serial input overflow while outputting to the LCD. We might consider resurrecting this and doing it at a higher level. Without buffering I don't see any way to fix this.
The 16550 is on the high end side in terms of capabilities. I don;t thinkwe should "buffer" more than a single character - otherwise it would be just a tiny step to implementing thingslike line disciplines and stuff, and I don;t think we need nor want this.
Yes I wanted to avoid that also. I guess we are left with signal handling as the solution. But for now I might just disable Ctrl-C for sandbox unless the 'raw' terminal is used. That will allow the tests to work correctly at least.
Regards, Simon
Dear Simon,
In message CAPnjgZ1HZ5Eh8b15sCgYKrmCqkbT5UBcZWPf6zDvqrebzS2N=A@mail.gmail.com you wrote:
Hm... ignoring it would mean there is no way to interrupt long running commands. I'm not sure if this is actually an improvement. Eventually we should try to define the wanted behaviour first. My initial feelingis that ^C should terminate a running command nd return us to the shell, but not terminate U-Boot. Outside of sandbox, the only regular way to terminate U-Boot is the "reboot" command. Maybe we should do the same in sandbox?
It is very convenient to terminate U-Boot with Ctrl-C - it makes it work like a 'normal' program, and you can still terminate a long-running command - it just quits U-Boot just like any other command-line utility. When quickly running tests it is helpful. Also it is less confusing I think for people who don't know how to exit.
But that's the point: U-Boot with it's CLI is NOT "a 'normal' program". It's an interactive tool like a shell or an editor. When you run a shell (say, bash) as CLI then you also expect that ^C will only terminate the currently running command, and not exit the shell.
You can use '-t raw' to get the behaviour you want. Is that good enough?
This should be the default, I think.
U-Boot sandbox does not yet support 'reboot', but 'reset' does quit U-Boot.
Ah, yes. Typo. I meant "reset", of course.
I'm not sure if you recall the serial driver buffer patch I sent for
I'm afraid I don't.
Actually I think I was thinking of Scott Wood's patch:
Ah, this one. Well, frankly, I don't lioke that for a number of reasons:
- We have a ton of different UART drivers.Any such implementation should be general enough to be usable on more than one type, ideally completely hardware independent. - This buffering of data in this patch is intended to solve a specific problem that could be avoided by more clever test scripts. Instead of just dumping an endless stream of characters to the serial console independent of what U-Boot is doing, one should always wit for the next CLI prompt before sending the next command. Tools like "expect" can do this easily. - We have to decide what we want. Either we define the serial input system of U-Boot as intentionally simple, allowing it to work with minimal resources (like very, very early after reset, long before relocation to RAM, i. e. without much space on the stack, without writable data segment, without .bss). Or we want a feature-rich, powerful input system with maximum data throuhput, buffering, type ahead, line disciplines, etc. The current implementation is clearly following the former design ideas, and I think this is OK so. The second method is indeed more powerful, but quickly caklls for additional complexity to implement properly - say, interrupt support for the UART drivers, which means we enter a whole new leel of complexity. The current implementation is clearly following the former design ideas, and I think this is OK so. The second method is indeed more powerful, but quickly caklls for additional complexity to implement properly - say, interrupt support for the UART drivers, which means we enter a whole new leel of complexity.
Yes I wanted to avoid that also. I guess we are left with signal handling as the solution. But for now I might just disable Ctrl-C for sandbox unless the 'raw' terminal is used. That will allow the tests to work correctly at least.
As mentioned, I think the default behaviour should be different.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On 12 June 2014 01:03, Wolfgang Denk wd@denx.de wrote:
Dear Simon,
In message CAPnjgZ1HZ5Eh8b15sCgYKrmCqkbT5UBcZWPf6zDvqrebzS2N=A@mail.gmail.com you wrote:
Hm... ignoring it would mean there is no way to interrupt long running commands. I'm not sure if this is actually an improvement. Eventually we should try to define the wanted behaviour first. My initial feelingis that ^C should terminate a running command nd return us to the shell, but not terminate U-Boot. Outside of sandbox, the only regular way to terminate U-Boot is the "reboot" command. Maybe we should do the same in sandbox?
It is very convenient to terminate U-Boot with Ctrl-C - it makes it work like a 'normal' program, and you can still terminate a long-running command - it just quits U-Boot just like any other command-line utility. When quickly running tests it is helpful. Also it is less confusing I think for people who don't know how to exit.
But that's the point: U-Boot with it's CLI is NOT "a 'normal' program". It's an interactive tool like a shell or an editor. When you run a shell (say, bash) as CLI then you also expect that ^C will only terminate the currently running command, and not exit the shell.
I agree that is more correct, although I must admit the current way is much easier to use IMO. And I run a lot of sandbox tests. But as they say, patches welcome :-) I suppose one solution would be to introduce a configuration file for sandbox, as with patman.
You can use '-t raw' to get the behaviour you want. Is that good enough?
This should be the default, I think.
U-Boot sandbox does not yet support 'reboot', but 'reset' does quit U-Boot.
Ah, yes. Typo. I meant "reset", of course.
OK, then this is fine.
I'm not sure if you recall the serial driver buffer patch I sent for
I'm afraid I don't.
Actually I think I was thinking of Scott Wood's patch:
Ah, this one. Well, frankly, I don't lioke that for a number of reasons:
- We have a ton of different UART drivers.Any such implementation should be general enough to be usable on more than one type, ideally completely hardware independent.
Yes, it could go at the console level.
- This buffering of data in this patch is intended to solve a specific problem that could be avoided by more clever test scripts. Instead of just dumping an endless stream of characters to the serial console independent of what U-Boot is doing, one should always wit for the next CLI prompt before sending the next command. Tools like "expect" can do this easily.
Agreed it could be done that way, but it is so much easier if U-Boot can behave in a simple way with input. We may end up with more complicated test scripts although I would prefer that we focus on unit tests a bit more.
- We have to decide what we want. Either we define the serial input system of U-Boot as intentionally simple, allowing it to work with minimal resources (like very, very early after reset, long before relocation to RAM, i. e. without much space on the stack, without writable data segment, without .bss). Or we want a feature-rich, powerful input system with maximum data throuhput, buffering, type ahead, line disciplines, etc. The current implementation is clearly following the former design ideas, and I think this is OK so. The second method is indeed more powerful, but quickly caklls for additional complexity to implement properly - say, interrupt support for the UART drivers, which means we enter a whole new leel of complexity. The current implementation is clearly following the former design ideas, and I think this is OK so. The second method is indeed more powerful, but quickly caklls for additional complexity to implement properly - say, interrupt support for the UART drivers, which means we enter a whole new leel of complexity.
Fair enough, I don't disagree with this at all. The use case for buffering is when you have the LCD running and it is quite slow to print characters. You then can't type quickly at the keyboard - U-Boot just gets behind. It happens on snow and seaboard when the caches are off.
Yes I wanted to avoid that also. I guess we are left with signal handling as the solution. But for now I might just disable Ctrl-C for sandbox unless the 'raw' terminal is used. That will allow the tests to work correctly at least.
As mentioned, I think the default behaviour should be different.
Understood.
Regards, Simon
Dear Simon,
In message CAPnjgZ3p20664FOc9vPaV=z3dEdv7T=mmLpwbSy8NjiFUP2A8w@mail.gmail.com you wrote:
- This buffering of data in this patch is intended to solve a specific problem that could be avoided by more clever test scripts. Instead of just dumping an endless stream of characters to the serial console independent of what U-Boot is doing, one should always wit for the next CLI prompt before sending the next command. Tools like "expect" can do this easily.
Agreed it could be done that way, but it is so much easier if U-Boot can behave in a simple way with input. We may end up with more complicated test scripts although I would prefer that we focus on unit tests a bit more.
As long as we don't support interrupts on all architectures and convert at least the UART drivers to use an interrupt driven mode we will always face the situation that U-Boot will bi strictly single- threaded and just does not read any input while doing something else, like running a command.
So all test scripts should consider this behaviour of the input interface. If they send more than one line of text to input, they should do it line by line, and always wait for the CLI prompt first.
Yes, this is inconvenient.
Fair enough, I don't disagree with this at all. The use case for buffering is when you have the LCD running and it is quite slow to print characters. You then can't type quickly at the keyboard - U-Boot just gets behind. It happens on snow and seaboard when the caches are off.
Heh. So the obvious solution is to turn the caches on :-)
No - I do understand what you want, and why, and I agree that it would be nice to have.
Best regards,
Wolfgang Denk
These cause U-Boot to print a list of available commands. It doesn't break the test, but it is best to remove them from the output.
Signed-off-by: Simon Glass sjg@chromium.org ---
test/trace/test-trace.sh | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/test/trace/test-trace.sh b/test/trace/test-trace.sh index aa02f09..973063c 100755 --- a/test/trace/test-trace.sh +++ b/test/trace/test-trace.sh @@ -27,17 +27,17 @@ build_uboot() { run_trace() { echo "Run trace" ./${OUTPUT_DIR}/u-boot <<END - trace stats - hash sha256 0 10000 - trace pause - trace stats - hash sha256 0 10000 - trace stats - trace resume - hash sha256 0 10000 - trace pause - trace stats - reset +trace stats +hash sha256 0 10000 +trace pause +trace stats +hash sha256 0 10000 +trace stats +trace resume +hash sha256 0 10000 +trace pause +trace stats +reset END }
This performs a command, then repeats it, and checks that the repeat happens.
Signed-off-by: Simon Glass sjg@chromium.org ---
test/cmd_repeat.sh | 29 +++++++++++++++++++++++++++++ test/common.sh | 20 ++++++++++++++++++++ test/trace/test-trace.sh | 20 +++----------------- 3 files changed, 52 insertions(+), 17 deletions(-) create mode 100755 test/cmd_repeat.sh create mode 100644 test/common.sh
diff --git a/test/cmd_repeat.sh b/test/cmd_repeat.sh new file mode 100755 index 0000000..990e799 --- /dev/null +++ b/test/cmd_repeat.sh @@ -0,0 +1,29 @@ +#!/bin/sh + +# Test for U-Boot cli including command repeat + +BASE="$(dirname $0)" +. $BASE/common.sh + +run_test() { + ./${OUTPUT_DIR}/u-boot <<END +setenv ctrlc_ignore y +md 0 + +reset +END +} +check_results() { + echo "Check results" + + grep -q 00000100 ${tmp} || fail "Command did not repeat" +} + +echo "Test CLI repeat" +echo +tmp="$(tempfile)" +build_uboot +run_test >${tmp} +check_results ${tmp} +rm ${tmp} +echo "Test passed" diff --git a/test/common.sh b/test/common.sh new file mode 100644 index 0000000..702d1ed --- /dev/null +++ b/test/common.sh @@ -0,0 +1,20 @@ +#!/bin/sh + +OUTPUT_DIR=sandbox + +fail() { + echo "Test failed: $1" + if [ -n ${tmp} ]; then + rm ${tmp} + fi + exit 1 +} + +build_uboot() { + echo "Build sandbox" + OPTS="O=${OUTPUT_DIR} $1" + NUM_CPUS=$(grep -c processor /proc/cpuinfo) + echo ${OPTS} + make ${OPTS} sandbox_config + make ${OPTS} -s -j${NUM_CPUS} +} diff --git a/test/trace/test-trace.sh b/test/trace/test-trace.sh index 973063c..3e8651e 100755 --- a/test/trace/test-trace.sh +++ b/test/trace/test-trace.sh @@ -5,24 +5,10 @@
# Simple test script for tracing with sandbox
-OUTPUT_DIR=sandbox TRACE_OPT="FTRACE=1"
-fail() { - echo "Test failed: $1" - if [ -n ${tmp} ]; then - rm ${tmp} - fi - exit 1 -} - -build_uboot() { - echo "Build sandbox" - OPTS="O=${OUTPUT_DIR} ${TRACE_OPT}" - NUM_CPUS=$(grep -c processor /proc/cpuinfo) - make ${OPTS} sandbox_config - make ${OPTS} -s -j${NUM_CPUS} -} +BASE="$(dirname $0)/.." +. $BASE/common.sh
run_trace() { echo "Run trace" @@ -69,7 +55,7 @@ check_results() { echo "Simple trace test / sanity check using sandbox" echo tmp="$(tempfile)" -build_uboot +build_uboot "${TRACE_OPT}" run_trace >${tmp} check_results ${tmp} rm ${tmp}
participants (3)
-
Masahiro Yamada -
Simon Glass -
Wolfgang Denk