| Message ID | 20140524232950.GH1904@beta.private.mielke.cc |
|---|---|
| State | New |
| Headers | show |
On 25 May 2014 00:29, Dave Mielke <dave@mielke.cc> wrote: > The attached patch (qemu-curses-delay-1.patch) allows the user to specify that > he needs -display curses to insert a delay in between key events. The current > behaviour is that it inserts key events immediately, one right after another, > which has proven to be too fast for some applications. Please let me know if > there are any improvements you'd like me to make to this patch, with the goal > that I'm hoping you'll accept it. A more detailed description of this patch is > as follows: > > Add support for the "-display curses" option to accept suboptions (-display > curses[,option...), and add the "delay=<msecs>" suboption. This suboption > causes a millisecond-based delay to be inserted in between key events so that > they won't be inserted immediately one after another. The delay is performed > using a qemu virtual clock timer. If the "delay" option isn't specified, or if > "delay=0" is specified, then the timer isn't used, thus making the default be > the original behaviour. The "=<msecs>" operand is optional - if it isn't > specified then 20 is assumed. Why is this a problem only for the curses UI frontend, and not for any of the other UIs which might send key events? thanks -- PMM
[quoted lines by Peter Maydell on 2014/05/25 at 01:04 +0100] >Why is this a problem only for the curses UI frontend, and not for >any of the other UIs which might send key events? One reason is that most UIs send key events as they receive them from the keyboard, one at a time, whereas the curses UI gets a single key event from curses and then sends all of the scancodes which it takes to represent that key. In other words, a single key event from curses could result in as many as 10 actual key events needing to be passed along - both the press and release for the actual key plus up to four modifiers (shift, control, alt, altgr). Another reason is that it's not a problem for a lot of applications. I happened to hit it in an application, and, therefore, coded a fix for it.
On 25 May 2014 02:21, Dave Mielke <dave@mielke.cc> wrote: > [quoted lines by Peter Maydell on 2014/05/25 at 01:04 +0100] >>Why is this a problem only for the curses UI frontend, and not for >>any of the other UIs which might send key events? > > One reason is that most UIs send key events as they receive them from the > keyboard, one at a time, whereas the curses UI gets a single key event from > curses and then sends all of the scancodes which it takes to represent that > key. In other words, a single key event from curses could result in as many as > 10 actual key events needing to be passed along - both the press and release > for the actual key plus up to four modifiers (shift, control, alt, altgr). Ah, I see. Still, I think it makes more sense for the queue and delay to be in the common key handling code, not in the curses frontend specifically. thanks -- PMM
[quoted lines by Peter Maydell on 2014/05/25 at 10:11 +0100] >Ah, I see. Still, I think it makes more sense for the queue and delay >to be in the common key handling code, not in the curses frontend >specifically. Yes, you're right. While the curses UI is especially vulnerable to the problem, others could be as well, especially if they're driven by a software "user" rather than by a human being. Also, who's to say that there won't be a new UI in the future which won't run into the same problem. I'll move the code and submit a new patch. Given that this'll require a brand new option, could you please let me know all the places that'll need updating insofar as documentation is concerned?
[quoted lines by Peter Maydell on 2014/05/25 at 10:11 +0100] >Ah, I see. Still, I think it makes more sense for the queue and delay >to be in the common key handling code, not in the curses frontend >specifically. The code has been moved. I can see a couple of possibilities insofar as an option for it is concerned. One is to extend the existing -k option to accept language= as an optional prefix for its currnet operand, and to add delay=. The other is to add a brand new -keydelay option. Which would be the better approach? I'm assuming that qemu-options.hx is the only file that needs to be updated insofar as documentation is concerned. Is that correct?
25.05.2014 03:29, Dave Mielke wrote: > Add support for the "-display curses" option to accept suboptions (-display > curses[,option...), and add the "delay=<msecs>" suboption. This suboption > causes a millisecond-based delay to be inserted in between key events so that In addition to what Peter said, I think this suboption is named poorly. Maybe it can be named, say, kbddelay, or keydelay, or something like that. Just "delay" may be interpreted as _video_ delay, ie, delay updating picture for so many millisecs. After all, this is -display, which is about video, and keyboard is a secondary function... Also, you still haven't told us what application is having problem with it, it is some mystery "some app". And provided this hasn't been a problem for so many years, maybe we shouldn't add it in the first place? Thanks, /mjt
[quoted lines by Michael Tokarev on 2014/05/25 at 16:41 +0400] >In addition to what Peter said, I think this suboption is named poorly. >Maybe it can be named, say, kbddelay, or keydelay, or something like that. >Just "delay" may be interpreted as _video_ delay, ie, delay updating picture >for so many millisecs. After all, this is -display, which is about video, >and keyboard is a secondary function... Yes, that makes good sense. >Also, you still haven't told us what application is having problem with it, >it is some mystery "some app". Where I ran into it is DJGPP termios input on MS-DOS. >And provided this hasn't been a problem for so many years, Perhaps it hasn't been observed before because the curses UI may not have that many users. I myself must use it as, being blind, it's much easier for me to work in text mode than in graphics mode. >maybe we shouldn't add it in the first place? Given that this feature does resolve a problem, given that it's default retains the original behaviour, and given that we have no idea whatsoever regarding when the problem may strike again, my personal opinion is that implementing it now would be a reasonable thing to do, especially since the current situation is that no one is being asked to figure it out, code it, etc.
On Sa, 2014-05-24 at 21:21 -0400, Dave Mielke wrote: > [quoted lines by Peter Maydell on 2014/05/25 at 01:04 +0100] > > >Why is this a problem only for the curses UI frontend, and not for > >any of the other UIs which might send key events? > > One reason is that most UIs send key events as they receive them from the > keyboard, one at a time, whereas the curses UI gets a single key event from > curses and then sends all of the scancodes which it takes to represent that > key. In other words, a single key event from curses could result in as many as > 10 actual key events needing to be passed along - both the press and release > for the actual key plus up to four modifiers (shift, control, alt, altgr). Tried to make the curses ui a bit more clever? You could try caching the modifier state, then send only the changes. That gets the number of events down to 6 max (4 to update modifier state, 2 for the actual key). Other question: has "git revert 2858ab09e6f708e381fc1a1cc87e747a690c4884" any effect? cheers, Gerd
[quoted lines by Gerd Hoffmann on 2014/05/26 at 15:38 +0200] >Tried to make the curses ui a bit more clever? You could try caching >the modifier state, then send only the changes. That gets the number of >events down to 6 max (4 to update modifier state, 2 for the actual key). Yes, except that, in my case, the problem already occurs on simple typing of lowercase letters, i.e. just two events. Also, that approach could introduce new problems due to modifeir keys appearing to be stuck. >Other question: has "git revert >2858ab09e6f708e381fc1a1cc87e747a690c4884" any effect? That gives me: could not revert 2858ab0... ps2: set ps/2 output buffer size as the same as kernel
On Mo, 2014-05-26 at 11:19 -0400, Dave Mielke wrote: > [quoted lines by Gerd Hoffmann on 2014/05/26 at 15:38 +0200] > > >Tried to make the curses ui a bit more clever? You could try caching > >the modifier state, then send only the changes. That gets the number of > >events down to 6 max (4 to update modifier state, 2 for the actual key). > > Yes, except that, in my case, the problem already occurs on simple typing of > lowercase letters, i.e. just two events. What exactly is the problem? > Also, that approach could introduce new problems due to modifeir keys appearing > to be stuck. Indeed. > >Other question: has "git revert > >2858ab09e6f708e381fc1a1cc87e747a690c4884" any effect? > > That gives me: > > could not revert 2858ab0... ps2: set ps/2 output buffer size as the same as kernel Given that only two keys already problematic this probably wouldn't change things anyway. cheers, Gerd
[quoted lines by Gerd Hoffmann on 2014/05/27 at 07:44 +0200]
>What exactly is the problem?
At the user level, the keyboard appears to be dead. An inspection of the
udnerlying code reveals that the application itsllf is querying the MS-DOS
keyboard input buffer in a bad way. Those apps can't be fixed, though, because
the bad code is in a library which is statically linked into every app using
it, which means lots of apps, many of which, as is always the case, may not
even have source code anymore.
Another thing to consider is that this, really, is a general problem. Most
keyboard input code, especially in older days, would've nly ever been tested at
typing speed. It should be expected, therefore, that there might be problems in
some of it when input events arrive faster - in this case, "infinitely" fater.
The solution I'm proposing, therefore, tackles it in a general way, and does
nothing more than allow the user to request that, regardless of what the UI
needs to do underneath, the keyboard events still arrive at the speed of a
typist.
Did you see my other question abot either adding -kbddelay [duration=]msecs or
merging it into -k (-k [language=]language[,delay=msecs]? Which would be the
better approach?
On Di, 2014-05-27 at 04:15 -0400, Dave Mielke wrote: > [quoted lines by Gerd Hoffmann on 2014/05/27 at 07:44 +0200] > > >What exactly is the problem? > > At the user level, the keyboard appears to be dead. An inspection of the > udnerlying code reveals that the application itsllf is querying the MS-DOS > keyboard input buffer in a bad way. Which means exactly? They are not prepared to find data for more than one key event in the buffer? > Those apps can't be fixed, though, because > the bad code is in a library which is statically linked into every app using > it, which means lots of apps, many of which, as is always the case, may not > even have source code anymore. Sure. > Another thing to consider is that this, really, is a general problem. Most > keyboard input code, especially in older days, would've nly ever been tested at > typing speed. It should be expected, therefore, that there might be problems in > some of it when input events arrive faster - in this case, "infinitely" fater. > The solution I'm proposing, therefore, tackles it in a general way, and does > nothing more than allow the user to request that, regardless of what the UI > needs to do underneath, the keyboard events still arrive at the speed of a > typist. I don't like the idea to do it in the UI code. I don't think the problem is as generic as you think it is. HID keyboards (usb, bluetooth) report keys in a completely different way, and probably your problem is specific to the ps/2 keyboard. I'd try to do it in the ps/2 code, and try to do it without a timer. Idea: Split the ps/2 keyboard queue into two. One will be guest-visible. The existing ps2 queue will do the job. The other, new queue will not be visible to the guest. Simliar to the queue you've added to the ui code. Put not more than one key event (1-3 scancodes) into the guest visible queue. Wait for the guest read the status register (signaling "no more data") once. Then go refill the guest-visible queue from the invisible queue. With some luck this makes things work without a delay. If not we are not so lucky we have to fire a timer for queue refill after signaling "no more data" to the guest. Even in case we need a timer this is better than doing it in the ui code as we can arm the timer when we know the guest has actually seen the key event. So the guest being busy with something else and not polling the kbd for a time span longer than the delay will not break things, so we can use a pretty short delay. cheers, Gerd
On 05/25/14 14:56, Dave Mielke wrote: > [quoted lines by Michael Tokarev on 2014/05/25 at 16:41 +0400] >> And provided this hasn't been a problem for so many years, > > Perhaps it hasn't been observed before because the curses UI may not have that > many users. I myself must use it as, being blind, it's much easier for me to > work in text mode than in graphics mode. You were too modest to link your own webpage, but I'll do it for us all, because it'll help our understanding: http://mielke.cc/brltty/index.html http://mielke.cc/resume.html#exp-2000-PRESENT Thanks Laszlo PS: stunned and amazed
[quoted lines by Gerd Hoffmann on 2014/05/27 at 11:09 +0200] >Which means exactly? They are not prepared to find data for more than >one key event in the buffer? Not exactly. The bad code is an attempt to do a nonblocking keyboard read in MS-DOS. It goes like this: 1) Peek the DOS keyboard buffer start and end pointers. If the buffer is empty then return "no input". 2) If no key is pressed then return "no input". 3) Use the blocking MS-DOS interface to wait for the next key and return it. So, as you can see, there's a serious problem. Let's say that a key press and release go in together. That give us the following situation: 1) Peek the DOS input buffer. It isn't empty so proceed to the next step. 2) No key is pressed so return "no input". I really do think, therefore, that the only way to deal with this is to slow down the input to typing speed.
diff --git a/include/ui/console.h b/include/ui/console.h index 8a86617..d131260 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -338,6 +338,7 @@ static inline int vnc_display_pw_expire(DisplayState *ds, time_t expires) #endif /* curses.c */ +void curses_parse_options(const char *options); void curses_display_init(DisplayState *ds, int full_screen); /* input.c */ diff --git a/qemu-options.hx b/qemu-options.hx index c2c0823..1e407bd 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -820,9 +820,11 @@ ETEXI DEF("display", HAS_ARG, QEMU_OPTION_display, "-display sdl[,frame=on|off][,alt_grab=on|off][,ctrl_grab=on|off]\n" - " [,window_close=on|off]|curses|none|\n" - " gtk[,grab_on_hover=on|off]|\n" - " vnc=<display>[,<optargs>]\n" + " [,window_close=on|off]\n" + " none\n" + " curses[,delay[=<msecs>]]\n" + " gtk[,grab_on_hover=on|off]\n" + " vnc=<display>[,<optargs>]\n" " select display type\n", QEMU_ARCH_ALL) STEXI @item -display @var{type} diff --git a/ui/curses.c b/ui/curses.c index b044790..45719a4 100644 --- a/ui/curses.c +++ b/ui/curses.c @@ -176,6 +176,82 @@ static void curses_cursor_position(DisplayChangeListener *dcl, static kbd_layout_t *kbd_layout = NULL; +struct KeyEvent { + struct KeyEvent *next; + int code; + bool down; +}; + +static struct KeyEvent *firstKeyEvent = NULL; +static struct KeyEvent *lastKeyEvent = NULL; + +static unsigned long long keyEventDelay = 0; +static QEMUTimer *keyEventTimer = NULL; + +static void curses_begin_key_event(void); + +static void curses_send_key_event(int code, bool down) +{ + qemu_input_event_send_key_number(NULL, code, down); +} + +static void curses_end_key_event(void *opaque) +{ + struct KeyEvent *event = firstKeyEvent; + + if ((firstKeyEvent = event->next)) + { + curses_begin_key_event(); + } + else + { + lastKeyEvent = NULL; + } + + g_free(event); +} + +static void curses_begin_key_event(void) +{ + struct KeyEvent *event = firstKeyEvent; + + if (!keyEventTimer) + { + keyEventTimer = timer_new_ms(QEMU_CLOCK_VIRTUAL, curses_end_key_event, NULL); + } + + curses_send_key_event(event->code, event->down); + timer_mod(keyEventTimer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + keyEventDelay); +} + +static void curses_enqueue_key_event(int code, bool down) +{ + if (keyEventDelay) + { + struct KeyEvent *event; + + event = g_malloc(sizeof(*event)); + event->next = NULL; + event->code = code; + event->down = down; + + if (lastKeyEvent) + { + lastKeyEvent->next = event; + lastKeyEvent = event; + } + else + { + firstKeyEvent = lastKeyEvent = event; + curses_begin_key_event(); + } + } + else + { + curses_send_key_event(code, down); + } +} + static void curses_refresh(DisplayChangeListener *dcl) { int chr, nextchr, keysym, keycode, keycode_alt; @@ -276,32 +352,32 @@ static void curses_refresh(DisplayChangeListener *dcl) /* since terminals don't know about key press and release * events, we need to emit both for each key received */ if (keycode & SHIFT) { - qemu_input_event_send_key_number(NULL, SHIFT_CODE, true); + curses_enqueue_key_event(SHIFT_CODE, true); } if (keycode & CNTRL) { - qemu_input_event_send_key_number(NULL, CNTRL_CODE, true); + curses_enqueue_key_event(CNTRL_CODE, true); } if (keycode & ALT) { - qemu_input_event_send_key_number(NULL, ALT_CODE, true); + curses_enqueue_key_event(ALT_CODE, true); } if (keycode & ALTGR) { - qemu_input_event_send_key_number(NULL, GREY | ALT_CODE, true); + curses_enqueue_key_event(GREY | ALT_CODE, true); } - qemu_input_event_send_key_number(NULL, keycode, true); - qemu_input_event_send_key_number(NULL, keycode, false); + curses_enqueue_key_event(keycode, true); + curses_enqueue_key_event(keycode, false); if (keycode & ALTGR) { - qemu_input_event_send_key_number(NULL, GREY | ALT_CODE, false); + curses_enqueue_key_event(GREY | ALT_CODE, false); } if (keycode & ALT) { - qemu_input_event_send_key_number(NULL, ALT_CODE, false); + curses_enqueue_key_event(ALT_CODE, false); } if (keycode & CNTRL) { - qemu_input_event_send_key_number(NULL, CNTRL_CODE, false); + curses_enqueue_key_event(CNTRL_CODE, false); } if (keycode & SHIFT) { - qemu_input_event_send_key_number(NULL, SHIFT_CODE, false); + curses_enqueue_key_event(SHIFT_CODE, false); } } else { keysym = curses2qemu[chr]; @@ -349,6 +425,51 @@ static void curses_keyboard_setup(void) } } +void curses_parse_options(const char *options) +{ + keyEventDelay = 0; + + while (*options) + { + const char *next; + char *end; + + if (!strstart(options, ",", &next)) + { + break; + } + options = next; + + if (strstart(options, "delay", &next)) + { + options = next; + keyEventDelay = 20; + + if (strstart(options, "=", &next)) + { + options = next; + + if (parse_uint(options, &keyEventDelay, &end, 10)) + { + break; + } + options = end; + } + } + + else + { + break; + } + } + + if (*options) + { + fprintf(stderr, "qemu: invalid Curses display option: %s\n", options); + exit(1); + } +} + static const DisplayChangeListenerOps dcl_ops = { .dpy_name = "curses", .dpy_text_update = curses_update, diff --git a/vl.c b/vl.c index 709d8cd..bd6c583 100644 --- a/vl.c +++ b/vl.c @@ -2300,6 +2300,7 @@ static DisplayType select_display(const char *p) } else if (strstart(p, "curses", &opts)) { #ifdef CONFIG_CURSES display = DT_CURSES; + curses_parse_options(opts); #else fprintf(stderr, "Curses support is disabled\n"); exit(1);