Add SPI (MCP23SXX) devices #41

Red-M · 2021-04-26T07:51:08Z

No description provided.

jposada202020 · 2021-04-26T14:05:26Z

@Red-M if you could run pylint locally first, you could follow this guide. and run a
https://learn.adafruit.com/improve-your-code-with-pylint
If you are cloning this and creating a virtual env, you could run pre-comit run -all-files, the repository is already configured for that and make life easier :)

Red-M · 2021-04-26T14:39:36Z

Thanks, I've run pre-commit and fixed all the issues there.

lesamouraipourpre

Caveat: I don't have the chips to test with.

These look like some great additions to the library. The bank bit in IOCON in mcp23s17.py is my main area of concern as I believe it will break the library if accidentally set (but without hardware I can't verify.)

pre-commit passes locally and sphinx builds the docs without issue.

The documentation needs an update but that can be done as a separate PR. If you want, I can do that for you just let me know.

adafruit_mcp230xx/mcp23xxx.py

lesamouraipourpre · 2021-04-28T07:37:29Z

adafruit_mcp230xx/mcp23s17.py

+
+    @io_control.setter
+    def io_control(self, val):
+        self._write_u8(_MCP23S17_IOCON, val)


If bit 7 (bank) is set to 1 most of the the _MCP23S17_XXX constants will be wrong including the _MCP23S17_IOCON constant so it can't be unset.

You could either mask out that bit to prevent change and print out a warning or raise an exception. I'm not sure which is the best option.

I don't have the SPI chips just yet but I can test with real hardware in about a week or so when I get my PCBs in.
If I am understanding this correctly, then the MCP23017 implementation also has this bug.

I think if the 2nd bank is disabled, any register values that are changed for the 2nd bank are not applied (but still recorded in the register and basically are not active until the IOCON register has bit 7 changed back to low) if the IOCON register bit 7 is set high because the MCP23017 implementation does the same as the MCP23S17 as far as I know.

The issue is how the additional registers of the 16 pin variants are addressed. There are two options, which the BANK bit of the IOCON registers determines. This library is hard coded with addresses for BANK=0. So someone could break things by sending in a value that results in the BANK bit being set to 1. I'd suggest checking the input value and raising an exception if that will happen.

adafruit_mcp230xx/mcp23s08.py

Red-M · 2021-04-28T09:05:13Z

Thanks for the review, I've made changes based on the feedback, I have a concern regarding the IOCON register value in the I2C implementation then.

jposada202020

Hello, sorry I am late to the discussion, I think we discuss over discord over the weekend regarding this.
First Thank you for this: and thanks @lesamouraipourpre for reviewing

Some comments regarding the library, and then a more general kind of approach.

Library

Functionality: Ideal, the library should continue working as it is, we can add things but people ideally should use the library the same way. we do not know how many projects are out there with this, and probably we do not want to break them
For new libraries, you could follow this guide,this provide some tips and tricks on how to create CircuitPython Libraries. There are some additions to it this week so keep updating it.
https://github.com/adafruit/circuitpython/blob/main/docs/design_guide.rst
For new libraries and functionalities it is preferable to include an example showing the new features

Other options and comments.

We could also include this new support in the community bundle, @kattni please advice regarding this

Either way we need to find a way to test, I could have access to S09/S18 not the S17. But I will need to order them. I am willing to test as I think is a fun project. and if @lesamouraipourpre help us out it could be a good addition.

Red-M · 2021-04-28T09:20:54Z

@jposada202020 That is fine but I've not broken any existing code and the original MCP23SXX library owner is wanting to get this in as well per romybompart/Adafruit_CircuitPython_MCP23Sxx#1.

These patches only add functionality while retaining the same exact structure to ensure backwards compatibility with zero changes required from any downstream projects.

jposada202020 · 2021-04-28T09:28:26Z

@Red-M No problem, thanks for this, just trying to put the general ideal out there what is optimal. I have not gone in deep through the code, lesamouraipourpre did a good job there already.

Red-M · 2021-04-28T09:31:35Z

Just getting the docs and examples ready to commit as well so the entire thing is all wrapped up in a neat little package.

…us device objects.

Fix copyright information.

Red-M · 2021-04-28T09:57:08Z

Should be another commit coming in soon from my internal version control system.

Fix docstring for mcp23sxx.

Red-M · 2021-04-28T10:27:19Z

This should be all good to merge now after reviews and CI testing.

Red-M · 2021-04-28T10:35:52Z

Either way we need to find a way to test, I could have access to S09/S18 not the S17. But I will need to order them. I am willing to test as I think is a fun project. and if @lesamouraipourpre help us out it could be a good addition.

I actually have s17 chips coming soon and boards to use them on, should be here in about a week.

dhalbert · 2021-04-28T12:12:29Z

This should be all good to merge now after reviews and CI testing.

I actually have s17 chips coming soon and boards to use them on, should be here in about a week.

Let's wait to merge until it's tested on actual hardware. I will make this a draft until then so it's not merged accidentally.

Red-M · 2021-04-28T12:15:42Z

Let's wait to merge until it's tested on actual hardware. I will make this a draft until then so it's not merged accidentally.

No worries, will let you know how well the chips work.

Red-M · 2021-05-05T10:24:35Z

Annoyingly the vendor selling me the SPI variant didn't have their stock listed correctly so its going to be another 3 weeks before I get these chips in.

EDIT: Now they've sent them even though its 3 weeks before the backorder date, so the chips should be about a week away.

Red-M · 2021-05-10T13:11:09Z

I have the chips and they're working perfectly, just missing the polarity inverse.

Red-M · 2021-06-13T15:09:27Z

Anyone mind reviewing this?

ladyada · 2021-06-13T16:26:27Z

@caternuson wanna look after you finish the arduino side?

caternuson · 2021-06-14T21:17:51Z

adafruit_mcp230xx/mcp23s17.py

+
+    @io_control.setter
+    def io_control(self, val):
+        self._write_u8(_MCP23S17_IOCON, val)


The issue is how the additional registers of the 16 pin variants are addressed. There are two options, which the BANK bit of the IOCON registers determines. This library is hard coded with addresses for BANK=0. So someone could break things by sending in a value that results in the BANK bit being set to 1. I'd suggest checking the input value and raising an exception if that will happen.

README.rst

caternuson · 2021-06-14T21:23:11Z

I'm working on a similar refactor for Arduino. So currently have a setup of each of I2C/SPI and 17/08 = 4 total variants. Tested this library with each for basic output (LED) and input (button). All work fine. Made a few suggestions in code review.

…anks.

Red-M · 2021-07-27T11:52:51Z

I've used some bitwise operators to just force bit 7 in the io_con register to 0 at all times regardless of user input.

caternuson · 2021-07-27T16:03:41Z

adafruit_mcp230xx/mcp23017.py

@@ -259,6 +259,8 @@ def io_control(self):

    @io_control.setter
    def io_control(self, val):
+        val |= 0b000001
+        val ^= 0b000001


You're wanting to force bit 7 to 0 here? How about:

@io_control.setter def io_control(self, val): # force BANK bit to 0 val &= ~0x80 self._write_u8(_MCP23017_IOCON, val)

But it may be better to raise an exception since someone may think this gives them full access to the IOCON register, and for some reason they want to use BANK=1 mode (although not sure why that'd ever matter). The exception text could say something like "Only BANK=0 is supported".

I'm not sure if an exception should even be needed, I don't think its a valid usecase since if you don't need the extra pins, just don't use them.
Just also mentioning that this was also in the original code and is technically a bugfix.

I think adding something to the documentation and forcing the BANK bit is enough.

caternuson · 2021-07-28T18:39:12Z

The CI is failing due to an annoying sphinx quirk. It doesn't like the single backticks in the docstring used for code markup. If I remember right, the fix is to change the single backtick to double backticks on both sides. Or can just remove them.

caternuson · 2021-07-29T17:24:24Z

OK. Cool. That fixed. OK, let's go with this as is.

Red-M added 2 commits April 26, 2021 12:36

Add MCP23SXX files from https://github.com/romybompart/Adafruit_Circu…

504c58d

…itPython_MCP23Sxx

Add composite support for MCP23SXX devices. This is currently untested.

b7d897a

ladyada requested a review from a team April 26, 2021 13:51

Fix up files as per pre-commit

8b5cbd0

lesamouraipourpre reviewed Apr 28, 2021

View reviewed changes

Remove unnecessary variables for mcp23xxx.py

f7dbc47

jposada202020 suggested changes Apr 28, 2021

View reviewed changes

jposada202020 requested a review from kattni April 28, 2021 09:17

Add documentation for the MCP23SXX devices.

60640fd

Red-M added 4 commits April 28, 2021 19:35

Add the most basic of examples as an initial

e405072

Add baudrate to SPI devices, this cannot be set the same way as I2C b…

1ed36cf

…us device objects.

Fix documentation for new SPI devices.

91d2485

Add another example file for SPI devices.

a1a94ac

Fix copyright information.

Red-M requested a review from jposada202020 April 28, 2021 09:56

Red-M added 3 commits April 28, 2021 20:00

More minor documentation changes.

b36aebe

Minor copyright fix.

e70de9f

Fix import in mcp23s08 for mcp23sxx.

1bc647a

Fix docstring for mcp23sxx.

Red-M force-pushed the master branch from f662d25 to 1bc647a Compare April 28, 2021 10:26

dhalbert marked this pull request as draft April 28, 2021 12:12

Add inverse polarity for MCP23S17

a3064ac

Red-M marked this pull request as ready for review May 10, 2021 13:11

evaherrada changed the base branch from master to main June 7, 2021 19:00

caternuson requested changes Jun 14, 2021

View reviewed changes

Red-M added 2 commits July 27, 2021 21:21

Fix README.rst as per suggestion.

7b76090

Just force io_control to not accept turning the banks into multiple b…

e5d86da

…anks.

caternuson requested changes Jul 27, 2021

View reviewed changes

Red-M added 2 commits July 28, 2021 18:16

Force BANK bit to 0 via suggested method.

2c3af9c

Add documentation on BANK bit ignored if being set to 1.

8f50b94

Fix docstring.

f8ddbe0

caternuson merged commit 63c459d into adafruit:main Jul 29, 2021

Add SPI (MCP23SXX) devices #41

Add SPI (MCP23SXX) devices #41

Uh oh!

Conversation

Red-M commented Apr 26, 2021

Uh oh!

jposada202020 commented Apr 26, 2021

Uh oh!

Red-M commented Apr 26, 2021

Uh oh!

lesamouraipourpre left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lesamouraipourpre Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

Red-M Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

Red-M Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caternuson Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Red-M commented Apr 28, 2021

Uh oh!

jposada202020 left a comment

Choose a reason for hiding this comment

Library

Other options and comments.

Uh oh!

Red-M commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jposada202020 commented Apr 28, 2021

Uh oh!

Red-M commented Apr 28, 2021

Uh oh!

Red-M commented Apr 28, 2021

Uh oh!

Red-M commented Apr 28, 2021

Uh oh!

Red-M commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhalbert commented Apr 28, 2021

Uh oh!

Red-M commented Apr 28, 2021

Uh oh!

Red-M commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Red-M commented May 10, 2021

Uh oh!

Red-M commented Jun 13, 2021

Uh oh!

ladyada commented Jun 13, 2021

Uh oh!

caternuson Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

caternuson commented Jun 14, 2021

Uh oh!

Red-M commented Jul 27, 2021

Uh oh!

caternuson Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

Red-M Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

caternuson commented Jul 28, 2021

Uh oh!

caternuson commented Jul 29, 2021

lesamouraipourpre left a comment •

edited

Loading

Red-M Apr 28, 2021 •

edited

Loading

Red-M commented Apr 28, 2021 •

edited

Loading

Red-M commented Apr 28, 2021 •

edited

Loading

Red-M commented May 5, 2021 •

edited

Loading