Key repeat can get stuck if handling of a key-press event takes too long

Issue #1819 resolved
Albert Shih created an issue

Even for a quick press/unpress of a direction key, if the processing of the key-press event takes too long, the key can get stuck in repeating mode.

Code-based details:

Consider a key press followed quickly by an unpress. If the processing of the key-press event by handle_ml_menu_keys takes too long, the key-unpress event will be backlogged in the event queue. During this time, menu_task continues to decrement and may produce a key-repeat event when the handling of the key-press event is done (and sets keyrep_ack to 1), but before the key-unpress event is actually handled. If that happens, after the key-unpress event is handled, it is followed by a key-repeat event that is "false": the direction key isn't actually being pressed anymore. This key-repeat event is interpreted as if the user has pressed the key again, and since there's no matching key-unpress event, the key then will get stuck in repeating mode.

Event-handling sequence:

press event -> (processing time exceeds countdown time) -> unpress event -> false press event -> (another countdown) -> repeating press events

Notes:

This is "easiest" to trigger by using the right direction to enter a directory in the File Manager module that takes a while to build because of the number of files. If the false press event is created, it quickly start diving deeper into the file structure until it toggles repeatedly between the deepest file list and the file menu for the first file in that list. It can be knocked out of the stuck repeat by hitting any direction key. A workaround is to use the SET key instead to enter directories, as that key does not generate keyrepeat events.

Comments (7)

  1. Albert Shih reporter

    For fixing this bug, I'm not familiar enough with menu.c and the event system to know what's possible. Would it be possible for menu_task to check the event queue and make sure there's not an unpress event for that key already queued up before it adds the key-repeat event?

  2. Alex

    The event queue is DryOS's message queue, and to get its contents you need to reverse engineer it, understand its internal structure, and hope it's the same on all cameras.

    There is a recent API, msg_queue_count, that probably counts the number of events in a queue (it's used in mlv_rec). Maybe supressing key repeat events if the queue is not empty could work?

    edit: probably not, because at while the event is being processed, it was already removed from the queue. What about suppressing key repeat events while handle_ml_menu_keys is busy?

    The GUI task loop is in gui.c.

  3. Albert Shih reporter

    Well, gui_main_struct from gui.c has the message queue, but getting access to that queue, or even just a count, from within menu.c seems to be more trouble than its worth.

    Your suggestion to suppress key-repeat events while handle_ml_menu_keys is busy is already implemented using the flag keyrep_ack. However, the bug here occurs because the key-repeat event is added by menu_task after the handler has completed processing an event, but is unaware that the handler will still be called one or more times on already-queued-up events.

    I've actually stumbled upon a solution, although I worry that it may be highly camera-dependent. This bug is a consequence of the handler not making a distinction between key-press events and key-repeat events, but it turns out (on the 50D, at least), there is a way to distinguish between real button events and ones created by fake_simple_button. From gui-common.h, the event structure is:

    /** Internal structure used by the gui code */
    struct event
    {
            int             type;
            int             param;
            void *                  obj;
            int             arg; // unknown meaning
    };
    

    On my camera, real key-press events of the direction keys have both obj and arg equal to 0, while fake_simple_button events either have obj equal to 1 (in playback mode or in the Canon menu) or have arg equal to 0xfffe1dc0 (in LV or in the ML menu). I don't know why this is, and I don't know how portable this behavior is.

    With the ability to distinguish between real button events and key-repeat events, it's possible to modify handle_ml_menu_keyrepeat and handle_ml_menu_keys such that the key-repeat event is squelched if it is preceded by a real key-unpress event (for a direction key). "Good" key-repeat events that are preceded in an unbroken line to a real key-press event are unaffected. Unfortunately, without some care, this could potentially break any existing code that generates fake button events (for purposes other than key repeating). Thoughts?

  4. Alex

    To distinguish between fake and real, use the IS_FAKE macro (which you just discovered by looking at arguments).

    Portability-wise, the fake flag is stripped before passing the event back to Canon code, so it should not have any side effects.

  5. Albert Shih reporter

    Ah, I see how I led myself astray. I had immediately dismissed the IS_FAKE macro as insufficient because I was seeing key-repeat events with obj equal to 1 and arg equal to 0 (and didn't make the connection between -123456 with 0xfffe1dc0), but I guess that is the signature of Canon-generated key-repeat events rather than ones created by fake_simple_button.

    I'll mull over whether there is a good way to implement the key-repeat rejection such it that fixes this bug without causing edge cases that might affect existing code.

    Edit: To be clear, I observed Canon-generated key-repeat events when I was trying to understand key-repeat events in general. They are not relevant to this bug.

  6. Albert Shih reporter

    Sometimes going simple is the way to go: rather than messing with the queue or trying to distinguish between key-repeat events and faked events from other sources, I can fix this bug by just slightly adjusting how keyrep_ack is used. I'll submit the pull request now.

  7. Log in to comment