Serial enumeration for Linux #14
Conversation
|
@Susurrus Thanks for looking at this. I've done some work to prepare for this exact feature, which you might find helpful: On Linux, serial ports can be listed with libudev. I wrote Rust bindings for libudev (https://github.com/dcuddeback/libudev-rs), which can enumerate existing ports or monitor for hotplug events. There's an example of listing devices here: https://github.com/dcuddeback/libudev-rs/blob/master/examples/list_devices.rs. On OS X, serial ports can be listed with IOKit. I wrote *-sys crates for IOKit and CoreFoundation (https://github.com/dcuddeback/iokit-sys and https://github.com/dcuddeback/core-foundation-sys), but no safe wrappers yet. The IOKit-sys project has an example of listing serial ports: https://github.com/dcuddeback/iokit-sys/blob/master/examples/list_serial_ports.rs. For Windows, I haven't implemented anything yet. I've been looking at this project as a reference: https://github.com/wjwwood/serial/blob/master/src/impl/list_ports/list_ports_win.cc. That project also has example of using libudev and IOKit. For other Unix operating systems (this crate supports FreeBSD and OpenBSD, and will support more in the future), I'm not sure what the best approach is going to be. Maybe fallback to scanning As for naming, I personally like |
|
So I've got enumeration working, but it pulls all 99 TTY devices on my system. I've plugged in an FTDI cable and here's what it says (using your example list_devices()) code. Now for the regular TTY devices they look less exciting: And then there are a few like: I'm not certain what the best way to distinguish between these devices is, but if you look at libserialport, then you can see that they first check to see if the device has a parent, discarding them if they don't. Additionally if it's a |
|
I've now added both of those checks that |
|
And if someone could comment on my use of unwrap(), I'd appreciate it. I don't think they should be used in non-example code as far as I understand, but the code gets tedious quite quickly without them. Any guidance on refining those lines would be appreciated. |
|
@Susurrus Thanks for taking the time to write this. I'm not crazy about the idea of opening random devices on the user's computer. I think that goes against expectations of a function that lists devices. Opening a serial device often causes it to toggle it's RTS and DTR lines, which are sometimes hooked up to external equipment. The parent check alone without attempting to open the device returns both of my FTDI devices and about 20 As for |
| @@ -20,7 +20,7 @@ pub use FlowControl::*; | |||
| /// use serial::prelude::*; | |||
| /// ``` | |||
| pub mod prelude { | |||
| pub use ::{SerialPort,SerialPortSettings}; | |||
| pub use ::{SerialPort,SerialPortSettings,SerialPortInfo}; | |||
There was a problem hiding this comment.
SerialPortInfo doesn't need to be part of the prelude.
|
Still not really sure how to deal with all of the |
|
@Susurrus Here's the source code for |
|
Do you mean via specialization or by making a custom |
|
I think I understand what you mean. Unfortunately using |
|
Alright, I've updated my commit. I got some help on r/rust that pointed out a few better ways to do this. |
|
Sorry for the noise, here's the latest. I accidentally included some debugging test code in there (wasn't suitable for a real test). Also I've bumped the libudev requirements for this to 0.1.3, which is when |
|
@Susurrus When I suggested write a macro like I was also thinking that it might make sense to return an iterator instead of a The CI build is failing. The |
| Ok(d) => { | ||
| for device in d { | ||
| if let Some(_) = device.parent() { | ||
| if let Some(path) = device.syspath().to_str() { |
There was a problem hiding this comment.
Seems like devnode (/dev/ttyUSB0) would be more useful to the user than syspath (/sys/devices/pci0000:00/0000:00:14.0/usb1/1-9/1-9:1.0/ttyUSB0/tty/ttyUSB0).
|
Alright, I've struggled a bit with error types. Finally hit that "fighting the compiler" problem that Rust has for newbies. I wasn't sure a) what error type to use (I think I just use Serial's Error type) and then b) expose the libudev errors through that error type (which I believe is the same). I'm still very new at this and I should probably read up on importing types, but I wanted to get the rest finished for now. I'll take another crack at this tomorrow probably, but all the rest of the changes you mentioned are now there. Looks like there are still Travis CI errors, but they seem to be regarding libudev itself, so I don't know how to resolve them. |
|
Ha! I made a little progress here. I have pretty much all the scaffolding done, but I'm having an issue with an error: I'm unclear why exactly I'm having this error. It's supposed to be referring to how the type inference in the compiler can't determine an approriate type for this, but I don't understand a) why that is or b) how to fix it. I feel like this is so close, too! |
|
@dcuddeback Any feedback on this? I've also gotten the Travis CI process fixed and working so now this does work on Linux and doesn't break OS X. So it's not the prettiest, but it's sound. As to your previous suggestion of using Error types, here is a patch that implements it. diff --git a/src/lib.rs b/src/lib.rs
index d171386..681365f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -49,7 +49,10 @@ pub enum ErrorKind {
/// A parameter was incorrect.
InvalidInput,
- /// An I/O error occured.
+ /// An unknown error occurred.
+ Unknown,
+
+ /// An I/O error occurred.
///
/// The type of I/O error is determined by the inner `io::ErrorKind`.
Io(io::ErrorKind)
@@ -866,7 +869,7 @@ mod tests {
}
-/// A device-indepenent implementation of serial port information.
+/// A device-independent implementation of serial port information.
#[derive(Debug,Clone,PartialEq,Eq)]
pub struct PortInfo {
/// Port name
@@ -874,6 +877,6 @@ pub struct PortInfo {
}
#[cfg(target_os = "linux")]
-pub fn list_ports() -> Vec<PortInfo> {
+pub fn list_ports() -> Result<Vec<PortInfo>> {
posix::list_ports()
}
diff --git a/src/posix/error.rs b/src/posix/error.rs
index b200e26..4e1bf31 100644
--- a/src/posix/error.rs
+++ b/src/posix/error.rs
@@ -1,4 +1,6 @@
extern crate libc;
+#[cfg(target_os = "linux")]
+extern crate libudev;
use std::error::Error;
use std::ffi::CStr;
@@ -37,6 +39,16 @@ pub fn from_io_error(io_error: io::Error) -> ::Error {
}
}
+#[cfg(target_os = "linux")]
+pub fn from_libudev_error(libudev_error: libudev::Error) -> ::Error {
+ let description = libudev_error.description().to_string();
+ match libudev_error.kind() {
+ NoMem => ::Error::new(::ErrorKind::Unknown, description),
+ InvalidInput => ::Error::new(::ErrorKind::InvalidInput, description),
+ libudev::ErrorKind::Io(a) => ::Error::new(::ErrorKind::Io(a), description)
+ }
+}
+
// the rest of this module is borrowed from libstd
const TMPBUF_SZ: usize = 128;
diff --git a/src/posix/tty.rs b/src/posix/tty.rs
index 5922690..b7cf378 100644
--- a/src/posix/tty.rs
+++ b/src/posix/tty.rs
@@ -13,7 +13,7 @@ use std::os::unix::prelude::*;
use self::libc::{c_int,c_void,size_t};
-use ::{SerialDevice,SerialPortSettings,PortInfo};
+use ::{SerialDevice,SerialPortSettings,PortInfo,ErrorKind,Error};
#[cfg(target_os = "linux")]
@@ -535,7 +535,7 @@ impl SerialPortSettings for TTYSettings {
mod tests {
use std::mem;
- use super::TTYSettings;
+ use super::{list_ports, TTYSettings};
use ::prelude::*;
fn default_settings() -> TTYSettings {
@@ -645,20 +645,28 @@ mod tests {
settings.set_flow_control(::FlowNone);
assert_eq!(settings.flow_control(), Some(::FlowNone));
}
+
+ #[test]
+ fn list_ports_example() {
+ for p in list_ports() {
+ println!("Port: {}", p.port_name);
+ }
+ }
}
#[cfg(target_os = "linux")]
-pub fn list_ports() -> Vec<PortInfo> {
+pub fn list_ports() -> ::Result<Vec<PortInfo>> {
let mut vec = Vec::new();
if let Ok(context) = libudev::Context::new() {
let mut enumerator = match libudev::Enumerator::new(&context) {
- Err(_) => return vec,
+ Err(e) => return Err(super::error:from_libudev_error(e)),
Ok(k) => k
};
if enumerator.match_subsystem("tty").is_err() {
- return vec;
+ return Err(Error::new(ErrorKind::Unknown, "An unknown error occurred.".to_string()))
}
match enumerator.scan_devices() {
+ Err(e) => return Err(super::error:from_libudev_error(e)),
Ok(d) => {
for device in d {
if device.parent().is_some() {
@@ -669,9 +677,8 @@ pub fn list_ports() -> Vec<PortInfo> {
}
}
}
- },
- Err(_) => return vec
+ }
}
}
- vec
+ Ok(vec)
}Unfortunately it fails with: And right now I don't really know how to fix that. |
|
Nevermind, someone in #rust on IRC helped me figure out I had a syntax error above. I've now gotten everything fixed and I'm using the |
|
Let me also say that I really think that we should check all the ttyS* ports by opening them. I end up with 32 devices in my list that are fake and 1 real one. Considering both Qt and libserialport have this functionality I would consider it safe, even though you don't like it. While I understand not liking it, I also think that if someone has a real serialport and they have something connected to it that blows up when the port is turned on, they have issues with their hardware that existed before they ran our software. |
|
Added an additional commit to remove serial8250 virtual ports in case we can get that merged. |
|
@dcuddeback Just pinging you on this. What do you think about merging this now? |
|
@Susurrus I'd like to have implementations ready for Windows, OS X, BSD, and other Unixes before merging this. If I were to give you some feedback on this PR now, I think I'd be repeating things from earlier in this thread. I think it's a little premature for that, since adding implementations for the other operating systems may change how this is implemented anyway. I also have a branch in progress that restructures this library quite heavily. I'd recommend putting this on the back-burner until that is done to avoid unnecessary merge conflicts. |
|
I've added Windows now, and also added an unimplemented error for unsupported OSes. |
|
Note that adding the |
|
Closing due to inactivity. I'll probably work on this feature myself. |
|
What?! Why? This is fully implemented and working, why did you close this? I'm actively using this code on Linux and I've tested the Windows version as well. There's no activity for a wile because I've been waiting for you to review the code, provide feedback, and merge this. |
Start of work for #13. This doesn't actually work yet, but I wanted feedback on the API that I'm adding to serial-rs before I go too far down this road. I'm not familiar with Rust enough to know what's "rusty", so I'm basically just copying from the existing API in this lib.
Note that SerialPortInfo will likely change to include more data (see http://code.qt.io/cgit/qt/qtserialport.git/tree/src/serialport/qserialportinfo.h#n62), but for the basic implementation only the port name is required, so I'm planning on leaving it at that for the first implementation.
Biggest bike-shedding issue is what to name
scan_ports(), Qt usesAvailablePorts()(which I like)_ while Python useslist_ports.comports()(which I don't). I thinkavailable_ports()would be good, and I'll change it in the next revision if there's no complaint.