[Core] PMW33XX drivers overhaul #17613

KarlK90 · 2022-07-10T14:40:39Z

Description

This combines the PMW3389 and PM3360 drivers as they only differ in the firmware blobs and CPI get and set functions. The following changes have been made:

Common shared code base between PMW3389 and PMW3360 drivers
PMW3389 now gets the same multi-sensor feature that is already available on the PMW3360.
Introduced a shared pmw33xx_report_t struct which is now directly readable via SPI transactions instead of individual byte-sized reads, saving multiple copies and bitshift operations.
pmw33(89/60)_get_report functions had unreachable branches in their motion detection logic these have been simplified as much as possible.
The fast firmware upload option has been removed as this becomes obsolete by the newly introduced polled waiting functions for ChibiOS ([Core] Use polled waiting on ChibiOS platforms that support it #17607)
PMW33(60/89)_SPI_LSBFIRST and PMW33(60/89)_SPI_MODE config options have been removed as they don't need to be configurable.
All PMW3389_ and PMW3360_ defines have been unified to a PMW33XX_ prefix to reduce code duplication and make the defines interchangeable
Pointing devices now have their own pd_dprintf macro that allows opt-in debug messages

Types of Changes

Issues Fixed or Closed by This PR

Sub-optimal performance on RP2040 split keyboards
Unnecessary code duplication

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).

KarlK90 · 2022-07-12T20:31:03Z

CC: @Alabastard-64 @uqs I refactored the PMW3360 and PMW3389 drivers into a common code base. Could you verify that it still works as intended for you? I only have a PMW3360 sensor on a ARM platform and missing AVR and PMW3389 sensors.

drashna

pmw3360 changes work on AVR, ploopy mouse.

KarlK90 · 2022-07-13T08:25:03Z

pmw3360 changes work on AVR, ploopy mouse.

Nice, just need the PMW3389 confirmation and I'll merge 🎉

drivers/sensors/pmw33xx_common.c

uqs

Thanks! I was gonna attempt this over the summer, now that I'm building a keeb with 2x 3389 sensors.

I can try to test this over the weekend, but no need to wait for me, I can complain after the fact should it not work properly for both sensor types :)

uqs · 2022-07-13T08:58:46Z

drivers/sensors/pmw33xx_common.c

+extern const uint8_t pmw33xx_firmware_data[PMW33XX_FIRMWARE_LENGTH] PROGMEM;
+extern const uint8_t pmw33xx_firmware_signature[3] PROGMEM;
+
+static const pin_t pmw33xx_cs_pins[]                                   = PMW33XX_CS_PINS;


naming it pmw33xx_cs_pins is a bit verbose in this pmw33xx file, consider calling it just pins

👍 It was a global for a short time, renamed to cs_pins as this is a bit more descriptive.

uqs · 2022-07-13T08:59:07Z

drivers/sensors/pmw33xx_common.c

+
+static const pin_t pmw33xx_cs_pins[]                                   = PMW33XX_CS_PINS;
+static bool        in_burst[(sizeof(pmw33xx_cs_pins) / sizeof(pin_t))] = {0};
+const size_t       pmw33xx_number_of_sensors                           = (sizeof(pmw33xx_cs_pins) / sizeof(pin_t));


can this be static? Also excessive parenthesis on the RHS

Removed parenthesis, this variable is used in extern files therefore can't be static.

uqs · 2022-07-13T09:00:47Z

drivers/sensors/pmw33xx_common.c

+static bool        in_burst[(sizeof(pmw33xx_cs_pins) / sizeof(pin_t))] = {0};
+const size_t       pmw33xx_number_of_sensors                           = (sizeof(pmw33xx_cs_pins) / sizeof(pin_t));
+
+bool pmw33xx_spi_start(uint8_t sensor);


unneeded declaration as it's defined just a couple lines later?

uqs · 2022-07-13T09:04:07Z

drivers/sensors/pmw33xx_common.c

+}
+
+bool pmw33xx_spi_start(uint8_t sensor) {
+    if (!spi_start(pmw33xx_cs_pins[sensor], false, 3, PMW33XX_SPI_DIVISOR)) {


why did you drop using the defines like PMW3360_SPI_LSBFIRST, PMW3360_SPI_MODE here?

We can be sure that the sensor are always not LSB first, so this can be a fixed value. I'm not 100% sure about the SPI mode but it should also always be mode 3.

uqs · 2022-07-13T09:10:05Z

drivers/sensors/pmw33xx_common.c

+    pmw33xx_write(sensor, REG_SROM_Enable, 0x18);
+
+    if (!pmw33xx_spi_start(sensor)) {
+        return;


return false?

uqs · 2022-07-13T09:10:23Z

drivers/sensors/pmw33xx_common.c

+
+void pmw33xx_write(uint8_t sensor, uint8_t reg_addr, uint8_t data) {
+    if (!pmw33xx_spi_start(sensor)) {
+        return;


return false
maybe?

uqs · 2022-07-13T09:11:25Z

drivers/sensors/pmw33xx_common.c

+    pmw33xx_read(sensor, REG_Delta_Y_L);
+    pmw33xx_read(sensor, REG_Delta_Y_H);
+
+    pmw33xx_upload_firmware(sensor);


this should return a bool, as it can fail and then we'd just blindly would continue. I guess it'll be harmless, it depends a bit on how "correct" you want this to be.

About correctness: The functions now abort if the first SPI interaction fails, but subsequent ones are assumed to always succeed.

uqs · 2022-07-13T09:16:37Z

drivers/sensors/pmw33xx_common.c

+
+    bool ok = pmw33xx_check_signature(sensor);
+
+    if (ok) {


I wonder how much flash space could be saved if you wrap this under
#ifdef CONSOLE_ENABLE
(assuming that the pd_dprintf() are no-ops in that case, does the compiler/optimizer throw all extra code away?

I simplified the statement to only one branch, dprintf is already a no-op if console is not enabled. So this is eliminated by the compiler.

uqs · 2022-07-13T09:16:46Z

drivers/sensors/pmw33xx_common.c

+    if (sensor >= pmw33xx_number_of_sensors) {
+        return report;
+    }
+u


u wat mate?
:)

hehe VIM in insert mode. Removed.

KarlK90 · 2022-07-13T11:46:47Z

Thanks! I was gonna attempt this over the summer, now that I'm building a keeb with 2x 3389 sensors.

I can try to test this over the weekend, but no need to wait for me, I can complain after the fact should it not work properly for both sensor types :)

Great, let me know what you think about the changes and I'll merge this evening.

daskygit · 2022-07-13T11:59:23Z

Pointing devices now have their own pd_dprintf macro that allows opt-in debug messages

I know it's only a fairly minor change but feel this should be a separate PR.

KarlK90 · 2022-07-13T13:41:41Z

Pointing devices now have their own pd_dprintf macro that allows opt-in debug messages

I know it's only a fairly minor change but feel this should be a separate PR.

No problem, I can pull this out of this PR.

KarlK90 · 2022-07-13T16:26:52Z

Pointing device specific debug messages are now in #17663

KarlK90 · 2022-07-13T16:31:12Z

@uqs I tested the recent changes and it works well with PMW3360. Let me know if you are happy with them and I'll merge right away 😉

drashna · 2022-07-14T07:07:04Z

Lints not happy because some jerk didn't properly format a json file (me, it was me. and for the 4x6 tractyl manuform)

KarlK90 · 2022-07-14T07:47:05Z

Lints not happy because some jerk didn't properly format a json file (me, it was me. and for the 4x6 tractyl manuform)

I just adjusted the json to use the correct 4x6 layout name, but it still complains. Could you open a PR with the corrected json (Or post it here)?

KarlK90 · 2022-07-14T09:33:52Z

Failing lint will be handled in #17681

This combines the PMW3389 and PM3360 drivers as they only differ in the firmware blobs and CPI get and set functions. The following changes have been made: * PMW3389 now gets the same multi-sensor feature that is already available on the PMW3360. * Introduced a shared pmw33xx_report_t struct is now directly readable via SPI transactions instead of individual byte-sized reads, saving multiple copies and bitshift operations. * pmw33(89/60)_get_report functions had unreachable branches in their motion detection logic these have been simplied as much as possible. * The fast firmware upload option has been removed as this becomes obsolete by the newly introduced polled waiting functions for ChibiOS * PMW33(60/89)_SPI_LSBFIRST and PMW33(60/89)_SPI_MODE config options have been removed as they don't need to be configurable. * All PMW3389 and PMW3360 defines have been unified to a PMW33XX prefix to reduce code duplication and make the defines interchangeable polled waiting

dkao · 2022-07-15T05:25:38Z

drivers/sensors/pmw33xx_common.c

+    wait_ms(1);
+
+    pmw33xx_write(sensor, REG_Config2, 0x00);
+    pmw33xx_write(sensor, REG_Angle_Tune, CONSTRAIN(ROTATIONAL_TRANSFORM_ANGLE, -127, 127));


A bit late here, shouldn't this be:

Suggested change

pmw33xx_write(sensor, REG_Angle_Tune, CONSTRAIN(ROTATIONAL_TRANSFORM_ANGLE, -127, 127));

pmw33xx_write(sensor, REG_Angle_Tune, CONSTRAIN(ROTATIONAL_TRANSFORM_ANGLE, -30, 30));

Register description for both PMW3360 and PMW3389 only suggest up to +/-30 degrees, so that's the constraint I originally added in the Ploopy sketch. Has this been empirically determined to go up to +/-127 degrees?

You are right, this is not according to the specs. I'll open an PR to rectify this error.

dkao · 2022-07-15T05:26:43Z

docs/feature_pointing_device.md

+| `PMW33XX_CLOCK_SPEED`        | (Optional) Sets the clock speed that the sensor runs at.                                    | `2000000`     |
+| `PMW33XX_SPI_DIVISOR`        | (Optional) Sets the SPI Divisor used for SPI communication.                                 | _varies_      |
+| `PMW33XX_LIFTOFF_DISTANCE`   | (Optional) Sets the lift off distance at run time                                           | `0x02`        |
+| `ROTATIONAL_TRANSFORM_ANGLE` | (Optional) Allows for the sensor data to be rotated +/- 127 degrees directly in the sensor. | `0`           |


This is a follow-up of qmk#17613.

* bastardkb: restructure folder hierarchy ahead of supporting other adapters/mcus Upcoming support for the following (adapter, mcu) pairs will be submitted in follow-up PRs: - `v2/elitec` - `v2/stemcell` - `blackpill` This PR contains the following changes: - Move previous implementation to an inner `v1/elitec` folder - Move keyboard USB IDs and strings to data driven - Update headers to update maintainers list - Run `qmk format-c` * bastardkb/charybdis: remove broken acceleration implementation * bastardkb/charybdis: fix debug output * bastardkb: add support for BastardKb the `v2/elitec` (adapter, mcu) pair * bastardkb: add Blackpill support * bastardkb/charybdis/3x5: add `bstiq` keymap * bastardkb/charybdis: add fake LEDs to the configuration For the Charybdis 3x5 (respectively 4x6), the LED config now simulates 36 (respectively 58) LEDs instead of the actual 35 (respectively 56) to prevent confusion when testing LEDs during assembly when handedness is not set correctly. Those fake LEDs are bound to the physical bottom-left corner. * bastardkbk/charybdis/readme.md: update build commands Merge pull request #5 from Nathancooke7/update_charybdis_readme_v2_shield. * bastardkb/charybdis: fix Via keymap with blackpill * bastardkb/charybdis: add 3x6 configuration * bastardkb/charybdis: remove unnecessary files * bastardkb/charybdis: remove obsolete code * bastardkb/charybdis/3x6: add Via keymap * bastardkb: add support for Splinky (RP2040) board * bastardkb: initial configuration for the Splinky (SPI not working yet) * bastardkb/charybdis/3x5/v2/splinky: tentative change to enable trackball * bastardkb/charybdis/3x5/v2/splinky: fix SCK, MISO, MOSI pins * bastardkb/charybdis/3x5/v2/splinky: fix SCK, MISO, MOSI pins * bastardkb/charybdis/4x6/v2/splinky: add SPI configuration and enable trackball * bastardkb/charybdis/3x6: add splinky config * bastardkb/*/v2/splinky: update drivers to `vendor` * bastardkb/dilemma: add new board * bastardkb/charybdis: fix infinite loop in `layer_state_set_user(…)` in the `via` keymaps * bastardkb/dilemma: add `bstiq` keymap * bastardkb: specify blackpill boards * bastardkb/charybdis: fix blackpill-specific define syntax * bastardkb: remove `NO_ACTION_MACRO` and `NO_ACTION_FUNCTION` which are no longer valid options * bastardkb: fix `QK_BOOT` keycodes * bastardkb/dilemma: fix mouse direction on X axis * bastardkb/charybdis/3x6: adjust CS * bastardkb/dilemma: adjust trackpad configuration * charybdis: fix `PWM33XX_CS_PIN` defines This is a follow-up of #17613. * bastardkb: remove Vial mentions from `bstiq` keymaps * Cleanup unnecessary comments Co-authored-by: Nathan <[email protected]> Co-authored-by: Charly Delay <[email protected]>

This is a follow-up of qmk#17613.

* PMW33XX drivers overhaul This combines the PMW3389 and PM3360 drivers as they only differ in the firmware blobs and CPI get and set functions. The following changes have been made: * PMW3389 now gets the same multi-sensor feature that is already available on the PMW3360. * Introduced a shared pmw33xx_report_t struct is now directly readable via SPI transactions instead of individual byte-sized reads, saving multiple copies and bitshift operations. * pmw33(89/60)_get_report functions had unreachable branches in their motion detection logic these have been simplied as much as possible. * The fast firmware upload option has been removed as this becomes obsolete by the newly introduced polled waiting functions for ChibiOS polled waiting * PMW33(60/89)_SPI_LSBFIRST and PMW33(60/89)_SPI_MODE config options have been removed as they don't need to be configurable. * All PMW3389 and PMW3360 defines have been unified to a PMW33XX prefix to reduce code duplication and make the defines interchangeable * Adjust keyboards to PMW33XX naming scheme

* bastardkb: restructure folder hierarchy ahead of supporting other adapters/mcus Upcoming support for the following (adapter, mcu) pairs will be submitted in follow-up PRs: - `v2/elitec` - `v2/stemcell` - `blackpill` This PR contains the following changes: - Move previous implementation to an inner `v1/elitec` folder - Move keyboard USB IDs and strings to data driven - Update headers to update maintainers list - Run `qmk format-c` * bastardkb/charybdis: remove broken acceleration implementation * bastardkb/charybdis: fix debug output * bastardkb: add support for BastardKb the `v2/elitec` (adapter, mcu) pair * bastardkb: add Blackpill support * bastardkb/charybdis/3x5: add `bstiq` keymap * bastardkb/charybdis: add fake LEDs to the configuration For the Charybdis 3x5 (respectively 4x6), the LED config now simulates 36 (respectively 58) LEDs instead of the actual 35 (respectively 56) to prevent confusion when testing LEDs during assembly when handedness is not set correctly. Those fake LEDs are bound to the physical bottom-left corner. * bastardkbk/charybdis/readme.md: update build commands Merge pull request qmk#5 from Nathancooke7/update_charybdis_readme_v2_shield. * bastardkb/charybdis: fix Via keymap with blackpill * bastardkb/charybdis: add 3x6 configuration * bastardkb/charybdis: remove unnecessary files * bastardkb/charybdis: remove obsolete code * bastardkb/charybdis/3x6: add Via keymap * bastardkb: add support for Splinky (RP2040) board * bastardkb: initial configuration for the Splinky (SPI not working yet) * bastardkb/charybdis/3x5/v2/splinky: tentative change to enable trackball * bastardkb/charybdis/3x5/v2/splinky: fix SCK, MISO, MOSI pins * bastardkb/charybdis/3x5/v2/splinky: fix SCK, MISO, MOSI pins * bastardkb/charybdis/4x6/v2/splinky: add SPI configuration and enable trackball * bastardkb/charybdis/3x6: add splinky config * bastardkb/*/v2/splinky: update drivers to `vendor` * bastardkb/dilemma: add new board * bastardkb/charybdis: fix infinite loop in `layer_state_set_user(…)` in the `via` keymaps * bastardkb/dilemma: add `bstiq` keymap * bastardkb: specify blackpill boards * bastardkb/charybdis: fix blackpill-specific define syntax * bastardkb: remove `NO_ACTION_MACRO` and `NO_ACTION_FUNCTION` which are no longer valid options * bastardkb: fix `QK_BOOT` keycodes * bastardkb/dilemma: fix mouse direction on X axis * bastardkb/charybdis/3x6: adjust CS * bastardkb/dilemma: adjust trackpad configuration * charybdis: fix `PWM33XX_CS_PIN` defines This is a follow-up of qmk#17613. * bastardkb: remove Vial mentions from `bstiq` keymaps * Cleanup unnecessary comments Co-authored-by: Nathan <[email protected]> Co-authored-by: Charly Delay <[email protected]>

github-actions bot added core documentation labels Jul 10, 2022

drashna requested review from drashna and a team July 10, 2022 23:21

KarlK90 force-pushed the feature/pmw3360-performance branch 2 times, most recently from 141c59d to 52693d6 Compare July 12, 2022 17:02

github-actions bot added keyboard keymap labels Jul 12, 2022

KarlK90 force-pushed the feature/pmw3360-performance branch from 52693d6 to bef09d1 Compare July 12, 2022 20:20

KarlK90 changed the title ~~[Core] WIP! PMW3360 driver overhaul~~ [Core] PMW33XX drivers overhaul Jul 12, 2022

KarlK90 marked this pull request as ready for review July 12, 2022 20:27

KarlK90 force-pushed the feature/pmw3360-performance branch 2 times, most recently from 741ebf0 to d27f9a5 Compare July 12, 2022 20:52

drashna approved these changes Jul 13, 2022

View reviewed changes

drashna requested a review from a team July 13, 2022 03:41

uqs reviewed Jul 13, 2022

View reviewed changes

drivers/sensors/pmw33xx_common.c Show resolved Hide resolved

KarlK90 force-pushed the feature/pmw3360-performance branch 2 times, most recently from 9111eb1 to 2d20bf6 Compare July 13, 2022 09:12

uqs suggested changes Jul 13, 2022

View reviewed changes

KarlK90 force-pushed the feature/pmw3360-performance branch from 2d20bf6 to 3d8161f Compare July 13, 2022 11:29

KarlK90 requested a review from drashna July 13, 2022 11:46

KarlK90 mentioned this pull request Jul 13, 2022

[Core] Introduce pointing device specific debug messages #17663

Merged

14 tasks

KarlK90 force-pushed the feature/pmw3360-performance branch from 3d8161f to 21e14da Compare July 13, 2022 16:26

uqs approved these changes Jul 13, 2022

View reviewed changes

KarlK90 force-pushed the feature/pmw3360-performance branch from 21e14da to b201496 Compare July 14, 2022 07:42

KarlK90 added 2 commits July 14, 2022 11:45

Adjust keyboards to PMW33XX naming scheme

4aa2c5a

KarlK90 force-pushed the feature/pmw3360-performance branch from b201496 to 4aa2c5a Compare July 14, 2022 09:45

KarlK90 merged commit 3c58f98 into qmk:develop Jul 14, 2022

dkao reviewed Jul 15, 2022

View reviewed changes

0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 23, 2022

charybdis: fix PWM33XX_CS_PIN defines

55bd457

This is a follow-up of qmk#17613.

0xcharly pushed a commit to 0xcharly/qmk_firmware that referenced this pull request Jul 25, 2022

charybdis: fix PWM33XX_CS_PIN defines

b64f9e9

This is a follow-up of qmk#17613.

drashna mentioned this pull request Aug 27, 2022

2022q3 Changelog. #18180

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] PMW33XX drivers overhaul #17613

[Core] PMW33XX drivers overhaul #17613

KarlK90 commented Jul 10, 2022 •

edited

Loading

KarlK90 commented Jul 12, 2022

drashna left a comment

KarlK90 commented Jul 13, 2022

uqs left a comment

uqs Jul 13, 2022

KarlK90 Jul 13, 2022

uqs Jul 13, 2022

KarlK90 Jul 13, 2022

uqs Jul 13, 2022

KarlK90 Jul 13, 2022

uqs Jul 13, 2022

KarlK90 Jul 13, 2022

uqs Jul 13, 2022

KarlK90 Jul 13, 2022 •

edited

Loading

uqs Jul 13, 2022

KarlK90 Jul 13, 2022

uqs Jul 13, 2022

KarlK90 Jul 13, 2022

KarlK90 Jul 13, 2022

uqs Jul 13, 2022

KarlK90 Jul 13, 2022

uqs Jul 13, 2022

KarlK90 Jul 13, 2022

KarlK90 commented Jul 13, 2022

daskygit commented Jul 13, 2022

KarlK90 commented Jul 13, 2022

KarlK90 commented Jul 13, 2022

KarlK90 commented Jul 13, 2022

drashna commented Jul 14, 2022

KarlK90 commented Jul 14, 2022

KarlK90 commented Jul 14, 2022

dkao Jul 15, 2022

KarlK90 Jul 15, 2022

dkao Jul 15, 2022

	pmw33xx_write(sensor, REG_Angle_Tune, CONSTRAIN(ROTATIONAL_TRANSFORM_ANGLE, -127, 127));
	pmw33xx_write(sensor, REG_Angle_Tune, CONSTRAIN(ROTATIONAL_TRANSFORM_ANGLE, -30, 30));

	\| `ROTATIONAL_TRANSFORM_ANGLE` \| (Optional) Allows for the sensor data to be rotated +/- 127 degrees directly in the sensor. \| `0` \|
	\| `ROTATIONAL_TRANSFORM_ANGLE` \| (Optional) Allows for the sensor data to be rotated +/- 30 degrees directly in the sensor. \| `0` \|

[Core] PMW33XX drivers overhaul #17613

[Core] PMW33XX drivers overhaul #17613

Conversation

KarlK90 commented Jul 10, 2022 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

KarlK90 commented Jul 12, 2022

drashna left a comment

Choose a reason for hiding this comment

KarlK90 commented Jul 13, 2022

uqs left a 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

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

Choose a reason for hiding this comment

KarlK90 Jul 13, 2022 • edited Loading

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

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

KarlK90 commented Jul 13, 2022

daskygit commented Jul 13, 2022

KarlK90 commented Jul 13, 2022

KarlK90 commented Jul 13, 2022

KarlK90 commented Jul 13, 2022

drashna commented Jul 14, 2022

KarlK90 commented Jul 14, 2022

KarlK90 commented Jul 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarlK90 commented Jul 10, 2022 •

edited

Loading

KarlK90 Jul 13, 2022 •

edited

Loading