Add string macro advanced key support

[yiancar]

As discussed here is a first draft for the string macro:)

I used AI to help make a UI for testing but it looks quite smart:
peppapighs/hmkconf#7

[peppapighs]

Oh I thought we want to implement the single linked list in case we want to support full macro in the future?

[yiancar]

Yes you are right this was simpler for me to write 😢
If you like I can go back and try the linked list approach even tho its a bit more complex.
I know right now this is not the best utilization of space as we might have dead space in the middle of macros, especially if macros get deleted and readded.
What do we mean by full macro?

[peppapighs]

By full macro, I meant macro system where we support setting delay for each action, and possibly pointing the event to the next one to create a loop etc.

I don't really have the full picture of how it will look like, but I imagine either the web configurator or the firmware to walk through the macro and compact/garbage collect the linked list buffer if it is full. Each node can have a key, delay in matrix cycle/milliseconds (potentially 0 so we can press a character and shift and the same time), and the next node.

For the first simple version, I think maybe we can start with just a linked list of characters to support sending string for now, and keep the UI you proposed for hmkconf since the UI for the full macro system probably requires a lot of thought.

For the garbage collection, I meant something like:

• We have a linked list buffer with a pointer to track the number of element, and the list of macro, which is just an array of indices into the buffer.
• When adding a new linked list, we just modify the nodes that the pointer points to and increment the pointer forward. Keep doing this until the end of macro.
• When deleting a macro, we simply just remove the index from the list of macro. This will result in some unused space in the buffer, but we don't care about it for now.
• Eventually, the buffer is full. We loop through each macro in the list, and compact them (kinda like disk defragmentation), so we claim back the unused elements in the array.

Might be too complex but I'm open for suggestions. Sorry for the back and forth, but it's kinda sad that if one day, we implement this macro system then its functionality will overlap with this PR.

[yiancar]

No issues I love the conversation!
Yes this makes a lot more sense to me right now. In the current implementation there is a delay associated with each key but indeed you cannot press two keys at the same time.

The proposed cleanup is quite sophisticated but also very bulletproof!
I will work on this:)

[yiancar]

Hello! I added another 2 bytes per "step" as a pointer to the next item in the list.
I have tested the patch and its working as expected.

BTW I dont think we need garbage collection neceserily.
Currently if a macro is removed (or modified, which rewrites the macro) the UI will find all the empty elements in the list and populate them. This way we get full use of our space even.

One disadvantage is that now with 512 byte space we only get around 100 total macro "steps" were before we were at 170. But this does make our list much more scalable.

Let me know what you think.
A few questions as well:

1. Currently if we have 2 keycodes with delay 0, the delay is not actually 0 but rather, next matrix scan cycle. I personally I am ok with this as it keeps the firmware cleaner but if you like I could make them register on the same cycle? If so maybe only on the Press or Release and not on TAP? (as tap clearly doesnt make sense).

2. Would you like me to give a go on making loops? Maybe I could make a thing where from element X to element Y can be repeated Z times?

Cheers!

[peppapighs]

Thanks for making the changes. I'm sick right now, so not sure if you can review this soon though.

[yiancar]

Hey Pep!
Sorry to hear. Take your time and get better. Let me know if I can help in any way:)

[peppapighs]

I left some comments from my first glance at the code. Let me know your thought and thank you for writing this PR!

As for your questions:

1. I agree with keeping the firmware clean. It probably doesn't matter in practice anyway whether we do it in the same cycle or not.
2. I think let's leave that aside for now. I still don't have a good picture of how the UI/UX would be like if we support repeating/looping. I suppose the implementation should not be too difficult since we are essentially implementing a tiny state machine. Open for some ideas though.

[peppapighs]

Not sure if I'm missing something but maybe we can use deferred action here for tapping, so that the tick rate config is in effect for this too.

[yiancar]

I tested this and I dont think we can use deferred tap here.
Mainly because the macro needs to run in order. In the situation where we have "PRESS shift, TAP a, RELEASE shift"
The release shift might happen (due to the tick of the macro) before the deferred tap has finished.

If you really prefer using deferred tap I suggest we do either:
A) hard wire a wait after macro TAP based on the tick time of deferred macro before moving to the next item in the list.
B) Add a callback from the deferred macro function to know that it has finished before moving to the next item on the list.

In both this situations the disadvantage is that the taps of macros will not be as fast as possible (currently next matrix scan cycle) but this might be acceptable. Let me know your thoughts on this.

[peppapighs]

I'm generally ok with A) to keep it consistent with tap-hold and DKS. I acknowledge the disadvantage here, but it's probably doesn't matter in practice.

[yiancar]

Sorry I have been out of the country I aim to fix this in a few days.
I think B) is better structure so I will try that and if to complex fall back on A).

[peppapighs]

I would prefer A) over B) though since I think deferred action should just be on its own, not needed to know about macros. Anyway, please take your time.

[yiancar]

Yes agreed I was thinking of making the "deferred action" function call you back or reply to you once the deferred action is done, so its reusable.

[yiancar]

@peppapighs Hey ya! ok its now done with option A)
I tried option B) but keeping a queue of each deferred action and then getting a callback was a but too much for this.
This way you get a small delay between issuing every deferred action which to work nicely.

[yiancar]

Done except the last point which I would like to discuss a bit more.
I also changed the UI to have macro instead of string macro:)

I have some UI ideas about making loops etc but maybe we can leave that for later indeed!

[yiancar]

Hey Pep just pinging in case you missed it. lmk if i can help with anything else now:)

[peppapighs]

Hi, sorry it was a busy week for me. I'll try to review it tomorrow.

[peppapighs]

@yiancar could you allow me to push some commit to your pull request? have some refactor I want to push. mostly just some refactoring, but I kinda regret saying that we should use deferred actions for tap macro, so I've made some changes to have macro maintain its own simple tap action. sorry for the back and forth.

[yiancar]

Yep no problem please feel free :) @peppapighs I think github already allows you to push ontop of this right?

[peppapighs]

Hmmm I have trouble pushing the commit. Can you help me apply this diff instead? Thanks!

refactor.patch

[yiancar]

I am making a change because as is your patch would infinitely run the macro while the key is help where i think the behaviour should be only once?

Also AI spotted some unguarded behaviour:
High: macros can auto-run without being pressed.
In src/advanced_keys.c (line 303), advanced_key_init() is empty, so static ak_states starts as all zeroes. But MACRO_NODE_NONE is 255, and advanced_key_tick (line 393) treats current_node == 0 as an active macro node. So any configured macro advanced key may start processing node 0 on tick even if the key was never pressed. Same problem after advanced_key_clear (line 330), because it memsets state back to zero after setting macro state to MACRO_NODE_NONE.

High: macro taps can leave a key stuck pressed.
For MACRO_ACTION_TAP, the patch directly calls layout_register() and waits for deferred_tap_ticks to unregister later. But advanced_key_macro_stop (line 230) only releases active_keycodes; tap keycodes are not added there. If the macro key is released, profile changes, or advanced keys are cleared before the tap timeout elapses, the tap key can remain registered.

I am pushing some changes on this lmk if you are happy or I can roll back?

[peppapighs]

I am making a change because as is your patch would infinitely run the macro while the key is help where i think the behaviour should be only once?

Would that only happen if the macro linked list contains a loop? My thought is that it is correct for macro to run infinitely in that case, so we can support a feature like keep repeating keys indefinitely while held. What do you think?

Also, thanks for the bug fix. AIs are so good at catching bugs these days 😄

[yiancar]

I think if we then make it a loop it should run infinitely. But that would be because the list point back to the origin for example and not by nature.

A lot of people use this simple macros to write their address quickly etc.

I have also asked ai to update my hmkconfig to match ur changes. That needs a bit more code review as I dont know how to write svelte 😆

[peppapighs]

Sorry, I still don't get why we need the visited nodes. Like if you want the macro to run once, would it not be sufficient to set the last node to point to MACRO_NODE_NONE?

[yiancar]

Sorry you are right... I was a bit hasty with pushing

[peppapighs]

Thank you for the PR!

[yiancar]

Thank you! could you also please look at the matching hmkconfig pr too?