Conversation
Add support for per-layer pointing device modes to PointingProcessor. Users can now configure different layers to have different trackball behaviors, enabling scroll and precision modes via layer switching. Changes: - Add PointingMode enum (Cursor/Scroll/Sniper) with configs - Add MotionAccumulator for sub-pixel delta tracking - Subscribe to LayerChangeEvent to receive layer changes - Add set_layer_mode() and with_layer_modes() builder methods - Refactor on_pointing_event() to support mode dispatch - Add 9 unit tests for accumulator and mode switching Usage example (in processor initialization): processor.set_layer_mode(1, PointingMode::Scroll(ScrollConfig::default())); processor.set_layer_mode(2, PointingMode::Sniper(SniperConfig::default())); Then configure MO(1) and MO(2) keys in keymap. Fully Vial compatible. Tests: 14/14 passed, 0 clippy warnings Memory overhead: ~17 bytes (4-layer keyboard) Backward compatible: default behavior unchanged Addresses HaoboGu#703
Add comprehensive documentation and examples for the new per-layer pointing modes feature (Cursor/Scroll/Sniper). Changes: - Add PMW33xx/PMW3610 docs with per-layer mode usage examples - Update nrf52840_ble_split_dongle example with layer mode configuration - Add 10 new unit tests for integration scenarios - Document mode details, sensitivity tuning, and keymap integration Documentation includes: - Complete API usage examples with all three modes - Sensitivity tuning guidelines (divisor ranges) - Keymap integration patterns (MO/TG layer switches) - Mode behavior details and use cases Tests added: - Mode selection and storage - Scroll/Sniper divisor behavior - Zero-motion prevention (scroll mode optimization) - Asymmetric divisor handling - Negative motion accumulation - Default config values - Saturation protection - Array bounds verification Total test coverage: 23 tests (14 existing + 9 from main commit + 10 new)
Binary Size Reportuse_config/nrf52832_bleDiffuse_config/nrf52840_bleDiffuse_config/nrf52840_ble_splitDiffCentral DiffPeripheral Diffuse_rust/nrf52840_ble_splitDiffCentral DiffPeripheral Diffuse_config/pi_pico_w_bleDiffuse_config/pi_pico_w_ble_splitDiffCentral DiffPeripheral Diffuse_rust/pi_pico_w_ble_splitDiffCentral DiffPeripheral Diffuse_config/rp2040Diffuse_config/rp2040_splitDiffCentral DiffPeripheral Diffuse_rust/rp2040_splitDiffCentral DiffPeripheral Diffuse_config/stm32f1Diffuse_config/stm32f4Diffuse_config/stm32h7Diff |
Schievel1
left a comment
Schievel1
left a comment
There was a problem hiding this comment.
Thanks for doing this. I was about to do a similar thing, but I use scrolling mode on my trackball rather seldom. So the pressure of not having it wasn't motivating me enough. :)
I find sticking this to layers a bit too restrictive, also PointingProcessor is doing a bit too much I think. A more granular approach with different structs, where one, a controller, handles what should be done in a given situation (e.g. a layer change, a special key code etc.) and PointingProcessor just taking commands and executing them (the mode change) is more user friendly and flexible imo.
Also I do not know what this Gazell stuff is about. Is it there accidentally and belongs to a different PR actually?
| KEYBOARD_REPORT_CHANNEL.send(Report::MouseReport(mouse_report)).await; | ||
| } | ||
|
|
||
| async fn on_layer_change_event(&mut self, event: LayerChangeEvent) { |
There was a problem hiding this comment.
Instead of keeping track of layers here, I would prefer having three new events similar to the PointingSetCpiEvent but for PointingProcessors instead of PointingDevice. Those events would be PointingSetCursor, -Scrolling, -Sniping and the PointingProcessor listens to them to set the mode.
This way we stay more flexible and users can implement controllers and activate the modes from there however they like. They are not bound to layers, they could activate the modes with a special key combination, user keys or whatever comes to their mind.
When implementing this please keep in mind that there could be more than one PointingProcessor on a keyboard, so you need a way to identify them and to send events to them separately. (Similar to what is done in the PointingDevices)
| #[cfg_attr(feature = "defmt", derive(defmt::Format))] | ||
| pub struct ScrollConfig { | ||
| /// Divisor for X axis (pan). Higher = slower scrolling. 0 treated as 1. | ||
| pub divisor_x: u8, |
There was a problem hiding this comment.
Maybe make those i8 so users can invert the scrolling, panning independently
(Do the calculations in accumulator still work then?)
There was a problem hiding this comment.
Thanks for the suggestion! I kept explicit invert_x/invert_y bools for the following reasons:
1.Readability — invert_y: true is clearer in intent than divisor_y: -8
2.divisor = 0 already has special meaning (disables the axis in ScrollConfig). Adding negative values as another special case increases cognitive load.
The accumulator math stays simple this way — it only deals with unsigned divisors, and inversion is applied as a straightforward sign flip afterward.
There was a problem hiding this comment.
I think it's better to have multipliers in addition to divisors. That allows more flexible configurations.
And set multiplier = 0 to disable the axis, that's more intuitive than divisor = 0.
There was a problem hiding this comment.
I think it's better to have multipliers in addition to divisors. That allows more flexible configurations. And set multiplier = 0 to disable the axis, that's more intuitive than divisor = 0.
Good point — using multiplier = 0 to disable an axis is indeed more intuitive than divisor = 0.
However, I prefer to stick with divisors only for the following reasons:
- Avoid overflow on embedded systems: If we used a multiplier, an input value multiplied by the multiplier could overflow 8-bit or 16-bit integers, forcing us to promote to 32-bit on every motion event. Using only divisors keeps the per-event arithmetic to a single integer division.
- Consistency with existing firmware: Using divisor = 0 as a disable flag is already a common pattern in other firmware (e.g., QMK), so it's familiar to developers and reduces surprise.
There was a problem hiding this comment.
overflow on embedded systems
I'm not sure if it actually produces such large values that it overflows, but it's worth noting that ZMK input processor supports both multipliers and divisors and handles them in uint16.
https://github.com/zmkfirmware/zmk/blob/main/app/src/pointing/input_processor_scaler.c#L25-L40
Using divisor = 0 as a disable flag is already a common pattern in other firmware (e.g., QMK), so it's familiar to developers and reduces surprise.
I'm against it. Even if I had experience using divisor = 0 in QMK or other firmware, I wouldn't know if RMK is the same until I read the exception explained in the docs, and would brace myself for a potential division-by-zero error. That could be stress for users.
|
|
||
| // Configure different modes for each layer | ||
| pointing_processor | ||
| .set_layer_mode(0, PointingMode::Cursor) // Layer 0: Normal cursor |
There was a problem hiding this comment.
As I understand it everything should be possible to configure via keyboard.toml as well. Mostly because once you go configuring in Rust you can't use keyboard.toml anymore. So if users want to use those layer modes with their pointing processor they would be forced to convert their whole configuration to rust.
But see my comment in pointing.rs suggesting to only listen for events in PointingProcessor and let the users program controllers to emit them. This way we can avoid this whole configuration section.
Controllers must be written in Rust, but they can be combined with keyboard.toml configuration.
Raymond8196
force-pushed
the
feat/pointing-mode
branch
from
1b4c08b to
3fb4c5d
Compare
When divisor is 0, that axis outputs 0 and does not accumulate remainder. This allows configurations like scroll-only-vertical (divisor_x=0) to prevent accidental horizontal panning on ergonomic trackballs.
Breaking change: PointingEvent is now a named struct carrying device_id, replacing the previous tuple struct. Both halves of a split keyboard must be flashed with the same firmware version. Multi-device routing: - PointingEvent gains device_id: u8 (producer-side: PointingDevice, NrfAdc) - PointingProcessorConfig gains device_id: u8 (default 255 = ALL_POINTING_DEVICES) - JoystickProcessor gains device_id: u8 filter - NrfAdc gains event_device_ids: [u8; EVENT_NUM] per event slot - JoystickConfig gains id: Option<u8> for TOML configuration - Codegen (adc.rs, pmw3610.rs, pmw33xx.rs) updated to wire device_id end-to-end Per-mode axis inversion: - ScrollConfig gains invert_x / invert_y (reverse pan / wheel direction) - SniperConfig gains invert_x / invert_y (reverse per-axis movement) - Processing order: global invert -> global swap_xy -> mode-specific invert - New unit tests: test_scroll_config_invert_y, test_sniper_config_invert_axes
- Add multi-device ID assignment guide for split keyboards - Add breaking change warning (both halves must be flashed together) - Document device_id field in PointingProcessorConfig example - Document invert_x/invert_y in ScrollConfig and SniperConfig - Clarify divisor=0 disables that axis entirely - Fix variable name typo: pmw3360 -> pmw3610 in Rust config example
Thanks for pointing that out! The Gazell code was accidentally included — it belongs to another experimental branch and isn't relevant to this PR. I have removed it and make sure only the intended changes are included. |
Unlike ScrollConfig where disabling one axis independently is useful, SniperConfig has a single divisor for both axes. Setting divisor=0 would freeze cursor output entirely, which has no practical use case.
- Add invert_x/invert_y fields to ScrollConfig/SniperConfig struct literals in examples and doc code blocks - Add event_device_ids parameter to all NrfAdc::new call sites - Add device_id to JoystickProcessor::new in joystick docs - Sync pmw33xx.md with pmw3610.md: add device_id to PointingProcessorConfig example, add per-mode inversion details to mode descriptions - Fix input_device.md doc example to use inline literals instead of undefined variables
Signed-off-by: Haobo Gu <haobogu@outlook.com>
|
I still think this should be bound to events switching the modes on instead of layers. Users can always implement their own processor that changes modes with layers, if that is really needed but the built-in stuff should be as generic as it gets. |
4 participants
Summary
Add per-layer pointing device modes to RMK, allowing trackballs to behave differently on different layers (e.g., scroll on layer 1, precision mode on layer 2).
Key Features
Per-Layer Pointing Modes:
Usage: