Added support for Sequences (aka macros)

[riskable]

Demo: https://youtu.be/M2OXrNP3fgI

Example usage:

use keyberon::action::{k, l, m, kp, kr, tap, Action, Action::*, SequenceEvent};
use keyberon::key_code::KeyCode::{self, *};

const MACROTEST: Action = Sequence {
    delay: 10,
    actions: &[
        SequenceEvent::Press(LCtrl), SequenceEvent::Press(LShift), SequenceEvent::Press(U), // Long form
        tap(Kb1), tap(F), tap(Kb4), tap(A), tap(F), // Short form
        kr(LCtrl), kr(LShift), kr(U), // Still need to get ReleaseAll() working
    ],
};

I couldn't get working SequenceEvent::ReleaseAll() (see: https://github.com/riskable/keyberon/blob/master/src/layout.rs#L324) working. Probably a trivial task for you =)

Also, I'm not sure what the purpose is of that transform() method (it's not used anywhere) so I just guessed in my own implementation, haha. I also didn't implement Action.key_codes() for Action::Sequence because I'm not sure it's necessary. I have a little commented out line in place where it'd go.

Overall it wasn't too bad and I learned a ton. Thanks for the assistance and let me know what I have to change or if it's all bad or whatever =)

[TeXitoi]

Please fmt your code.

[TeXitoi]

You can't modify this enum. It's the public contract of events entering a layout. This enum is not non_exhaustive, and thus adding variants is a breaking change.

[gilescope]

are breaking changes bad for keyboard firmware - I'm build and flash it all in one go so from my side I wouldn't be affected by a breaking change? - maybe we should break now, up the semver and make it non exhaustive at the same time?

[TeXitoi]

Breaking change will say that you don't to just do cargo update to update keyberon. I don't care of making breaking changes, but it must add something to the user.

But here, the problem is more that implementation details leak in the public interface.

[riskable]

@gilescope I think TeXitoi was just making a point (in a nice, technical way) that how I implemented this particular feature sucks, haha. He's right though! I've got a much better solution that I'm currently working on that should allow us to incorporate specific delays between keystrokes instead of just one universal delay that's the same for all Press() and Release() events (in a given macro).

I'm still new to Rust and his wisdom is appreciated 😄 . I'm currently struggling with writing code (a map(), filter() or filiter_map()) that figures out which keycodes need to remain down during the execution of any given sequence. It's not so simple because if you have a Press() but no corresponding Release() until the end of the sequence you need to A) figure out where in the sequence you are and B) cancel out matching pairs of Press() and Release() events but only if they're within the same window of the sequence (that you're currently at).

I actually got the code working to execute simple sequences but it currently doesn't work with modifiers or anything that needs to remain held during the sequence. Hopefully not much longer now!

Aside: In the process of writing this I've learned a lot about how HID keyboards work, haha. Like how you can only have a maximum of six keys down at a time--even though there's no real technical reason behind this limitation. It's just that when folks were defining the USB HID spec they didn't say how many keycodes could be sent at once so vendors (e.g. Apple and Microsoft) limited it to six simultaneous keycodes in their implementations (or at least, that's how it was explained to me). No idea what happens if you try to send more but it's in my TODO list of things to play with =)

[TeXitoi]

The "usb keyboard boot protocol" has this 6 keys (+8 modifiers) limitation. There is alternative USB descriptor, but the one used in keyberon has this limitation (that I didn't find limiting, so I didn't try the "NKRO" descriptor support).

[riskable]

All fixed I hope! I followed your instructions as best I understood them and also made Delay() work! Though to use it you have to do something like this:

const MACROTEST2: Action = Sequence {
    events: &[
        Press(A), Release(A),
        Delay {since: 0, ticks: 5000},
        Press(F), Release(F),
    ],
};

Maybe it could use a shortcut function?

[riskable]

Just want to mention #10 so this PR gets tracked in that issue.

[TeXitoi]

Please run fmt and fix clippy warnings.

[riskable]

I made the changes necessary to make Clippy happy but they weren't to my code... That's why I left them alone previously (because I didn't want to mess up your stuff--in case you intentionally wanted the code to be as it was).

Anyways, all set!

[riskable]

I have decided to become the hero we all deserve! Just pushed something awesome: Not only do we now support sequences but they can be of nearly limitless length! Muwahahaha!

Clippy is still complaining about a "never loop" and I tried a whole bunch of ways to fix it and none of them worked. It's annoying the heck out of me right now.

I've been pestering various Rust chats asking for help but I'm getting a surprising lack of answers.

Tried this but the unit tests fail:

let chunk_length = self.sequenced.capacity() - 1; // -1: Keep a slot for Continue()
                for key_event in events.chunks(chunk_length).next() {
                    match *key_event {
                        [SequenceEvent::Press(keycode)] => {
                            self.sequenced.push_back(SequenceEvent::Press(keycode));
                        }
                        [SequenceEvent::Release(keycode)] => {
                            self.sequenced.push_back(SequenceEvent::Release(keycode));
                        }
                        [SequenceEvent::Delay { since, ticks }] => {
                            self.sequenced
                                .push_back(SequenceEvent::Delay { since, ticks });
                        }
                        _ => {} // Should never reach a continue here
                    }
                }
                // Add a continuation
                self.sequenced.push_back(SequenceEvent::Continue {
                    index: 0,
                    events,
                });

[TeXitoi]

You still have a lot of not fmted code.

For the clippy warning,

for e in iter {
    // something
    break;
}

is equivalent to

if let Some(e) = iter.next() {
    // something
}

[riskable]

Those two are not equivalents. I get the same error as I get when I changed it to for key_event in events.chunks(chunk_length).next():

I'm still trying to figure it out.

Also, I did run rustfmt on the code. What do you see that isn't formatted?

[riskable]

Aha! Thanks to some help in Discord I've successfully made Clippy happy. I was so confused on that one, haha.

I also re-ran both the rust-analyzer fmt and rustfmt on the code and as far as I can tell the only thing it changed is the rustfmt wants a space after elements in the unit test arrays (e.g. [LCtrl, U] instead of [LCtrl,U] (which rust-analyzer's fmt function doesn't care about).

[riskable]

In my testing I accidentally added a sequence with a ridiculously long delay (two extra 0s haha) and it occurred to me that there's no mechanism to cancel a running sequence without hitting the reset switch. To solve that problem I just added a CancelSequence action (which can be bound to a key like anything else) and SequenceEvent::Complete which can be used instead of several Release() actions. Complete is basically the same thing as the ReleaseAll() concept we discussed previously except it will work on any type of SequenceEvent in case we support stuff like emulated mouse/joystick events in the future.

[riskable]

Anything else you need me to change? The code is passing all tests and has had rustfmt run. I'll change whatever else you want just need some direction 👍

[TeXitoi]

Sorry, not so much time to pause and carefully read this PR. Hopefully soon, I will not forget.

[TeXitoi]

For such a case, maybe that's even better to have some kind of recording directly in the keyboard.

As you described, that's a very complicated feature, needing json parsing and layout modification on the fly.

[riskable]

For such a case, maybe that's even better to have some kind of recording directly in the keyboard.

Shall I get started on that then? 😄

[TeXitoi]

You can really simplify this. You only need one variant at a time, having a deque of 16 is really overkill for memory, and for complexity of code.

You can do everything with

    cur_event: Option<Event>,
   awaiting_events: &'static [Event],

And when you need another event, something like

match self.awaiting_events {
    [e, tail @ ..] => {
        self.cur_event = Some(e);
        self.awaiting_event = tail;
    }
    [] => (),
}

[riskable]

I'll see if I can make that work... I actually tried something like this initially but couldn't get past the borrow checker complaining about not knowing the length of the events ahead of time... If I only had one sequence I could get it to work but if there were multiple sequences of different lengths it would complain because the size of the events array would be different every time.

[TeXitoi]

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=25221a8bfda08ec6eb055ea5a3efcdb7

You can also have a ArrayDeque<&'static [Event]> if you need several slots.

[TeXitoi]

https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first is more appropriate.

[TeXitoi]

My proposition will simplify this.

And what if a sequence is executed during the execution of another sequence? It need a test.

[riskable]

Yeah, multiple simultaneous sequences is an edge case in the TODO list. The way it works right now is that if you kick off a sequence while another one is already running it ends the current sequence and starts the new one. I made the sequenced ArrayDeque 16 items long originally so I could have multiple simultaneous sequences running but the way the logic works right now that can't work. I had planned to use the length of sequenced divided by four so you could have up to four simultaneous sequences going on but the amount of logic that would need to change for that to happen was too much (for the current state of the PR--can't have everything at the start of these things).

[TeXitoi]

You can document the actual behavior, and add an issue.

[TeXitoi]

Any problem with the revîew? Do you need some help to fix it?

I hope I didn't discourage you with this review 😇

[riskable]

You haven't discouraged me... It just took a while for the initial review that I started work on other things 😄

I'm finishing up those other things (tease: https://imgur.com/a/IFsvBGh) and will be returning to this in the next few days. I did start work on it though... Already implemented a decrementing counter instead of using the since/ticks method 👍

[riskable]

I'm done making my keyboard PCB and was able to get back to work on this! I implemented all of your suggested changes and added some general improvements (can handle up to four simultaneous running sequences now!). Had a hell of a time with the merge from upstream though haha. Hopefully I didn't clobber anything! Double-check that please 😄

I've got complete unit tests for everything except multiple simultaneous sequences running at the same time because I'm not sure how to handle that (in test cases).

[riskable]

Just checking in... In case you missed Github's notification that I've submitted a new update 😁

[TeXitoi]

Seen, just not so much time to work on this. I also have to implement tap hod, and release v0.2

[riskable]

I just merged upstream (everything's up to date again). Any chance you can take a look?

[TeXitoi]

I really want a clean public interface to merge it. Implementation details can be changed later, so OK to merge without the other proposed change.

[riskable]

OK I think I've resolved the "clean public interface" issue which was the last remaining hurdle (I think). @TeXitoi What do you think?