kmk_firmware icon indicating copy to clipboard operation
kmk_firmware copied to clipboard

pystack exhausted when holdtap and combos are used together

Open ald0o opened this issue 2 years ago • 8 comments

Here is the debug trace:

Traceback (appels les plus récents en dernier) :
  Fichier "main.py", ligne 209, dans <module>
  Fichier "kmk/kmk_keyboard.py", ligne 440, dans go
  Fichier "kmk/kmk_keyboard.py", ligne 500, dans _main_loop
  Fichier "kmk/kmk_keyboard.py", ligne 271, dans _process_timeouts
  Fichier "kmk/modules/combos.py", ligne 170, dans <lambda>
  Fichier "kmk/modules/combos.py", ligne 272, dans on_timeout
  Fichier "kmk/modules/combos.py", ligne 287, dans send_key_buffer
  Fichier "kmk/kmk_keyboard.py", ligne 208, dans resume_process_key
  Fichier "kmk/kmk_keyboard.py", ligne 188, dans pre_process_key
  Fichier "kmk/kmk_keyboard.py", ligne 196, dans process_key
  Fichier "kmk/keys.py", ligne 493, dans on_release
  Fichier "kmk/modules/holdtap.py", ligne 148, dans ht_released
  Fichier "kmk/modules/holdtap.py", ligne 192, dans ht_activate_tap
  Fichier "kmk/kmk_keyboard.py", ligne 208, dans resume_process_key
  Fichier "kmk/kmk_keyboard.py", ligne 188, dans pre_process_key
  Fichier "kmk/kmk_keyboard.py", ligne 194, dans process_key
  Fichier "kmk/keys.py", ligne 479, dans on_press
RuntimeError: pystack exhausted

This happens when quickly pressing a combo key then a holdtap key belonging to a combo (provided the two keys fail to form a combo together). It can be triggered by pressing twice the same key, for instance, provided it's both a holdtap key and a combo component.

This issue is rather new and did not happen with master head from August 2022 (but I don't know precisely what is the first revision that triggers the crash).

ald0o avatar Sep 14 '22 15:09 ald0o

It is not clear to me that there is an infinite recursion (well, I failed to find what could cause it). Might it be that CircuitPython's stack is just so small that it cannot handle 16 nested calls?

If there is indeed no infinite recursion, the cure could be to limit the stack height to a constant (independent of the numbers of modules using resume_process_key). This could be achieved by a standard trick such as using an event queue instead of handling the next module immediately in a nested call.

  • instead of directly calling process_key, the functions resume_process_key and pre_process_key would feed the queue (call a new function queue_process_key that does just that)
  • the main loop would call a function flush_process_key_queue. Flushing consists in calling process_key on each queue element and emptying the queue.

It seems a LIFO queue would respect current behavior. But having reified this queue makes it easy to change the ordering policy later on, which might help fixing potential semantic issues. Also note that suggested changes are only located in kmk_keyboard.py.

ald0o avatar Sep 16 '22 08:09 ald0o

The pystack is bit more complicated than "up to n-levels deep". We had issues with the pystack before, however, and we just introduced a new mechanic into the key-evaluation. It's possible that there's one recursion to much going on. Did you follow the recommendation of setting the stack size to 8k?

xs5871 avatar Sep 16 '22 08:09 xs5871

The pystack is bit more complicated than "up to n-levels deep".

This was just a rough estimation of what is going on. I understand that all functions do not need the same frame size when called.

We had issues with the pystack before, however, and we just introduced a new mechanic into the key-evaluation. It's possible that there's one recursion to much going on. Did you follow the recommendation of setting the stack size to 8k?

Indeed, I put supervisor.set_next_stack_limit(4096 + 4096) in my boot.py at a time I did not know what I was doing, probably copy/pasting from the documentation. I forgot about it since... Should try and I increase this amount? 8k for 16 calls makes an average of 512 bytes per call, which does not look huge (understatement... ).

ald0o avatar Sep 16 '22 09:09 ald0o

You can adjust that value as needed. Some boards need an increase, some a decrease. It's always a balance of stack VS heap, so you never want to push it too far in any direction. You can also try #588 and see if that reduces fragmentation enough to have an effect.

kdb424 avatar Sep 16 '22 15:09 kdb424

You can also try #588 and see if that reduces fragmentation enough to have an effect.

Wouldn't that just free some heap space?

Did you follow the recommendation of setting the stack size to 8k?

So I tried with a stack twice as big, it changes nothing. I also tried without setting the limit, same result.

ald0o avatar Sep 17 '22 15:09 ald0o

Also reverting the module order (as you suggested on discord) does not change the result. I mean, the stack trace is identical.

ald0o avatar Sep 17 '22 15:09 ald0o

Just in case this helps this also appears to be the case when using oneshot keys with combos. I also tested this when trying to upgrade about a week ago. The last commit I tried that was working was fad6a4 (2022-08-19). The commit f7e0f8 (2022-09-14) is what I tried to upgrade to but it wasn't working. From what I remember the traceback was very similar to the original post. I haven't had the time to track down which commit broke everything.

ldsands avatar Sep 21 '22 21:09 ldsands

@ald0o could you share your config via pastebin/gist/etc. @ldsands:thanks for the hint. We'll look into that as well.

xs5871 avatar Sep 21 '22 21:09 xs5871

I could finally reproduce one of the errors. Working on a fix that doesn't make half of KMK modules unusable.

xs5871 avatar Sep 22 '22 18:09 xs5871

With current master this also happens with just LT in use and with even 12k of stack:

  • hold an LT key
  • use other keys on that layer around 20 times
  • release the LT key and tap it again right away

I can reproduce this almost every time

Stack is a little different:

Traceback (most recent call last):
  File "main.py", line 96, in <module>
  File "kmk/kmk_keyboard.py", line 440, in go
  File "kmk/kmk_keyboard.py", line 499, in _main_loop
  File "kmk/kmk_keyboard.py", line 109, in _handle_matrix_report
  File "kmk/kmk_keyboard.py", line 159, in _on_matrix_changed
  File "kmk/kmk_keyboard.py", line 196, in pre_process_key
  File "kmk/kmk_keyboard.py", line 202, in process_key
  File "kmk/keys.py", line 474, in on_press
  File "kmk/modules/holdtap.py", line 138, in ht_pressed
  File "kmk/modules/holdtap.py", line 251, in ht_activate_on_interrupt
  File "kmk/modules/holdtap.py", line 223, in ht_activate_hold
  File "kmk/kmk_keyboard.py", line 214, in resume_process_key
  File "kmk/kmk_keyboard.py", line 196, in pre_process_key
  File "kmk/kmk_keyboard.py", line 202, in process_key
  File "kmk/keys.py", line 474, in on_press
  File "kmk/modules/layers.py", line 102, in _mo_pressed
  File "kmk/modules/layers.py", line 158, in _print_debug
RuntimeError: pystack exhausted

prokopiff avatar Oct 02 '22 18:10 prokopiff

For those of you still getting errors, can we get a main.py to possibly find something else blowing your stack out and just showing up there?

kdb424 avatar Oct 03 '22 15:10 kdb424

For those of you still getting errors, can we get a main.py to possibly find something else blowing your stack out and just showing up there?

https://github.com/prokopiff/kmk_firmware/blob/027d03df386a2491af96c7b68f13ef03494872c4/boards/lily58/main.py

prokopiff avatar Oct 03 '22 16:10 prokopiff

by the way, I've been working on a general solution for this for the past 2 weeks. We don't want single if-then-eh-it-works... solutions for singular issues -- not sustainable. It's a bit more involved and will likely end up in an architectural change to how we handle our key processing pipeline. That's why it's been taking a while. I apologize for the on-going inconvenience. I hope to have a draft PR up within the next couple of days.

xs5871 avatar Oct 04 '22 18:10 xs5871