Add support for multiple MCP2221 I2C buses (improved)

[ezio-melotti]

This PR is an improved version and a follow-up of:

• 496

It includes all the changes from #496 with some additional changes:

• Replaced __init__ (and get_instance()) with a __new__ method that either returns a new instance or a cached one;
• Added an available_paths() method that returns a list of all available MCP2221 addresses;
• Added a new_instance() method that loops through all the available MCP2221 addresses and tries to open them;
• Updated one of the examples;
• Restored the module-level mcp2221 variable in mcp2221.py

The PR is against main, so the diff doesn't show the removal of get_instance and the restoration of the mcp2221 var.

I left some inline comments regarding the implementation and API and marked the PR as draft, since it still contains some debug print() to help debugging and the API is still subject to changes.

cc @caternuson, @fgervais

[fgervais]

Looks good to me, I will give it a try.

[ezio-melotti]

Looks good to me, I will give it a try.

Thanks! I'll work on fixing the CI failures and if it works for you I'll mark the PR as "ready for review".

[ezio-melotti]

Pylint seems to get confused by the use of __new__ (possibly related to https://github.com/PyCQA/pylint/issues/7258), so I just ignored the errors/warnings.

[fgervais]

I did a quick test this morning. I cannot debug more at this time so I will just put my current state here.

My script is:

import board

I have another unrelated script which consumes one MCP and then I run the script above and I get:

MCP2221.__new__; bus_id=None; instances=[]
bus_ids=[b'1-4.4.4.2:1.2', b'1-4.4.2.4:1.2']
Opening b'1-4.4.4.2:1.2'... [ERR]: open failed
Opening b'1-4.4.2.4:1.2'... [OK]
MCP2221.__new__; bus_id=None; instances=[b'1-4.4.2.4:1.2']
bus_ids=[b'1-4.4.4.2:1.2', b'1-4.4.2.4:1.2']
Opening b'1-4.4.4.2:1.2'... [ERR]: open failed
Opening b'1-4.4.2.4:1.2'... [ERR]: open failed
Failed to open all hid devices
Traceback (most recent call last):
  File "/app/mute.py", line 1, in <module>
    import board
  File "/.venv/lib/python3.10/site-packages/board.py", line 200, in <module>
    from adafruit_blinka.board.microchip_mcp2221 import *
  File "/.venv/lib/python3.10/site-packages/adafruit_blinka/board/microchip_mcp2221.py", line 5, in <module>
    from adafruit_blinka.microcontroller.mcp2221 import pin
  File "/.venv/lib/python3.10/site-packages/adafruit_blinka/microcontroller/mcp2221/pin.py", line 8, in <module>
    mcp2221 = MCP2221()
  File "/.venv/lib/python3.10/site-packages/adafruit_blinka/microcontroller/mcp2221/mcp2221.py", line 89, in __new__
    self = cls.new_instance()
  File "/.venv/lib/python3.10/site-packages/adafruit_blinka/microcontroller/mcp2221/mcp2221.py", line 138, in new_instance
    raise RuntimeError(
RuntimeError: Can not open any of the 2 connected MCP2221 devices.

It seems to work fine but then tries to open a second free device and there isn't anymore and so fails.

[ezio-melotti]

Thanks for testing!

The error you see seems to be caused by a combination of factors:

• the unrelated script is using the first MCP2221, so it's not available to the other process
• with my changes, if you call MCP2221() without specifying the bus, the code will try to:
  1. return the first MCP2221 if it's already opened/cached (in this case it's not)
  2. open the first MCP2221 if it's available (and in this case it's not, since it's used by the other script)
  3. open the next available MCP2221 -- this will open a new MCP2221 every time MCP2221() is called
• importing board might now trigger two calls to MCP2221():
  • one in mcp2221.py (the old module-level instance)
  • one in pin.py (see https://github.com/adafruit/Adafruit_Blinka/pull/637#discussion_r1039152449)

Therefore, with my changes, calling MCP2221() without specifying the bus will do one of the following two things:

• if the first MCP2221 is available, open and return it (including in subsequent calls to MCP2221(), where the cached instance is returned)
• if it's not available, instead of returning an error like it did before, find the next available/unopened MCP2221 and open/return it

This is explained in https://github.com/adafruit/Adafruit_Blinka/pull/637#discussion_r1039144438, where other possible options are also discussed.

So, normally importing board would open and cache the first MCP2221 in mcp2221.py (the module-level var), and the other call in pin.py would return the same cached instance (since it's the first). In your case however, the first MCP2221 wasn't available so the first MCP2221() call in mcp2221.py opened the second MCP2221 (successfully), and the call in pin.py tried (and failed) to opened a third MCP2221 (since the first wasn't available, and there was no third).

This issue can be mitigated easily by removing the second call to MCP2221() in pin.py[^1], however, with the current design, any other attempt to call MCP2221() will still try to open a new MCP2221 and fail since the two available ones are both already opened.

[^1]: I now removed it.

[fgervais]

Yes I think I get it now. I was trying to understand why it was a valid use-case to have bus_id=None, len(cls.instances) > 0 but still return a new instance but it's to allow a user to call MCP2221() multiple times and each time receive a new instance until there is no more free hardware. Right?

[fgervais]

I tested again and it does in fact fix the issue.

As a reference I tested with this specific code revision: https://github.com/fgervais/project-mute-button/tree/eed6ba5eb9a1cd4e945b5c36cec59ce22487578b/firmware

and 2 MCP2221 connected, 1 already opened and a second one free.

I get the following output which is expected and correct:

fgervais@fgervais-System-Product-Name:~/personal/project-mute-button/firmware$ docker compose run --rm button python /app/mute.py
MCP2221.__new__; bus_id=None; instances=[]
bus_ids=[b'1-4.4.4.2:1.2', b'1-4.4.2.4:1.2']
Opening b'1-4.4.4.2:1.2'... [ERR]: open failed
Opening b'1-4.4.2.4:1.2'... [OK]

[ezio-melotti]

I was trying to understand why it was a valid use-case to have bus_id=None, len(cls.instances) > 0 but still return a new instance but it's to allow a user to call MCP2221() multiple times and each time receive a new instance until there is no more free hardware. Right?

That's correct. I'm trying to find a compromise between the old behavior (i.e. only try to open the first and fail if it's not available), and a new behavior that would be convenient (i.e. open the first available MCP2221 and return that).

Now that I think about it, with the old behavior the only way to get an instance was to import the module-level one, since the first MCP2221 was already used by the module-level variable and further calls to MCP2221() would always try to open the first MCP2221 and always fail.

This means that:

• people should currently only be relying on the existence of the module-level var
• we don't have constraints for the behavior of MCP2221.__init__/MCP2221.__new__

---

A solution for maintaining backward-compatibility is to use PEP-0562's module-level __getattr__ to do, at the end of mcp2221.py, something like:

def __getattr__(name):
    if name == 'mcp2221':
        # someone is trying to import the module-level instance
        # raise a deprecation warning here, and then return it
        return MCP2221(MCP2221.available_paths[0])

By doing this old code that tries to import the module-level instance will get a deprecation warning but will still get the old behavior (either the first MCP if available, or an error if it's not).

Since pin.py still needs an instance, I can add back the from .mcp2221 import MCP2221; mcp2221 = MCP2221() at the beginning of pin.py, and this should get the first available MCP[^1].

---

With this out of the way, we should be able to refactor the MCP2221 class to remove the special-casing for the first MCP2221 and just return the first available MCP that is not already open (i.e. find/open/return a new one).

Another option would be to return one of the already opened/cached instance and find/open/return a new one only if there are no opened/cached instance, but this seems less useful.

[^1]: This is technically a different behavior since it might open a different MCP instead of failing when the first is not available, but without this import board would fail. In your case this will now open the second MCP, since the first is used by the other unrelated script, and now the second is no longer automatically instantiated at the end of mcp2221.py.

[ezio-melotti]

In the last commit I implemented the solutions discussed in the previous message, in particular:

• moved the module-level mcp2221 instance inside the __getattr__, so that it's only created when someone tries to access it directly;
• added a DeprecationWarning when someone tries to access it (might need python3 -Wall or python3 -Wonce to see the warning);
• removed special-casing for the first MCP from __new__;
• removed the new_instance method and integrated it into __new__;

[fgervais]

So I guess this is a mergeable form at this point right?

It might be a good time to cancel my PR?

[ezio-melotti]

If this PR works for you, you can cancel your PR and I'll remove the debug prints and mark it as ready for review.

[ezio-melotti]

I now removed the debug prints and marked the PR as ready for review.

To summarize, this PR:

• enables the user to have multiple MCP2221 connected at the same time
• builds on @fgervais's work started in #496
• adds a new optional bus_id argument to the constructor of the MCP2221 class:
  • if the bus_id is specified, the class will return a new instance corresponding to that bus_id
  • if not, it will open and return the first available MCP.
• adds a new available_path() static method that return a list of valid paths/bus_id
• removes the module-level mcp2221 instance, replacing it with a fallback that uses __getattr__ to maintain backward-compatibility in case someone was using it directly

To test this PR, connect 2+ MCP2221s to your machine and run the example scripts. Possibly add some sensors/devices to the MCPs.

(Click to see my setup)

Physical setup: ``` +-----+ PC==>| USB |==>MCP2221-->SGP30-->SCD30 | HUB |==>MCP2221-->BME688 +-----+ USB: ==> / STEMMA QT: --> ``` Sample output: ```sh $ BLINKA_MCP2221=1 python3 examples/mcp2221_multiple_busio_i2c.py 2 MCP2221(s) found: [b'2-2.4:1.2', b'2-2.3:1.2'] I2C devices found on 2-2.4:1.2: ['0x77'] I2C devices found on 2-2.3:1.2: ['0x58', '0x61'] ``` (`0x77`, `0x58`, `0x61` are, respectively, the default I2C addresses of the BME688, SGP30, and SCD30). ---

Optional TODOs (both have inline comments with more details):

• Bikeshed on the terminology and methods/argument names (e.g. path vs bus_id, or require_mcps vs require_mcp2221s, etc.);
• Rewrite the mcp2221_single_busio_i2c.py example to accept a full path instead of reconstructing it from the bus/device.

cc @caternuson.

[caternuson]

Sorry for lagging in response here. Just to double check intentions - this is intended to be only for I2C? So it would be a very special use case? There is no expectation of support for accessing the other MCP2221's features - GPIO/ADC/Serial?

[ezio-melotti]

I've only used and tested the MCP2221 with a Stemma Qt connected to sensors such as SCD30, SGP30, BME688, and I'm not familiar with the other MCP2221 features.

By looking at the code, both GPIO and ADC seem to go through self._hid_xfer(), which then calls self._hid.write(). Since now each MCP2221 instance is created from a different hid path, then I would expect the self.hid_xfer() method to write to the right MCP2221 and therefore support the other MCP2221 features, however I have not tested this myself.

[fgervais]

I did not test extensively but GPIO seems to work on my side.

[caternuson]

The question is general and philosophical at this point. Not really specific to what this PR is actually doing or not doing code-wise. What specific capabilities do you see this PR as trying to add? Consider how others will interpret and use what this PR is providing. Many will only see "multiple MCP2221" and think that means they can use multiple MCP2221's to create tons of GPIO pins. Or lots of ADC's. etc. However, the title of the PR is "multiple MCP2221 I2C buses". Supporting "multiple MCP2221's" is much different than supporting only "multiple MCP2221 I2C buses".

Are either of you using I2C in conjunction with anything else on the MCP2221 in your multiple MCP2221 setups? Like GPIO or ADC?

[fgervais]

I posted some general information on my use-cases in my original PR: https://github.com/adafruit/Adafruit_Blinka/pull/496#issuecomment-887938341

For the moment I'm using multiple mcp2221 in 2 projects I can easily share:

1. My fridge project where I use multiple MCP2221s in the same python script/process: https://github.com/fgervais/project-smart-fridge/blob/043bafc83f9bad86ea2f18b37e432731a74de9db/firmware/main.py#L106

Note that it it using my original PR code which is not as optimal as this new improved PR.

2. My mute button project which uses GPIOs https://github.com/fgervais/project-mute-button/blob/b79803f304832e2889fec3c4005d8461c703db71/firmware/mute.py#L9 You actually don't see anything special in the code but on my computer I have other MCP2221s connected and so without this PR, the script would simply fail on import board.

It shows the usefulness when using multiple MCP2221s with multiple scripts/processes.

---

Also I think it actually allows for a lot of use-cases combination which "tons of GPIO pins" is also part of.

[caternuson]

Example 1 is only using I2C on multiple MCP's. Example 2 is only using GPIO on a single MCP. There is no example with mixed usage on multiple MCPs?

You actually don't see anything special in the code but on my computer I have other MCP2221s connected and so without this PR, the script would simply fail on import board.

The issue here seems to be supporting access to a single MCP on setups that potentially have multiple MCP's. The current library code expects only a single MCP to be present since it initiates access using USB VID and PID.

Also I think it actually allows for a lot of use-cases combination which "tons of GPIO pins" is also part of.

So you think this PR should provide access to the GPIO pins of all the attached MCP's in addition to I2C?

[fgervais]

There is no example with mixed usage on multiple MCPs?

Unfortunately these are the only 2 examples I can link to right now.

So you think this PR should provide access to the GPIO pins of all the attached MCP's in addition to I2C?

I will let @ezio-melotti post his opinion on how wide he wants to push this PR. On my side I'm happy with how it is right now.

However if we do want to allow all use-cases for Pin, it seems to me like we are not that far.

[ezio-melotti]

What specific capabilities do you see this PR as trying to add?

My use case is accessing multiple sensors connected with Stemma Qt cables to multiple MCP2221, in turn connected to my PC via USB. All the libraries for these sensors accept an I2C bus, e.g.: https://github.com/adafruit/Adafruit_CircuitPython_SCD30/blob/d5698affa8bd2af49c4933d83dd0579fe23ec495/adafruit_scd30.py#L95

Therefore the primary goal of this PR is to add support for multiple MCP2221 instances, each connected to a different MCP2221 through hid, instead of being limited to a single global instance connected to the first MCP2221 and no easy way to access the others.

Since this PR updates the code so that each instance of the MCP2221 class is connected -- through hid -- to a specific MCP2221 device, the change should be transparent to most of the methods (including the ones for GPIO and ADC), since they write on and read from the hid device (self._hid).

In fact, the only changes made by this PR to the MCP2221 class are related to the opening/closing of the hid device, and there are no I2C-specific changes to its methods.

However the pin.py file still tries to open and access the first available MCP2221, so perhaps it also needs to be updated (like the i2c.py file)?

The issue here seems to be supporting access to a single MCP on setups that potentially have multiple MCP's. The current library code expects only a single MCP to be present since it initiates access using USB VID and PID.

My understanding is that:

• the MCP2221(s) are accessed via hid
• the VID and PID are the same for all the MCP2221s (0x04D8 and 0x00DD respectively)
• hid.enumerate(VID, PID) returns a list of all connected MCP2221s
• hid.open(VID, PID) always tries to open the first MCP2221 of the list, and fails if it's already opened
• the path is different for each MCP2221 (depending on the USB port it is connected to), allowing connection to different MCP2221s (through hid.open_path(path))

The current code uses hid.open(VID, PID) to open the first MCP2221 and is therefore limited to a single (the first, assuming it's not open) MCP2221: https://github.com/adafruit/Adafruit_Blinka/blob/d094997a471301a2147d58f1693aa1d3a49df6cd/src/adafruit_blinka/microcontroller/mcp2221/mcp2221.py#L58

This PR either opens the first available MCP2221 if no path is specified, or opens the MCP2221 that corresponds to the given path. The list of paths is returned by MCP2221.available_path(), and they corresponds to specific USB ports (so they are consistent across reboots).

So you think this PR should provide access to the GPIO pins of all the attached MCP's in addition to I2C?

It should already allow this, but it should be tested and verified.

[caternuson]

It should already allow this, but it should be tested and verified.

It does not and is easy to demonstrate with the following, with multiple MCP's attached:

import board
dir(board)

I think implementing access to all the GPIO pins will be non-trivial. Further, there are the ADC's to consider.

It sounds like your motivation and use case was entirely I2C driven. But you feel the other MCP2221 feature's should be supported?

[tannewt]

I don't think that the board module makes any sense in the context of MCP because they aren't an inherent part of the board. Instead, one should instantiate an MCP object that has library compatible APIs. Maybe that's best done as a new library?

[ezio-melotti]

But you feel the other MCP2221 feature's should be supported?

They could, but it's probably better to implement them with separate PRs. This PR is already an improvement over the existing code, and solves both my and @fgervais use case. If other people need GPIO/ADC/etc. they could submit new PRs, but I don't think that should block this PR.

I don't think that the board module makes any sense in the context of MCP

FWIW I need to import board to use the sensors that I have connected to the MCP2221s (see e.g. this example: https://github.com/adafruit/Adafruit_CircuitPython_SCD30#usage-example).

[tannewt]

They could, but it's probably better to implement them with separate PRs. This PR is already an improvement over the existing code, and solves both my and @fgervais use case. If other people need GPIO/ADC/etc. they could submit new PRs, but I don't think that should block this PR.

This adds a new kwarg, bus_id, to the busio.I2C interface that means code written for it won't work on other CircuitPython devices. It really shouldn't be merged here. The separate library is a better option. (more below)

FWIW I need to import board to use the sensors that I have connected to the MCP2221s (see e.g. this example: https://github.com/adafruit/Adafruit_CircuitPython_SCD30#usage-example).

For that exact example yes. However, the MCP library could provide its own busio.I2C equivalent that is then passed into the SCD30 object. That way you don't need busio or board replaced. So you'd do something like:

import time
import mcp2221
import adafruit_scd30

# SCD-30 has tempremental I2C with clock stretching, datasheet recommends
# starting at 50KHz

# This is an object for the specific connected MCP.
mcp = mcp2221.MCP2221(b'2-3.3:1.2')
# mcp.i2c is an instance of the I2C class for this specific MCP object.
scd = adafruit_scd30.SCD30(mcp.i2c)

CircuitPython drivers should work with any busio.I2C-like object. No need for board and busio.

[ezio-melotti]

Adds non-standard kwarg to busio.I2C.

FWIW the arg is optional, so it shouldn't affect existing code. If the MCP2221 is removed from the equation, I would assume the code will need to be updated anyway and the path that corresponds to a specific USB removed (unless there are also other USB-based I2C-supporting devices). However, if there is a requirement for all the I2C interface to have the exact same interface (with no extra args), then I understand.

# This is an object for the specific connected MCP.
mcp = mcp2221.MCP2221(b'2-3.3:1.2')
# mcp.i2c is an instance of the I2C class for this specific MCP object.
scd = adafruit_scd30.SCD30(mcp.i2c)

This is actually not a bad idea, however the I2C object in the original example is instantiated with busio.I2C(board.SCL, board.SDA, frequency=50000).

If we simply add an mcp.i2c attribute, it is not possible to pass different arguments, but that can be solved easily by converting it into an mcp.get_i2c(...) method, roughly implemented as:

class MCP2221:
    ...
    def get_i2c(self, *args, **kwargs):
        self._i2c = mcp2221.I2C(*args, ** kwargs)
        return self._i2c

Even if we do this, we would still need to import board, at least in the case of the SCD30, in order to access board.SCL and board.SDA.

The final code will then look like:

import board
import mcp2221
import adafruit_scd30

# SCD-30 has tempremental I2C with clock stretching, datasheet recommends
# starting at 50KHz

# This is an object for the specific connected MCP.
mcp = mcp2221.MCP2221(b'2-3.3:1.2')
# i2c is an instance of the I2C class for this specific MCP object.
i2c = mcp.get_i2c(board.SCL, board.SDA, frequency=50000)
scd = adafruit_scd30.SCD30(i2c)

Finally, we might want to rethink the available_paths method -- the paths it returned were then fed to I2C(), but if we don't do that anymore we might as well returns instances directly, to make the code more convenient.

[tannewt]

FWIW the arg is optional, so it shouldn't affect existing code. If the MCP2221 is removed from the equation, I would assume the code will need to be updated anyway and the path that corresponds to a specific USB removed (unless there are also other USB-based I2C-supporting devices). However, if there is a requirement for all the I2C interface to have the exact same interface (with no extra args), then I understand.

I generally don't want extra options here because code written for it won't work on other platforms. Code written for other platforms won't work with this code because it needs bus_id.

If we simply add an mcp.i2c attribute, it is not possible to pass different arguments, but that can be solved easily by converting it into an mcp.get_i2c(...) method

Yup, that would work. You'd want to ensure you only return one object and that the frequency matched.

Even if we do this, we would still need to import board, at least in the case of the SCD30, in order to access board.SCL and board.SDA.

You don't because the MCP I2C constructor ignores the pin arguments. There must only be one set of i2c pins on it. https://github.com/adafruit/Adafruit_Blinka/blob/28ae3d3743aed536bf532951f3bbb268676a155e/src/adafruit_blinka/microcontroller/mcp2221/i2c.py#L11-L14

Finally, we might want to rethink the available_paths method -- the paths it returned were then fed to I2C(), but if we don't do that anymore we might as well returns instances directly, to make the code more convenient.

That would be more convenient but it may not be what you want. Returning an instance for each would claim them all, even if the user only wanted to use one of them. I think it would be useful to still have the discovery function at the top level of the library.

[caternuson]

I suggest we close this. I agree with @tannewt 's suggestion that this is better done in a separate library. To be a part of Blinka carries specific requirements on maintaining API compatibility. While a single MCP2221 can be represented via the board module (as currently done), this generally does not hold up when considering multiple MCP2221's.

Similar arguments apply to FT232H.

[ezio-melotti]

With separate library, do you mean a separate module in this repo, a new repo under adafruit, or a standalone repo (and possibly related PyPI package) under my account?

FWIW I'm also fine implementing the solution suggested above by @tannewt (i.e. adding an mcp.get_i2c() method).

[caternuson]

A separate repo. There is the Community Bundle repo which maybe this could be added to? https://github.com/adafruit/CircuitPython_Community_Bundle But also maybe not. This would end up being sort of a special use case library. So not like a typical CircuitPython Library. But it also wouldn't be a general purpose library, since it would be providing a CircuitPython specific interface.

[tannewt]

I'd suggest a separate repo under your account @ezio-melotti. Once it is setup, feel free to add a link to it from a README here. Happy to link folks to it.