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.

Sign up for GitHub

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

Jump to bottom

Refactor Leopold keyboards and add support for new FC980C controller #22260

Closed
wants to merge 14 commits into from
Closed

Refactor Leopold keyboards and add support for new FC980C controller #22260

wants to merge 14 commits into from

Conversation

wlhlm
Copy link
Contributor

@wlhlm wlhlm commented Oct 12, 2023 •
edited
Loading

Description

This supersedes #19734, which I unfortunately forgot about. I want to apologize to the original reviewers that graciously offered their time reviewing the original PR.

The ultimate goal of this is adding support for a new FC980C controller, but first I have to move the existing Leopold boards under a common directory and de-duplicate driver code. The steps have been broken out into individual commits.

Summary:

  • move fc660c and fc980c under leopold/
    • move existing fc980c config under to hasu to make room for the new controller configs
  • de-duplicate AD5258 driver
    • add I2C error handling
  • de-duplicate actuation point handling code
  • fix a bug introduced by fc660c/fc980c: clean up actuation point adjustment code #21964: the QMK i2c_master API has a quirk where it requires a I2C address to be shifted to the left which the old driver didn't have
  • rework the actuation point API
  • add ability to store actuation point value in QMK EEPROM for FC980C.
  • rework the actuation point example keymap for FC980C

I'm a bit unsure about the includes. It all compiles, but not sure about the paths. The build systems seems to mush the paths together, for example both #include "ad5258.h" and #include "../../ad5258.h" seem to work so I'm not sure what's preferred here.

This incorporates all the feedback from the original PR except for the includes since the structure changed slightly, so I'm not sure how much of it is still relevant.

This will be affected by #22253, so I'll update once that is merged. Done

Types of Changes

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

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

@github-actions github-actions bot added keyboard keymap via Adds via keymap and/or updates keyboard for via support dd Data Driven Changes labels Oct 12, 2023
@wlhlm wlhlm force-pushed the new-fc980c-controller-v2 branch from 4eb2823 to ef8305d Compare October 12, 2023 22:58
wlhlm added 11 commits October 13, 2023 09:30
@wlhlm
Move FC660C and FC980C under leopold/
99141c6 
Also rewrite bare links in READMEs.
@wlhlm
Depulicate AD5258 driver in leopold keyboards
ddcae6b
@wlhlm
leopold/fc980c: Move existing config to tmk subtarget
9d8632a
@wlhlm
Add aliases for moved Leopold keyboards
dd8969d
@wlhlm
leopold: Deduplicate acuation point adjustment code
ffafef1
@wlhlm
leopold: Rework AD5258 driver
b37f5d5 
Adds I2C error handling. Fixes a bug where the I2C address was not
shifted left by one bit, which is a quirky requirement of the QMK
i2c_master API.
@wlhlm
leopold: Rework actuation point API
ca6cefc
@wlhlm
leopold/fc980c: Support saving actuation point in QMK EEPROM
089661e 
While the AD5258 has its own EEPROM store, we try not to touch it
prevent overwriting the factory value, instead store and load the value
from QMK's EEPROM.
@wlhlm
leopold/fc980c/tmk: Enable LTO
7696b8a
@wlhlm
leopold/fc980c: Rework actuation point exmaple keymap
d875354
@wlhlm
Add support for new FC980C controller
13fe732
@wlhlm wlhlm force-pushed the new-fc980c-controller-v2 branch from ef8305d to 13fe732 Compare October 13, 2023 07:38
@wlhlm wlhlm changed the title Refactor Leopold Keyboards and add support for new FC980C controller Refactor Leopold keyboards and add support for new FC980C controller Oct 13, 2023
@wlhlm
Copy link
Contributor Author

wlhlm commented Nov 4, 2023 •
edited
Loading

Is there anything else that needs to be changed/updated here? Shall I spin out the bug fix into a separate PR (in light of the breaking changes timeline)? Otherwise the upcoming cycle will have broken AD5258 support for the FC980C/FC660C in master, though I imagine the impact will be pretty low.

Apologies for poking.

@wlhlm @drashna
Shorten include path in fc660c.h
bee5baa 
Co-authored-by: Drashna Jaelre <drashna@live.com>
@wlhlm wlhlm mentioned this pull request Nov 28, 2023
Merged
14 tasks
@github-actions GitHub Actions
Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Dec 24, 2023
@github-actions GitHub Actions
Copy link

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

@github-actions github-actions bot closed this Jan 23, 2024
@wlhlm wlhlm mentioned this pull request Sep 9, 2024
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drashna drashna drashna left review comments

Assignees
No one assigned
Labels
dd Data Driven Changes keyboard keymap stale Issues or pull requests that have become inactive without resolution. via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wlhlm @drashna