* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
@ 2017-09-07 20:28 Rob Clark
2017-09-07 20:28 ` [U-Boot] [PATCH] dm: video: Fix cache flushes Rob Clark
2017-09-11 6:18 ` [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Simon Glass
0 siblings, 2 replies; 8+ messages in thread
From: Rob Clark @ 2017-09-07 20:28 UTC (permalink / raw)
To: u-boot
Really just the subset that is needed by efi_console. Perhaps more will
be added later, for example color support would be useful to implement
efi_cout_set_attribute().
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
drivers/video/video-uclass.c | 4 +-
include/video.h | 7 +++
include/video_console.h | 11 ++++
4 files changed, 131 insertions(+), 3 deletions(-)
diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
index e081d5a0ee..7998b4cf5f 100644
--- a/drivers/video/vidconsole-uclass.c
+++ b/drivers/video/vidconsole-uclass.c
@@ -9,6 +9,7 @@
*/
#include <common.h>
+#include <linux/ctype.h>
#include <dm.h>
#include <video.h>
#include <video_console.h>
@@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
video_sync(dev->parent);
}
+/*
+ * Parse a number from string that ends in a non-numeric character..
+ * sscanf() would be nice. This is just enough for parsing ANSI escape
+ * sequences.
+ */
+static char *parsenum(char *s, int *num)
+{
+ int n = 0;
+
+ while (isdigit(*s)) {
+ n = (10 * n) + (*s - '0');
+ s++;
+ }
+
+ *num = n;
+ return s;
+}
+
+static void vidconsole_escape_char(struct udevice *dev, char ch)
+{
+ struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
+
+ /* Sanity checking for bogus ESC sequences: */
+ if (priv->escape_len >= sizeof(priv->escape_buf))
+ goto error;
+ if (priv->escape_len == 0 && ch != '[')
+ goto error;
+
+ priv->escape_buf[priv->escape_len++] = ch;
+
+ /*
+ * Escape sequences are terminated by a letter, so keep
+ * accumulating until we get one:
+ */
+ if (!isalpha(ch))
+ return;
+
+ /*
+ * clear escape mode first, otherwise things will get highly
+ * surprising if you hit any debug prints that come back to
+ * this console.
+ */
+ priv->escape = 0;
+
+ switch (ch) {
+ case 'H':
+ case 'f': {
+ int row, col;
+ char *s = priv->escape_buf;
+
+ /*
+ * Set cursor position: [%d;%df or [%d;%dH
+ */
+ s++; /* [ */
+ s = parsenum(s, &row);
+ s++; /* ; */
+ s = parsenum(s, &col);
+
+ priv->ycur = row * priv->y_charsize;
+ priv->xcur_frac = priv->xstart_frac +
+ VID_TO_POS(col * priv->x_charsize);
+
+ break;
+ }
+ case 'J': {
+ int mode;
+
+ /*
+ * Clear part/all screen:
+ * [J or [0J - clear screen from cursor down
+ * [1J - clear screen from cursor up
+ * [2J - clear entire screen
+ *
+ * TODO we really only handle entire-screen case, others
+ * probably require some additions to video-uclass (and
+ * are not really needed yet by efi_console)
+ */
+ parsenum(priv->escape_buf + 1, &mode);
+
+ if (mode == 2) {
+ video_clear(dev->parent);
+ video_sync(dev->parent);
+ priv->ycur = 0;
+ priv->xcur_frac = priv->xstart_frac;
+ } else {
+ debug("unsupported clear mode: %d\n", mode);
+ }
+ break;
+ }
+ default:
+ debug("unrecognized escape sequence: %*s\n",
+ priv->escape_len, priv->escape_buf);
+ }
+
+ return;
+
+error:
+ /* something went wrong, just revert to normal mode: */
+ priv->escape = 0;
+ return;
+}
+
int vidconsole_put_char(struct udevice *dev, char ch)
{
struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
int ret;
+ if (priv->escape) {
+ vidconsole_escape_char(dev, ch);
+ return 0;
+ }
+
switch (ch) {
+ case '\x1b':
+ priv->escape_len = 0;
+ priv->escape = 1;
+ break;
case '\a':
/* beep */
break;
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index dfa39b0d1b..dcaceed42c 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -87,7 +87,7 @@ int video_reserve(ulong *addrp)
return 0;
}
-static int video_clear(struct udevice *dev)
+void video_clear(struct udevice *dev)
{
struct video_priv *priv = dev_get_uclass_priv(dev);
@@ -100,8 +100,6 @@ static int video_clear(struct udevice *dev)
} else {
memset(priv->fb, priv->colour_bg, priv->fb_size);
}
-
- return 0;
}
/* Flush video activity to the caches */
diff --git a/include/video.h b/include/video.h
index 5b4e78b182..61ff653121 100644
--- a/include/video.h
+++ b/include/video.h
@@ -115,6 +115,13 @@ struct video_ops {
int video_reserve(ulong *addrp);
/**
+ * video_clear() - Clear a device's frame buffer to background color.
+ *
+ * @dev: Device to clear
+ */
+void video_clear(struct udevice *dev);
+
+/**
* video_sync() - Sync a device's frame buffer with its hardware
*
* Some frame buffers are cached or have a secondary frame buffer. This
diff --git a/include/video_console.h b/include/video_console.h
index 26047934da..9dce234bd9 100644
--- a/include/video_console.h
+++ b/include/video_console.h
@@ -29,6 +29,9 @@
* @xsize_frac: Width of the display in fractional units
* @xstart_frac: Left margin for the text console in fractional units
* @last_ch: Last character written to the text console on this line
+ * @escape: TRUE if currently accumulating an ANSI escape sequence
+ * @escape_len: Length of accumulated escape sequence so far
+ * @escape_buf: Buffer to accumulate escape sequence
*/
struct vidconsole_priv {
struct stdio_dev sdev;
@@ -42,6 +45,14 @@ struct vidconsole_priv {
int xsize_frac;
int xstart_frac;
int last_ch;
+ /*
+ * ANSI escape sequences are accumulated character by character,
+ * starting after the ESC char (0x1b) until the entire sequence
+ * is consumed at which point it is acted upon.
+ */
+ int escape;
+ int escape_len;
+ char escape_buf[32];
};
/**
--
2.13.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] dm: video: Fix cache flushes
2017-09-07 20:28 [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Rob Clark
@ 2017-09-07 20:28 ` Rob Clark
2017-09-11 6:18 ` [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Simon Glass
1 sibling, 0 replies; 8+ messages in thread
From: Rob Clark @ 2017-09-07 20:28 UTC (permalink / raw)
To: u-boot
Content can come to screen via putc() and we cannot always rely on
updates ending with a puts(). This is the case with efi_console output
to vidconsole. Fixes corruption with Shell.efi.
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
drivers/video/vidconsole-uclass.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
index b5afd72227..e081d5a0ee 100644
--- a/drivers/video/vidconsole-uclass.c
+++ b/drivers/video/vidconsole-uclass.c
@@ -163,6 +163,7 @@ static void vidconsole_putc(struct stdio_dev *sdev, const char ch)
struct udevice *dev = sdev->priv;
vidconsole_put_char(dev, ch);
+ video_sync(dev->parent);
}
static void vidconsole_puts(struct stdio_dev *sdev, const char *s)
@@ -260,6 +261,8 @@ static int do_video_puts(cmd_tbl_t *cmdtp, int flag, int argc,
for (s = argv[1]; *s; s++)
vidconsole_put_char(dev, *s);
+ video_sync(dev->parent);
+
return 0;
}
--
2.13.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
2017-09-07 20:28 [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Rob Clark
2017-09-07 20:28 ` [U-Boot] [PATCH] dm: video: Fix cache flushes Rob Clark
@ 2017-09-11 6:18 ` Simon Glass
2017-09-11 9:42 ` Rob Clark
1 sibling, 1 reply; 8+ messages in thread
From: Simon Glass @ 2017-09-11 6:18 UTC (permalink / raw)
To: u-boot
On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
> Really just the subset that is needed by efi_console. Perhaps more will
> be added later, for example color support would be useful to implement
> efi_cout_set_attribute().
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
> drivers/video/video-uclass.c | 4 +-
> include/video.h | 7 +++
> include/video_console.h | 11 ++++
> 4 files changed, 131 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
> index e081d5a0ee..7998b4cf5f 100644
> --- a/drivers/video/vidconsole-uclass.c
> +++ b/drivers/video/vidconsole-uclass.c
> @@ -9,6 +9,7 @@
> */
>
> #include <common.h>
> +#include <linux/ctype.h>
> #include <dm.h>
> #include <video.h>
> #include <video_console.h>
> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
> video_sync(dev->parent);
> }
>
> +/*
> + * Parse a number from string that ends in a non-numeric character..
> + * sscanf() would be nice. This is just enough for parsing ANSI escape
> + * sequences.
> + */
> +static char *parsenum(char *s, int *num)
Can you use simple_strtoul() or similar?
> +{
> + int n = 0;
> +
> + while (isdigit(*s)) {
> + n = (10 * n) + (*s - '0');
> + s++;
> + }
> +
> + *num = n;
> + return s;
> +}
> +
> +static void vidconsole_escape_char(struct udevice *dev, char ch)
Please add a function comment
> +{
> + struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
> +
> + /* Sanity checking for bogus ESC sequences: */
> + if (priv->escape_len >= sizeof(priv->escape_buf))
> + goto error;
> + if (priv->escape_len == 0 && ch != '[')
> + goto error;
> +
> + priv->escape_buf[priv->escape_len++] = ch;
> +
> + /*
> + * Escape sequences are terminated by a letter, so keep
> + * accumulating until we get one:
> + */
> + if (!isalpha(ch))
> + return;
> +
> + /*
> + * clear escape mode first, otherwise things will get highly
> + * surprising if you hit any debug prints that come back to
> + * this console.
> + */
> + priv->escape = 0;
> +
> + switch (ch) {
> + case 'H':
> + case 'f': {
> + int row, col;
> + char *s = priv->escape_buf;
> +
> + /*
> + * Set cursor position: [%d;%df or [%d;%dH
> + */
> + s++; /* [ */
> + s = parsenum(s, &row);
> + s++; /* ; */
> + s = parsenum(s, &col);
> +
> + priv->ycur = row * priv->y_charsize;
> + priv->xcur_frac = priv->xstart_frac +
> + VID_TO_POS(col * priv->x_charsize);
> +
> + break;
> + }
> + case 'J': {
> + int mode;
> +
> + /*
> + * Clear part/all screen:
> + * [J or [0J - clear screen from cursor down
> + * [1J - clear screen from cursor up
> + * [2J - clear entire screen
> + *
> + * TODO we really only handle entire-screen case, others
> + * probably require some additions to video-uclass (and
> + * are not really needed yet by efi_console)
> + */
> + parsenum(priv->escape_buf + 1, &mode);
> +
> + if (mode == 2) {
> + video_clear(dev->parent);
> + video_sync(dev->parent);
> + priv->ycur = 0;
> + priv->xcur_frac = priv->xstart_frac;
> + } else {
> + debug("unsupported clear mode: %d\n", mode);
> + }
> + break;
> + }
> + default:
> + debug("unrecognized escape sequence: %*s\n",
> + priv->escape_len, priv->escape_buf);
> + }
> +
> + return;
> +
> +error:
> + /* something went wrong, just revert to normal mode: */
> + priv->escape = 0;
> + return;
> +}
> +
> int vidconsole_put_char(struct udevice *dev, char ch)
> {
> struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
> int ret;
>
> + if (priv->escape) {
> + vidconsole_escape_char(dev, ch);
> + return 0;
> + }
Is it possible to add a CONFIG_VIDEO_ANSI option to enable this?
Perhaps it could be on by default. Then:
if (CONFIG_IS_ENABLED(VIDEO_ANSI) && priv-escape) {
...
This helps to reduce base code size.
> +
> switch (ch) {
> + case '\x1b':
> + priv->escape_len = 0;
> + priv->escape = 1;
> + break;
> case '\a':
> /* beep */
> break;
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index dfa39b0d1b..dcaceed42c 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -87,7 +87,7 @@ int video_reserve(ulong *addrp)
> return 0;
> }
>
> -static int video_clear(struct udevice *dev)
> +void video_clear(struct udevice *dev)
> {
> struct video_priv *priv = dev_get_uclass_priv(dev);
>
> @@ -100,8 +100,6 @@ static int video_clear(struct udevice *dev)
> } else {
> memset(priv->fb, priv->colour_bg, priv->fb_size);
> }
> -
> - return 0;
> }
>
> /* Flush video activity to the caches */
> diff --git a/include/video.h b/include/video.h
> index 5b4e78b182..61ff653121 100644
> --- a/include/video.h
> +++ b/include/video.h
> @@ -115,6 +115,13 @@ struct video_ops {
> int video_reserve(ulong *addrp);
>
> /**
> + * video_clear() - Clear a device's frame buffer to background color.
> + *
> + * @dev: Device to clear
> + */
> +void video_clear(struct udevice *dev);
> +
> +/**
> * video_sync() - Sync a device's frame buffer with its hardware
> *
> * Some frame buffers are cached or have a secondary frame buffer. This
> diff --git a/include/video_console.h b/include/video_console.h
> index 26047934da..9dce234bd9 100644
> --- a/include/video_console.h
> +++ b/include/video_console.h
> @@ -29,6 +29,9 @@
> * @xsize_frac: Width of the display in fractional units
> * @xstart_frac: Left margin for the text console in fractional units
> * @last_ch: Last character written to the text console on this line
> + * @escape: TRUE if currently accumulating an ANSI escape sequence
> + * @escape_len: Length of accumulated escape sequence so far
> + * @escape_buf: Buffer to accumulate escape sequence
> */
> struct vidconsole_priv {
> struct stdio_dev sdev;
> @@ -42,6 +45,14 @@ struct vidconsole_priv {
> int xsize_frac;
> int xstart_frac;
> int last_ch;
> + /*
> + * ANSI escape sequences are accumulated character by character,
> + * starting after the ESC char (0x1b) until the entire sequence
> + * is consumed at which point it is acted upon.
> + */
> + int escape;
> + int escape_len;
> + char escape_buf[32];
> };
>
> /**
> --
> 2.13.5
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
2017-09-11 6:18 ` [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Simon Glass
@ 2017-09-11 9:42 ` Rob Clark
2017-09-11 11:50 ` Lothar Waßmann
0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2017-09-11 9:42 UTC (permalink / raw)
To: u-boot
On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
> On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
>> Really just the subset that is needed by efi_console. Perhaps more will
>> be added later, for example color support would be useful to implement
>> efi_cout_set_attribute().
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>> drivers/video/video-uclass.c | 4 +-
>> include/video.h | 7 +++
>> include/video_console.h | 11 ++++
>> 4 files changed, 131 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
>> index e081d5a0ee..7998b4cf5f 100644
>> --- a/drivers/video/vidconsole-uclass.c
>> +++ b/drivers/video/vidconsole-uclass.c
>> @@ -9,6 +9,7 @@
>> */
>>
>> #include <common.h>
>> +#include <linux/ctype.h>
>> #include <dm.h>
>> #include <video.h>
>> #include <video_console.h>
>> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>> video_sync(dev->parent);
>> }
>>
>> +/*
>> + * Parse a number from string that ends in a non-numeric character..
>> + * sscanf() would be nice. This is just enough for parsing ANSI escape
>> + * sequences.
>> + */
>> +static char *parsenum(char *s, int *num)
>
> Can you use simple_strtoul() or similar?
Possibly, but I'm not sure it is a good idea.. I don't think escape
sequences are meant to be encoded with hex or octal number strings.
From a quick look, I don't see any escape code terminated with 'x', so
maybe it would end up working ok.. but something like ESC[0x1234m
should be an escape sequence terminated with x followed by normal
chars 1234m and strtoul would get that wrong..
BR,
-R
>> +{
>> + int n = 0;
>> +
>> + while (isdigit(*s)) {
>> + n = (10 * n) + (*s - '0');
>> + s++;
>> + }
>> +
>> + *num = n;
>> + return s;
>> +}
>> +
>> +static void vidconsole_escape_char(struct udevice *dev, char ch)
>
> Please add a function comment
>
>> +{
>> + struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
>> +
>> + /* Sanity checking for bogus ESC sequences: */
>> + if (priv->escape_len >= sizeof(priv->escape_buf))
>> + goto error;
>> + if (priv->escape_len == 0 && ch != '[')
>> + goto error;
>> +
>> + priv->escape_buf[priv->escape_len++] = ch;
>> +
>> + /*
>> + * Escape sequences are terminated by a letter, so keep
>> + * accumulating until we get one:
>> + */
>> + if (!isalpha(ch))
>> + return;
>> +
>> + /*
>> + * clear escape mode first, otherwise things will get highly
>> + * surprising if you hit any debug prints that come back to
>> + * this console.
>> + */
>> + priv->escape = 0;
>> +
>> + switch (ch) {
>> + case 'H':
>> + case 'f': {
>> + int row, col;
>> + char *s = priv->escape_buf;
>> +
>> + /*
>> + * Set cursor position: [%d;%df or [%d;%dH
>> + */
>> + s++; /* [ */
>> + s = parsenum(s, &row);
>> + s++; /* ; */
>> + s = parsenum(s, &col);
>> +
>> + priv->ycur = row * priv->y_charsize;
>> + priv->xcur_frac = priv->xstart_frac +
>> + VID_TO_POS(col * priv->x_charsize);
>> +
>> + break;
>> + }
>> + case 'J': {
>> + int mode;
>> +
>> + /*
>> + * Clear part/all screen:
>> + * [J or [0J - clear screen from cursor down
>> + * [1J - clear screen from cursor up
>> + * [2J - clear entire screen
>> + *
>> + * TODO we really only handle entire-screen case, others
>> + * probably require some additions to video-uclass (and
>> + * are not really needed yet by efi_console)
>> + */
>> + parsenum(priv->escape_buf + 1, &mode);
>> +
>> + if (mode == 2) {
>> + video_clear(dev->parent);
>> + video_sync(dev->parent);
>> + priv->ycur = 0;
>> + priv->xcur_frac = priv->xstart_frac;
>> + } else {
>> + debug("unsupported clear mode: %d\n", mode);
>> + }
>> + break;
>> + }
>> + default:
>> + debug("unrecognized escape sequence: %*s\n",
>> + priv->escape_len, priv->escape_buf);
>> + }
>> +
>> + return;
>> +
>> +error:
>> + /* something went wrong, just revert to normal mode: */
>> + priv->escape = 0;
>> + return;
>> +}
>> +
>> int vidconsole_put_char(struct udevice *dev, char ch)
>> {
>> struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
>> int ret;
>>
>> + if (priv->escape) {
>> + vidconsole_escape_char(dev, ch);
>> + return 0;
>> + }
>
> Is it possible to add a CONFIG_VIDEO_ANSI option to enable this?
> Perhaps it could be on by default. Then:
>
> if (CONFIG_IS_ENABLED(VIDEO_ANSI) && priv-escape) {
> ...
>
> This helps to reduce base code size.
>
>> +
>> switch (ch) {
>> + case '\x1b':
>> + priv->escape_len = 0;
>> + priv->escape = 1;
>> + break;
>> case '\a':
>> /* beep */
>> break;
>> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
>> index dfa39b0d1b..dcaceed42c 100644
>> --- a/drivers/video/video-uclass.c
>> +++ b/drivers/video/video-uclass.c
>> @@ -87,7 +87,7 @@ int video_reserve(ulong *addrp)
>> return 0;
>> }
>>
>> -static int video_clear(struct udevice *dev)
>> +void video_clear(struct udevice *dev)
>> {
>> struct video_priv *priv = dev_get_uclass_priv(dev);
>>
>> @@ -100,8 +100,6 @@ static int video_clear(struct udevice *dev)
>> } else {
>> memset(priv->fb, priv->colour_bg, priv->fb_size);
>> }
>> -
>> - return 0;
>> }
>>
>> /* Flush video activity to the caches */
>> diff --git a/include/video.h b/include/video.h
>> index 5b4e78b182..61ff653121 100644
>> --- a/include/video.h
>> +++ b/include/video.h
>> @@ -115,6 +115,13 @@ struct video_ops {
>> int video_reserve(ulong *addrp);
>>
>> /**
>> + * video_clear() - Clear a device's frame buffer to background color.
>> + *
>> + * @dev: Device to clear
>> + */
>> +void video_clear(struct udevice *dev);
>> +
>> +/**
>> * video_sync() - Sync a device's frame buffer with its hardware
>> *
>> * Some frame buffers are cached or have a secondary frame buffer. This
>> diff --git a/include/video_console.h b/include/video_console.h
>> index 26047934da..9dce234bd9 100644
>> --- a/include/video_console.h
>> +++ b/include/video_console.h
>> @@ -29,6 +29,9 @@
>> * @xsize_frac: Width of the display in fractional units
>> * @xstart_frac: Left margin for the text console in fractional units
>> * @last_ch: Last character written to the text console on this line
>> + * @escape: TRUE if currently accumulating an ANSI escape sequence
>> + * @escape_len: Length of accumulated escape sequence so far
>> + * @escape_buf: Buffer to accumulate escape sequence
>> */
>> struct vidconsole_priv {
>> struct stdio_dev sdev;
>> @@ -42,6 +45,14 @@ struct vidconsole_priv {
>> int xsize_frac;
>> int xstart_frac;
>> int last_ch;
>> + /*
>> + * ANSI escape sequences are accumulated character by character,
>> + * starting after the ESC char (0x1b) until the entire sequence
>> + * is consumed at which point it is acted upon.
>> + */
>> + int escape;
>> + int escape_len;
>> + char escape_buf[32];
>> };
>>
>> /**
>> --
>> 2.13.5
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
2017-09-11 9:42 ` Rob Clark
@ 2017-09-11 11:50 ` Lothar Waßmann
2017-09-11 12:31 ` Rob Clark
0 siblings, 1 reply; 8+ messages in thread
From: Lothar Waßmann @ 2017-09-11 11:50 UTC (permalink / raw)
To: u-boot
Hi,
On Mon, 11 Sep 2017 05:42:01 -0400 Rob Clark wrote:
> On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
> > On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
> >> Really just the subset that is needed by efi_console. Perhaps more will
> >> be added later, for example color support would be useful to implement
> >> efi_cout_set_attribute().
> >>
> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >> ---
> >> drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
> >> drivers/video/video-uclass.c | 4 +-
> >> include/video.h | 7 +++
> >> include/video_console.h | 11 ++++
> >> 4 files changed, 131 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
> >> index e081d5a0ee..7998b4cf5f 100644
> >> --- a/drivers/video/vidconsole-uclass.c
> >> +++ b/drivers/video/vidconsole-uclass.c
> >> @@ -9,6 +9,7 @@
> >> */
> >>
> >> #include <common.h>
> >> +#include <linux/ctype.h>
> >> #include <dm.h>
> >> #include <video.h>
> >> #include <video_console.h>
> >> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
> >> video_sync(dev->parent);
> >> }
> >>
> >> +/*
> >> + * Parse a number from string that ends in a non-numeric character..
> >> + * sscanf() would be nice. This is just enough for parsing ANSI escape
> >> + * sequences.
> >> + */
> >> +static char *parsenum(char *s, int *num)
> >
> > Can you use simple_strtoul() or similar?
>
> Possibly, but I'm not sure it is a good idea.. I don't think escape
> sequences are meant to be encoded with hex or octal number strings.
> From a quick look, I don't see any escape code terminated with 'x', so
> maybe it would end up working ok.. but something like ESC[0x1234m
> should be an escape sequence terminated with x followed by normal
> chars 1234m and strtoul would get that wrong..
>
stroul(s, NULL, 10) will only parse decimal numbers and stop at
non-decimal digits.
Lothar Waßmann
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
2017-09-11 11:50 ` Lothar Waßmann
@ 2017-09-11 12:31 ` Rob Clark
2017-09-11 20:42 ` Rob Clark
2017-09-12 2:44 ` Simon Glass
0 siblings, 2 replies; 8+ messages in thread
From: Rob Clark @ 2017-09-11 12:31 UTC (permalink / raw)
To: u-boot
On Mon, Sep 11, 2017 at 7:50 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> On Mon, 11 Sep 2017 05:42:01 -0400 Rob Clark wrote:
>> On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
>> > On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
>> >> Really just the subset that is needed by efi_console. Perhaps more will
>> >> be added later, for example color support would be useful to implement
>> >> efi_cout_set_attribute().
>> >>
>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >> ---
>> >> drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>> >> drivers/video/video-uclass.c | 4 +-
>> >> include/video.h | 7 +++
>> >> include/video_console.h | 11 ++++
>> >> 4 files changed, 131 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
>> >> index e081d5a0ee..7998b4cf5f 100644
>> >> --- a/drivers/video/vidconsole-uclass.c
>> >> +++ b/drivers/video/vidconsole-uclass.c
>> >> @@ -9,6 +9,7 @@
>> >> */
>> >>
>> >> #include <common.h>
>> >> +#include <linux/ctype.h>
>> >> #include <dm.h>
>> >> #include <video.h>
>> >> #include <video_console.h>
>> >> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>> >> video_sync(dev->parent);
>> >> }
>> >>
>> >> +/*
>> >> + * Parse a number from string that ends in a non-numeric character..
>> >> + * sscanf() would be nice. This is just enough for parsing ANSI escape
>> >> + * sequences.
>> >> + */
>> >> +static char *parsenum(char *s, int *num)
>> >
>> > Can you use simple_strtoul() or similar?
>>
>> Possibly, but I'm not sure it is a good idea.. I don't think escape
>> sequences are meant to be encoded with hex or octal number strings.
>> From a quick look, I don't see any escape code terminated with 'x', so
>> maybe it would end up working ok.. but something like ESC[0x1234m
>> should be an escape sequence terminated with x followed by normal
>> chars 1234m and strtoul would get that wrong..
>>
> stroul(s, NULL, 10) will only parse decimal numbers and stop at
> non-decimal digits.
>
And you'd expect simple_strtoul() would too.. but that does not appear
to be the case. Not sure if that is intentional.
BR,
-R
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
2017-09-11 12:31 ` Rob Clark
@ 2017-09-11 20:42 ` Rob Clark
2017-09-12 2:44 ` Simon Glass
1 sibling, 0 replies; 8+ messages in thread
From: Rob Clark @ 2017-09-11 20:42 UTC (permalink / raw)
To: u-boot
On Mon, Sep 11, 2017 at 8:31 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Mon, Sep 11, 2017 at 7:50 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
>> Hi,
>>
>> On Mon, 11 Sep 2017 05:42:01 -0400 Rob Clark wrote:
>>> On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
>>> > On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
>>> >> Really just the subset that is needed by efi_console. Perhaps more will
>>> >> be added later, for example color support would be useful to implement
>>> >> efi_cout_set_attribute().
>>> >>
>>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>> >> ---
>>> >> drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>>> >> drivers/video/video-uclass.c | 4 +-
>>> >> include/video.h | 7 +++
>>> >> include/video_console.h | 11 ++++
>>> >> 4 files changed, 131 insertions(+), 3 deletions(-)
>>> >>
>>> >> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
>>> >> index e081d5a0ee..7998b4cf5f 100644
>>> >> --- a/drivers/video/vidconsole-uclass.c
>>> >> +++ b/drivers/video/vidconsole-uclass.c
>>> >> @@ -9,6 +9,7 @@
>>> >> */
>>> >>
>>> >> #include <common.h>
>>> >> +#include <linux/ctype.h>
>>> >> #include <dm.h>
>>> >> #include <video.h>
>>> >> #include <video_console.h>
>>> >> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>>> >> video_sync(dev->parent);
>>> >> }
>>> >>
>>> >> +/*
>>> >> + * Parse a number from string that ends in a non-numeric character..
>>> >> + * sscanf() would be nice. This is just enough for parsing ANSI escape
>>> >> + * sequences.
>>> >> + */
>>> >> +static char *parsenum(char *s, int *num)
>>> >
>>> > Can you use simple_strtoul() or similar?
>>>
>>> Possibly, but I'm not sure it is a good idea.. I don't think escape
>>> sequences are meant to be encoded with hex or octal number strings.
>>> From a quick look, I don't see any escape code terminated with 'x', so
>>> maybe it would end up working ok.. but something like ESC[0x1234m
>>> should be an escape sequence terminated with x followed by normal
>>> chars 1234m and strtoul would get that wrong..
>>>
>> stroul(s, NULL, 10) will only parse decimal numbers and stop at
>> non-decimal digits.
>>
>
> And you'd expect simple_strtoul() would too.. but that does not appear
> to be the case. Not sure if that is intentional.
>
So I double checked simple_strtoul() in upstream kernel, and (apart
from being re-written) it seems to have fixed this bug. So I'm
guessing the u-boot copied a buggy version from the linux kernel src
and never got a fix. I'll send a patch for that. It will be easy
enough to drop parsenum() once the fix is merged.
BR,
-R
^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
2017-09-11 12:31 ` Rob Clark
2017-09-11 20:42 ` Rob Clark
@ 2017-09-12 2:44 ` Simon Glass
1 sibling, 0 replies; 8+ messages in thread
From: Simon Glass @ 2017-09-12 2:44 UTC (permalink / raw)
To: u-boot
Hi Rob,
On 11 September 2017 at 06:31, Rob Clark <robdclark@gmail.com> wrote:
> On Mon, Sep 11, 2017 at 7:50 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
>> Hi,
>>
>> On Mon, 11 Sep 2017 05:42:01 -0400 Rob Clark wrote:
>>> On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
>>> > On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
>>> >> Really just the subset that is needed by efi_console. Perhaps more will
>>> >> be added later, for example color support would be useful to implement
>>> >> efi_cout_set_attribute().
>>> >>
>>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>> >> ---
>>> >> drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>>> >> drivers/video/video-uclass.c | 4 +-
>>> >> include/video.h | 7 +++
>>> >> include/video_console.h | 11 ++++
>>> >> 4 files changed, 131 insertions(+), 3 deletions(-)
>>> >>
>>> >> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
>>> >> index e081d5a0ee..7998b4cf5f 100644
>>> >> --- a/drivers/video/vidconsole-uclass.c
>>> >> +++ b/drivers/video/vidconsole-uclass.c
>>> >> @@ -9,6 +9,7 @@
>>> >> */
>>> >>
>>> >> #include <common.h>
>>> >> +#include <linux/ctype.h>
>>> >> #include <dm.h>
>>> >> #include <video.h>
>>> >> #include <video_console.h>
>>> >> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>>> >> video_sync(dev->parent);
>>> >> }
>>> >>
>>> >> +/*
>>> >> + * Parse a number from string that ends in a non-numeric character..
>>> >> + * sscanf() would be nice. This is just enough for parsing ANSI escape
>>> >> + * sequences.
>>> >> + */
>>> >> +static char *parsenum(char *s, int *num)
>>> >
>>> > Can you use simple_strtoul() or similar?
>>>
>>> Possibly, but I'm not sure it is a good idea.. I don't think escape
>>> sequences are meant to be encoded with hex or octal number strings.
>>> From a quick look, I don't see any escape code terminated with 'x', so
>>> maybe it would end up working ok.. but something like ESC[0x1234m
>>> should be an escape sequence terminated with x followed by normal
>>> chars 1234m and strtoul would get that wrong..
>>>
>> stroul(s, NULL, 10) will only parse decimal numbers and stop at
>> non-decimal digits.
>>
>
> And you'd expect simple_strtoul() would too.. but that does not appear
> to be the case. Not sure if that is intentional.
That looks like a bug to me although there is no documentation or
tests for that function. The while loop should stop at any character
beyond base I think.
>
> BR,
> -R
Regards,
Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-12 2:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-07 20:28 [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Rob Clark
2017-09-07 20:28 ` [U-Boot] [PATCH] dm: video: Fix cache flushes Rob Clark
2017-09-11 6:18 ` [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Simon Glass
2017-09-11 9:42 ` Rob Clark
2017-09-11 11:50 ` Lothar Waßmann
2017-09-11 12:31 ` Rob Clark
2017-09-11 20:42 ` Rob Clark
2017-09-12 2:44 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.