Skip to content

new taphold implement, permissive hold and chordal hold #371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hitsmaxft
Copy link
Contributor

@hitsmaxft hitsmaxft commented May 16, 2025

To resolving HRM bugs like #288 #362 #363

and introducing more features, i make a refactory on keyboard taphold processing , with
Integration test cases

Refractory tap hold code

from polling event channel while TH pressing , to using buffer , to fix bug for multiple hold key

I try to use a event buffer to save first tap hold keys and following keys to trigger hold and tap behavior as user expected.

Every buffered event will be check in main event channel loop

match select(  timers in event  buffer , key_channel).await {
   timer timeout  => {  process buffer event...  } ,
   new key arrive => {  add to buffer or update existing hold keys...  } 
}

It would be easier to buffering more key sequence while user typing, and do long-distance decision on tap hold.

  • iterating buffer events while new key arrives, to implement different hold styles just like ZMK does.
  • can export key processor API to user space, just like QMK. User can implement a custom per-key strategy.

Introduce new features for timerless HRM

two new option for hrm

for mod key combination, for example

left hand
TH(lgui, a) , th(lalt,s)

right hand

th(rgui, j), single(k)

to send mod and single with hrm , there should be key pressing sequence like this:

th, th, th... single.single......

###permissive hold

choosing hold behavior on releasing any key (except first TH,but not necessarily), should flush buffer and turn th keys which pressed earlier, into hold keys,then later pressed should be tap

single keys are buffered too. them will be send hid when hold timer reach deadline or releasing key event should up

if single key press in middle of TH keys, this situation ,
by default, flush buffer and all keys still initially should into tap key. but I need more research to make a right strategy

chordal hold (experimental)

for two hands key combination. fire all th into hold in first hand, when a pressing key event raise from cross-hand

permissive hold is name from QMK, which equals to "balance favor" in ZMK , or hold-on-release option
chordal hold is very simple implemented right now, per-key position configuration is not support yet

todo list

  1. deprecate and remove post wait timeout
  2. complete chordalhold will be done at next pr. stay in experimental right now
  3. if bugs are not so much, I will continue finish
  4. userpace API need discussing. but will not implement

* refactory tap build from pooling event channel to buffer vec
* fix bug for multiple hold key

introduce new featues to timeless HRM

* permissive hold: for same hand hold while release following
  keys
* chordal hold: hold on presss corss hand

TODO

deprecate and remove post wait timeout
@hitsmaxft hitsmaxft force-pushed the feature/new_hold_tap branch from 848ece0 to 31e6007 Compare May 16, 2025 06:37
@hitsmaxft hitsmaxft changed the title new taphold implement, buffering and state new taphold implement, permissive hold and chordal May 17, 2025
@hitsmaxft hitsmaxft changed the title new taphold implement, permissive hold and chordal new taphold implement, permissive hold and chordal hold May 17, 2025
@@ -12,10 +12,12 @@ use heapless::Vec;
use macro_config::KeyboardMacrosConfig;
#[cfg(feature = "_nrf_ble")]
pub use nrf_config::BleBatteryConfig;
use num_enum::FromPrimitive;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused import?

pub hand: ChordHoldHand,
}

impl<const COLS: usize> ChordHoldState<COLS> {
Copy link
Owner

@HaoboGu HaoboGu May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes the keyboard are split by rows, not only cols, for example: https://github.com/eduardobraun/corners/blob/main/keyboard.toml

Some(
self.holding_buffer
.iter()
.filter_map(|e| match e {
Copy link
Owner

@HaoboGu HaoboGu May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each find_earliest_event needs a iteration of holding_buffer seems not efficient?

I think something like a priority-queue might be better? So the queue will be resorted only when new items are pushed into the queue, and for getting, it's O(1).

self.process_inner(key_event).await;
debug!("new loop, buffer: {:?}", self.holding_buffer);
//TODO add post time wait timer here
let hold_timeout_event = self.find_earliest_event();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make it consistent, use find_hold_timeout_event?

[2, 3, false, 100], // press d
];
let expected_reports = key_report![
[0, [kc8!(A), 0, 0, 0, 0, 0]],
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective, the result should be:

let expected_reports = key_report![
                    [0, [kc8!(A), 0, 0, 0, 0, 0]],
                    [0, [kc8!(A), kc8!(S), 0, 0, 0, 0]],
                    [0, [kc8!(A), kc8!(S), kc8!(D), 0, 0, 0]],
                    [0, [0, kc8!(S), kc8!(D), 0, 0, 0]],
                    [0, [0, 0, kc8!(D), 0, 0, 0]],
                    [0, [0, 0, 0, 0, 0, 0]],
];

which respects the order and the state of pressed key?

@hitsmaxft
Copy link
Contributor Author

After discussing with Gu, we've decided to remove the post timer from this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants