Keyboard code changes tracking

[davidqge]

Following are the problems I am running into, and the change I think need to fix them:

• mod tap is un-usable. I think the problem is while mod key is in delay mode, the second key was processed out of order. The channel should queue keypress before keymap processing, instead of after.
• holding the key will keep sending key reports even though they haven't changed. The scroll of logs prevent look at the timing of wrong key presses. I think this can be solved by queue above too.
• keyboard output should change to keycode instead of hid report. To allow enter BLE password, and allow more flexibility in how to send report.
• mod tap, mouse button 2, quick tap would press mouse button 2, not release it. I think above change the keyboard to output keycode would ensure that keycode are generated properly like non mouse keys.
• Scanner currently output high/low, this would short circuit on matrix without diode. Change to open drain low, which is designed for such task.

[davidqge]

Here is log for mod taping TAB , released properly

19.520418 DEBUG matrix true 4 5
19.631786 DEBUG matrix false 4 5
19.631851 DEBUG Release tap hold, got TAP: Key(Tab), KeyState { pressed: true, changed: true, hold_start: Some(Instant { ticks: 19520480 }) }
19.642110 DEBUG Sending keyboard report: [0x2B, 0x0, 0x0, 0x0, 0x0, 0x0], modifier: 0
19.642214 DEBUG BLE notify 8 [0x0, 0x0, 0x2B, 0x0, 0x0, 0x0, 0x0, 0x0]
I (19642) NimBLE: GATT procedure initiated: notify;
I (19642) NimBLE: att_handle=31

19.643959 DEBUG Sending keyboard report: [0x0, 0x0, 0x0, 0x0, 0x0, 0x0], modifier: 0
19.650495 DEBUG BLE notify 8 [0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0]
I (19650) NimBLE: GATT procedure initiated: notify;
I (19650) NimBLE: att_handle=31

But mod tap mouse button are not released

210.628076 DEBUG matrix true 6 4
210.740198 DEBUG matrix false 6 4
210.740266 DEBUG Release tap hold, got TAP: Key(MouseBtn2), KeyState { pressed: true, changed: true, hold_start: Some(Instant { ticks: 210628131 }) }
210.750701 DEBUG Sending other report: [0x1, 0x2, 0x0, 0x0, 0x0, 0x0]
210.750760 DEBUG BLE notify 5 [0x2, 0x0, 0x0, 0x0, 0x0]
I (210750) NimBLE: GATT procedure initiated: notify; 
I (210750) NimBLE: att_handle=46

[HaoboGu]

=Thanks for your feedback!

I reviewed current mod-tap implementation, there are some issues that I can tell:

1. If another key is pressed when within the mod-tap's threshold(200ms), this key will be recognized as a normal key, without modifier activated. There are actually 3 modes, see qmk tap hold doc: https://docs.qmk.fm/tap_hold#tap-or-hold-decision-modes. Now RMK uses default mode.

2. holding the key will keep sending key reports.

   this is not expected. Holding a normal key should send only one report. The cause should be this code:
   

   rmk/rmk/src/keyboard.rs

   Lines 509 to 512 in 428d8fb

   	if d > 200 {
   	self.process_key_action_normal(hold_action, key_state, sender)
   	.await;
   	}

3. keyboard output should change to keycode

   If we emit keycodes from Keyboard, then the communication task is responsible to convert keycodes to HID report. This operation is same with what we've done in Keyboard, I don't see the advantage of this change actually. We can discuss about it more to find a better solution, since I'm planning to add other input devices such as encoder/trackpad, the modularity of the project is quite related imo.

4. mod tap, mouse button 2, quick tap would press mouse button 2

   yeah, this is a bug! I can repro it. I guess this is because I somehow mixed keyboard mouse keys and mouse report(but I haven't found the cause

5. Good catch! we should set the pin mode in the pin initializer

[davidqge]

1. If another key is pressed when within the mod-tap's threshold(200ms), this key will be recognized as a normal key, without modifier activated. There are actually 3 modes, see qmk tap hold doc: https://docs.qmk.fm/tap_hold#tap-or-hold-decision-modes. Now RMK uses default mode.

Thanks for the link, I think PERMISSIVE_HOLD is most natural, and like to get it ported.

If we emit keycodes from Keyboard, then the communication task is responsible to convert keycodes to HID report. This operation is same with what we've done in Keyboard, I don't see the advantage of this change actually. We can discuss about it more to find a better solution, since I'm planning to add other input devices such as encoder/trackpad, the modularity of the project is quite related imo.

Here are changes I needed for reporting code

• get keycode for BLE passcode
• boot mode keyboard
• esp32s3 USB-OTG only has 6 endpoints, 2 for keyboard, 1 for mouse, 2 for Vial, 1 for boot mode. I like to merge the system control and consumer into keyboard.
• NKRO

Current link between action code and report code are hardcoded to one specific usage configuration, also needs two steps: process and then send. This is error prone. consider that both action code and report code will get more complex, I think move report code to a separate module, with keycode between them will make code much easier to maintain

If you are going to add trackpad, encoder, then separate report code makes more sense, because now you can reuse the report code. Assuming you are not planning to put everything into the current keyboard module.

[davidqge]

another problem with mouse key, quick click mouse move sends multiple messages. the reason I want mouse key is to move single pixel.

61.230299 DEBUG matrix true 7 0
62.383077 DEBUG matrix true 2 3
62.383324 DEBUG Sending other report: [0x1, 0x0, 0x0, 0x8, 0x0, 0x0]
62.383374 DEBUG BLE notify 5 [0x0, 0x0, 0x8, 0x0, 0x0]
I (62383) NimBLE: GATT procedure initiated: notify;
I (62383) NimBLE: att_handle=46

62.404757 DEBUG Sending other report: [0x1, 0x0, 0x0, 0x8, 0x0, 0x0]
62.404813 DEBUG BLE notify 5 [0x0, 0x0, 0x8, 0x0, 0x0]
I (62404) NimBLE: GATT procedure initiated: notify;
I (62404) NimBLE: att_handle=46

62.425614 DEBUG Sending other report: [0x1, 0x0, 0x0, 0x8, 0x0, 0x0]
62.425683 DEBUG BLE notify 5 [0x0, 0x0, 0x8, 0x0, 0x0]
I (62425) NimBLE: GATT procedure initiated: notify;
I (62425) NimBLE: att_handle=46

62.446542 DEBUG Sending other report: [0x1, 0x0, 0x0, 0x8, 0x0, 0x0]
62.446603 DEBUG BLE notify 5 [0x0, 0x0, 0x8, 0x0, 0x0]
I (62446) NimBLE: GATT procedure initiated: notify;
I (62446) NimBLE: att_handle=46

62.467621 DEBUG Sending other report: [0x1, 0x0, 0x0, 0x8, 0x0, 0x0]
62.467682 DEBUG BLE notify 5 [0x0, 0x0, 0x8, 0x0, 0x0]
I (62467) NimBLE: GATT procedure initiated: notify;
I (62467) NimBLE: att_handle=46

62.488630 DEBUG Sending other report: [0x1, 0x0, 0x0, 0x8, 0x0, 0x0]
62.488677 DEBUG BLE notify 5 [0x0, 0x0, 0x8, 0x0, 0x0]
I (62488) NimBLE: GATT procedure initiated: notify;
I (62488) NimBLE: att_handle=46

62.492038 DEBUG matrix false 2 3
63.430705 DEBUG matrix false 7 0
63.430783 DEBUG Release tap hold, got HOLD: LayerOn(3), KeyState { pressed: false, changed: true, hold_start: Some(Instant { ticks: 61230354 }) }

[davidqge]

I am thinking just remove need_send_key_report: bool, and combine process and send, would fix these missing message, and extra messages issue. Also make the code a lot cleaner, without moving the report code to another module.
and we can implement the repeat key with initial delay for the repeat case you wanted.

[HaoboGu]

I am thinking just remove need_send_key_report: bool, and combine process and send, would fix these missing message, and extra messages issue. Also make the code a lot cleaner, without moving the report code to another module.

and we can implement the repeat key with initial delay for the repeat case you wanted.

if I understand you correctly, this will cause the sent keyboard report will have only one key in each report. am I right?

It will be a report flood and I don't know whether it will cause problems, since the USB host requests hid report in 1khz frequency. Could you please have it a try? I recently don't have a lot of time doing this... if it's good, I'd be happy to change.

[davidqge]

if I understand you correctly, this will cause the sent keyboard report will have only one key in each report. am I right?

It will be a report flood and I don't know whether it will cause problems, since the USB host requests hid report in 1khz frequency. Could you please have it a try? I recently don't have a lot of time doing this... if it's good, I'd be happy to change.

Each key change will send a report. That's what you do currently. You call different functions depends on whether it's the first or last or middle report in order to send.

I just simplified the code, and that bug, mouse button tap hold don't release are automatically fixed. This is strange, because standalone mouse button works, and normal tap hold works. complicate code breeds bugs.

[HaoboGu]

Each key change will send a report. That's what you do currently. You call different functions depends on whether it's the first or last or middle report in order to send.

Current implementation register a key while processing, send all registered key after a full scan.
Only a few exception here, most of them needs an async delay, such as a tap key or macro delay. So theoretically at most 6 key changes are sent each scan

[davidqge]

So theoretically at most 6 key changes are sent each scan

The reason you want to scan faster is to distinguish which key changes happen earlier. The order of change is important. We don't want multi key changes in one scan. At least very rarely.

[HaoboGu]

The reason you want to scan faster is to distinguish which key changes happen earlier. The order of change is important. We don't want multi key changes in one scan. At least very rarely

I don't think so. The scan rate is faster than the communication rate. So if we can compress multiple key changes in one scan loop and send it, it's actually faster than send many reports where each report has only one key.

Scanning faster and maximizing the bandwidth are both important (for me)

[davidqge]

I don't think so. The scan rate is faster than the communication rate. So if we can compress multiple key changes in one scan loop and send it, it's actually faster than send many reports where each report has only one key.

Because you are scanning very fast, you'll hardly get multiple change in one loop. On the other hand, you can combine change from multiple loops. That's why I want to decouple scan loop and report generation.

The change I made is to simplify front end key to action map process. when I am worried about what action to generate, I don't want to worry what actions can be combined into one report.

Now, we got the action stream generated correctly; we need to do all kinds of crazy things to transform actions:

• Cap word, this like a god send, when the Cap make vim commands go crazy.
• Colemak option
• windows, mac option
• tap dance, double click action.
• Quick tap, repeat after tap and hold
• MACRO playback after channel, to avoid fill up channel space

how about have a process_keycode folder, like QMK?

And finally, when all done, and time to report,

• combine multiple key changes in one report,
• mouse key repeat and acceleration
• BLE password, I need to get this working.

Move these code except BLE under USB folder?

• KeyAction is 10 bytes. Via Keycode is only 2 bytes, and they are the code used in storage. So I am thinking use Via code in keymap, and channel, this would save a lot of space, and can add a lot of features without that stack protection error.
• enum KeyCode. code above 0xff is different from the Via code. Do you mind change this enum to match Via? So we have less conversion to do.

[HaoboGu]

I agree that generating action and processing key action to report can be decoupled, and we can move the key processing to a separate module. BUT I also worry about the extra latency added: last time I added the key report channel, the latency increased about 0.5-1ms(Channels are static and introduce lots of mem copy). I think this needs more testing to ensure that this refactoring won't have much negative impact on performance.

This is a huge refactoring, I want to make all input/output devices compatible with the new structure, so before we start I'd like to write a design doc that makes everything clear. And when we actually do this, splitting them to many small PRs would be good.

As for the keycode issue, I use KeyAction in keymap is similar: I prefer one-time conversion other than converting via keycode in key processing. KeyAction also makes key processing code more readable. This adds 0.8KB RAM usage per layer for a full-size(104 key) keyboard, which is acceptable for me.

In my plan, I'm not going to be compatible with all via/vial keycodes(they are different). RMK would have a separate web user interface that works natively with BLE. I choose to support vial just because we don't have it right now.

Anyway, there are lots of things to be discussed. If you're interested, you can join the discord channel, I feel chatting on dc can be more casual.