Skip to content

tools/mpremote: Fix auto-connection bug in macOS and add config option to filter ports eligible for auto-connection. #9140

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mdaeron
Copy link

@mdaeron mdaeron commented Aug 29, 2022

Signed-off-by: Mathieu Daëron mathieu@daeron.fr

Signed-off-by: Mathieu Daëron <mathieu@daeron.fr>
Signed-off-by: Mathieu Daëron <mathieu@daeron.fr>
@mdaeron
Copy link
Author

mdaeron commented Aug 29, 2022

This PR corrects a bug on macOS, where cu.Bluetooth-Incoming-Port is considered an eligible port for mpremote auto connect (#8485). The fix is perhaps brittle (check sys.platform), and could need to be extended to correct similar bugs on other operating systems.

It also introduces a new configuration option, autoconnect_regexp, which is a regular expression used to filter ports eligible for auto connection. This option will generally not be needed, but is useful when more granular control of which ports to use is called for, e.g. when debugging code in several boards communicating/working together

@dpgeorge dpgeorge added the tools Relates to tools/ directory in source, or other tooling label Aug 30, 2022
@bulletmark
Copy link
Contributor

I don't think that PR is a good idea. Since the docs say "This command automatically detects and connects to the first available serial device" and Macs for some reason are not doing that then surely the solution is to fix that automatic connection logic rather than add an obscure "autoconnect_regexp" option which users are required to discover and configure?

@mdaeron
Copy link
Author

mdaeron commented Aug 30, 2022

I don't think that PR is a good idea. Since the docs say "This command automatically detects and connects to the first available serial device" and Macs for some reason are not doing that then surely the solution is to fix that automatic connection logic rather than add an obscure "autoconnect_regexp" option which users are required to discover and configure?

Fair point, thanks. I agree that solving the auto connect bug and adding an option to filter ports are two separate issues. I've edited the PR accordingly.

  • mpremote connect auto now works out of the box on my Mac. Something similar might be needed for Windows.
  • autoconnect_regexp remains as an (obscure, yet documented) option for those few who might need it. In my experience, it can save a lot of time and frustration when debugging two or more boards working/communicating together (in which case auto connect is great) without interference from other ports which might be used for other things.

Signed-off-by: Mathieu Daëron <mathieu@daeron.fr>
Signed-off-by: Mathieu Daëron <mathieu@daeron.fr>
@mdaeron mdaeron force-pushed the add_grep_to_mpremote_autoconnect branch from 0a73138 to f3cc3b7 Compare August 30, 2022 12:32
@mdaeron mdaeron changed the title tools/mpremote: Add autoconnect_regexp config to filter auto ports. tools/mpremote: Fix auto-connection bug in macOS and add config option to filter ports eligible for auto-connection. Aug 30, 2022
@ide
Copy link

ide commented Oct 14, 2022

rshell has about 30 lines of heuristics for autoconnection that work pretty well and may be worth borrowing: https://github.com/dhylands/rshell/blob/b87878c01aa02d6cd871ff4666983dee5584af16/rshell/main.py#L227-L254

@dpgeorge
Copy link
Member

Thanks for the contribution. The ideas in this PR are good, namely:

  1. improve auto connection on mac
  2. allow the user to have more control over auto connection

The solution to 1, restricting to /dev/cu.usb seems pretty good.

But I think for 2, since it's going to be a rarely used command and therefore advanced, how about making it fully general by allowing an arbitrary filter function to be provided by the user? Eg something like

             port_filter = lambda p: True # default filter
             ...
             for p in sorted(serial.tools.list_ports.comports()):
                 if not port_filter(p):
                     continue
                 ...

@Neradoc
Copy link

Neradoc commented Feb 26, 2023

Note that a board with a WCH chip like the CH340/CH9102 (a common replacement for CP210x these days), using the official driver (which is required to work properly, at least for esptool for example) will have its name appear in the form of cu.wchusbserial*, not to mention that the other, default driver port will also still be there (cu.usbmodem*).

I would personally just exclude the default bluetooth ports if a more comprehensive filter/search is not in scope.

/dev/cu.BLTH
/dev/cu.Bluetooth-Incoming-Port

@mdaeron
Copy link
Author

mdaeron commented Feb 27, 2023

Note that a board with a WCH chip like the CH340/CH9102 (a common replacement for CP210x these days), using the official driver (which is required to work properly, at least for esptool for example) will have its name appear in the form of cu.wchusbserial*, not to mention that the other, default driver port will also still be there (cu.usbmodem*).

@Neradoc, what does sys.platform return for your WCH chip? Sorry, don't know what I was thinking!

@mdaeron
Copy link
Author

mdaeron commented Feb 27, 2023

I've tried to look into this again:

  • When installing mpremote from PyPI in a clean environment, I get version 0.4.0 and it treats /dev/cu.Bluetooth-Incoming-Port as a valid port.
  • But when doing pip install -e . from tools/mpremote in a clean environment, I get version 0.3.0 but it (rightfully) ignores /dev/cu.Bluetooth-Incoming-Port.

I'm guessing the cu.Bluetooth* issue has now been fixed, perhaps inadvertently, but what's with the version decrease?

@Josverl
Copy link
Contributor

Josverl commented Mar 13, 2023

@mdaeron , could it be that you have checked out the v1.19.1 tag , rather than the latest (master HEAD ) ?

RetiredWizard pushed a commit to RetiredWizard/micropython that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Relates to tools/ directory in source, or other tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants