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

Add support for multiple mcp2221 i2c busses #496

Closed
wants to merge 17 commits into from

Conversation

fgervais
Copy link
Contributor

There a couple different use-cases highlighted in the provided examples. The main one probably being the one where we get all mcp2221 busses:

addresses = [mcp["path"] for mcp in hid.enumerate(MCP2221_VID, MCP2221_PID)]

for address in addresses:
    i2c_busses.append(busio.I2C(bus_id=address))

for i2c in i2c_busses:
    print("I2C devices found: ", [hex(i) for i in i2c.scan()])

The current use-case with a single or the first device is still supported as-is or with sda/scl arguments left out:

i2c = busio.I2C()
print("I2C devices found: ", [hex(i) for i in i2c.scan()])

Those changes should also allow merging in https://github.com/adafruit/Adafruit_Python_Extended_Bus quite easily.

Other Considerations

For now the multiple device support only works for i2c and the pin functionality always work only on the first one like it was before.

I will probably provide the multiple device support for pins in a follow up if I need it in my project. As there is no regression I think it's acceptable to leave it out for now.

Note that the change in busio.I2C._init_() has been done forcing the new parameter as a named argument so you stay in control of the positional arguments and so could add some in the future without breaking code using multiple MCP2221 devices.

@caternuson
Copy link
Contributor

This is for using with multiple MCP221s? What is driving the need for more than one I2C bus?

@fgervais
Copy link
Contributor Author

On my side I use multiple mcp2221a modules for these 3 use-cases:

  1. On a fridge size project where I need sensors in two distinct areas 1 to 2 meters apart, I do the "long" cable run with the differential usb cable and once at both destinations I just need short i2c cable.
    I prefer using the usb cable to something like a qwiicbus as the usb 2 cable is smaller than the ethernet cable. Also using multiple mcp2221a is cheaper than using the qwiicbus.
    As a bonus, I like having 1 bus dedicated per area of the project.

  2. On a project where I need a lot of tmp117 sensors I use 1 mcp2221a per 4 tmp117 as 4 is the maximum per bus.

  3. On a project where the product is basically one mcp2221a coupled with a mlx90640 camera on a board, if I want to use multiple cameras, I need the multiple mcp2221a support as the product only have usb IO and doesn't readily give access to the i2c bus directly.
    Accessing the i2c bus in that case would also mean breaking the product apart.

@fgervais
Copy link
Contributor Author

fgervais commented Aug 1, 2021

Rebased on 6.13.0

@ezio-melotti
Copy link
Contributor

  1. On a fridge size project where I need sensors in two distinct areas 1 to 2 meters apart, I do the "long" cable run with the differential usb cable and once at both destinations I just need short i2c cable.

I have a similar setup, and I need to access different sensors connected to multiple MCPs.

I tried this PR and the example works:
>>> import os
>>> os.environ['BLINKA_MCP2221'] = '1' 
>>> os.environ['BLINKA_MCP2221_RESET_DELAY'] = '-1'
>>> import board
>>>
>>> import hid, busio
>>>
>>> MCP2221_VID = 0x04D8
>>> MCP2221_PID = 0x00DD
>>> 
>>> i2c_busses = []
>>> addresses = [mcp["path"] for mcp in hid.enumerate(MCP2221_VID, MCP2221_PID)]
>>> 
>>> for address in addresses:
...     i2c_busses.append(busio.I2C(bus_id=address))
... 
>>> for i2c in i2c_busses:
...     print("I2C devices found: ", [hex(i) for i in i2c.scan()])
... 
I2C devices found:  ['0x58']
I2C devices found:  ['0x77']

However, if I try the following I get an error:

>>> from adafruit_blinka.microcontroller.mcp2221.mcp2221 import MCP2221
>>> MCP2221.get_instance()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../adafruit_blinka/microcontroller/mcp2221/mcp2221.py", line 80, in get_instance
    instance = MCP2221()
  File ".../adafruit_blinka/microcontroller/mcp2221/mcp2221.py", line 65, in __init__
    self._reset()
  File ".../adafruit_blinka/microcontroller/mcp2221/mcp2221.py", line 169, in _reset
    self._hid.open_path(device_new_path)
  File "hid.pyx", line 139, in hid.device.open_path
RuntimeError: already open

I also tried physically disconnecting the sensors and reconnecting and still got the same error.

The same happens when I simply try to `import board` in my code:
File "myscript.py", line 16, in <module>
    import board
  File ".../board.py", line 180, in <module>
    from adafruit_blinka.board.microchip_mcp2221 import *
  File ".../adafruit_blinka/board/microchip_mcp2221.py", line 2, in <module>
    from adafruit_blinka.microcontroller.mcp2221 import pin
  File ".../adafruit_blinka/microcontroller/mcp2221/pin.py", line 5, in <module>
    mcp2221 = MCP2221.get_instance()
  File ".../adafruit_blinka/microcontroller/mcp2221/mcp2221.py", line 80, in get_instance
    instance = MCP2221()
  File ".../adafruit_blinka/microcontroller/mcp2221/mcp2221.py", line 65, in __init__
    self._reset()
  File ".../adafruit_blinka/microcontroller/mcp2221/mcp2221.py", line 169, in _reset
    self._hid.open_path(device_new_path)
  File "hid.pyx", line 139, in hid.device.open_path
RuntimeError: already open

This prevents me to run further tests.


In addition, since I'm using two different sensors (one per MCP), in order to create two sensor instances I should:

  1. use hid to get the addresses of the two MCP2221s
  2. use the addresses to open the two I2C buses
  3. scan the buses to find which bus the sensor is connected to (using its I2C address)
  4. use the matching I2C bus to create the sensor instance

This is a bit complicated, but I can't think of any other way to find and instantiate the sensors.

It might be useful to have a method in the MCP2221 class that automatically finds all the connected MCPs and populates the .instances attribute, so that in step 1 I can just import MCP2221 and do for address in MCP2221.instances: busio.I2C(bus_id=address).


Finally, the PR removes the global mcp2221 variable from the mcp2221.py module, which seems a backward-incompatible change. Perhaps it should be replaced with mcp2221 = MCP2221.get_instance()?

If this is done, the changes to pin.py should become unnecessary, and busio.py can still import the MCP2221 class instead.

@fgervais
Copy link
Contributor Author

It looks like the mcp2221 is already used by something else, maybe a test code left running or something?

Did you physically disconnect only the sensor from the i2c bus or the full mcp2221 from the usb bus?

Disconnecting the mcp2221 from the usb bus should unbind it from at least userspace code.

Another datapoint that I can provide right now is that import board is not expected to fail as I do do it in my code where I use this PR:

https://github.com/fgervais/project-smart-fridge/blob/278bfebd6eab1c3af514a8b03d029bf7243e18bc/firmware/main.py#L2

@ezio-melotti
Copy link
Contributor

I was able to solve the errors reported in my previous message and run a single sensor connected through an MCP2221:

Once I had one sensor running, I worked towards having both of them running. Currently I have multiple independent scripts -- one per sensor -- that I launch in separate processes, whereas I think you (@fgervais) run everything in a single process. The proposed PR seems to work for the single-process scenario, but not for multiple processes:

  • Opening an hid device locks it at the OS-level, preventing other processes to open/access it.
  • The PR, without a bus_id specified, always tries to open the first available address, and fails if it's already opened.
  • Importing board with BLINKA_MCP2221 = 1 set, automatically executes from adafruit_blinka.board.microchip_mcp2221 import * and mcp2221 = MCP2221.get_instance() (in pin.py) without providing a bus_id, which fails if another process already opened the first device.

In order to fix these issues, I tweaked the MCP2221.__init__ so that instead of opening the first available device, it loops through them returning the first one that can be opened without errors. After these changes I was eventually able to run two sensors connected to two MCPs in two different processes.

Currently, if a valid bus_id is specified and the corresponding hid device is available (i.e. not open by another process), then the MCP2221 class will return an instance corresponding to the given bus_id. When the bus_id is not specified:

  1. With one MCP and a single process it will just connect to the first (and only) MCP (if available). This is equivalent to the pre-PR behavior.
  2. With one MCP and multiple processes the second process will fail to open the device if the first process already has. This is also equivalent to the pre-PR behavior.
  3. With multiple MCPs and a single process it will connect to the first available MCP, meaning that repeated calls to MCP2221() will eventually return an instance for each available MCP, even without specifying the bus_id. Support for multiple MCPs has been added by this PR, and with my changes it's no longer necessary to specify the bus_id.
  4. With multiple MCPs and multiple processes they will connect to the first available MCP, meaning that if the first process gets the 1st MCP, the second process will get the 2nd (since the 1st is already in use by the first process). Support for this use-case was added by my changes.

While this is working fine for me, I'm still not convinced it's the best approach:

  • Opening the first available MCP is not deterministic, since the result may vary depending on the order of the addresses (returned by hid.enumerate()) and by the status of the devices (whether they are are accessible or already opened and inaccessible). This might also lead to race conditions with multiple processes.
  • While passing the bus_id solves the issue and is less error-prone, it is not always possible since the MCP is already automatically instantiated, e.g. while importing board.
  • In order to figure out which sensor is connected to which bus, all the buses need to be opened and scanned (and then possibly closed). This further increases the chances of race conditions.
  • If none of the MCPs is available (e.g. they are all already opened), the resulting error might end up being more generic and less informative, unless we list all the addresses that we tried.

I'm still working on solving these issues and doing more tests. Let me know if you want me to update this PR or create a separate one, either based on this or ex-novo.

Sync the `multiple-mcp2221` branch with upstream
@ezio-melotti
Copy link
Contributor

Upon further testing it appears that supporting multiple separate processes might not be doable -- at least without breaking backward compatibility. The mcp2221.py module currently creates a module-level mcp2221 variable that opens (and locks) the first MCP returned by hid.enumerate(). This variable is imported by other modules (e.g. pin.py, i2c.py), but possibly by user code too. Therefore:

  • If we remove it, we could adapt the Adafruit_Blinka code, but we might break user code (unless the mcp2221 module and variable are considered private and/or implementation details).
  • If we don't remove it, every process that uses the module will automatically try to open (and lock) the first available MCP, even if it's not the one they need to use, and by doing this they make it unavailable to the other processes.

@caternuson, would it be ok to remove the global mcp2221 defined in mcp2221.py?

@fgervais
Copy link
Contributor Author

fgervais commented Jan 2, 2023

Superseded by #637.

@fgervais fgervais closed this Jan 2, 2023
@ezio-melotti
Copy link
Contributor

Thanks for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants