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

Partially revert #18940 for Ploopy Thumb Trackball #18943

Merged
merged 1 commit into from
Jan 1, 2023

Conversation

drashna
Copy link
Member

@drashna drashna commented Nov 3, 2022

Description

As title.

Types of Changes

  • Bugfix
  • Keyboard (addition or update)

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

@drashna drashna added the bug label Nov 3, 2022
@drashna drashna requested a review from a team November 3, 2022 18:05
@drashna drashna requested a review from fauxpark November 3, 2022 18:06
@fauxpark
Copy link
Member

fauxpark commented Nov 3, 2022

I don't understand why this needs to be done. Could you elaborate?

@drashna
Copy link
Member Author

drashna commented Nov 4, 2022

These pins are directly wired to ground. And from I understand, should not be left floating.

@fauxpark
Copy link
Member

fauxpark commented Nov 4, 2022

If they're directly connected to GND then they're not floating :)

@tzarc tzarc merged commit 9b8f186 into qmk:develop Jan 1, 2023
@drashna drashna deleted the keyboard/ploopy_fix branch January 1, 2023 01:21
@fauxpark
Copy link
Member

fauxpark commented Jan 4, 2023

I still don't understand this. I think it should probably be reverted.

omikronik pushed a commit to omikronik/qmk_firmware that referenced this pull request Jan 22, 2023
@GDsouza GDsouza mentioned this pull request Sep 17, 2023
14 tasks
@ploopyco
Copy link
Contributor

I did the following for the Madromys mouse:

const pin_t unused_pins[] = { GP1, GP3, GP4, GP6, GP8, GP10, GP14, GP16,
    GP18, GP20, GP22, GP24, GP25, GP26, GP27, GP28, GP29 };

for (uint8_t i = 0; i < (sizeof(unused_pins) / sizeof(pin_t)); i++) {
    setPinOutput(unused_pins[i]);
    writePinLow(unused_pins[i]);
}

More can be seen here.

This subroutine isn't strictly necessary for Madromys (or the Thumb, wrt to the functionality that @drashna restored via this PR) to function. However, if it's left out and somebody drives one of these pins (presumably by accident), the MCU will experience a fatal overcurrent situation.

@fauxpark
Copy link
Member

This sounds like bad PCB design, to be quite honest. I'm not a fan of the bandaid fix in software...

@ploopyco
Copy link
Contributor

Design is about tradeoffs; this is one such occasion.

MCUs like the 32u4 suffer from an interesting problem. Due to the fact that they are missing dedicated grounding pads underneath the chip, they are only grounded through dedicated ground pins. This is problematic because insufficient grounding can lead to numerous issues such as ground loops, floating ground, and excessive EMI. In the worst case scenarios, currents can flow backwards through the chip. By grounding additional, unused GPIO pins, the grounding load on the very few dedicated ground pins is relaxed. The tradeoff is that poorly written firmware can cause the chip to go into an overcurrent situation.

I think of my design choice as similar to adding seatbelts to cars. Yes, car crashes with seatbelts being used can result in neck injuries. Car crashes without them result in ejections.

@fauxpark
Copy link
Member

The tradeoff is that poorly written firmware can cause the chip to go into an overcurrent situation.

I'm sure this is fine if you are the only one in control of the firmware, as is the case for most other device manufacturers... but this is QMK, where the whole point is that the user can do pretty much whatever they want.
You also have to keep in mind that most QMK users are not particularly well versed in programming or electronics, so when I say it's bad design, I mean it more in that context.

@ploopyco
Copy link
Contributor

You're correct in that the open-source nature of the firmware means that users can do whatever they want, but that means that there is no electrical topology that guards against mistakes. Even if I removed the additional grounding, users writing bad firmware could still find ways of destroying the MCU. Removing code that tries to safeguard the MCU because users might ignore or delete it anyways is setting users up for failure.

I can't force you to sign off on this review. It's ultimately your choice. In the same vein, the board design is my choice. I supply firmware and a fork for it, which includes these guardrails. I've given my opinion; I won't weigh in anymore.

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.

4 participants