[U-Boot] [PATCH 0/4] Improve/fix tests for commands and trace

[U-Boot] [PATCH 0/4] Improve/fix tests for commands and trace

[Simon Glass]

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

-- 
2.0.0.526.g5318336

[U-Boot] [PATCH 1/4] Reactivate the tracing feature

[Simon Glass]

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)
-- 
2.0.0.526.g5318336

[U-Boot] [PATCH 1/4] Reactivate the tracing feature

[Masahiro Yamada]

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

[U-Boot] [PATCH 1/4] Reactivate the tracing feature

[Simon Glass]

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

[U-Boot] [PATCH 1/4] Reactivate the tracing feature

[Simon Glass]

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

[U-Boot] [PATCH 1/4] Reactivate the tracing feature

[Masahiro Yamada]

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

[U-Boot] [PATCH 1/4] Reactivate the tracing feature

[Simon Glass]

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

[U-Boot] [PATCH 1/4] Reactivate the tracing feature

[Masahiro Yamada]

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

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Simon Glass]

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," \
-- 
2.0.0.526.g5318336

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Nothing difficult is ever easy.

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Simon Glass]

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

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Our business is run on trust.  We trust you will pay in advance.

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Simon Glass]

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:
>
> 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?

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.

>
> 2) 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

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ2BC0MD6NyoOJRmhYeabm3EYMBs8nQA3hcyD71BB+jqDw@mail.gmail.com> you wrote:
> 
> > 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?
> 
> 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?

> > 2) 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Science makes godlike -- it is all over with priests and  gods  when
man becomes scientific. Moral: science is the forbidden as such -- it
alone  is  forbidden. Science is the *first* sin, the *original* sin.
*This alone is  morality.*  ``Thou  shalt  not  know''  --  the  rest
follows."                                       - Friedrich Nietzsche

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Simon Glass]

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:
>>
>> > 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?
>>
>> 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.

>
>> > 2) 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

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Wolfgang Denk]

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:
> 
> http://patchwork.ozlabs.org/patch/90066/

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The pathology is to want control, not that you ever get it,  because
of course you never do."                            - Gregory Bateson

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Simon Glass]

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:
>>
>> http://patchwork.ozlabs.org/patch/90066/
>
> 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

[U-Boot] [PATCH 2/4] Add ctrlc_ignore environment variable to ignore Ctrl-C

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I can type faster than I can move a  mouse,  so  I  find  menu-driven
drawing packages time consuming and frustrating.      - W. R. Stevens

[U-Boot] [PATCH 3/4] test: Remove tabs from trace test

[Simon Glass]

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
 }
 
-- 
2.0.0.526.g5318336

[U-Boot] [PATCH 4/4] test: Add a test for command repeat

[Simon Glass]

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}
-- 
2.0.0.526.g5318336