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

Added right vs left specific pin assignments for dip switch #13074

Merged
merged 3 commits into from
Aug 19, 2021

Conversation

XScorpion2
Copy link
Contributor

Description

Added right vs left support for dip switch pin configuration

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

docs/feature_dip_switch.md Outdated Show resolved Hide resolved
@mtei
Copy link
Contributor

mtei commented Aug 1, 2021

The following code seems a little hard to read to me.

#ifdef DIP_SWITCH_PINS
#    if defined(SPLIT_KEYBOARD) && defined(DIP_SWITCH_PINS_RIGHT)
        if (isLeftHand) {
#    endif
            dip_switch_state[i] = !readPin(dip_switch_pad[i]);
#    if defined(SPLIT_KEYBOARD) && defined(DIP_SWITCH_PINS_RIGHT)
        } else {
            dip_switch_state[i] = !readPin(dip_switch_pad_right[i]);
        }
#    endif
#endif

I suggest code like 197ea02.

@drashna drashna requested a review from a team August 12, 2021 15:37
@tzarc
Copy link
Member

tzarc commented Aug 17, 2021

I've marked this with the breaking_2021q3 label -- if we can get mtei's suggestion sorted I think this can go in.

@zvecr
Copy link
Member

zvecr commented Aug 19, 2021

While i agree the code could be cleaner, the suggestion in 197ea02 is way worse.

The context of the code is completely lost because the left/right logic bleeds out into code that still sometimes doesn't compile. However now you need to know context from the confusingly named variable names.

I'm going to merge this as is due to the time to develop merge. The interface is fine and we can tidy up the implementation in another iteration.

@zvecr zvecr merged commit 9d1c98c into qmk:develop Aug 19, 2021
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
* Added right vs left specific pin assignments for dip switch

* Update feature_dip_switch.md

* Ran formatting tools
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Added right vs left specific pin assignments for dip switch

* Update feature_dip_switch.md

* Ran formatting tools
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.

7 participants