Espressif-IDE is not clearing serial port before trying to upload (IEP-1398) #1168
Conversation
WalkthroughThe changes involve the removal of exception handling for the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (1)
109-130: Improved resource management with try-with-resourcesThe thread implementation has been enhanced with try-with-resources for proper resource cleanup. This ensures that the InputStream is properly closed regardless of how the thread terminates (normal completion or exception). The exception handling has also been simplified with a single catch block for all exceptions.
Additionally, the finally block ensures that the serial connector is properly disconnected and the terminal state is updated, improving reliability of the serial port handling.
Consider explicitly handling InterruptedException separately to restore the thread's interrupted status:
catch (Exception e) { Activator.log(e); + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } } finally
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunch.java(2 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java(3 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java(10 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (14)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunch.java (2)
69-71:✅ Verification successful
Exception handling removed for serial port resume operation
The try-catch block that previously handled IOExceptions during
serialPort.resume()has been removed. This improves code simplicity but shifts exception handling responsibility to the caller. This change supports the PR objective of ensuring proper port management during upload operations by ensuring port resume operations aren't silently failing.
🏁 Script executed:
#!/bin/bash # Confirm the SerialPortHandler.resume() signature matches this change rg -A 2 "public void resume\(\)" bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.javaLength of output: 220
Verified: Removal of Exception Handling in
serialPort.resume()is Acceptable
- The output confirms that
SerialPortHandler.resume()has the signaturepublic void resume()without any declared exceptions.- Removing the try-catch block in
SerialFlashLaunch.java(lines 69-71) is valid since the method does not propagate exceptions, effectively shifting the exception management responsibility to the caller.- This change aligns with the PR’s objective of ensuring proper port management during upload operations.
54-55:❓ Verification inconclusive
Exception handling removed for serial port pause operation
The try-catch block that previously handled IOExceptions during the
serialPort.pause()call has been removed. This change improves code clarity but shifts exception handling responsibility to the caller. This aligns with the PR objective of ensuring proper port management but ensure that exceptions are properly handled higher in the call stack.
🏁 Script executed:
#!/bin/bash # Check for exception handling higher in the call stack rg -A 2 -B 2 "catch.*IOException" bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launchLength of output: 1452
Attention: Confirm Upstream IOException Handling
The removal of the try-catch block around
serialPort.pause()inSerialFlashLaunch.javastreamlines the code by shifting exception handling responsibility to the caller. Our search confirms that exceptions are caught in higher-level components—noticeable in:
SerialFlashLaunchConfigDelegate.java: Contains a catch forIOExceptionthat logs the exception.ESPFlashUtil.java: Similarly catches and logsIOException.Please ensure these handlers correctly capture exceptions thrown from
serialPort.pause()and manage them appropriately.bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java (5)
30-46: Code formatting standardizedThe opening braces have been moved to the same line as method/class declarations, improving code consistency. This is a stylistic change that enhances readability without affecting functionality.
48-63: Consistent formatting applied to getter methodsMethod formatting has been standardized with opening braces on the same line. This improves code style consistency across the codebase.
65-75: Consistent formatting applied to persistence methodsMethod formatting has been standardized with opening braces on the same line for the load and save methods. This improves code style consistency without affecting functionality.
77-91: Formatting changes to connect method and conditional logicThe formatting has been standardized with opening braces on the same line for both the method declaration and the conditional statement. While this is primarily a stylistic change, it ensures better code consistency.
105-112:✅ Verification successful
Exception handling removed during port disconnection
The try-catch block around
serialPort.close()has been removed, which aligns with similar changes in other files. This improves code clarity and ensures that port closure exceptions are properly propagated rather than being silently logged. This change directly supports the PR objective of ensuring serial ports are properly cleared before upload operations.
🏁 Script executed:
#!/bin/bash # Verify the SerialPortHandler.close() method no longer throws IOException rg "public synchronized void close\(\)" bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.javaLength of output: 208
Confirmed: Exception Handling Removal Works as Intended
The try-catch block around
serialPort.close()has been intentionally removed. Our verification shows thatSerialPortHandler.close()no longer specifies any exception (e.g., nothrows IOException), aligning with the similar changes in other files. This update improves code clarity and ensures that any exceptions during port closure are properly propagated, directly supporting the PR objective of clearing serial ports before upload operations.bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (7)
7-8: Improved thread safety with ConcurrentHashMap and direct referencesThe code has been enhanced in two important ways:
- Changed from HashMap to ConcurrentHashMap for improved thread safety
- Removed WeakReferences, now storing SerialPortHandler instances directly
This change ensures better thread safety when accessing the openPorts map from multiple threads and simplifies the code by removing the need to check for expired WeakReferences. This directly supports the PR objective by providing more reliable port management.
Also applies to: 25-26
140-142: Simplified port tracking with computeIfAbsentThe code now uses
computeIfAbsentfor cleaner map operations. This approach ensures that the list is created only if needed and the operation is atomic, which is important for thread safety. This improvement directly addresses the PR objective by ensuring more reliable port tracking.
144-145: Exception handling responsibility shiftedThe
closemethod signature has been updated to no longer throw IOException. This shifts the responsibility of exception handling to the method implementation rather than propagating it to callers. This change is consistent with the changes in other files and helps ensure that ports are always properly closed even in exception scenarios.
160-163: Improved clean-up of inactive ports with computeIfPresentThe code now uses
computeIfPresentfor cleaner map operations during port removal. If the list becomes empty after removing this handler, the entire map entry is removed. This prevents memory leaks and ensures that the openPorts map stays clean. This improvement addresses the PR objective by ensuring more reliable port tracking and cleanup.
182-183: Exception handling responsibility shifted in pause methodThe
pausemethod signature has been updated to no longer throw IOException. This shifts the responsibility of exception handling to the method implementation rather than propagating it to callers. This change is consistent with the overall approach of simplifying exception handling.
204-205: Exception handling responsibility shifted in resume methodThe
resumemethod signature has been updated to no longer throw IOException. This change is consistent with the modifications to thepauseandclosemethods, creating a coherent approach to exception handling throughout the class. This addresses the PR objective by ensuring that the resume operation completes properly.
40-65:❓ Verification inconclusive
Simplified port handling logic
The
getmethod has been refactored to:
- Remove the
adjustPortNamemethod, eliminating OS-specific port name adjustments- Simplify the synchronization approach by locking only on the specific list rather than the entire map
- Use direct SerialPortHandler references instead of WeakReferences
These changes improve concurrency by using fine-grained locking and simplify the logic by removing the need to dereference WeakReferences. This contributes to more reliable port management as required by the PR objective.
🏁 Script executed:
#!/bin/bash # Check if there were any OS-specific port name handling that might still be needed rg "adjustPortName|COM|/dev/tty" bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/Length of output: 145
Refactored port handling logic verified – please manually confirm absence of any lingering OS-specific dependencies.
The updated
getmethod now:
- Eliminates any OS-specific port name adjustments (the removal of
adjustPortNameis consistent; our search for references toadjustPortName,COM, or/dev/ttyreturned no evidence of residual usage).- Uses fine-grained locking by synchronizing on the individual port list.
- Employs direct
SerialPortHandlerreferences instead of weak references, simplifying the code and improving reliability.Please manually verify that no external code path depends on the previous OS-specific behavior before merging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (1)
109-131: Greatly improved resource management and error handlingThe use of try-with-resources ensures proper cleanup of the input stream, even in exceptional cases. The simplified exception handling with a dedicated finally block guarantees that the serial connector will always be disconnected and resources released properly.
Consider adding a debug log entry at the beginning of this thread to aid in troubleshooting port connection issues:
thread = new Thread(() -> { + Logger.log("Starting serial port handler thread for port: " + portName); try (InputStream targetIn = process.getInputStream()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (10)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (10)
7-7: Good addition of ConcurrentHashMap importThis import supports the change to thread-safe collection implementation for managing port handlers.
25-25: Improved thread safety by switching to ConcurrentHashMapReplacing HashMap with ConcurrentHashMap enhances thread safety without requiring explicit synchronization on the map itself. Also, removing WeakReference simplifies memory management by ensuring port handlers remain in the collection until explicitly removed.
42-46: Simplified retrieval logic with improved null checkingThe code now performs a cleaner null check on the retrieved list of handlers. This simplifies the control flow and makes the code more readable.
48-54: Better synchronization granularitySynchronizing on the list itself rather than the entire map reduces lock contention and improves concurrency. The simplified handler retrieval (without WeakReference unwrapping) also makes the code more maintainable.
69-69: Direct port name assignmentSimplified constructor by directly using the provided port name without unnecessary processing, which improves code clarity.
142-142: Improved thread safety with computeIfAbsentUsing
computeIfAbsentprovides atomic operation for checking and initializing the list if needed. This is more concise and thread-safe than separate check-then-act operations.
145-145: Accurate method signatureRemoving the throws IOException declaration aligns with the actual behavior of the method, which doesn't throw checked exceptions.
161-164: Elegant cleanup with computeIfPresentUsing
computeIfPresentfor removing the handler and cleaning up empty lists is an elegant approach that maintains thread safety. The lambda function handles both removal and map cleanup in a concise way.
183-183: Consistent method signaturesRemoving the throws IOException declarations from pause() and resume() methods creates consistency with their actual behavior. These methods don't throw checked exceptions, so the signatures now accurately reflect their behavior.
Also applies to: 205-205
110-128: Addresses the core PR objective of port availability issuesThe improved cleanup in the finally block (lines 125-130) ensures that the serial port is always properly disconnected, even when exceptions occur. This directly addresses the PR objective of fixing "port is not available" errors by ensuring resources are released properly before attempting to reuse the port.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (1)
205-217: Potential redundancy in resume methodThe code sets
isOpen = trueat line 215, but this seems redundant sinceopen()at line 214 should already set this flag.public void resume() { synchronized (pauseMutex) { if (!isPaused) { return; } isPaused = false; open(); - isOpen = true; pauseMutex.notifyAll(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (13)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (13)
7-7: Good addition of ConcurrentHashMap importThis import is needed for the updated thread-safe implementation of the
openPortsmap.
25-25: Improved thread safety with ConcurrentHashMapThe change from a regular HashMap to ConcurrentHashMap provides better thread safety. Additionally, removing WeakReferences simplifies memory management and prevents potential garbage collection issues, which could have been contributing to the "port is not available" error.
42-46: Simplified null check with early returnThe code now performs an early return if the list is null, making the flow more straightforward and easier to follow.
48-54: Improved synchronization modelThis change correctly moves synchronization from the map to the list level, which is more granular and reduces contention. Direct access to SerialPortHandler objects instead of weak references also simplifies the code.
64-64: Consistent return null placementMoving the return statement outside the synchronized block ensures consistent behavior and cleaner code structure.
69-69: Simplified port name handlingDirect assignment of the port name parameter eliminates unnecessary complexity and potential points of failure.
81-81: Simplified variable declarationMinor readability improvement by declaring the variable on a single line.
109-131: Significantly improved resource management with try-with-resourcesThis is an excellent enhancement that:
- Uses try-with-resources to automatically close the input stream
- Simplifies exception handling
- Adds a crucial finally block that ensures the serial connector is always disconnected and the process is forcibly terminated
This directly addresses the PR objective of clearing the serial port before trying to upload by ensuring proper cleanup.
142-142: More robust port management with computeIfAbsentUsing
computeIfAbsentis a clean, thread-safe way to handle the creation and population of the port list. This approach is more concise and less error-prone than manual list creation and management.
145-145: Simplified method signatureRemoving the
throws IOExceptiondeclaration is appropriate since the method no longer throws this exception directly. This makes the API clearer and more consistent.
154-154: More definitive process terminationUsing
destroyForcibly()instead ofdestroy()ensures a more aggressive termination of the process, which helps prevent lingering processes that could block the serial port.
161-164: Elegant port list cleanup with computeIfPresentThe use of
computeIfPresentis a clean, functional approach to:
- Remove the current handler from the list
- Remove the entire list if it becomes empty
- Do all this in a thread-safe manner
This helps prevent memory leaks and ensures proper cleanup of resources.
183-183: Simplified pause method signatureRemoving the
throws IOExceptiondeclaration is appropriate since the method no longer throws this exception, making the API more consistent.
There was a problem hiding this comment.
LGTM!
@AndriiFilippov kindly test the openocd gdb debugging with this PR as well.
There was a problem hiding this comment.
Hi @sigmaaa Thanks for the PR! Since this PR includes multiple changes—code cleanup, refactoring, and WebSocket closure—could you keep only the WebSocket-related closing changes in this PR to avoid impacting other areas? The refactoring changes you’ve made could be moved to a separate PR. Also, I looked at the code, but I’m not entirely sure what specifically helped fix the issue. Keeping the changes isolated and testing them separately would be helpful.
Description
clean-up the code and use try-with-resources for better resource management. Also, forcing closing terminal before re-opening it to avoid "port is not available" issue
Fixes # (IEP-1398)
Type of change
Please delete options that are not relevant.
How has this been tested?
Open serial monitor with different conditions.
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Refactor
Style