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

Adding ability to use more than one Cirque Pinnacle via SPI #24791

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

haxwagon
Copy link

@haxwagon haxwagon commented Jan 6, 2025

There is now a callback to get the current SPI CS pin when operations are being used. Implement cirque_pinnacle_spi_get_cs_pin to set the pin to be used. If this function is not implemented, the weak method will use the legacy method of CIRQUE_PINNACLE_SPI_CS_PIN.

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout (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).

There is now a callback to get the current SPI CS pin when operations
are being used.  Implement cirque_pinnacle_spi_get_cs_pin to set the pin
to be used.  If this function is not implemented, the weak method will
use the legacy method of CIRQUE_PINNACLE_SPI_CS_PIN.
@zvecr zvecr changed the base branch from master to develop January 6, 2025 16:38
@drashna
Copy link
Member

drashna commented Jan 6, 2025

Long term, I think that #20374 is the end goal here.

That said, this code is .... messy at best, and is likely to cause issues (eg, selecting the wrong device, if you're not careful).

@drashna drashna requested review from daskygit and a team January 6, 2025 20:32
@haxwagon
Copy link
Author

haxwagon commented Jan 6, 2025

Yikes! "messy at best"? It's three lines and is also how SPI is designed to work. The default implementation is for backwards compatibility.

This supports non pointing device usages of the devices... which is how I use them. I'm fine if you want to close this PR. These changes were the smallest changes I could make and still get my work done.

@drashna
Copy link
Member

drashna commented Jan 6, 2025

To be clear, the implementation itself isn't what I meant by messy, but rather the functionality.

From what I'm guessing, you'd be scanning the second cirque by replacing the cs pin function and switch between the main and secondary pads, correct?

If so, I'm really, really not a fan of this. The way that the pmw33xx multiple sensor support may be a better way to handle this, IMO.

But I'm not sure on which is the best method forward, though.

Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree the consuming usage would appear be quite fragile, but will defer to others with more domain knowledge in this area.

However, the following suggestion should at least make it pass the failing CI checks.

Comment on lines +12 to +13

__attribute__((weak)) uint8_t cirque_pinnacle_spi_get_cs_pin(void) { return CIRQUE_PINNACLE_SPI_CS_PIN; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
__attribute__((weak)) uint8_t cirque_pinnacle_spi_get_cs_pin(void) { return CIRQUE_PINNACLE_SPI_CS_PIN; }
__attribute__((weak)) pin_t cirque_pinnacle_spi_get_cs_pin(void) {
return CIRQUE_PINNACLE_SPI_CS_PIN;
}

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.

3 participants