feat(layer): add hold passthrough output emitted while a layer is active (#332)#358
Conversation
|
Warning Review limit reached
More reviews will be available in 18 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a per-layer ChangesLayer hold passthrough feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoAdd layer
WalkthroughsDescription• Add optional hold field to layer config for passthrough output while layer is active • Implement layer hold state management with gamepad bit re-assertion and key/mouse press/release edges • Release held output on all deactivation paths (trigger release, layer switch, mapping switch, reset) • Add comprehensive test coverage including regression guards for held-output leaks and suppression ordering • Document hold field semantics in mapping guide and config reference Diagramflowchart LR
Config["Layer Config<br/>hold field"] -->|parse & validate| Mapper["Mapper State<br/>layer_held_gamepad<br/>layer_hold_aux_down"]
Mapper -->|apply frame| ReAssert["Re-assert gamepad bit<br/>or skip key/mouse"]
Mapper -->|layer activates| UpdateHold["updateLayerHold<br/>press key/mouse"]
Mapper -->|layer deactivates| Release["Release held output<br/>releaseHeldAux"]
Release -->|all exit paths| Clean["Clear state<br/>layer_held_gamepad=0"]
File Changes1. src/config/mapping.zig
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/mapping.zig (1)
301-306:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
layer.holdinto aux capability derivation.Adding
holdhere without updatingderiveAuxFromMapping()means configs likehold = "KEY_LEFTSHIFT"orhold = "mouse_left"can parse and validate, but still fail to provision keyboard/mouse aux support because the layer scan only looks atremap/gyro/stick/dpad. That breaks the new feature for hold-only aux outputs.Possible fix
if (cfg.layer) |layers| { for (layers) |*layer| { if (layer.tap) |target| scanTarget(&caps, target); if (layer.hold) |target| scanTarget(&caps, target); if (layer.gyro) |g| { if (std.mem.eql(u8, g.mode, "mouse")) caps.needs_rel = true; } scanStick(&caps, layer.stick_left); scanStick(&caps, layer.stick_right); if (layer.dpad) |d| { if (std.mem.eql(u8, d.mode, "arrows")) caps.needs_keyboard = true; } if (layer.remap) |*remap| scanRemapTargets(&caps, cfg, remap); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config/mapping.zig` around lines 301 - 306, deriveAuxFromMapping() currently ignores LayerConfig.hold so configs with hold-only aux targets (e.g., hold = "KEY_LEFTSHIFT" or "mouse_left") won't mark keyboard/mouse capabilities; update deriveAuxFromMapping() to treat layer.hold the same as layer.tap by calling scanTarget(&caps, target) when layer.hold is present, and ensure any mode checks (e.g., for mouse gyro -> caps.needs_rel or dpad arrows -> caps.needs_keyboard) and existing calls to scanStick() and scanRemapTargets() remain unchanged so hold-targets are included in the aux capability derivation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/mapper.zig`:
- Around line 785-803: The updateLayerHold currently unconditionally calls
emitAuxDownRelease(self.layer_hold_aux_down, aux) and clears layer_hold_aux_down
before resolving the next active layer, causing a release+press when the
resolved target stays the same; change the logic in updateLayerHold to first
compute the next target (use self.layer.getActiveIndex(configs) and
self.resolved_layer_holds[idx] and auxDownTarget/enum value) and compare it to
the existing self.layer_hold_aux_down (and gamepad bit in
self.layer_held_gamepad) — only call emitAuxDownRelease and clear the old state
if the new target differs, otherwise skip release/press and keep the existing
hold; ensure you still update layer_held_gamepad when switching to/from a
gamepad target and still call remap_mod.applyTarget(..., .press, ...) only when
transitioning to a different aux target.
---
Outside diff comments:
In `@src/config/mapping.zig`:
- Around line 301-306: deriveAuxFromMapping() currently ignores LayerConfig.hold
so configs with hold-only aux targets (e.g., hold = "KEY_LEFTSHIFT" or
"mouse_left") won't mark keyboard/mouse capabilities; update
deriveAuxFromMapping() to treat layer.hold the same as layer.tap by calling
scanTarget(&caps, target) when layer.hold is present, and ensure any mode checks
(e.g., for mouse gyro -> caps.needs_rel or dpad arrows -> caps.needs_keyboard)
and existing calls to scanStick() and scanRemapTargets() remain unchanged so
hold-targets are included in the aux capability derivation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e488ae6-6725-4c94-98c9-51a34509d041
📒 Files selected for processing (5)
docs/src/mapping-config.mddocs/src/mapping-guide.mdsrc/config/mapping.zigsrc/core/mapper.zigsrc/device_instance.zig
…ten suppression assertion (#332)
2cc26a3 to
433ab02
Compare
What
Adds an optional
holdfield to[[layer]]: while the layer is active, padctl continuously emits a chosen output in addition to the layer's other effects (remap/gyro/stick). This lets a button that triggers a layer also keep emitting an output — e.g. Witcher-3 "Sense":LBactivates a layer and still sendsLB.Semantics are uniform "held while the layer is active" across all activation modes (
hold/hold_toggle/toggle) — it hangs off the single active-state chokepoint, with no per-mode special-casing.Changes
src/config/mapping.zig— newLayerConfig.hold: ?[]const u8(auto-accepted by the structFieldNames schema linter);validate()rejects amacro:target (error.LayerHoldCannotBeMacro).src/core/mapper.zig— pre-resolve each layer's hold target (resolved_layer_holds, parallel toresolved_layers). Gamepad target: re-assert the bit intoinjected_buttonsevery frame while active, and intocurrentMappedGamepadFramefor the sticky/timer frame. Key/mouse target: explicit.press/.releaseedges via the singleupdateLayerHoldchokepoint. Held output is released on every deactivation path: trigger release, layer switch, mapping/profile switch (quiesceOutputs→releaseMapperAux), andresetRuntimeState.docs/src/mapping-guide.md,docs/src/mapping-config.md— documentholdnext totap, with the "fires only after activation, not on a short tap" note.Notes
holdandtapare orthogonal:tap= short-press output,hold= held-while-active output.Tests (Layer-0, no device I/O)
macro:target rejectedhold_togglesticky on/off;togglelatched on/offresetRuntimeState+quiesceOutputsaux path (DeviceInstance integration)hold == triggernets a single clean press (suppression ordering, both emit sites)Each test was confirmed falsifiable by mutation testing (reverting each load-bearing line makes a specific test fail).
Test plan
./scripts/padctl-docker test: exit 0zig build -Doptimize=ReleaseSafe -Dtarget=x86_64-linux-musl -Dlibusb=false: exit 0zig fmt --check: cleanRefs #332
Summary by CodeRabbit
New Features
Documentation
Bug Fixes