-
Notifications
You must be signed in to change notification settings - Fork 131
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 a command to list available serial ports #761
Conversation
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.
Hi! Thanks for your contribution, mostly LGTM only a few details! Did some suscesfull testing:
❯ cargo r -r --bin espflash -- list-ports --name-only
...
/dev/cu.usbserial-1110
/dev/cu.usbserial-1120
/dev/tty.usbserial-1110
/dev/tty.usbserial-1120
❯ cargo r -r --bin espflash -- list-ports
...
/dev/cu.usbserial-1110 Silicon Labs CP2102N USB to UART Bridge Controller [EA60:10C4]
/dev/cu.usbserial-1120 Silicon Labs CP2102N USB to UART Bridge Controller [EA60:10C4]
/dev/tty.usbserial-1110 Silicon Labs CP2102N USB to UART Bridge Controller [EA60:10C4]
/dev/tty.usbserial-1120 Silicon Labs CP2102N USB to UART Bridge Controller [EA60:10C4]
Maybe we could improve the formatting when printing all the details with some separators (-
or |
) between fields or adding the name of the field being printed (name
, manufacturer..) for UsbPort
s, but this is a nitpick that can be ignored
espflash/src/bin/espflash.rs
Outdated
/// List serial ports available for flashing. | ||
/// | ||
/// Only lists ports of devices known to be used on development boards. Use | ||
/// -a to show all available ports. |
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.
/// List serial ports available for flashing. | |
/// | |
/// Only lists ports of devices known to be used on development boards. Use | |
/// -a to show all available ports. | |
/// List serial ports available for flashing. |
The -a
info is incomplete and its already displayed when using -h/--help
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.
Updated, hopefully this is a bit clearer:
List serial ports available for flashing.
The default behavior is to only list ports of devices known to be used on development boards.
Usage: cargo espflash list-ports [OPTIONS]
Options:
-a, --list-all-ports
List all available serial ports, instead of just those likely to be development boards.
Includes non-usb ports such as PCI devices
...
cargo-espflash/src/main.rs
Outdated
/// List serial ports available for flashing. | ||
/// | ||
/// Only lists ports of devices known to be used on development boards. Use | ||
/// -a to show all available ports. |
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.
/// List serial ports available for flashing. | |
/// | |
/// Only lists ports of devices known to be used on development boards. Use | |
/// -a to show all available ports. | |
/// List serial ports available for flashing. |
The -a
info is incomplete and its already displayed when using -h/--help
espflash/src/cli/mod.rs
Outdated
pub struct ListPortsArgs { | ||
/// List all available serial ports. | ||
#[arg(short, long)] | ||
pub all_ports: bool, |
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.
pub all_ports: bool, | |
pub list_all_ports: bool, |
To be consistent with the similar arg in ConnectArgs
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 updated this one, but kept the -a short argument, that just feels like the obvious 'all' argument to me.
It does actually have a fixed width first column of 15 characters for the name, but OS-X device names are longer then that. But I could create columns based on the length of the actual content, that would be something like this:
Or maybe this looks a bit nicer:
Or maybe the USB ID isn't that interesting here and we should just leave it out?
Personallly I like the second one best, it seems the easiest to read to me. And keeping the USB ID might be useful for someone in some script. |
Agree, second format is the one also like the most! |
Ok, that was slightly more commits then should have been needed, but this should have better comments and column output. |
Do you also mind rebasing and resolving the conflicts? |
6ef2bd5
to
c1abbdb
Compare
Done, I also squashed it to a single commit. |
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.
LGTM! Thanks
This pull request uses the existing logic for finding the serial ports to to create a new 'list-ports' command that just lists the ports that are available, with some additional information. This allows for some additional information when trying to resolve issues when flashing. It also opens some opportunities for scripting, e.g. to flash multiple connected devices from a single script.
By default only the ports that are 'known', that is ports that might be automatically selected are shown. The -a (or --all-ports) flag shows all serial ports available.
Tested on Ubuntu, Windows and OS-X.