bloguetronica / cp2130-conf

CP2130 Configurator (cp2130-conf) is an application that can be used to configure CP2130 devices, including VID, PID, as well as other descriptors. Most importantly, the application allows you to configure pin functions and states.
GNU General Public License v3.0
1 stars 1 forks source link

ConfiguratorWindow::resetDevice() requires refactoring #18

Closed samuelfmlourenco closed 2 years ago

samuelfmlourenco commented 2 years ago

In the present moment, inside "configuratorwindow.cpp", from lines 648 to 677, we have resetDevice() implemented as follows:

// Resets the device
void ConfiguratorWindow::resetDevice()
{
    int errcnt = 0;
    QString errstr;
    cp2130_.reset(errcnt, errstr);
    cp2130_.close();  // Important! - This should be done always, even if the previous reset operation shows an error, because an error doesn't mean that a device reset was not effected
    int err;
    for (int i = 0; i < ENUM_RETRIES; ++i) {  // Verify enumeration according to the number of times set by "ENUM_RETRIES" [10]
        NonBlocking::msleep(500);  // Wait 500ms each time
        err = cp2130_.open(vid_, pid_, serialstr_);
        if (err != 2) {  // Retry only if the device was not found yet (as it may take some time to enumerate)
            break;
        }
    }
    if (err == CP2130::ERROR_INIT) {  // Failed to initialize libusb
        QMessageBox::critical(this, tr("Critical Error"), tr("Could not reinitialize libusb.\n\nThis is a critical error and execution will be aborted."));
        exit(EXIT_FAILURE);  // This error is critical because libusb failed to initialize
    } else if (err == CP2130::ERROR_NOT_FOUND) {  // Failed to find device
        err_ = true;
        errmsg_ = tr("Device disconnected.\n\nThe corresponding window will be disabled.");  // Since version 1.2, the error message is not shown here
    } else if (err == CP2130::ERROR_BUSY) {  // Failed to claim interface
        err_ = true;
        errmsg_ = tr("Device ceased to be available. It could be in use by another application.\n\nThe corresponding window will be disabled.");  // Same as above
    } else {
        readDeviceConfiguration();
        this->setWindowTitle(tr("CP2130 Configurator (S/N: %1)").arg(serialstr_));
        displayConfiguration(deviceConfig_);
    }
}

However, similarly to what happens with openDevice() (see issue https://github.com/bloguetronica/cp2130-conf/issues/16), this function assumes that the device was successfully reopened if CP2130::open() doesn't return any error values, that is, doesn't return CP2130::ERROR_INIT, CP2130::ERROR_NOT_FOUND or CP2130::ERROR_BUSY. However, if in the future CP2130::open() is changed so it returns new error conditions, this will potentially cause issues. Thus, resetDevice() should be rewritten. Below is a proposal:

// Resets the device
void ConfiguratorWindow::resetDevice()
{
    int errcnt = 0;
    QString errstr;
    cp2130_.reset(errcnt, errstr);
    cp2130_.close();  // Important! - This should be done always, even if the previous reset operation shows an error, because an error doesn't mean that a device reset was not effected
    int err;
    for (int i = 0; i < ENUM_RETRIES; ++i) {  // Verify enumeration according to the number of times set by "ENUM_RETRIES" [10]
        NonBlocking::msleep(500);  // Wait 500ms each time
        err = cp2130_.open(vid_, pid_, serialstr_);
        if (err != CP2130::ERROR_NOT_FOUND) {  // Retry only if the device was not found yet (as it may take some time to enumerate)
            break;
        }
    }
    if (err == CP2130::SUCCESS) {  // Device was successfully reopened
        readDeviceConfiguration();
        this->setWindowTitle(tr("CP2130 Configurator (S/N: %1)").arg(serialstr_));
        displayConfiguration(deviceConfig_);
    } else if (err == CP2130::ERROR_INIT) {  // Failed to initialize libusb
        QMessageBox::critical(this, tr("Critical Error"), tr("Could not reinitialize libusb.\n\nThis is a critical error and execution will be aborted."));
        exit(EXIT_FAILURE);  // This error is critical because libusb failed to initialize
    } else if (err == CP2130::ERROR_NOT_FOUND) {  // Failed to find device
        err_ = true;
        errmsg_ = tr("Device disconnected.\n\nPlease reconnect it and try again.");  // Since version 1.2, the error message is not shown here
    } else if (err == CP2130::ERROR_BUSY) {  // Failed to claim interface
        err_ = true;
        errmsg_ = tr("Device ceased to be available. It could be in use by another application.");  // Same as above
    }
}

If ITUSB2Device::open() returns an error value that is not expected at this time, this new algorithm will do nothing. Note that this refactor also solves issue https://github.com/bloguetronica/cp2130-conf/issues/13.

samuelfmlourenco commented 2 years ago

Code refactored in version 1.4.