Keyboard events go missing when issued too fast

[29jm]

Keyboard input is dropped when keys are pressed too fast and/or at the same time.

Reproduce steps:

• start typing fast in the terminal app, something like "abcdefghijkl"
• some letters will surely be missing in the terminal prompt

[29jm]

This happens because the window manager stores only the last keyboard event it receives from the keyboard driver, here, and clients (process that have opened a window) may not poll for events fast enough. A program whose event loop is slow is sure to drop key presses.

What should happen is that the wm should store the keyboard events in a queue, and when a client asks for its events, give out the oldest key pressed in the event struct, popping it from the queue.

It'd be better not to modify wm_event_t and instead add a per-window key press queue (list_t is good), and fill out the wm_event_t struct as described above in wm_get_event. Or something like resembling that in some way ^^

[xadaemon]

Maybe something like a ringbuff works better here?

[29jm]

You're right, a fixed size structure is even better than a list. A ring buffer implementation already exists in pipe.c, it'll be the occasion to make it an independent an reusable thing.

[xadaemon]

I will do the splitting it into a reusable component first, and then if you are up to giving me a few pointers I can then try to solve this issue what do you think?

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Sunday, December 20th, 2020 at 06:59, Johan Manuel notifications@github.com wrote:

You're right, a fixed size structure is even better than a list. A ring buffer implementation already exists in pipe.c, it'll be the occasion to make it an independent an reusable thing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[29jm]

@octetd That'd be awesome!

For the first part, the ringbuffer code should end up in the libc (as ringbuffer.c perhaps?), same as the existing list structure. It'll probably be useful to have a ringbuffer_t ringbuffer_new(uint32_t size) function that allocates via malloc, and functions to read and write n bytes from/to the buffer, get the content size... In "pipe.c", pipe_fs_t should then end up with two members, fs plus a ringbuffer.

As for fixing this bug, there are choices to make regarding how to communicate those multiple events to userspace

• we could have wm_get_event return them one by one, until there's no more catching up to do and it'd return a code like WM_NO_EVENTS or something (it's called from here)
• copy each window's keyboard ringbuffer to a userspace-provided one, let the client deal with extracting events from the buffer
• some other option I haven't thought of

The second approach has the benefit of passing events in a single system call, but the interface becomes a bit complicated perhaps. The first one always requires n+1 syscalls for n events, but it has the advantage of being easier to implement, easier to use for the client window.

[xadaemon]

maybe the first solution can work as a poc about fixing it with a ringbuff then if we feel like it or it becomes necessary we can spend the effort on the 2nd option, what do you think?

[29jm]

@octetd Sounds good to me, that's probably best :+1:

[xadaemon]

One thing I couldn't find in the coding guidelines is about the use of typedef so should I typedef ringbuff like this:

struct __ringbuff {
    size_t sz;
    size_t used;
    uint8_t* data;
    size_t w_pos;
    size_t r_pos;
};

typedef struct __ringbuff ringbuff_t;

or not and use it with struct ringbuff on the code

struct ringbuff {
    size_t sz;
    size_t used;
    uint8_t* data;
    size_t w_pos;
    size_t r_pos;
};

edit: clarify the question ps: sorry for the amount of questions

[29jm]

I use typedef struct type_t { ... } type_t (on several lines though, on mobile ^^) !

No issue at all on the questions! Do ask if you have more.

[xadaemon]

One more stylistic question, I'm used to be very explicit about castings (to make code easier to follow), do you prefer something like

size_t ringbuffer_read(ringbuffer_t* ref, size_t n, void* buffer) {
    uint8_t* tgt;
    /*snip*/
    tgt = (uint8_t*) buffer + i;
    /*snip*/
}

or

size_t ringbuffer_read(ringbuffer_t* ref, size_t n, void* buffer) {
    uint8_t* tgt;
    /*snip*/
    tgt = buffer + i; // implicit cast happens here
    /*snip*/
}

[xadaemon]

asking because these can be a pain to refactor out after you put them in many places 😄

[29jm]

I'd rather have buffer be of type uint8_t* if you don't mind. That avoids those casts, and it makes sense since buffer really is a byte buffer. Given the two options though, I prefer the first, I don't even know what arithmetic on void pointers does ^^ Is that even legal C?

[xadaemon]

Yes void pointer arithmetic is illegal however with the cast it becomes an uint8_t pointer, hence why you can malloc an array by doing len * item_size so something like:

uint8_t* aloc_arr = malloc(8 * sizeof(uint8_t));
alloc_arr[1] = 0x00;

works because the void* returned by malloc is cast to uint8_t* making the arithmetic on the second line valid. note: gcc seems to support void pointer arithmetic as an extension but the case on my question and the example are using the cast implicit or otherwise so this does not apply. note2: this would be rejected by a C++ compiler apparently because C++ disallows the implicit cast, but would be ok if the cast is made explicit. ps: leaving this here for reference

[xadaemon]

I'd rather have buffer be of type uint8_t* if you don't mind. That avoids those casts, and it makes sense since buffer really is a byte buffer. Given the two options though, I prefer the first, I don't even know what arithmetic on void pointers does ^^ Is that even legal C?

I was thinking about using uint8_t* on the api however most apis I have worked with in C seem to favour a void*, however it does make more sense and helps make the api harder to misuse, so I'm going to forgo the void* and use 'uint8_t edit: extending the previous point all apis that usevoid` accept something generic so in this one the typed pointer only makes sense.

[29jm]

In your last snippet there is an implicit cast on assignment, which is clear enough, however in tgt = buffer + i with buffer a void*, there's void* arithmetic on the right hand side. While gcc apparently doesn't complain unless compiling with -pedantic, I'd rather avoid doing that entirely. I'll know that sizeof(void) == 1 now though :)

[xadaemon]

oh, I see gcc can be a bit of a trap here then lol ^^ thanks

[xadaemon]

I opened a PR with just the libc ringbuffer implementation so it may be used elsewhere while I take a crack at this bug

[xadaemon]

update events are no longer lost, however you need to keep a key down a few seconds for the event to register it seems, I will commit the code as is ,can you take a look and see if you have any ideas as to why?

[29jm]

I'll take a good look, sure! Can you open a PR with the changes? An initial thought would be that if you haven't updated the apps to consume all events every frame, they might end up getting only mouse move events, something like that.

[xadaemon]

Good guess I will open the pr in a few minutes

-------- Original Message -------- On 30 Dec 2020, 18:53, Johan Manuel wrote:

I'll take a good look, sure! Can you open a PR with the changes? An initial thought would be that if you haven't updated the apps to consume all events every frame, they might end up getting only mouse move events, something like that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.