IEP-1033 esptool write_flash command in the IDE #826
Conversation
WalkthroughThe changes introduce a new feature to the ESP-IDF Eclipse plugin that allows users to write binary data to the ESP's flash chip. This is achieved through the addition of new classes, methods, constants, and UI elements, as well as updates to the documentation. The feature includes a new dialog for writing flash memory, command handlers, and error validation. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files ignored due to filter (1)
- bundles/com.espressif.idf.ui/plugin.xml
Files selected for processing (7)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFConstants.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java (5 hunks)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/WriteFlashDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/WriteFlashCommandHandler.java (1 hunks)
Files skipped from review due to trivial changes (3)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFConstants.java
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java
Additional comments (Suppressed): 8
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/WriteFlashCommandHandler.java (1)
- 14-20: The
executemethod is correctly implemented to open theWriteFlashDialog. However, it's important to note that this method always returnsnull. If there are any callers expecting a return value from this method, they may not behave as expected. Please verify if this is the intended behavior.public Object execute(ExecutionEvent event) throws ExecutionException { Shell activeShell = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell(); WriteFlashDialog flashDialog = new WriteFlashDialog(activeShell); flashDialog.create(); flashDialog.open(); - return null; + // Consider returning a meaningful value or void }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1)
- 24-32: The new keys and values added to the properties file are related to the new feature of writing binary data to flash memory. They include labels, error messages, and instructions for the new dialog box
WriteFlashDialog. The spelling, grammar, and formatting appear to be correct.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/WriteFlashDialog.java (1)
- 1-360: The
WriteFlashDialogclass is well structured and follows good practices for creating a dialog in SWT. However, there are some areas that could be improved or need to be verified.bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java (5)
20-23: The new
writeFlashProcessvariable is introduced to manage the process of writing binary data to flash memory. This is consistent with the existing pattern of managing processes in this class.39-43: A new method
writeFlash(String port, String path, String offset)is added to start the write flash process. It first destroys any running chip info process, then starts a new write flash process using the command generated bygetWriteFlashCommand(port, path, offset). The started process is returned.46-57: The
getWriteFlashCommand(String port, String path, String offset)method constructs the command list for writing binary data to flash memory. It includes the Python environment path, theesptool.pyscript file path, the port, the write flash command, the offset, and the binary file path. This is consistent with the existing pattern of constructing command lists in this class.107-115: The
checkActiveWriteFlashProcess()method checks if there is an active write flash process running. This is consistent with the existing pattern of checking for active processes in this class.117-123: The
killWriteFlashProcess()method kills the write flash process if it exists. This is consistent with the existing pattern of killing processes in this class.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/WriteFlashDialog.java
Outdated
Show resolved
Hide resolved
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/WriteFlashDialog.java
Outdated
Show resolved
Hide resolved
|
@AndriiFilippov Please verify this |
|
Hi @sigmaaa Can you also documentation for this improvement? |
Hi @kolipakakondal, I'm waiting for a new icon for this command and after that will add documentation |
|
@kolipakakondal, @AndriiFilippov added documentation |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files selected for processing (2)
- README.md (3 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/WriteFlashDialog.java (1 hunks)
Additional comments (Suppressed): 4
README.md (3)
59-65: The new feature "Write Binary Data to Flash" has been added to the table of contents. Ensure that the link
#writeFlashBinarycorrectly navigates to the corresponding section in the document.790-802: The new section "Write Binary to Flash" provides a detailed explanation of the new feature. It includes instructions on how to access the feature and how to use it, along with screenshots for visual aid. Ensure that the screenshots are correctly displayed and that they accurately represent the described steps.
866-869: No changes have been made in this hunk. The instructions for raising bugs remain the same.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/WriteFlashDialog.java (1)
- 1-361: The
WriteFlashDialogclass is well-structured and follows good practices for creating a dialog in SWT. However, there are a few areas that could be improved for better maintainability and error handling.
|
added new icon |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- bundles/com.espressif.idf.ui/icons/Write_Binary_Data_to_Flash.png
- bundles/com.espressif.idf.ui/plugin.xml
Files selected for processing (8)
- README.md (2 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFConstants.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java (5 hunks)
- bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/WriteFlashDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/WriteFlashCommandHandler.java (1 hunks)
Files skipped from review due to trivial changes (4)
- README.md
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFConstants.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/handlers/WriteFlashCommandHandler.java
Additional comments: 7
bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties (1)
- 67-74: The new command and its label have been added correctly. Ensure that the command name and label are consistent throughout the codebase.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1)
- 20-32: The new labels, error messages, and button labels added to the properties file are clear and descriptive. They provide the necessary information to the user and are consistent with the existing messages. The spelling error in line 25 should be corrected.
- WriteFlashDialog_BinFileErrFormatErrMsg=%s bin file doens't exist + WriteFlashDialog_BinFileErrFormatErrMsg=%s bin file doesn't existbundles/com.espressif.idf.core/src/com/espressif/idf/core/util/EspToolCommands.java (5)
8-8: The import statement for
java.io.IOExceptionis added to handle the exception thrown by thewriteFlashmethod.20-23: The
writeFlashProcessvariable is added to manage the process of writing to flash memory.39-44: The
writeFlashmethod is added to start the process of writing to flash memory. It calls thegetWriteFlashCommandmethod to construct the command and then starts the process. It also destroys any active chip info process before starting the write flash process. This could potentially interrupt other operations, so it's important to ensure that this behavior is intended and properly handled in the rest of the code.46-57: The
getWriteFlashCommandmethod is added to construct the command for writing to flash memory. It uses theIDFUtil.getIDFPythonEnvPathandIDFUtil.getEspToolScriptFilemethods to get the necessary paths, and theIDFConstants.ESP_WRITE_FLASH_CMDconstant for the command. It also takes theport,path, andoffsetas parameters.107-123: The
checkActiveWriteFlashProcessandkillWriteFlashProcessmethods are added to manage the write flash process. ThecheckActiveWriteFlashProcessmethod checks if the write flash process is active, and thekillWriteFlashProcessmethod kills the write flash process if it's active. These methods provide a way to manage the write flash process from outside theEspToolCommandsclass.
| private void setDefaults() | ||
| { | ||
| offsetText.setText(DEFAULT_OFFSET); | ||
| ISelection selection = PlatformUI.getWorkbench().getActiveWorkbenchWindow().getSelectionService() | ||
| .getSelection(); | ||
| if (selection instanceof IStructuredSelection) | ||
| { | ||
| Object element = ((IStructuredSelection) selection).getFirstElement(); | ||
|
|
||
| if (element instanceof IResource) | ||
| { | ||
| project = ((IResource) element).getProject(); | ||
| } | ||
| } | ||
| String defaultPathToBin = project.getLocationURI().getPath().concat(File.separator).concat(DEFAULT_BIN_NAME); | ||
| if (new File(defaultPathToBin).exists()) | ||
| { | ||
| binPathText.setText(defaultPathToBin); | ||
| } | ||
| else | ||
| { | ||
| binPathText.setMessage(defaultPathToBin); | ||
| } | ||
|
|
||
| ILaunchBarManager barManager = IDFCorePlugin.getService(ILaunchBarManager.class); | ||
| try | ||
| { | ||
| String serialPortFromTarget = barManager.getActiveLaunchTarget().getAttribute(ATTR_SERIAL_PORT, | ||
| StringUtil.EMPTY); | ||
| comPortsCombo.setText(serialPortFromTarget); | ||
| if (!serialPortFromTarget.isEmpty()) | ||
| { | ||
| comPortsCombo.notifyListeners(SWT.Selection, null); | ||
| } | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| doChangeErrorMessage = true; | ||
| } |
There was a problem hiding this comment.
The setDefaults method is also quite long and does a lot of different things. Consider breaking it up into smaller, more manageable methods. This would make the code easier to read and maintain. For example, you could have separate methods for setting the default offset, setting the default bin path, and setting the default serial port.
| Thread writeFlashThread = new Thread(() -> | ||
| { | ||
| try | ||
| { | ||
| Process writeFlashProcess = espToolCommands.writeFlash(selectedPort, binsPathString, | ||
| offString); | ||
| InputStream targetIn = writeFlashProcess.getInputStream(); | ||
| BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(targetIn)); | ||
| String line = StringUtil.EMPTY; | ||
| while ((line = bufferedReader.readLine()) != null) | ||
| { | ||
| final String toWrite = line; | ||
| Display.getDefault().asyncExec(() -> { | ||
| if (!deviceInformationText.isDisposed()) | ||
| { | ||
| deviceInformationText.append(toWrite + StringUtil.LINE_SEPARATOR); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
|
|
||
| Display.getDefault().syncExec(() -> { | ||
| if (!comPortsCombo.isDisposed()) | ||
| { | ||
| comPortsCombo.setEnabled(true); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The okPressed method starts a new thread to execute the writeFlash command. However, it doesn't handle any exceptions that might occur during the execution of this command. Consider adding a try-catch block inside the thread to handle any exceptions and inform the user if something goes wrong.
| boolean validateStatus = true; | ||
| if (comPortsCombo.getText().isEmpty()) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.WriteFlashDialog_SerialPortErrMsg); | ||
| } | ||
| if (!new File(binPathText.getText()).exists()) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(String.format(Messages.WriteFlashDialog_BinFileErrFormatErrMsg, binPathText.getText())); | ||
| } | ||
| try | ||
| { | ||
| BigIntDecoder.decode(offsetText.getText()); | ||
| } | ||
| catch (NumberFormatException e) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.WriteFlashDialog_OffsetErrMsg); | ||
| } | ||
| if (validateStatus) | ||
| { | ||
| setErrorMessage(null); | ||
| } | ||
| return validateStatus; | ||
| } |
There was a problem hiding this comment.
The validateInputs method checks if the inputs are valid but doesn't provide any feedback to the user if they are not. Consider showing an error message to the user if the inputs are not valid. This would make the application more user-friendly and easier to use.
|
@sigmaaa hi ! Tested under: Please, delete "/" because Python could not recognize the PATH
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/WriteFlashDialog.java (1 hunks)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java
Additional comments: 1
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1)
- 20-35: The changes in this hunk are adding new messages for the WriteFlashDialog. These messages are used for labels, error messages, and information messages. The messages are clear and descriptive, which should help users understand how to use the new feature and what errors might occur.
| public WriteFlashDialog(Shell parentShell) | ||
| { | ||
| super(parentShell); | ||
| ports = new String[] {}; | ||
| try | ||
| { | ||
| ports = SerialPort.list(); | ||
| espToolCommands = new EspToolCommands(); | ||
| } | ||
| catch (IOException e) | ||
| { | ||
| Logger.log(e); | ||
| } |
There was a problem hiding this comment.
The constructor of WriteFlashDialog is catching an IOException but only logs it and continues execution. This could lead to a situation where espToolCommands is null but the program continues to run, potentially causing a NullPointerException later. Consider handling this exception more gracefully, perhaps by showing an error message to the user or disabling the dialog if this exception occurs.
| protected Control createDialogArea(Composite parent) | ||
| { | ||
| Composite area = (Composite) super.createDialogArea(parent); | ||
| Composite container = new Composite(area, SWT.NONE); | ||
| container.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true)); | ||
| GridLayout layout = new GridLayout(3, false); | ||
| container.setLayout(layout); | ||
| GridData comboLayoutData = new GridData(); | ||
| comboLayoutData.grabExcessHorizontalSpace = true; | ||
| comboLayoutData.horizontalAlignment = GridData.FILL; | ||
| comboLayoutData.horizontalSpan = 2; | ||
| Label comPortsLabel = new Label(container, SWT.NONE); | ||
| comPortsLabel.setText(Messages.EraseFlashDialog_ComPortLabel); | ||
|
|
||
| comPortsCombo = new Combo(container, SWT.READ_ONLY); | ||
|
|
||
| for (String port : ports) | ||
| { | ||
| comPortsCombo.add(port); | ||
| } | ||
|
|
||
| comPortsCombo.setLayoutData(comboLayoutData); | ||
| Label binPathLabel = new Label(container, SWT.NONE); | ||
| binPathLabel.setText(Messages.WriteFlashDialog_Bin_Path_Lbl); | ||
| binPathText = new Text(container, SWT.SINGLE | SWT.BORDER | SWT.H_SCROLL); | ||
| binPathText.setLayoutData(new GridData(GridData.FILL_HORIZONTAL)); | ||
|
|
||
| Button browseButton = new Button(container, SWT.PUSH); | ||
| browseButton.setLayoutData(new GridData(SWT.RIGHT, SWT.FILL, false, false, 1, 1)); | ||
| browseButton.setText(Messages.WriteFlashDialog_Browse_Btn); | ||
| browseButton.addSelectionListener(new SelectionAdapter() | ||
| { | ||
| @Override | ||
| public void widgetSelected(SelectionEvent evt) | ||
| { | ||
| FileDialog fileSelectionDialog = new FileDialog(getParentShell()); | ||
| fileSelectionDialog.setFilterExtensions(EXTENSIONS); | ||
| fileSelectionDialog.setFilterPath(project.getLocationURI().getPath()); | ||
| String selectedFilePath = fileSelectionDialog.open(); | ||
|
|
||
| if (selectedFilePath != null && !selectedFilePath.isEmpty()) | ||
| { | ||
| binPathText.setText(selectedFilePath); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Label offsetLabel = new Label(container, SWT.NONE); | ||
| offsetLabel.setText(Messages.WriteFlashDialog_Offset_Lbl); | ||
| offsetText = new Text(container, SWT.SINGLE | SWT.BORDER | SWT.H_SCROLL); | ||
| offsetText.setLayoutData(new GridData(SWT.FILL, SWT.FILL, false, false, 2, 1)); | ||
|
|
||
| deviceInformationText = new Text(container, SWT.READ_ONLY | SWT.BORDER | SWT.H_SCROLL | SWT.V_SCROLL); | ||
| deviceInformationText.setText(Messages.EraseFlashDialog_DeviceInformationAreaInitialText); | ||
| deviceInformationText.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true, 3, 1)); | ||
|
|
||
| comPortsCombo.addSelectionListener(new ComPortSelectionListener()); | ||
| binPathText.addListener(SWT.Modify, e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInputs())); | ||
| offsetText.addListener(SWT.Modify, e -> getButton(IDialogConstants.OK_ID).setEnabled(validateInputs())); | ||
|
|
||
| return container; | ||
| } |
There was a problem hiding this comment.
The createDialogArea method is quite long and does a lot of different things. Consider breaking it up into smaller, more manageable methods. This would make the code easier to read and maintain. For example, you could have separate methods for creating the combo box, the text fields, and the buttons.
| { | ||
| boolean validateStatus = true; | ||
| if (comPortsCombo.getText().isEmpty()) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.WriteFlashDialog_SerialPortErrMsg); | ||
| } | ||
| if (!new File(binPathText.getText()).exists()) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(String.format(Messages.WriteFlashDialog_BinFileErrFormatErrMsg, binPathText.getText())); | ||
| } | ||
| try | ||
| { | ||
| BigIntDecoder.decode(offsetText.getText()); | ||
| } | ||
| catch (NumberFormatException e) | ||
| { | ||
| validateStatus = false; | ||
| setErrorMessage(Messages.WriteFlashDialog_OffsetErrMsg); | ||
| } | ||
| if (validateStatus) | ||
| { | ||
| setErrorMessage(null); | ||
| } | ||
| return validateStatus; | ||
| } |
There was a problem hiding this comment.
The validateInputs method checks the validity of the inputs provided by the user. It's good practice to validate user input, but the current implementation could be improved. The method sets error messages directly, which means that only the last error message will be displayed if multiple inputs are invalid. Consider collecting all error messages and displaying them together to provide more comprehensive feedback to the user.
| { | ||
| private String selectedPort; | ||
|
|
||
| @Override | ||
| public void widgetSelected(SelectionEvent event) | ||
| { | ||
| Thread infoThread; | ||
| getButton(IDialogConstants.OK_ID).setEnabled(validateInputs()); | ||
| selectedPort = ((Combo) event.getSource()).getText(); | ||
| try | ||
| { | ||
| deviceInformationText.setText(Messages.EraseFlashDialog_LoadingMessage); | ||
| infoThread = new Thread(this); | ||
| infoThread.start(); | ||
| } | ||
| catch (Exception exception) | ||
| { | ||
| Logger.log(exception); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void run() | ||
| { | ||
| try | ||
| { | ||
| Process chipInfoProcess = espToolCommands.chipInformation(selectedPort); | ||
| InputStream targetIn = chipInfoProcess.getInputStream(); | ||
| BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(targetIn)); | ||
| int readCharInt = 0; | ||
| while ((readCharInt = bufferedReader.read()) != -1) | ||
| { | ||
| final char charToWrite = (char) readCharInt; | ||
| Display.getDefault().asyncExec(() -> { | ||
| if (!deviceInformationText.isDisposed()) | ||
| { | ||
| deviceInformationText.append(Character.toString(charToWrite)); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ComPortSelectionListener class runs a long-running operation (fetching chip information) in a separate thread, which is good practice. However, it doesn't handle exceptions that might occur during this operation. Any exceptions are simply logged and not communicated to the user. Consider adding error handling to inform the user if fetching chip information fails.
|
@sigmaaa hi ! Tested: LGTM 👍 |
Description
added esptool: write_flash command. UI is made based on also esptool command
erase flash.Fixes # (IEP-1033)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test A:
NVS Table EditorTest Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features:
User Interface:
Documentation: