Added support for Sequences (aka macros) #30

riskable · 2020-10-16T19:12:44Z

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 =)

…events are executed in succession.

TeXitoi · 2020-10-16T20:00:28Z

Please fmt your code.

Cargo.toml

src/action.rs

TeXitoi · 2020-10-16T20:07:17Z

src/layout.rs

+    /// Press event with just a keycode (for sequences)
+    SequencePress(KeyCode),
+    /// Release event with just a keycode (for sequences)
+    SequenceRelease(KeyCode),


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.

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?

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.

@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 =)

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).

src/action.rs

src/layout.rs

…events are executed in succession.

…lso added a new Delay() feature that lets you put arbitrary delays between sequence (key macro) events.

riskable · 2020-10-19T02:41:32Z

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 · 2020-10-19T15:59:03Z

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

TeXitoi · 2020-10-19T19:39:38Z

Please run fmt and fix clippy warnings.

riskable · 2020-10-19T23:02:07Z

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 · 2020-10-20T02:48:22Z

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 · 2020-10-20T09:06:24Z

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 · 2020-10-20T12:47:30Z

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 · 2020-10-20T13:47:01Z

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).

as well as the ability to use a `Complete` as a sort of "release all".

riskable · 2020-10-20T17:12:56Z

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 · 2020-10-21T18:57:15Z

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 · 2020-10-21T19:54:31Z

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

as a shortcut for `Press(),Release()`

TeXitoi · 2020-10-28T12:58:24Z

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 · 2020-10-28T18:38:47Z

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? 😄

src/action.rs

src/layout.rs

src/action.rs

src/layout.rs

TeXitoi · 2020-10-28T20:38:56Z

src/layout.rs

        self.keycodes()
    }
+    /// Takes care of draining and populating the `sequenced` ArrayDeque,
+    /// giving us sequences (aka macros) of nearly limitless length!
+    fn process_sequences(&mut self) {


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; } [] => (), }

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.

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.

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

TeXitoi · 2020-10-28T20:42:41Z

src/layout.rs

+                // Copy the first chunk of `SequenceEvent`s into the `sequenced` ArrayDeque
+                // ...we'll cover the remaining chunks as we drain them from `sequenced`
+                let chunk_length = self.sequenced.capacity() - 1; // -1: Keep a slot for Continue()
+                if let Some(chunk) = events.chunks(chunk_length).next() {


My proposition will simplify this.

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

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).

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

TeXitoi · 2020-11-08T18:25:18Z

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 · 2020-11-10T01:09:58Z

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 👍

…6a3aadba5caf0474998e1c2eb69af11&dn=Les.Blagues.De.Toto.2020.FRENCH.1080p.WEB.x264-PREUMS.mkv&tr=udp://tracker.openbittorrent.com:80&tr=udp://tracker.opentrackr.org:1337/announce

Added support for multiple simultaneous sequences executed at once (max 4). Renamed CancelSequence to CancelSequences since we now do up to 4 at once.

riskable · 2020-12-15T22:01:06Z

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 · 2021-01-05T14:53:20Z

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

TeXitoi · 2021-01-05T16:32:41Z

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

… upstream)

riskable · 2021-05-03T15:40:17Z

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

TeXitoi · 2021-05-03T16:47:02Z

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 · 2022-05-02T19:30:04Z

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

riskable added 2 commits October 15, 2020 14:02

Fixed description and updated the either dependency to 1.6

3a83060

Added support for Sequence Actions (aka macros) whereby multiple key …

6e5e068

…events are executed in succession.

TeXitoi requested changes Oct 16, 2020

View reviewed changes

riskable added 3 commits October 16, 2020 18:17

Added support for Sequence Actions (aka macros) whereby multiple key …

b56de4f

…events are executed in succession.

Modified per TeXitoi's instructions (as best I understood them) and a…

7df7519

…lso added a new Delay() feature that lets you put arbitrary delays between sequence (key macro) events.

Merge branch 'master' of github.com:riskable/keyberon

d65daac

Removed some leftover commented-out cruft

6067648

riskable force-pushed the master branch from 10ca6f6 to 6067648 Compare October 19, 2020 15:56

riskable added 2 commits October 19, 2020 13:58

Made some changes to make Clippy happy.

95650fd

Unit tests for sequences and improved tick start

255c4d3

Minor fixes to make Clippy happy

18fb401

riskable added 2 commits October 19, 2020 22:10

Support for sequences of nearly limitless length!

37e6b31

Make Clippy happy

b7a77d3

Changed do_action() to make Clippy happy

999b9f2

riskable added 2 commits October 20, 2020 11:30

Fixed a bug where Delay() wasn't working properly

ab0bfe1

Added the ability to cancel running sequences

0785151

as well as the ability to use a `Complete` as a sort of "release all".

Fixed an edge case with CancelSequence

60b08b9

Added SequenceEvent::Tap

0d89989

as a shortcut for `Press(),Release()`

TeXitoi requested changes Oct 28, 2020

View reviewed changes

Guillaume Pinot and others added 3 commits December 12, 2020 22:08

CHANGELOG and more doc for custom actionmagnet:?xt=urn:btih:a6721ad2e…

0660d2b

…6a3aadba5caf0474998e1c2eb69af11&dn=Les.Blagues.De.Toto.2020.FRENCH.1080p.WEB.x264-PREUMS.mkv&tr=udp://tracker.openbittorrent.com:80&tr=udp://tracker.opentrackr.org:1337/announce

Merge remote-tracking branch 'upstream/master'

f3ad3ff

Changed how sequences are processed (MUCH simpler).

3fe71e5

Added support for multiple simultaneous sequences executed at once (max 4). Renamed CancelSequence to CancelSequences since we now do up to 4 at once.

riskable mentioned this pull request Dec 16, 2020

Example Rust firmware implementation? hid-io/hid-io-core#17

Open

riskable added 2 commits May 3, 2021 11:18

Merge remote-tracking branch 'upstream/master'

43ed3e5

Added SequenceEvent back to layout.rs (got removed with last merge of…

f93d8ec

… upstream)

TeXitoi mentioned this pull request Jul 12, 2021

Emulating typing #64

Closed

Merge remote-tracking branch 'upstream/master'

f0e7d65

wezm mentioned this pull request Sep 27, 2021

Macro support wezm/keyberon#1

Merged

riskable added 5 commits December 25, 2021 16:10

Merged upstream

a26a3b6

Added some Sequence test cases

4604ec7

Merge remote-tracking branch 'upstream/master'

2585845

Fixed Sequence tests to use the new syntax

c990d9d

Removed unused enum variant and modified to use a const generic value.

d93436d

riskable added 4 commits October 4, 2022 12:51

Merged upstream

8e2dfa9

Merge remote-tracking branch 'upstream/master' into sequences

883a2d1

Added SequenceEvent back to action.rs

5e67102

Fixed duplicate HoldTapConfig lines

16be78a

borisfaure mentioned this pull request Jul 27, 2023

Shifted sequences #122

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Sequences (aka macros) #30

Added support for Sequences (aka macros) #30

riskable commented Oct 16, 2020

TeXitoi commented Oct 16, 2020

TeXitoi Oct 16, 2020

gilescope Oct 18, 2020

TeXitoi Oct 18, 2020

riskable Oct 18, 2020

TeXitoi Oct 19, 2020

riskable commented Oct 19, 2020

riskable commented Oct 19, 2020

TeXitoi commented Oct 19, 2020

riskable commented Oct 19, 2020

riskable commented Oct 20, 2020

TeXitoi commented Oct 20, 2020

riskable commented Oct 20, 2020

riskable commented Oct 20, 2020

riskable commented Oct 20, 2020

riskable commented Oct 21, 2020

TeXitoi commented Oct 21, 2020

TeXitoi commented Oct 28, 2020

riskable commented Oct 28, 2020

TeXitoi Oct 28, 2020

riskable Oct 30, 2020

TeXitoi Oct 30, 2020

TeXitoi Oct 30, 2020

TeXitoi Oct 28, 2020

riskable Oct 30, 2020

TeXitoi Oct 30, 2020

TeXitoi commented Nov 8, 2020

riskable commented Nov 10, 2020

riskable commented Dec 15, 2020

riskable commented Jan 5, 2021 •

edited

Loading

TeXitoi commented Jan 5, 2021

riskable commented May 3, 2021

TeXitoi commented May 3, 2021

riskable commented May 2, 2022

Added support for Sequences (aka macros) #30

Are you sure you want to change the base?

Added support for Sequences (aka macros) #30

Conversation

riskable commented Oct 16, 2020

TeXitoi commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riskable commented Oct 19, 2020

riskable commented Oct 19, 2020

TeXitoi commented Oct 19, 2020

riskable commented Oct 19, 2020

riskable commented Oct 20, 2020

TeXitoi commented Oct 20, 2020

riskable commented Oct 20, 2020

riskable commented Oct 20, 2020

riskable commented Oct 20, 2020

riskable commented Oct 21, 2020

TeXitoi commented Oct 21, 2020

TeXitoi commented Oct 28, 2020

riskable commented Oct 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TeXitoi commented Nov 8, 2020

riskable commented Nov 10, 2020

riskable commented Dec 15, 2020

riskable commented Jan 5, 2021 • edited Loading

TeXitoi commented Jan 5, 2021

riskable commented May 3, 2021

TeXitoi commented May 3, 2021

riskable commented May 2, 2022

riskable commented Jan 5, 2021 •

edited

Loading