Skip to content
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

Add *_RIGHT configuration for PMW33XX driver #19243

Merged
merged 6 commits into from
Dec 10, 2022
Merged

Conversation

elpekenin
Copy link
Contributor

Description

This PR allows to define different CS pin(s) for each half when using PMW33XX, in a similar way that it is done for matrix or encoder pins.
This is just a monkeypatch to extend the flexibility slightly while a completely flexible solution isn't available.

@freznel10 asked for a QoL solution on Discord, to avoid recompiling each half changing pins manually. As far as he has tested, everything seems to work fine. (i dont have the hw so i cant make any test)

The changes reduce to splitting some data structures which were common before into one for each side, and some defines to replace the old variable name with the correct data structure on each side.
Code was made with backwards compatibility in mind, thus everything defaults to be the same as left side if no configuration was provided, this should mean that nothing breaks.

While editing the docs i also noticed they stated cable select instead of chip select, and the default values weren't really listed, so i changed that too

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

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

@freznel10
Copy link

Can confirm this works well !

@drashna drashna requested a review from a team December 8, 2022 20:45
@tzarc tzarc merged commit d9cba6e into qmk:develop Dec 10, 2022
@elpekenin elpekenin deleted the cs_right branch December 11, 2022 00:30
omikronik pushed a commit to omikronik/qmk_firmware that referenced this pull request Jan 22, 2023
@uqs
Copy link

uqs commented Jun 15, 2024

This of course broke the multiple sensors support.

When using #define PMW33XX_CS_PINS { F6, F7 } then PMW33XX_CS_PIN is not defined of course, and then the code tries to blindly do this:

// Support single spelling and default to be the same as left side
#if !defined(PMW33XX_CS_PINS_RIGHT)
#    if !defined(PMW33XX_CS_PIN_RIGHT)
#        define PMW33XX_CS_PIN_RIGHT PMW33XX_CS_PIN
#    endif
#    define PMW33XX_CS_PINS_RIGHT \ 
        { PMW33XX_CS_PIN_RIGHT }
#endif

At which point PMW33XX_CS_PIN still isn't defined, hence compiler errors:

In file included from drivers/sensors/pmw33xx_common.h:16,
                 from drivers/sensors/pmw3360.c:9:
drivers/sensors/pmw3360.c: In function ‘pmw33xx_get_cpi’:
drivers/sensors/pmw33xx_common.h:87:38: error: ‘PMW33XX_CS_PIN’ undeclared (first use in this function); did you mean ‘PMW33XX_CS_PINS’?
   87 | #        define PMW33XX_CS_PIN_RIGHT PMW33XX_CS_PIN
      |                                      ^~~~~~~~~~~~~~

This will of course happen over and over again, as we don't seem to have test cases for this. What is the best place to add some bare bones test cases that make sure the various combinations at least compile?

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

Successfully merging this pull request may close these issues.

None yet

5 participants