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

Use the usb description for CP board name #2327

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

Conversation

tannewt
Copy link
Contributor

@tannewt tannewt commented Aug 10, 2022

It appears this is the product name on Linux at least.

It appears this is the product name on Linux at least.
@carlosperate
Copy link
Member

Thanks for the PR @tannewt!

PR #2323 has been merged, so rebasing or merging main should resolve the CI failures.

Has this been tested in Windows and macOS?

Looks like the default value for ListPortInfo.description might be n/a:
https://github.com/pyserial/pyserial/blob/v3.5/serial/tools/list_ports_common.py#L41
Do all CircuitPython devices provide the USB data for this? In that case it doesn't matter that much, but if some don't then we might want to check for the default value and replace it with the standard CircuitPython board name?

Another thing to keep in mind is that ListPortInfo.description could also contain the ListPortInfo.interface string as part of it:
https://github.com/pyserial/pyserial/blob/31fa4807d73ed4eb9891a88a15817b439c4eea2d/serial/tools/list_ports_common.py#L74
https://github.com/pyserial/pyserial/blob/v3.5/serial/tools/list_ports_common.py#L55-L58
I assume ListPortInfo.interface is None for CircuitPython devices? But if this changes in the future it will end up adding that info here.

@dhalbert
Copy link
Contributor

I assume ListPortInfo.interface is None for CircuitPython devices? But if this changes in the future it will end up adding that info here.

It's not None, it's 'CircuitPython CDC Control'. That's how mu identifies the board as a CircuitPython board, using Adafruit_Board_Toolkit. See mu #1371, where I added that capability instead of using VID/PID. But maybe you know that already.

Stock pyserial doesn't do this right on Windows or earlier versions of macOS. We have two unmerged PR's to pyserial to fix this:
pyserial/pyserial#566
pyserial/pyserial#571
Hence need for Adafruit_Board_Toolkit, which incorporates the code in those pyserial PR's.

Has this been tested in Windows and macOS?

Another thing to keep in mind is that ListPortInfo.description could also contain the ListPortInfo.interface string as part of it:

I tested on all three OS's. I see that description varies on the three OS's. Windows seems to substitute its own idea, but on the others, description includes interface. It looks like macOS 12.5 now doesn't need the special macOS workaround in Adafruit_Board_Toolkit. (@Neradoc this will be of interest to you.)

Stock pyserial is still wrong on Windows.

(transcripts below are a bit elided and don't show me tracking down the correct port index from multiple ports)

Ubuntu 22.04

# adafruit_board_toolkit works
>>> import adafruit_board_toolkit.circuitpython_serial
>>> ports = adafruit_board_toolkit.circuitpython_serial.comports()
>>> ports[0].interface
'CircuitPython CDC control'
>>> ports[0].description
'Metro M4 Express - CircuitPython CDC control'

# pyserial works
>>> import serial
>>> import serial.tools.list_ports
>>> ports = serial.tools.list_ports.comports()
>>> ports[2].interface
'CircuitPython CDC control'
>>> ports[2].description
'Metro M4 Express - CircuitPython CDC control'

macOS 12.5 on an Intel machine

# adafruit_board_toolkit works
>>> import adafruit_board_toolkit.circuitpython_serial
>>> ports = adafruit_board_toolkit.circuitpython_serial.comports()
>>> ports[0].interface
'CircuitPython CDC data'
>>> ports[0].description
'Metro M4 Express - CircuitPython CDC data'

# pyserial works now (didn't used to)
>>> import serial.tools.list_ports
>>> ports = serial.tools.list_ports.comports()
>>> ports[0].interface
'CircuitPython CDC data'
>>> ports[0].description
'Metro M4 Express - CircuitPython CDC data'

Windows 10 21H2

# Windows substitutes its own description
>>> import adafruit_board_toolkit.circuitpython_serial
>>> ports =adafruit_board_toolkit.circuitpython_serial.comports()
>>> ports[0].interface
'CircuitPython CDC control'
>>> ports[0].description
'USB Serial Device (COM8)'

# pyserial can't get interface and gives its own description
>>> import serial
>>> import serial.tools.list_ports
>>> ports = serial.tools.list_ports.comports()
>>> ports[1].interface
>>> ports[1].description
'USB Serial Device (COM8)'

@Neradoc
Copy link
Contributor

Neradoc commented Aug 11, 2022

I think it would be better to use .product followed by REPL or data based on the value of .interface.
Or fallback to .description if you don't get something reasonable from those.
EDIT: oh it's only the REPL ports ok.

On windows the product is missing, and the description is generic garbage (but localized !)

>>> ports[0].product
>>> ports[0].description
'Périphérique série USB (COM5)'

In discotool I found that the WMI interface truncates the USB product field (to 16 characters I think - think of the Circuitplayground Bluefruit), so I use this code to dive into the low level Windows USB API and list the USB devices with the proper full name. Anyone knowing more about those APIs could probably make a simpler version... I'd take it.
https://github.com/Neradoc/discotool/blob/main/discotool/usbinfos/usb_descriptor_win32.py

@carlosperate
Copy link
Member

So rather than Metro M4 Express - CircuitPython CDC data maybe what we want is CircuitPython - Metro M4 Express?

Would something like this work? "CircuitPython - {}".format(port.product)
Does Windows correctly set the product property?

@carlosperate
Copy link
Member

My last message went almost at the same time as @Neradoc's, and that responds to some of these questions, thanks! Oh and thanks @dhalbert for the details info before as well!

So even with the adafruit_board_toolkit workarounds, product and description don't provide the "device name/type", but on Linux and macOS we can use product. Is that correct?

If that's the case, do we know if there is any other way to retrieve the name from a different USB property or with some ctypes workaround?
I'd be happy to include a patch that works in macOS/Linux and has the default "CircuitPython" name on Windows, but it'd be awesome if we could find a workaround for Windows as well.

@Neradoc
Copy link
Contributor

Neradoc commented Aug 11, 2022

I believe I tried to add the product to the pyserial code by poking around with ctypes where it gets the description, and failed, but then again I don't know those APIs. My conclusion was that the APIs used by pyserial only relate to COM ports and don't have that information, so other APIs have to be used. The fact that the USB name is not available in Device Manager seemed to confirm that.

So I found that code that uses pywin32 to list the USB devices and pull the actual fields from them and adapted it.

@tannewt
Copy link
Contributor Author

tannewt commented Aug 11, 2022

I only tested on Arch Linux.

This serial info isn't coming from pyserial directly. It is a QSerialPortInfo. The description for me and my test board is "ESP32-S3-USB-OTG-N8" and does not contain the interface name.

@Neradoc
Copy link
Contributor

Neradoc commented Aug 11, 2022

Oh, port is not the comport from adafruit_board_toolkit 🤦 Well I misread that...
I'm really curious what it tells us on windows then.

On mac I get that, which is an accurate list of connected boards.
Capture d’écran 2022-08-11 à 21 08 00

@Neradoc
Copy link
Contributor

Neradoc commented Aug 11, 2022

Oh well, that's what I get on windows. 😞

mu-windows

@tannewt
Copy link
Contributor Author

tannewt commented Aug 19, 2022

@dhalbert Please finish this for me.

@ntoll
Copy link
Member

ntoll commented Aug 22, 2022

Love this.

Great delegation there @tannewt. 😛 I think you missed sudo in your comment (context: https://xkcd.com/149/)

Big hugs to all Adafruits in this chat, and many thanks for your continued contributions and support.

@aivarannamaa
Copy link

adafruit/Adafruit_Board_Toolkit#11 explains how you could get the proper product value for a serial port in Windows.

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.

6 participants