search

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::configureDevice() requires refactoring #24

Closed samuelfmlourenco closed 1 year ago

samuelfmlourenco commented 2 years ago

Currently, configureDevice() is implemented as follows (lines 408 to 457 of "configuratorwindow.cpp"):

// This is the main configuration routine, used to configure the CP2130 OTP ROM according to the tasks in the task list
void ConfiguratorWindow::configureDevice()
{
    err_ = false;
    requiresReset_ = false;
    QStringList tasks = prepareTaskList();  // Create a new task list
    int nTasks = tasks.size();
    QProgressDialog configProgress(tr("Configuring device..."), tr("Abort"), 0, nTasks, this);
    configProgress.setWindowModality(Qt::WindowModal);
    configProgress.setMinimumDuration(0);
    configProgress.setValue(0);  // This, along with setMinimumDuration(), will cause the progress dialog to display immediately
    bool aborted = false;
    for (int i = 0; i < nTasks; ++i) {  // Iterate through the newly created task list
        if (configProgress.wasCanceled()) {  // If user clicked "Abort"
            QMessageBox::information(this, tr("Configuration Aborted"), tr("The device configuration was aborted."));
            aborted = true;
            break;  // Abort the configuration
        }
        QMetaObject::invokeMethod(this, tasks[i].toStdString().c_str());  // The task list entry is converted to a C string
        if (err_) {  // If an error has occured
            configProgress.cancel();  // This hides the progress dialog (fix implemented in version 1.1)
            handleError();  // Mind that this shows a message, so it is important to hide the progress dialog first in order to avoid unresponsiveness
            QMessageBox::critical(this, tr("Error"), tr("The device configuration could not be completed."));
            break;  // Abort the configuration
        }
        configProgress.setValue(i + 1);  // Update the progress bar for each task done
    }
    if (!err_ && !aborted) {  // If the configuration was sucessful
        if (ui->checkBoxVerify->isChecked()) {
            QMessageBox::information(this, tr("Device Configured"), tr("Device was successfully configured and verified."));
        } else {
            QMessageBox::information(this, tr("Device Configured"), tr("Device was successfully configured."));
        }
    }
    if (!cp2130_.disconnected() && requiresReset_) {
        QProgressDialog resetProgress(tr("Resetting device..."), tr("Cancel"), 0, 1, this);
        resetProgress.setWindowModality(Qt::WindowModal);
        resetProgress.setMinimumDuration(0);
        resetProgress.setValue(0);  // As before, the progress dialog should appear immediately
        if (!resetProgress.wasCanceled()) {
            resetDevice();
            if (err_) {
                resetProgress.cancel();  // Hide the progress dialog
                handleError();  // Since version 1.2, resetDevice() requires non-critical errors to be handled externally (see function implementation below)
            } else {
                resetProgress.setValue(1);
            }
        }
    }
}

However, that implementation is convoluted. The boolean "aborted" is not actually necessary, because it is possible to see if the progress dialog was canceled, via the QProgressDialog::wasCanceled() function. By the same means, if is also possible to check if an error has occurred, since the progress dialog is canceled when that happens. Therefore, the condition "!err_ && !aborted" can be simply replaced with "!configProgress.wasCanceled()".

Having said that, it is possible to further refine this, by relocating the message handling to outside the for() loop. Despite this having somewhat redundant conditions, the code is cleaner and easier to debug. The progress dialog and message handling are totally separated, and no issues should occur as long as the progress dialog is guaranteed to be closed before exiting the loop. Thus, configureDevice() can be simplified even more:

// This is the main configuration routine, used to configure the CP2130 OTP ROM according to the tasks in the task list
void ConfiguratorWindow::configureDevice()
{
    err_ = false;
    requiresReset_ = false;
    QStringList tasks = prepareTaskList();  // Create a new task list
    int nTasks = tasks.size();
    QProgressDialog configProgress(tr("Configuring device..."), tr("Abort"), 0, nTasks, this);
    configProgress.setWindowModality(Qt::WindowModal);
    configProgress.setMinimumDuration(0);  // The progress dialog needs to be displayed right away, since resetDevice() is non-blocking
    configProgress.setValue(0);  // This, along with setMinimumDuration(), will cause the progress dialog to display immediately
    for (int i = 0; i < nTasks; ++i) {  // Iterate through the newly created task list (refactored in version 1.5)
        if (configProgress.wasCanceled()) {  // If the user clicks "Abort"
            break;  // Abort the configuration
        }
        QMetaObject::invokeMethod(this, tasks[i].toStdString().c_str());  // The task list entry is converted to a C string
        if (err_) {  // If an error has occured
            configProgress.cancel();  // This hides the progress dialog (fix implemented in version 1.1)
            break;  // Abort the configuration
        }
        configProgress.setValue(i + 1);  // Update the progress bar for each task done
    }
    if (err_) {  // If an error occured (refactored in version 1.5)
        handleError();
        QMessageBox::critical(this, tr("Error"), tr("The device configuration could not be completed."));
    } else if (configProgress.wasCanceled()) {  // If the device configuration was aborted by the user
        QMessageBox::information(this, tr("Configuration Aborted"), tr("The device configuration was aborted."));
    } else if (ui->checkBoxVerify->isChecked()) {  // Successul configuration with verification
        QMessageBox::information(this, tr("Device Configured"), tr("Device was successfully configured and verified."));
    } else {  // Successul configuration without verification
        QMessageBox::information(this, tr("Device Configured"), tr("Device was successfully configured."));
    }
    if (!cp2130_.disconnected() && requiresReset_) {
        QProgressDialog resetProgress(tr("Resetting device..."), tr("Cancel"), 0, 1, this);
        resetProgress.setWindowModality(Qt::WindowModal);
        resetProgress.setMinimumDuration(0);
        resetProgress.setValue(0);  // As before, the progress dialog should appear immediately
        if (!resetProgress.wasCanceled()) {
            resetDevice();
            if (err_) {
                resetProgress.cancel();  // Hide the progress dialog
                handleError();  // Since version 1.2, resetDevice() requires non-critical errors to be handled externally (see function implementation below)
            } else {
                resetProgress.setValue(1);
            }
        }
    }
}

Mind that a solution close to this was implemented in version 1.1, but then changed to the current form since version 1.2. However, both the topology implemented in version 1.1 and the new one are easier to follow.

samuelfmlourenco commented 1 year ago

Refactored in version 1.5.