Rewrite sdl keyboard_handler to return SDL key and release state

[daviel]

Doing it this way made it possible to find out the release state of one or multiple keys. Also you can add listeners for keys beside the LVGL ones by using the SDL-key-constants.

For whatever reason as soon as I add a group to retrieve events from widgets there is still fired a LV_INDEV_STATE_RELEASED immediately after a LV_INDEV_STATE_PRESSED event. Maybe there is some issue in the way events are handled in the LVGL core? In think that indev_keypad_proc in lv_indev.c may be the reason.

Let me know what you think. Maybe I misunderstood something in the LVGL implementation.

[amirgon]

Is this correct to return sdl_key here?
Until now we only returned ctrl_key (or LVGL codes)

[daviel]

When there is no way to map the SDL_KEY to a LVGL key we can return the sdl_key. This could be useful if you try to map events to other keys.

[amirgon]

But how do you guarantee there are no "collisions"?
Could some sdl_key value be the same as another LV_KEY_*, so that the result is ambiguous?

[daviel]

It does not look like they are conflicting. It seems the constants for the lv_keys were used from sdl:

lv.KEY.dict
{'UP': 17, 'DOWN': 18, 'RIGHT': 19, 'LEFT': 20, 'ESC': 27, 'DEL': 127, 'BACKSPACE': 8, 'ENTER': 10, 'NEXT': 9, 'PREV': 11, 'HOME': 2, 'END': 3}

https://wiki.libsdl.org/SDLKeycodeLookup

[amirgon]

That's interesting.

@kisvegabor - Could you confirm it's OK for an input driver to return to LVGL the SDL key codes directly?

If it is, that means:

• We assume that LVGL key code will always match SDL key codes
• LVGL might receive codes that are not part of the LV_KEY_* enum

[kisvegabor]

LV_KEY_*s are non printable characters and it's fine to pass normal character, like 'a', 'b', 'c' as they are. E.g. in this example you can type anything into the Text area with your keyboard.

[amirgon]

LV_KEY_*s are non printable characters and it's fine to pass normal character, like 'a', 'b', 'c' as they are. E.g. in this example you can type anything into the Text area with your keyboard.

@kisvegabor Right. But is it also ok for a read_cb callback of an LVGL input device to update key to something that is neither a normal character nor LV_KEY_*?
Here we return sdl_key so LVGL might be getting some unexpected codes - SDL codes which are not regular characters and are missing (or unmatching with) LV_KEY_* enum.

Maybe that's ok - I would just like to make sure it really is.

[kisvegabor]

Maybe that's ok - I would just like to make sure it really is.

Confirmed, it's fine this way.

[amirgon]

Is it correct to set state = key_state here?
Since we are under SDL_TEXTINPUT, key_state will always be "PRESSED" here.
So when is each of these keys "released"?

Perhaps you could add two events for each character, "press" and "release", to simulate a click on a key.

But in that case, if SDL sends you SDL_TEXTINPUT events, you will not be able to detect which keys are pressed together.
Do you know when SDL sends SDL_TEXTINPUT and when it sends SDL_KEYDOWN/SDL_KEYUP? Can we control that?

[daviel]

Here is a good explanation of what SDL_TEXTINPUT does: https://wiki.libsdl.org/Tutorials-TextInput

So what would happen is that you would get events for special characters. Defaulting to PRESSED is okay as the implementation before did the same I think.

[amirgon]

So what would happen is that you would get events for special characters. Defaulting to PRESSED is okay as the implementation before did the same I think.

What previous implementation did, was to send both "PRESSED" and "RELEASED" states for each character, one after the other. This simulates a key press where a key was pressed and immediately released and that is the reason you weren't able to detect situations where two keys were pressed simultaneously.

But here you are sending only "PRESSED" and not "RELEASED" event. That's why I suggested adding two events for each character, a "press" event followed by a "release" event, to simulate a key click.

Here is a good explanation of what SDL_TEXTINPUT does: https://wiki.libsdl.org/Tutorials-TextInput

Thanks for the link, I wasn't familiar with these subtleties.

So suppose the user enables IME and sends some characters to the application.
Don't you think we need both "PRESSED" and "RELEASED" events for each character? Otherwise, we might get the impression that all these characters were pressed together and never released.

[daviel]

Okay, I have added a released event directly afterwards.

In my test application I can now see the default LVGL events when they are being hold but I still can't reproduce an LV_EVENT_LONG_PRESSED_REPEAT with pressing and holding enter. Is this intended? The behavior changes also when I add a group and focus it. Then I receive a pressed and released event for every key everytime.

Also the not-defined keys (SDL_KEYS) are now showing a released event immediately after being pressed. I am not sure what is happening here. This is my testing program which I run locally: https://sim.lvgl.io/v9.0/micropython/ports/javascript/index.html?script_direct=f1e7b7008363a47c9837289fe961a0bf8412143b

[amirgon]

I tried to run your code and I can see multiple problems.

It looks like SDL is sending both SDL_KEY* and SDL_TEXTINPUT events for the same click.
This, or course, is a source of confusion, since LVGL sees every key twice.

I've tried removing SDL_TEXTINPUT altogether, although this is incorrect not only for IME but also for simple SHIFT+key (for typing capital letters or punctuation characters for example).

Next problem was that LVGL sets key in lv_indev_data_t to the last key automatically, but only the last key that was released. So it looks like LVGL itself was not built to handle correctly simultaneous key presses.

To work around this I tried to always set data in sdl_keyboard_read to either key_buffer[0] or, when key_buffer is empty, to the last state and key that were set, like this:

diff --git a/driver/SDL/sdl_common.c b/driver/SDL/sdl_common.c
index 071a1f1..4ef8511 100644
--- a/driver/SDL/sdl_common.c
+++ b/driver/SDL/sdl_common.c
@@ -79,14 +79,16 @@ void sdl_mousewheel_read(lv_indev_drv_t * indev_drv, lv_indev_data_t * data)
 void sdl_keyboard_read(lv_indev_drv_t * indev_drv, lv_indev_data_t * data)
 {
     (void) indev_drv;      /*Unused*/
+    static struct key_event last_state = {0};
 
     if(key_buffer_len > 0){
-        data->state = key_buffer[0].state;
-        data->key = key_buffer[0].key;
+        last_state = key_buffer[0];
         key_buffer_len--;
         data->continue_reading = key_buffer_len > 0;
         memmove(&key_buffer[0], &key_buffer[1], sizeof(struct key_event) * key_buffer_len);
     }
+    data->state = last_state.state;
+    data->key = last_state.key;
 }

With this fix, the test program prints the "Pressed" and "Released" events correctly, even in cases where multiple keys are pressed simultaneously. (with SDL_TEXTINPUT handling disabled for the test)
However, LVGL does not behave correctly in such cases and loses key events...

So to summarize, the challenges I see are:

• How to handle both SDL_KEY* and SDL_TEXTINPUT events without sending duplicated key events to LVGL
• How to fix LVGL to work correctly in situations where keys are pressed simultaneously (@kisvegabor and @embeddedt might have some ideas about that)

[kisvegabor]

One disadvantage of SDL_TEXTINPUT is that it doesn't detect simultaneous key presses, which is the motivation for this change.

What do you exactly mean by "simultaneous key presses"? E.g. when If press S and D on my notebook at the same time I get: sddddddddddddddddddd

So what would be expected behavior?

[amirgon]

The expected behavior will be not to lose key strokes....
Here is an example:

https://sim.lvgl.io/v9.0/micropython/ports/javascript/index.html?script_direct=1f683e2db0e13bb25905bbfeaec508d5924b9bd4

The first line in the textarea is ABC where each key is pressed and released in turn.
The second and third lines are the result of the same ABC key presses, where their release is in different order.
As you can see, on the second and third lines, only A is displayed by LVGL.

[kisvegabor]

In the online C examples SDL_TEXTINPUT is enabled and text areas work like any text input. You can try it here. I don't know exactly how the current driver in MP works but I think it's a reasonable default to be similar to other text inputs on desktop/web.

I think the root of the problem is that PC keyboard sends the keys on press while LVGL sends them on release. If we send the keys on release, the newly pressed key will overwrite the old one, and the last one will be released.

We a little trick the read_cb can be modified to react on press: https://sim.lvgl.io/v9.0/micropython/ports/javascript/index.html?script_direct=efcd5f334fb6b46a367f591d77acb3088c435061

[amirgon]

We a little trick the read_cb can be modified to react on press:

@kisvegabor In that trick you triggered a fake "release" for every "press".
If I understand correctly, you are saying that in order for a driver to work correctly, every "press" must be immediately followed by a "release" of the same key....

So now I don't understand the point in data.state.
If every "press" must be followed by a "release" (or we lose key strokes), why can't we give up data.state altogether?
In each call read_cb could update only data.key, and let LVGL do the rest. Why do we need to bother with "press" and "release" states, unless they have some meaning?

[kisvegabor]

Without letting LVGL to handle the pressed and released state LVGL couldn't manage the long pressed and long pressed repeat events. So it's something for something.

The simplest would be to make LVGL send the key events on press instead of release. I have to try it out as it might have unexpected consequences but seems working at first glance.

[amirgon]

Now that you are adding both "pressed" and "released" events, I think you also need to change this "if" condition to prevent overflowing the buffer, since you are now adding two elements to the buffer for each character and not one.

[amirgon]

Micropython bindings removed this driver and now uses LVGL built-in SDL driver.
Any change to SDL driver should be done on the LVGL SDL driver.