IEP-1509 GH #1200: Terminal monitor still cannot build, flash or app flash #1281
Conversation
WalkthroughA boolean encryption option was added and propagated through settings, UI, launcher, connector, port/monitor handlers, and IDFMonitor; IDFMonitor now appends the CLI flag "--encrypted" when the option is enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SerialSettingsPage
participant SerialSettings
participant SerialConnector
participant SerialPortHandler
participant SerialMonitorHandler
participant IDFMonitor
User->>SerialSettingsPage: toggle encryption checkbox
SerialSettingsPage->>SerialSettings: setEncryptionOption(value) / save()
User->>SerialConnector: open connection
SerialConnector->>SerialSettings: getEncryptionOption()
SerialConnector->>SerialPortHandler: open(...)
SerialPortHandler->>SerialMonitorHandler: new SerialMonitorHandler(..., encryptionOption, ...)
SerialMonitorHandler->>IDFMonitor: new IDFMonitor(..., encryptionOption, ...)
IDFMonitor->>IDFMonitor: if encryptionOption then add "--encrypted"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
🧹 Nitpick comments (2)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialSettings.java (2)
33-33: Consider renaming for consistency.The field name
extraMonitorOptsis inconsistent with the getter method namegetExtraMonitorOptions()and the purpose (encryption). Consider renaming toencryptionOptionfor clarity and consistency.-private boolean extraMonitorOpts; +private boolean encryptionOption;
72-75: Method naming inconsistency.The getter method name
getExtraMonitorOptions()and setter method namesetExtraOptions()are inconsistent with each other and with the field nameextraMonitorOpts. Consider using consistent naming throughout.-public boolean getExtraMonitorOptions() +public boolean getEncryptionOption() { - return extraMonitorOpts; + return encryptionOption; } -public void setExtraOptions(boolean extraOptions) +public void setEncryptionOption(boolean encryptionOption) { - this.extraMonitorOpts = extraOptions; + this.encryptionOption = encryptionOption; }Also applies to: 105-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java(2 hunks)bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/handlers/SerialMonitorHandler.java(1 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(2 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialSettings.java(1 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialConfigPanel.java(2 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java(6 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java(4 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/nls/Messages.java(1 hunks)bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/nls/Messages.properties(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (1)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/server/SocketServerMessageHandler.java (1)
SocketServerMessageHandler(37-41)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/nls/Messages.java (8)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/Messages.java (1)
Messages(20-36)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/Messages.java (2)
Messages(20-73)Messages(70-72)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/nvs/dialog/Messages.java (1)
Messages(10-54)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/partitiontable/handlers/Messages.java (1)
Messages(5-19)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.java (1)
Messages(5-41)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/installcomponents/Messages.java (1)
Messages(16-32)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/Messages.java (1)
Messages(5-20)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/update/Messages.java (1)
Messages(9-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (21)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/nls/Messages.properties (1)
14-14: LGTM! Clean internationalization property addition.The new property key follows proper Eclipse NLS naming conventions and provides a clear, user-friendly label for the encryption option UI element.
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/nls/Messages.java (1)
37-37: LGTM! Standard Eclipse NLS implementation.The new externalized string field follows the established pattern and properly corresponds to the property key added in Messages.properties.
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java (1)
33-40: LGTM! Good code style improvements.The formatting changes adding braces to single-line methods and conditionals improve code consistency and maintainability.
Also applies to: 43-46, 49-50, 55-58, 72-78, 84-85, 94-97, 102-105
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java (2)
31-32: LGTM! Consistent formatting improvements.The formatting changes adding braces improve code consistency and readability throughout the class.
Also applies to: 45-48, 51-54, 56-59, 67-70, 74-77, 80-81, 88-93, 107-121
39-39: LGTM! Clean encryption option integration.The encryption option is properly integrated into the connector:
- Added as a protected field for accessibility by related components
- Correctly initialized from settings during connection setup
- Follows the established pattern of other configuration fields
Also applies to: 98-98
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java (2)
38-38: LGTM! Method simplification.The adjustPortName method is simplified to directly return the port name. This appears to remove unnecessary conditional logic, making the code cleaner.
115-116: LGTM! Proper encryption option propagation.The SerialMonitorHandler constructor call is correctly updated to pass the encryption option from the serial connector. This ensures the encryption setting flows through to the monitor handler as intended.
bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/core/IDFMonitor.java (3)
43-43: LGTM: Clean field addition.The new
encryptionOptionboolean field is properly declared and follows the existing field naming conventions.
45-46: LGTM: Constructor properly updated.The constructor signature is correctly updated to include the
encryptionOptionparameter, and the field assignment is properly implemented.Also applies to: 52-52
77-80: LGTM: Conditional encryption argument handling.The logic correctly adds the
--encryptedcommand line argument only when the encryption option is enabled, which is the expected behavior.bundles/com.espressif.idf.serial.monitor/src/com/espressif/idf/serial/monitor/handlers/SerialMonitorHandler.java (3)
20-20: LGTM: Field declaration is correct.The
encryptionOptionboolean field is properly declared following the existing pattern.
22-23: LGTM: Constructor properly handles new parameter.The constructor signature is correctly updated and the field assignment is implemented properly.
Also applies to: 29-29
37-38: LGTM: IDFMonitor constructor call updated correctly.The constructor call properly passes the
encryptionOptionparameter to match the updatedIDFMonitorconstructor signature.bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialSettings.java (2)
28-28: LGTM: Constant declaration follows conventions.The new
ENCRYPTION_ATTRconstant is properly declared and follows the existing naming pattern.
43-43: LGTM: Settings persistence implemented correctly.The load and save methods properly handle the boolean conversion using
Boolean.parseBoolean()andBoolean.toString()with appropriate default values.Also applies to: 54-54
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialConfigPanel.java (1)
70-70: LGTM: Data extraction implemented correctly.The encryption option is properly extracted from settings and stored in the data map using the correct attribute key.
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialSettingsPage.java (5)
64-65: LGTM: UI fields properly declared.The new checkbox and boolean fields are properly declared following the existing pattern.
77-77: LGTM: Proper initialization from dialog settings.The encryption option is correctly initialized from dialog settings with proper boolean parsing.
159-161: LGTM: UI checkbox creation is well implemented.The encryption checkbox is properly created with appropriate layout data and uses the externalized message for the label.
204-204: LGTM: Settings loading handles checkbox state.The loadSettings method properly sets the checkbox selection based on the stored encryption option.
213-213: LGTM: Settings persistence is comprehensive.The saveSettings method correctly updates both the settings object and dialog settings with the checkbox state, ensuring proper persistence across different layers.
Also applies to: 218-218
...ector.serial/src/com/espressif/idf/terminal/connector/serial/controls/SerialConfigPanel.java
Outdated
Show resolved
Hide resolved
....serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java
Outdated
Show resolved
Hide resolved
|
Hi @kolipakakondal, @alirana01 I added a simple option, but it ended up requiring changes to 10 files. I'm planning to refactor the related code in a separate PR. |
|
Can we please verify if the failure on tests is intermittent or related to this PR? |
|
@sigmaaa hi ! Tested under: the |
Description
Added an encryption option to the serial monitor page:
Fixes # (IEP-1509)
Type of change
Please delete options that are not relevant.
How has this been tested?
Start the serial monitor with or without the "Enable Encryption" option → press Ctrl+T Ctrl+A to run flash from the monitor → idf.py encrypted-app-flash or idf.py app-flash will be executed.
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Style
Documentation