-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Implementation for IO-Expanders #44
Conversation
@PercyJW-2 I have one question. I think the kbexpander.go is handling switches connected in a matrix to the MCP23017. Is the source code based on kbmatrix.go? |
Exactly.
Sent from Proton Mail Android
…-------- Original Message --------
On 7/30/24 17:02, sago35 wrote:
***@***.***(https://github.com/PercyJW-2)
Thank you for the PR. It looks good at first glance.
I have one question. I think the kbexpander.go is handling switches connected in a matrix to the MCP23017. Is the source code based on kbmatrix.go?
—
Reply to this email directly, [view it on GitHub](#44 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AMOCQA365LAZHEZ6DSQ6TWLZO6TKFAVCNFSM6AAAAABLSZK6VSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJYGU3TENZVHE).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
if !o.InvertDiode { | ||
_ = expanderDevice.Pin(c).SetMode(mcp23017.Output) | ||
_ = expanderDevice.Pin(c).Set(true) | ||
} else { | ||
_ = expanderDevice.Pin(c).SetMode(mcp23017.Input | mcp23017.Pullup) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you are using Pullup here? I think the PR can be merged as it is now.
Depending on the reason, it might be necessary to create a PR in the future that allows for selecting either Pullup or Pulldown, including for kbmatrix.go.
I have one more request. Could you share the expected circuit diagram? kbmatrix.go assumes the following. I assume it's the same circuit, just differing in whether to use pullup or pulldown.
https://kicanvas.org/?github=https%3A%2F%2Fgithub.com%2Fsago35%2Ftinygo-keyboard%2Ftree%2Fmain%2Fkicad%2Ffric10key%2Ffric10key
I used pull-ups, because the mcp has no pulldowns and on a schematic level it's the same, just with rotated diodes. I currently don't have a correct schematic, as I soldered all diodes in reverse direction according to my schematic
Sent from Proton Mail Android
…-------- Original Message --------
On 7/31/24 02:06, sago35 wrote:
@sago35 commented on this pull request.
---------------------------------------------------------------
In [kbexpander.go](#44 (comment)):
> + if !o.InvertDiode {
+ _ = expanderDevice.Pin(c).SetMode(mcp23017.Output)
+ _ = expanderDevice.Pin(c).Set(true)
+ } else {
+ _ = expanderDevice.Pin(c).SetMode(mcp23017.Input | mcp23017.Pullup)
+ }
Is there a reason you are using Pullup here? I think the PR can be merged as it is now.
Depending on the reason, it might be necessary to create a PR in the future that allows for selecting either Pullup or Pulldown, including for kbmatrix.go.
I have one more request. Could you share the expected circuit diagram? kbmatrix.go assumes the following. I assume it's the same circuit, just differing in whether to use pullup or pulldown.
[image.png (view on web)](https://github.com/user-attachments/assets/ea766aa2-07b2-40e1-9de4-56d535234798)
https://kicanvas.org/?github=https%3A%2F%2Fgithub.com%2Fsago35%2Ftinygo-keyboard%2Ftree%2Fmain%2Fkicad%2Ffric10key%2Ffric10key
—
Reply to this email directly, [view it on GitHub](#44 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AMOCQA42CNROYQXOWYI6QFDZPATBDAVCNFSM6AAAAABLSZK6VSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBYHEYTCNBZGE).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I see, it only supports pull-up. Understood. https://ww1.microchip.com/downloads/en/DeviceDoc/20001952C.pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for your contoribution!! @PercyJW-2
Currently, only MCP23017 Devices are possible, but I have tested the Implementation and it works