Add support for multiple MCP2221 I2C buses (improved) #637
Conversation
It's now superseded by `examples/mcp2221_multiple_busio_i2c.py`.
Sync the `multiple-mcp2221` branch with upstream
| if bus_id is None and cls.instances: | ||
| # return the instance for the first bus_id, if already cached | ||
| first_bus_id = cls.available_paths(require_mcps=True)[0] | ||
| if first_bus_id in cls.instances: | ||
| print(f'Returning cached {first_bus_id}...') | ||
| return cls.instances[first_bus_id] |
There was a problem hiding this comment.
Here I tried to emulate the original behavior: if the user calls MCP2221() without the bus, it will check if the first device is already opened/cached and return that, if not it will try to open a different one below.
Other possible behaviors are:
- return one of the cached MCP (if any), even if it's not the first
- if no bus is specified, only try to open the first device
Note that simply importing the module will automatically create an instance of the first device, so any subsequent MCP2221 call (without the bus) will return that instance. This might not happen if another process already opened the first device -- in that case the class will try to open a different one.
There was a problem hiding this comment.
I now changed this so that:
MCP2221()(with nobus_idspecified) opens and returns the first available MCP (and raises an error if there are no MCP2221 available).MCP2221(bus_id)opens and returns the corresponding MCP2221, or return the cached instance if it's already open.
| cls.instances[bus_id] = self | ||
| else: | ||
| # find the first available MCP | ||
| self = cls.new_instance() |
There was a problem hiding this comment.
If no bus is specified and the first device is not cached (e.g. because some other process already opened it), then it will try to find and open another one. This behavior is non-deterministic, since it depends on the order the devices have been connected, and on whether other processes have opened the device.
If we want MCP2221() (without the bus) to only ever access the first device, the call to new_instance can be removed and the code changed to just try to open the first device (and possibly failing and raising an error without falling back on the next available device).
There was a problem hiding this comment.
I now removed the new_instance method and updated the code so that MCP2221 will try to find and open the first available MCP.
| return self | ||
|
|
||
| @staticmethod | ||
| def available_paths(*, require_mcps=False): |
There was a problem hiding this comment.
For this method I considered a few different options:
- a read-only
@property, but in order to work in__new__it must work without instance (and@propertyrequires the instance); - a stand-alone function, but it would need to be imported separately, and having it as a method is handy.
Regarding the name:
- I used
pathsbecause that's the name used byhid.enumerate. Perhapsaddressesordevicesmight be better. - I used
required_mcps, butrequired_mcp2221smight be better even if longer. Other options (likeraise_if_no_mcp2221s) feel too verbose.
| except (OSError, RuntimeError) as e: | ||
| print(f'[ERR]: {e}') | ||
| if len(bus_ids) == 1: | ||
| raise # we failed to open the only MCP | ||
| continue # try opening the next one | ||
| else: | ||
| print('Failed to open all hid devices') | ||
| raise RuntimeError(f'Can not open any of the {len(bus_ids)} ' | ||
| f'connected MCP2221 devices.') |
There was a problem hiding this comment.
To maintain backward compatibility, if there's only one MCP, the error is re-raised as is. When there are multiple MCPs, a generic error is returned if none of them can be opened. This might end up hiding some information on the cause of the errors though.
There was a problem hiding this comment.
This code is now integrated in __new__. The same comment applies.
| args = parser.parse_args() | ||
|
|
||
| # Need to be bytes, always function 2 | ||
| device_path = "{:04x}:{:04x}:02".format(args.bus, args.device).encode() |
There was a problem hiding this comment.
This script doesn't work for me because of this line. The paths to my devices look like 2-3.3:1.2, whereas this will produce something like 0002:0003:02. Perhaps the example could be updated to print a list of available addresses, and prompt the user to select one.
|
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". |
|
Pylint seems to get confused by the use of |
|
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 boardI have another unrelated script which consumes one MCP and then I run the script above and I get: It seems to work fine but then tries to open a second free device and there isn't anymore and so fails. |
|
Thanks for testing! The error you see seems to be caused by a combination of factors:
Therefore, with my changes, calling
This is explained in #637 (comment), where other possible options are also discussed. So, normally importing This issue can be mitigated easily by removing the second call to Footnotes
|
|
So I guess this is a mergeable form at this point right? It might be a good time to cancel my PR? |
|
If this PR works for you, you can cancel your PR and I'll remove the debug |
|
I now removed the debug To summarize, this PR:
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: Sample output: $ 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']( Optional TODOs (both have inline comments with more details):
cc @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? |
|
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 |
|
I did not test extensively but GPIO seems to work on my side. |
|
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? |
|
I posted some general information on my use-cases in my original PR: #496 (comment) For the moment I'm using multiple mcp2221 in 2 projects I can easily share:
Note that it it using my original PR code which is not as optimal as this new improved PR.
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. |
|
Example 1 is only using I2C on multiple MCP's.
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.
So you think this PR should provide access to the GPIO pins of all the attached MCP's in addition to I2C? |
Unfortunately these are the only 2 examples I can link to right now.
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 |
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.: Therefore the primary goal of this PR is to add support for multiple Since this PR updates the code so that each instance of the In fact, the only changes made by this PR to the However the
My understanding is that:
The current code uses 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
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? |
|
I don't think that the |
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.
FWIW I need to import |
This adds a new kwarg,
For that exact example yes. However, the MCP library could provide its own 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 |
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 actually not a bad idea, however the I2C object in the original example is instantiated with If we simply add an class MCP2221:
...
def get_i2c(self, *args, **kwargs):
self._i2c = mcp2221.I2C(*args, ** kwargs)
return self._i2cEven if we do this, we would still need to import 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 |
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.
Yup, that would work. You'd want to ensure you only return one object and that the frequency matched.
You don't because the MCP I2C constructor ignores the pin arguments. There must only be one set of i2c pins on it. Adafruit_Blinka/src/adafruit_blinka/microcontroller/mcp2221/i2c.py Lines 11 to 14 in 28ae3d3
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. |
|
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 Similar arguments apply to FT232H. |
|
With separate library, do you mean a separate module in this repo, a new repo under FWIW I'm also fine implementing the solution suggested above by @tannewt (i.e. adding an |
|
A separate repo. There is the Community Bundle repo which maybe this could be added to? |
|
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. |
This is a follow-up of: * #48 #48 requires adafruit/Adafruit_Blinka#637 in order to work, so the `get_sensor_i2c_bus()` function can't be used unless we install `Adafruit_Blinka` from my fork.
This PR is an improved version and a follow-up of:
It includes all the changes from #496 with some additional changes:
__init__(andget_instance()) with a__new__method that either returns a new instance or a cached one;available_paths()method that returns a list of all available MCP2221 addresses;new_instance()method that loops through all the available MCP2221 addresses and tries to open them;mcp2221variable inmcp2221.pyThe PR is against
main, so the diff doesn't show the removal ofget_instanceand the restoration of themcp2221var.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