Cirque trackpad features: circular scroll, inertial cursor #17482

dkao · 2022-06-26T11:37:06Z

Adding gestures such as circular scroll and inertial cursor, along with some general QoL changes

Description

Circular scroll gesture - Disabled by default. Preloaded configuration detects touches originating in outer ring, after validation to differentiate between swipe from edge vs. scroll gesture, the trackpad turns into a scroll knob until finger lift.
Inertial cursor - Disabled by default. If enabled, cursor will continue moving after a flick gesture and slows down by kinetic friction. Similar to trackball mode on Steam Controller.
Add function to disable trackpad tap gesture.
Recalibrate baseline after setting ADC attenuation.
Set CPI according to trackpad diameter.
Control cursor smoothing.

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

ERAControlValue set to 0xFF and waits for read value as 0x00. If sensor not connected, it may never change. No timeout specified for extended register access in datasheet, use driver default timeout.

Pinnacle automatically captures baseline on powerup, that baseline is invalid after changing ADC attenuation so recapture it. Expose function in header so calibration can be triggered by user. e.g. Press a key combo to recalibrate after finger gets captured in baseline

Set trackpad diameter with CIRQUE_PINNACLE_DIAMETER_MM.

Extract tap logic into a separate function, and add the ability to disable it at runtime.

Scroll initiates from touch to the outer ring. Left half starts vertical scroll, right half starts horizontal scroll. Rotate clockwise to scroll down/right, keep rotating to keep scrolling. Disabled by default, can be configured at runtime.

Cursor continues moving after flick, slows down by kinetic friction, similar to trackball emulation on Steam Controller. Based on https://hal.archives-ouvertes.fr/hal-00989252. Disabled by default, can be enabled at runtime.

dkao · 2022-06-26T11:45:36Z

pointing_device_drivers.c has gotten quite large, any suggestions for where the extra processing code should live? Should I move cirque_pinnacle_get_report() and other functions into cirque_pinnacle.c?
Also should I add compile flags to disable features and save memory, or would that add too much complexity to user configuration?

Kriechi · 2022-06-26T12:22:53Z

drivers/sensors/cirque_pinnacle.c

        do {
            RAP_ReadBytes(ERA_CONTROL, &ERAControlValue, 1);
-        } while (ERAControlValue != 0x00);
+        } while ((ERAControlValue != 0x00) && (timer_elapsed(timeout_timer) <= CIRQUE_PINNACLE_TIMEOUT));


At least on I2C we could use && touchpad_init==true directly, as this will be set to false if there is any I2C error (or timeout). Not sure if SPI considers CIRQUE_PINNACLE_TIMEOUT at all?

SPI doesn't have ack or check for any timeout. It will return whatever state MISO happens to be floating in, if the device is not connected. That could be either 0xFF or 0x00 (or something inbetween if there's noise on the line?).
Looking at AVR's spi_master.c for example: spi_start() always returns true, except if chip select pin is invalid but that's an error before ever interacting with the device. So touch_init check can't really apply for SPI.

This actually fixed a dead lock for me that happened with the code before. I started a small refactoring of the RAP_Read/Write functions. If you find it useful feel free to include it in this pr cirque_timeout.patch.txt

As a side note, if a single I2C transfer fails the whole driver will turn into an invalid state - which requires a reboot of the keyboard. I can think of mechanism on the pointing device driver level that will try to re-init a driver in such a case after a certain timeout. But this is not in the scope of this PR.

I think handling touch_init and I2C transaction failures adds too much to this PR, let's handle that in a separate one.

drivers/sensors/cirque_pinnacle.c

Kriechi · 2022-06-26T12:33:51Z

drivers/sensors/cirque_pinnacle.c

+void cirque_pinnacle_cursor_smoothing(bool enable) {
+    uint8_t feedconfig3;
+
+    // FeedConfig3 (Advanced feature flags)


Is this register available in all trackpad versions (TM040040/TM035035/TM023023) and Pinnacle firmwares?
GT-AN-090620 does not list it at all (presumably hidden under RESERVED).

FeedConfig3 register is listed in GT-AN-090625, which is stated as a companion document to GT-AN-090620. Same thing is stated for the ERA document (GT-AN-090623) and compensation document (GT-AN-090624).
Additionally, it is documented in Cirque's circle trackpad example code here. I don't see any indication it would not be present for a particular version of circle trackpad.
Empirically, I've tested this register on TM035035 and TM040040; the effect is subtle on 40mm but more prominent on 35mm.

KarlK90

Thanks for continued effort to improve the cirque track-pads. Excited to try these myself (I have some heading my way). I have to review the code in greater detail later on. On thing that I would like to see addressed is the introduction of bitfield structs for all the configuration registers that get manipulated and that are currently only described indirectly. This should help with type safety and readability of the code.

drivers/sensors/cirque_pinnacle.c

KarlK90 · 2022-06-27T18:59:23Z

pointing_device_drivers.c has gotten quite large, any suggestions for where the extra processing code should live? Should I move cirque_pinnacle_get_report() and other functions into cirque_pinnacle.c?

I'm not sure here. Either we should go with the current approach to not mix the actual mouse report generation with the low level driver code. In this case we should split the pointing_device_drivers.c into sensor types and add them under a folder. Or we move the report generation logic into the low level drivers itself. Both might be out of scope for this PR though. Also I would like to hear @drashna opinion on this.

Also should I add compile flags to disable features and save memory, or would that add too much complexity to user configuration?

I wouldn't add that just to save memory. If a feature introduced behavior that might be unwanted it should be made opt-out either at compile time or runtime.

Register definitions header based on Pinnacle.h from Cirque's Arduino AnyMeas_Example as that is the most complete definition anywhere. Extended register details taken from GT-AN-090625. Default values from a register dump on TM040040.

tuneEdgeSensitivity() in Cirque examples were only called for curved overlay. Disabling cirque_pinnacle_tune_edge_sensitivity() for flat overlay seems to help with the issue where finger near the outer perimeter is captured into baseline and renders the trackpad unusable until baseline recovers. Default ADC attenuation in Cirque's curved overlay examples is 2x.

Follow sequence from Cirque sample code. Set ADC attenuation, tune edge sensitivity, then calibrate.

drashna

Yeah, I'd prefer that the drivers file here be kept simple as possible.

Ideally, "fancy" features (kind of like this), be handled by pointing_device.c, so that any device can benefit from that code.

I do understand that the cirque is very much unique, especially in comparison to the other sensors. I'm not sure what the best solution is, especially long term.

However, it may be best to pull out the features that would work with other sensors (the inertial cursor code, perhaps), and move that to the pointing_devices.c file, while device specific features (such as the circular scroll) be moved into the cirque files.

Long term, if we get other trackpads supported, it may be worth having those features in the core (rather than sensor code), and having them behind defines. Eg, "optical sensor" vs "trackpads" (and probably added by default).

drashna · 2022-06-29T08:17:15Z

quantum/pointing_device_drivers.c

    }

    return mouse_report;
 }

+uint16_t cirque_pinnacle_get_cpi(void) {
+    return (uint16_t)roundf((float)cirque_pinnacle_get_scale() * 25.4f / (float)CIRQUE_PINNACLE_DIAMETER_MM);


I'm not a huge fan of using floats here, since AVR is still being used.

If it helps, the lib8tion library is present already (used for rgb stuff mostly), and that may help.

However, it may be best to pull out the features that would work with other sensors (the inertial cursor code, perhaps), and move that to the pointing_devices.c file, while device specific features (such as the circular scroll) be moved into the cirque files.

The report structure currently doesn't support pulling these features to higher level code. Inertial cursor depends on knowing when a finger lifts, and circular scroll depends on absolute coordinates, both of which are not exposed by report_mouse_t. It might be possible to differentiate finger lift vs. stationary finger by the last velocity before mouse_report.x and mouse_report.y become 0, but that sounds pretty janky to me.
Seems like I should move these lower level to somewhere in drivers/sensors/ then, at least in the short term? I'm not sure about modifying the get_report() API for this one use case.

I'm not a huge fan of using floats here, since AVR is still being used.
If it helps, the lib8tion library is present already (used for rgb stuff mostly), and that may help.

For simple stuff like converting between inch and millimeter, sure I can get rid of the float usage.
I had tried earlier on to use lib8tion for the fancy trigonometry further in, but wasn't able to get it to behave correctly. Some inputs were too wide for lib8tion and had wrapped around before I could evaluate the accuracy.
Been running this as is on AVR; while it has trouble reporting at 1KHz on master side, it's still above 500Hz (tested by making inertial cursor reports run at 1ms interval and turning off friction coefficient).

Inertial cursor is usable by any sensor with a lift status. Extracted into quantum/pointing_device_gestures.[ch], to be extracted into other get_report() functions if desired. For example, one could mount a PMW3360 upside down with a clear sheet over the lens, then use !(report.motion & 0x08) as Z and flick a finger on said clear surface as a pointing device. Circular scroll is specific to a circle trackpad reporting absolute coordinates, Cirque is currently the only sensor of this type in QMK. Tap could be generalized to multiple sensors, keeping it to lower level for now since tap enable will be a register write for relative mode. These two live in drivers/sensors/cirque_pinnacle_gestures.[ch].

dx & dy can be int16_t, up the accumulated position elements to 32-bits. Change coef to Q8.

Floating point is actually faster on AVR; cursor_glide_start() is around 33% faster with the call to hypotf() (~218us vs. ~327us), cursor_glide() is about equal between floating point and fixed point. Fixed point is much faster on a CPU with native 32-bit words and no FPU, such as ARM Cortex-M0. (~55us vs. ~18us for cursor_glide_start on an M0) Did not have a microcontroller board with Cortex-M4F to test performance with FPU.

Scroll wheel clicks are a rather coarse output, make use of lib8tion to reduce code size and speed up the process. atan2_8() did not produce enough resolution for correct scroll movement, replaced with a clone of that function in higher precision. Adjusted configuration to be easier to understand, scroll validation criteria now expressed in pixels and angles.

dkao · 2022-07-04T01:11:59Z

Extracted circular scroll and tap to drivers/sensors/cirque_pinnacle_gestures.[ch].
Extracted inertial cursor to quantum/pointing_device_gestures.[ch], these are still called from pointing_device_drivers.c but are usable by other drivers if desired.
Floating point math converted to integer/fixed point. Inertial scroll fixed point is slightly slower than floating point on AVR, should be much faster on architectures with native 32-bit words.

Make configurations explicit. Also make the cursor glide cleanup more complete by memsetting to 0.

KarlK90

@dkao My cirque trackpads finally arrived and I have tested the additions! Great work and I'm looking forward to see more keyboards using these in the future.

Now to the review part:

I wouldn't add that just to save memory. If a feature introduced behavior that might be unwanted it should be made opt-out either at compile time or runtime.

I gave this statement before actually looking at the numbers and the code, so sorry for revoking that decision so late in the process. This is due to comparing the compile size before and after this PR on AVR Pro Micros gave a difference of ~2.3KB in data, which is unfortunately the lowest denominator we have to address and huge in the world of these mcus. Therefore my proposal:

The gestures are opt-in at compile time.
The gestures are enabled at runtime by default.
The gestures enabled state can be switched at runtime (already implemented).

Otherwise LGTM!

quantum/pointing_device_drivers.c

drivers/sensors/cirque_pinnacle_gestures.h

drivers/sensors/cirque_pinnacle_gestures.c

Make all gestures opt-in at compile time. This includes tap which was previously enabled by default. Also quashed a -Werror=maybe-uninitialized in cirque_pinnacle_get_report()

Remove stale math.h include

dkao · 2022-07-10T00:00:03Z

Added enable flags for these three gestures, to be defined in config.h. Tap gesture changes from default enabled to opt-in.

KarlK90

@dkao Thanks for addressing my remarks so fast! Any final changes before the merge?

dkao · 2022-07-13T00:10:07Z

@KarlK90 Thanks, I have some changes queued up to handle POINTING_DEVICE_COMBINED for circular scroll. Was thinking to open up a separate PR since this one has already been reviewed. Can add them here if you don't mind.
Stats:

 drivers/sensors/cirque_pinnacle_gestures.c | 29 +++++++++++++++++------------
 quantum/pointing_device.c                  |  2 +-
 2 files changed, 18 insertions(+), 13 deletions(-)

EDIT: #17654

dkao added 7 commits June 25, 2022 21:01

cirque_pinnacle: Add timeout for potential loops

26bbb93

ERAControlValue set to 0xFF and waits for read value as 0x00. If sensor not connected, it may never change. No timeout specified for extended register access in datasheet, use driver default timeout.

cirque_pinnacle: Function to control smoothing

421c2f9

cirque_pinnacle: Convert CPI according to diameter

0f2d232

Set trackpad diameter with CIRQUE_PINNACLE_DIAMETER_MM.

cirque pointing driver: Make tap optional

fbee3bb

Extract tap logic into a separate function, and add the ability to disable it at runtime.

cirque pointing driver: Inertial cursor (glide)

0c35461

Cursor continues moving after flick, slows down by kinetic friction, similar to trackball emulation on Steam Controller. Based on https://hal.archives-ouvertes.fr/hal-00989252. Disabled by default, can be enabled at runtime.

github-actions bot added core documentation labels Jun 26, 2022

cirque pointing driver: formatting

f873816

Kriechi reviewed Jun 26, 2022

View reviewed changes

drivers/sensors/cirque_pinnacle.c Outdated Show resolved Hide resolved

Kriechi reviewed Jun 26, 2022

View reviewed changes

dkao force-pushed the cirque_trackpad_features_on_17091 branch from 67e9f24 to ae20d9a Compare June 26, 2022 20:54

dkao added 2 commits June 26, 2022 13:57

cirque pointing driver: Adjust configuration functions

7428dc1

cirque_pinnacle: Reword comment on calibration

43b2f99

dkao force-pushed the cirque_trackpad_features_on_17091 branch from ae20d9a to 43b2f99 Compare June 26, 2022 20:58

KarlK90 requested changes Jun 27, 2022

View reviewed changes

drivers/sensors/cirque_pinnacle.c Outdated Show resolved Hide resolved

drivers/sensors/cirque_pinnacle.c Outdated Show resolved Hide resolved

dkao added 2 commits June 28, 2022 23:14

cirque_pinnacle: Import register definition header

4d46e59

Register definitions header based on Pinnacle.h from Cirque's Arduino AnyMeas_Example as that is the most complete definition anywhere. Extended register details taken from GT-AN-090625. Default values from a register dump on TM040040.

dkao force-pushed the cirque_trackpad_features_on_17091 branch from 28cc3a8 to f85df63 Compare June 29, 2022 06:15

cirque_pinnacle: Adjust calibration sequence

8344eaa

Follow sequence from Cirque sample code. Set ADC attenuation, tune edge sensitivity, then calibrate.

drashna reviewed Jun 29, 2022

View reviewed changes

dkao added 5 commits July 3, 2022 17:55

cirque pointing driver: Integer math for inch<->pixel conversion

de833e8

pointing_device_gestures: Adjust cursor glide types

969e612

dx & dy can be int16_t, up the accumulated position elements to 32-bits. Change coef to Q8.

pointing device gestures: Update comments

7158ed6

pointing device gestures: div0 checks

f08d5e7

dkao force-pushed the cirque_trackpad_features_on_17091 branch from fa20e58 to f08d5e7 Compare July 4, 2022 01:53

dkao added 2 commits July 4, 2022 16:25

pointing device gestures: Extract configurations to explicit struct

3ff32b0

Make configurations explicit. Also make the cursor glide cleanup more complete by memsetting to 0.

circular scroll: Add left-handed mode

60da778

KarlK90 requested changes Jul 8, 2022

View reviewed changes

quantum/pointing_device_drivers.c Outdated Show resolved Hide resolved

drivers/sensors/cirque_pinnacle_gestures.h Show resolved Hide resolved

KarlK90 requested review from a team and drashna July 8, 2022 15:23

Kriechi reviewed Jul 9, 2022

View reviewed changes

drivers/sensors/cirque_pinnacle_gestures.c Show resolved Hide resolved

Kriechi reviewed Jul 9, 2022

View reviewed changes

drivers/sensors/cirque_pinnacle_gestures.c Outdated Show resolved Hide resolved

dkao added 4 commits July 9, 2022 16:34

pointing device gestures: Conditional compilation

7bf6c68

Make all gestures opt-in at compile time. This includes tap which was previously enabled by default. Also quashed a -Werror=maybe-uninitialized in cirque_pinnacle_get_report()

cirque pointing driver: Add a default triggir_px for inertial cursor

064d9e4

pointing device gestures: Style

e48a3d1

circular scroll: Use stdlib abs()

18aa3a1

Remove stale math.h include

dkao force-pushed the cirque_trackpad_features_on_17091 branch from abbab2b to 18aa3a1 Compare July 9, 2022 23:35

cirque_pinnacle: Minor change to read-modify-write variable names

8bd6c92

drashna approved these changes Jul 11, 2022

View reviewed changes

drashna requested a review from a team July 11, 2022 11:52

KarlK90 approved these changes Jul 12, 2022

View reviewed changes

drashna merged commit 5db705d into qmk:develop Jul 13, 2022

0xcharly mentioned this pull request Jul 14, 2022

Cirque trackpad features Bastardkb/bastardkb-qmk#19

Closed

drashna mentioned this pull request Aug 27, 2022

2022q3 Changelog. #18180

Merged

14 tasks

nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023

Cirque trackpad features: circular scroll, inertial cursor (qmk#17482)

943e29c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cirque trackpad features: circular scroll, inertial cursor #17482

Cirque trackpad features: circular scroll, inertial cursor #17482

dkao commented Jun 26, 2022

dkao commented Jun 26, 2022

Kriechi Jun 26, 2022

dkao Jun 26, 2022

KarlK90 Jul 8, 2022

KarlK90 Jul 8, 2022

dkao Jul 9, 2022

Kriechi Jun 26, 2022 •

edited

Loading

dkao Jun 26, 2022

KarlK90 left a comment •

edited

Loading

KarlK90 commented Jun 27, 2022

drashna left a comment

drashna Jun 29, 2022

dkao Jun 29, 2022

dkao commented Jul 4, 2022

KarlK90 left a comment

dkao commented Jul 10, 2022

KarlK90 left a comment •

edited

Loading

dkao commented Jul 13, 2022 •

edited

Loading

Cirque trackpad features: circular scroll, inertial cursor #17482

Cirque trackpad features: circular scroll, inertial cursor #17482

Conversation

dkao commented Jun 26, 2022

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

dkao commented Jun 26, 2022

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

Kriechi Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarlK90 left a comment • edited Loading

Choose a reason for hiding this comment

KarlK90 commented Jun 27, 2022

drashna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkao commented Jul 4, 2022

KarlK90 left a comment

Choose a reason for hiding this comment

dkao commented Jul 10, 2022

KarlK90 left a comment • edited Loading

Choose a reason for hiding this comment

dkao commented Jul 13, 2022 • edited Loading

Kriechi Jun 26, 2022 •

edited

Loading

KarlK90 left a comment •

edited

Loading

KarlK90 left a comment •

edited

Loading

dkao commented Jul 13, 2022 •

edited

Loading