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

The action on_pushButtonWrite_clicked() should check if the device is open before anything else #7

Closed samuelfmlourenco closed 2 years ago

samuelfmlourenco commented 2 years ago

Currently, on_pushButtonWrite_clicked() checks if the device is open after doing some checks. The original intent was to give time for the device to enumerate if the user previously canceled an ongoing device configuration. However, the dialog boxes halt the process, thus defeating that intent.

void ConfiguratorWindow::on_pushButtonWrite_clicked()
{
    if(showInvalidInput()) {
        QMessageBox::critical(this, tr("Error"), tr("One or more fields have invalid information.\n\nPlease correct the information in the fields highlighted in red."));
    } else {
        getEditedConfiguration();
        if (editedConfig_ == deviceConfig_ && !ui->checkBoxLock->isChecked()) {
            QMessageBox::information(this, tr("No Changes Done"), tr("No changes were effected, because no values were modified."));
        } else {
            int qmret = QMessageBox::question(this, tr("Write Configuration?"), tr("This will write the changes to the OTP ROM of your device. These changes will be permanent.\n\nDo you wish to proceed?"), QMessageBox::Yes | QMessageBox::No, QMessageBox::No);
            if (qmret == QMessageBox::Yes && cp2130_.isOpen()) {  // It is important to check if the device is open, since resetDevice() is non-blocking (a device reset could still be underway)
                configureDevice();
            }
        }
    }
}

Thus, it is better to make the button unresponsive to let the user know that there is a pending action that needs to be completed. Ideally, the "Write" button should be greyed out by the reset procedure, but that can interfere with other operations.

samuelfmlourenco commented 2 years ago

Bug fixed in version 1.2. Implementation as follows:

void ConfiguratorWindow::on_pushButtonWrite_clicked()
{
    if (cp2130_.isOpen()) {  // It is important to check if the device is open, since resetDevice() is non-blocking (a device reset could still be underway)
        if(showInvalidInput()) {
            QMessageBox::critical(this, tr("Error"), tr("One or more fields have invalid information.\n\nPlease correct the information in the fields highlighted in red."));
        } else {
            getEditedConfiguration();
            if (editedConfig_ == deviceConfig_ && !ui->checkBoxLock->isChecked()) {
                QMessageBox::information(this, tr("No Changes Done"), tr("No changes were effected, because no values were modified."));
            } else {
                int qmret = QMessageBox::question(this, tr("Write Configuration?"), tr("This will write the changes to the OTP ROM of your device. These changes will be permanent.\n\nDo you wish to proceed?"), QMessageBox::Yes | QMessageBox::No, QMessageBox::No);
                if (qmret == QMessageBox::Yes) {
                    configureDevice();
                }
            }
        }
    }
}