From 6aa21015ea0a956d8a563dd3b52f4701368b4f2d Mon Sep 17 00:00:00 2001 From: Denis Shienkov Date: Sun, 1 Jun 2014 14:48:59 +0000 Subject: Simplify of the availablePortsByUdev() There is no sense to use udev_device_get_subsystem() for comparing with the predetermined names of subsystems (like "pnp", "usb" and so on). This idea was preliminary and required further revising after accumulation of some statistic. The main ideas in favor of refactoring are given below: * It seems that reasonable to use the "ID_MODEL", "ID_VENDOR" and others identifiers to query of properties for all types of serial ports. We lose nothing, because in the worst case the getUdevPropertyValue() function will simply return an empty string, as before. * Earlier we excluded from the list all ports which have a subsystem with the "platform" name. Practice showed that for these devices is used the serial8250 driver. Therefore it is reasonable to exclude this type of devices by the name of drivers instead of by the name of subsystems. Also are made related changes: * Is added the resolving of chars for the new udev_device_get_driver() function * The functions of getting properties are wrapped for reduction of length up to 100 characters. Tested on ArchLinux 64-bit with the on-board, PL2303 serial devices using Qt4 and then Qt5. Tested build on Android x86 using Qt5. Change-Id: I1a66164ca1893180e1ed97524cff98b4a933a63b Reviewed-by: Sergey Belyashov Reviewed-by: Denis Shienkov --- src/serialport/qserialportinfo_unix.cpp | 80 +++++++++++++++++++-------------- src/serialport/qtudev_p.h | 2 + 2 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/serialport/qserialportinfo_unix.cpp b/src/serialport/qserialportinfo_unix.cpp index 4fc6650..61d21d6 100644 --- a/src/serialport/qserialportinfo_unix.cpp +++ b/src/serialport/qserialportinfo_unix.cpp @@ -234,6 +234,45 @@ QString getUdevPropertyValue(struct ::udev_device *dev, const char *name) return QString::fromLatin1(::udev_device_get_property_value(dev, name)); } +static +bool checkUdevForSerial8250Driver(struct ::udev_device *dev) +{ + const QString driverName = QString::fromLatin1(::udev_device_get_driver(dev)); + return (driverName == QStringLiteral("serial8250")); +} + +static +QString getUdevModelName(struct ::udev_device *dev) +{ + return getUdevPropertyValue(dev, "ID_MODEL") + .replace(QLatin1Char('_'), QLatin1Char(' ')); +} + +static +QString getUdevVendorName(struct ::udev_device *dev) +{ + return getUdevPropertyValue(dev, "ID_VENDOR") + .replace(QLatin1Char('_'), QLatin1Char(' ')); +} + +static +quint16 getUdevModelIdentifier(struct ::udev_device *dev, bool &hasIdentifier) +{ + return getUdevPropertyValue(dev, "ID_MODEL_ID").toInt(&hasIdentifier, 16); +} + +static +quint16 getUdevVendorIdentifier(struct ::udev_device *dev, bool &hasIdentifier) +{ + return getUdevPropertyValue(dev, "ID_VENDOR_ID").toInt(&hasIdentifier, 16); +} + +static +QString getUdevSerialNumber(struct ::udev_device *dev) +{ + return getUdevPropertyValue(dev,"ID_SERIAL_SHORT"); +} + QList availablePortsByUdev() { #ifndef LINK_LIBUDEV @@ -276,43 +315,18 @@ QList availablePortsByUdev() if (parentdev) { - QString subsys = QString::fromLatin1(::udev_device_get_subsystem(parentdev)); - - if (subsys == QStringLiteral("usb-serial") - || subsys == QStringLiteral("usb")) { - serialPortInfo.d_ptr->description = - getUdevPropertyValue(dev.data(), "ID_MODEL").replace(QLatin1Char('_'), QLatin1Char(' ')); - - serialPortInfo.d_ptr->manufacturer = - getUdevPropertyValue(dev.data(), "ID_VENDOR").replace(QLatin1Char('_'), QLatin1Char(' ')); - - serialPortInfo.d_ptr->serialNumber = getUdevPropertyValue(dev.data(),"ID_SERIAL_SHORT"); - - serialPortInfo.d_ptr->vendorIdentifier = - getUdevPropertyValue(dev.data(), "ID_VENDOR_ID").toInt(&serialPortInfo.d_ptr->hasVendorIdentifier, 16); - - serialPortInfo.d_ptr->productIdentifier = - getUdevPropertyValue(dev.data(), "ID_MODEL_ID").toInt(&serialPortInfo.d_ptr->hasProductIdentifier, 16); - - } else if (subsys == QStringLiteral("pnp")) { - // TODO: Obtain more information - } else if (subsys == QStringLiteral("platform")) { + if (checkUdevForSerial8250Driver(parentdev)) continue; - } else if (subsys == QStringLiteral("pci")) { - serialPortInfo.d_ptr->description = - getUdevPropertyValue(dev.data(), "ID_MODEL"); - serialPortInfo.d_ptr->manufacturer = - getUdevPropertyValue(dev.data(), "ID_VENDOR"); + serialPortInfo.d_ptr->description = getUdevModelName(dev.data()); + serialPortInfo.d_ptr->manufacturer = getUdevVendorName(dev.data()); + serialPortInfo.d_ptr->serialNumber = getUdevSerialNumber(dev.data()); - serialPortInfo.d_ptr->vendorIdentifier = - getUdevPropertyValue(dev.data(), "ID_VENDOR_ID").toInt(&serialPortInfo.d_ptr->hasVendorIdentifier, 16); + serialPortInfo.d_ptr->vendorIdentifier = getUdevModelIdentifier( + dev.data(), serialPortInfo.d_ptr->hasVendorIdentifier); - serialPortInfo.d_ptr->productIdentifier = - getUdevPropertyValue(dev.data(), "ID_MODEL_ID").toInt(&serialPortInfo.d_ptr->hasProductIdentifier, 16); - } else { - // FIXME: Obtain more information - } + serialPortInfo.d_ptr->productIdentifier = getUdevVendorIdentifier( + dev.data(), serialPortInfo.d_ptr->hasProductIdentifier); } else { if (serialPortInfo.d_ptr->portName.startsWith(rfcommDeviceName)) { bool ok; diff --git a/src/serialport/qtudev_p.h b/src/serialport/qtudev_p.h index 434292d..0f0246d 100644 --- a/src/serialport/qtudev_p.h +++ b/src/serialport/qtudev_p.h @@ -82,6 +82,7 @@ GENERATE_SYMBOL_VARIABLE(struct udev_device *, udev_device_new_from_syspath, str GENERATE_SYMBOL_VARIABLE(const char *, udev_list_entry_get_name, struct udev_list_entry *) GENERATE_SYMBOL_VARIABLE(const char *, udev_device_get_devnode, struct udev_device *) GENERATE_SYMBOL_VARIABLE(const char *, udev_device_get_sysname, struct udev_device *) +GENERATE_SYMBOL_VARIABLE(const char *, udev_device_get_driver, struct udev_device *) GENERATE_SYMBOL_VARIABLE(struct udev_device *, udev_device_get_parent, struct udev_device *) GENERATE_SYMBOL_VARIABLE(const char *, udev_device_get_subsystem, struct udev_device *) GENERATE_SYMBOL_VARIABLE(const char *, udev_device_get_property_value, struct udev_device *, const char *) @@ -127,6 +128,7 @@ inline bool resolveSymbols(QLibrary *udevLibrary) RESOLVE_SYMBOL(udev_list_entry_get_name) RESOLVE_SYMBOL(udev_device_get_devnode) RESOLVE_SYMBOL(udev_device_get_sysname) + RESOLVE_SYMBOL(udev_device_get_driver) RESOLVE_SYMBOL(udev_device_get_parent) RESOLVE_SYMBOL(udev_device_get_subsystem) RESOLVE_SYMBOL(udev_device_get_property_value) -- cgit v1.2.1 From 2cde429d3bd3adc2bfc7d268fb64aad97a07e0e1 Mon Sep 17 00:00:00 2001 From: Denis Shienkov Date: Mon, 2 Jun 2014 23:38:37 +0000 Subject: Use QScopedPointer (RAII) to do not care of the udev resources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is a good idea to get rid from the manually releasing of the allocated udev resources. Tested on ArchLinux 64 bit with on-board and PL2303 serial ports using Qt4 and then Qt5. Change-Id: Ib25e100bea37ad3cc0bee015b7de6d4de762f2cc Reviewed-by: Peter Kümmel Reviewed-by: Sergey Belyashov --- src/serialport/qserialportinfo_unix.cpp | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/serialport/qserialportinfo_unix.cpp b/src/serialport/qserialportinfo_unix.cpp index 61d21d6..0529973 100644 --- a/src/serialport/qserialportinfo_unix.cpp +++ b/src/serialport/qserialportinfo_unix.cpp @@ -217,6 +217,22 @@ QList availablePortsBySysfs() } struct ScopedPointerUdevDeleter +{ + static inline void cleanup(struct ::udev *pointer) + { + ::udev_unref(pointer); + } +}; + +struct ScopedPointerUdevEnumeratorDeleter +{ + static inline void cleanup(struct ::udev_enumerate *pointer) + { + ::udev_enumerate_unref(pointer); + } +}; + +struct ScopedPointerUdevDeviceDeleter { static inline void cleanup(struct ::udev_device *pointer) { @@ -284,24 +300,24 @@ QList availablePortsByUdev() static const QString rfcommDeviceName(QStringLiteral("rfcomm")); - struct ::udev *udev = ::udev_new(); + QScopedPointer udev(::udev_new()); + if (udev) { - struct ::udev_enumerate *enumerate = - ::udev_enumerate_new(udev); + QScopedPointer enumerate(::udev_enumerate_new(udev.data())); if (enumerate) { - ::udev_enumerate_add_match_subsystem(enumerate, "tty"); - ::udev_enumerate_scan_devices(enumerate); + ::udev_enumerate_add_match_subsystem(enumerate.data(), "tty"); + ::udev_enumerate_scan_devices(enumerate.data()); struct ::udev_list_entry *devices = - ::udev_enumerate_get_list_entry(enumerate); + ::udev_enumerate_get_list_entry(enumerate.data()); struct ::udev_list_entry *dev_list_entry; udev_list_entry_foreach(dev_list_entry, devices) { - QScopedPointer dev(::udev_device_new_from_syspath(udev, + QScopedPointer dev(::udev_device_new_from_syspath(udev.data(), ::udev_list_entry_get_name(dev_list_entry))); if (dev) { @@ -344,10 +360,8 @@ QList availablePortsByUdev() } - ::udev_enumerate_unref(enumerate); } - ::udev_unref(udev); } return serialPortInfoList; -- cgit v1.2.1 From 19da18b96897c7d0b9dce2dd589d0135b397005e Mon Sep 17 00:00:00 2001 From: Denis Shienkov Date: Tue, 3 Jun 2014 22:06:31 +0400 Subject: Refactor the availablePortsByUdev() in favor to readability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implementation of the availablePortsByUdev() function is a little complicated by excess "if/else" conditions and also too long lines, that worsens readability. It is reasonable to make the following: * To get rid of the big "if {...}" blocks in favor to immediate return from function in case of an error. It will allow to reduce a quantity of lines and will shift code alignment to the left. * To split declaration of some long variables and functions into separate lines, with length at least up to 80~100 characters. * To drop of the 'struct' keywords and the '::' global namespace operator for variables. Tested on ArchLinux 64 bit with the on-board and PL2303 serial ports using Qt4 and then Qt5. Tested build on Android x86 using Qt5. Change-Id: Iddc2a9511230e56e4a9d01a4c22af7b2eaeae60c Reviewed-by: Peter Kümmel Reviewed-by: Sergey Belyashov Reviewed-by: Denis Shienkov --- src/serialport/qserialportinfo_unix.cpp | 86 +++++++++++++++------------------ 1 file changed, 38 insertions(+), 48 deletions(-) diff --git a/src/serialport/qserialportinfo_unix.cpp b/src/serialport/qserialportinfo_unix.cpp index 0529973..b4caf88 100644 --- a/src/serialport/qserialportinfo_unix.cpp +++ b/src/serialport/qserialportinfo_unix.cpp @@ -296,72 +296,62 @@ QList availablePortsByUdev() if (!symbolsResolved) return QList(); #endif - QList serialPortInfoList; - static const QString rfcommDeviceName(QStringLiteral("rfcomm")); QScopedPointer udev(::udev_new()); - if (udev) { - - QScopedPointer enumerate(::udev_enumerate_new(udev.data())); - - if (enumerate) { - - ::udev_enumerate_add_match_subsystem(enumerate.data(), "tty"); - ::udev_enumerate_scan_devices(enumerate.data()); - - struct ::udev_list_entry *devices = - ::udev_enumerate_get_list_entry(enumerate.data()); - - struct ::udev_list_entry *dev_list_entry; - udev_list_entry_foreach(dev_list_entry, devices) { - - QScopedPointer dev(::udev_device_new_from_syspath(udev.data(), - ::udev_list_entry_get_name(dev_list_entry))); - - if (dev) { + if (!udev) + return QList(); - QSerialPortInfo serialPortInfo; + QScopedPointer + enumerate(::udev_enumerate_new(udev.data())); - serialPortInfo.d_ptr->device = QString::fromLatin1(::udev_device_get_devnode(dev.data())); - serialPortInfo.d_ptr->portName = QString::fromLatin1(::udev_device_get_sysname(dev.data())); + if (!enumerate) + return QList(); - struct ::udev_device *parentdev = ::udev_device_get_parent(dev.data()); + ::udev_enumerate_add_match_subsystem(enumerate.data(), "tty"); + ::udev_enumerate_scan_devices(enumerate.data()); - if (parentdev) { + udev_list_entry *devices = ::udev_enumerate_get_list_entry(enumerate.data()); - if (checkUdevForSerial8250Driver(parentdev)) - continue; + QList serialPortInfoList; + udev_list_entry *dev_list_entry; + udev_list_entry_foreach(dev_list_entry, devices) { - serialPortInfo.d_ptr->description = getUdevModelName(dev.data()); - serialPortInfo.d_ptr->manufacturer = getUdevVendorName(dev.data()); - serialPortInfo.d_ptr->serialNumber = getUdevSerialNumber(dev.data()); + QScopedPointer + dev(::udev_device_new_from_syspath( + udev.data(), ::udev_list_entry_get_name(dev_list_entry))); - serialPortInfo.d_ptr->vendorIdentifier = getUdevModelIdentifier( - dev.data(), serialPortInfo.d_ptr->hasVendorIdentifier); + if (!dev) + return serialPortInfoList; - serialPortInfo.d_ptr->productIdentifier = getUdevVendorIdentifier( - dev.data(), serialPortInfo.d_ptr->hasProductIdentifier); - } else { - if (serialPortInfo.d_ptr->portName.startsWith(rfcommDeviceName)) { - bool ok; - int portNumber = serialPortInfo.d_ptr->portName.mid(rfcommDeviceName.length()).toInt(&ok); + QSerialPortInfo serialPortInfo; - if (!ok || (portNumber < 0) || (portNumber > 255)) - continue; - } else { - continue; - } - } + serialPortInfo.d_ptr->device = QString::fromLatin1(::udev_device_get_devnode(dev.data())); + serialPortInfo.d_ptr->portName = QString::fromLatin1(::udev_device_get_sysname(dev.data())); - serialPortInfoList.append(serialPortInfo); - } + udev_device *parentdev = ::udev_device_get_parent(dev.data()); + if (parentdev) { + if (checkUdevForSerial8250Driver(parentdev)) + continue; + serialPortInfo.d_ptr->description = getUdevModelName(dev.data()); + serialPortInfo.d_ptr->manufacturer = getUdevVendorName(dev.data()); + serialPortInfo.d_ptr->serialNumber = getUdevSerialNumber(dev.data()); + serialPortInfo.d_ptr->vendorIdentifier = getUdevModelIdentifier(dev.data(), serialPortInfo.d_ptr->hasVendorIdentifier); + serialPortInfo.d_ptr->productIdentifier = getUdevVendorIdentifier(dev.data(), serialPortInfo.d_ptr->hasProductIdentifier); + } else { + if (serialPortInfo.d_ptr->portName.startsWith(rfcommDeviceName)) { + bool ok; + int portNumber = serialPortInfo.d_ptr->portName.mid(rfcommDeviceName.length()).toInt(&ok); + if (!ok || (portNumber < 0) || (portNumber > 255)) + continue; + } else { + continue; } - } + serialPortInfoList.append(serialPortInfo); } return serialPortInfoList; -- cgit v1.2.1