Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 buses (improved) #637
Add support for multiple MCP2221 I2C buses (improved) #637
Changes from 19 commits
f86d610
b428f71
32852ce
877aae7
24a73e2
ce4032f
ca9d6ec
a7b6441
1224623
301fa1f
5104e5d
9447b81
6e98be4
1fe958f
b2e6c67
db91cce
6b89397
d739532
bb0ea63
3477c4f
eafe43f
a584d04
505be77
c8d9318
4ea3997
86ac218
041462e
28ae3d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 like0002:0003:02
. Perhaps the example could be updated to print a list of available addresses, and prompt the user to select one.This file was deleted.
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.
Here I tried to emulate the original behavior: if the user calls
MCP2221()
without thebus
, 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:
Note that simply importing the module will automatically create an instance of the first device, so any subsequent
MCP2221
call (without thebus
) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now changed this so that:
MCP2221()
(with nobus_id
specified) 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.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.
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 thebus
) to only ever access the first device, the call tonew_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now removed the
new_instance
method and updated the code so thatMCP2221
will try to find and open the first available MCP.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.
For this method I considered a few different options:
@property
, but in order to work in__new__
it must work without instance (and@property
requires the instance);Regarding the name:
paths
because that's the name used byhid.enumerate
. Perhapsaddresses
ordevices
might be better.required_mcps
, butrequired_mcp2221s
might be better even if longer. Other options (likeraise_if_no_mcp2221s
) feel too verbose.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is now integrated in
__new__
. The same comment applies.