IEP-1613 Encrypted Flash: regular flash command is not separated from encrypted-flash command #1301
Conversation
WalkthroughAdds a dynamic variable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ESPFlashUtil
participant VariablesPlugin
participant UartVariableResolver
participant IDFUtil
participant LaunchBarProj as LaunchBar/ProjectConfig
User->>ESPFlashUtil: getParseableEspFlashCommand(serialPort)
ESPFlashUtil->>VariablesPlugin: generateVariableExpression("flash_command")
VariablesPlugin->>UartVariableResolver: resolve(FLASH_COMMAND)
UartVariableResolver->>IDFUtil: isFlashEncrypted()
IDFUtil->>LaunchBarProj: query active launch config / project
LaunchBarProj-->>IDFUtil: encryption=true/false
alt Encryption enabled
UartVariableResolver-->>VariablesPlugin: "encrypted-flash"
else Encryption disabled
UartVariableResolver-->>VariablesPlugin: "flash"
end
VariablesPlugin-->>ESPFlashUtil: resolved command
ESPFlashUtil-->>User: parseable flash command using resolved value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
bundles/com.espressif.idf.core/plugin.xml (2)
344-348: Externalize description string and avoid drift-prone examples.Existing dynamic variables use localized descriptions (e.g., "%serial_port"). Please align and remove hardcoded examples that can fall out of sync with actual targets.
Apply this diff and add a
flash_commandentry to the appropriateplugin.properties:- <variable - description="flash_command: Specifies which idf.py flash command should be executed, depending on whether flash encryption is enabled (e.g., "flash" or "encrypted-app-flash")." - name="flash_command" - resolver="com.espressif.idf.core.variable.UartVariableResolver"> + <variable + description="%flash_command" + name="flash_command" + resolver="com.espressif.idf.core.variable.UartVariableResolver">plugin.properties (add):
flash_command=Selects the idf.py flash target based on flash encryption state.
344-348: Resolver coupling nit: consider a non-UART resolver.
flash_commandisn’t UART-specific. Long-term, consider a generic resolver (e.g.,IDFVariableResolver) to avoid overloadingUartVariableResolver.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
bundles/com.espressif.idf.core/plugin.xml(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFConstants.java(6 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/variable/UartDynamicVariable.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/variable/UartVariableResolver.java(2 hunks)bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/util/ESPFlashUtil.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePlugin.java (1)
IDFCorePlugin(20-77)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/Messages.java (1)
Messages(13-63)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/variable/UartVariableResolver.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(59-1018)
⏰ 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). (1)
- GitHub Check: build_macos
🔇 Additional comments (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/variable/UartDynamicVariable.java (1)
5-6: Enum addition LGTM.Name/value pair matches plugin variable naming; no issues spotted.
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/util/ESPFlashUtil.java (1)
79-81: Use encryption-aware flash command in getEspFlashCommand- commands.add(IDFConstants.FLASH_CMD); + commands.add(IDFUtil.isFlashEncrypted() ? IDFConstants.FLASH_ENCRYPTED_CMD : IDFConstants.FLASH_CMD);Likely an incorrect or invalid review comment.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/variable/UartVariableResolver.java (1)
39-40: Switch addition LGTM; resolves to encrypted vs regular command. Once the constant value is corrected, this mapping will be correct.
| <variable | ||
| description="flash_command: Specifies which idf.py flash command should be executed, depending on whether flash encryption is enabled (e.g., "flash" or "encrypted-app-flash")." | ||
| name="flash_command" | ||
| resolver="com.espressif.idf.core.variable.UartVariableResolver"> | ||
| </variable> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify command naming consistency ("encrypted-flash" vs "encrypted-app-flash").
The description mentions "encrypted-app-flash" while the new constant reportedly uses "encrypted-flash". Ensure the emitted value exactly matches supported idf.py targets across ESP-IDF versions.
Run:
🏁 Script executed:
#!/bin/bash
# Confirm resolver mapping and constants are consistent and valid.
rg -nC2 -g '!**/node_modules/**' -P '\bFLASH_ENCRYPTED_CMD\b|encrypted-.*flash\b|FLASH_CMD\b' bundles
echo
# Check UartVariableResolver maps FLASH_COMMAND correctly
rg -nC3 -P 'FLASH_COMMAND|resolveForDynamicEnum|flash_command' bundles
echo
# Ensure localization key exists
fd -a plugin.properties | xargs -r rg -n '^flash_command='Length of output: 14698
Align plugin.xml description with actual command name
In plugin.xml (lines 345–348), replace “encrypted-app-flash” with “encrypted-flash” so the description matches IDFConstants.FLASH_ENCRYPTED_CMD ("encrypted-flash") and the idf.py CLI targets.
🤖 Prompt for AI Agents
In bundles/com.espressif.idf.core/plugin.xml around lines 344 to 348, the
description currently references the wrong encrypted flash command name
("encrypted-app-flash"); update that string to "encrypted-flash" so the
human-readable description matches IDFConstants.FLASH_ENCRYPTED_CMD and the
idf.py CLI target names.
| String FLASH_CMD = "flash"; //$NON-NLS-1$ | ||
|
|
||
| String FLASH_ENCRYPTED_CMD = "encrypted-flash"; //$NON-NLS-1$ | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Encrypted command name likely incorrect; align with idf.py target "encrypted-app-flash".
idf.py exposes "encrypted-app" and "encrypted-app-flash" targets; "encrypted-flash" is not a known target. Using the wrong subcommand will fail at runtime. Recommend switching the value to "encrypted-app-flash" (keep the constant name if you want to minimize ripples), and audit call sites.
Apply:
- String FLASH_ENCRYPTED_CMD = "encrypted-flash"; //$NON-NLS-1$
+ String FLASH_ENCRYPTED_CMD = "encrypted-app-flash"; //$NON-NLS-1$To double-check usages and docs across the repo:
🏁 Script executed:
#!/bin/bash
# Look for potential mismatches
rg -nP -C2 '(encrypted[-_]app[-_]flash|encrypted[-_]flash|FLASH_ENCRYPTED_CMD|flash_command)'Length of output: 3520
Replace incorrect “encrypted-flash” constant and update UI tooltip
--- a/bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFConstants.java
+++ b/bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFConstants.java
@@ -88,7 +88,7 @@
String FLASH_CMD = "flash"; //$NON-NLS-1$
- String FLASH_ENCRYPTED_CMD = "encrypted-flash"; //$NON-NLS-1$
+ String FLASH_ENCRYPTED_CMD = "encrypted-app-flash"; //$NON-NLS-1$Also update the Messages.properties tooltip so it reflects the two valid targets (“flash” and “encrypted-app-flash”):
--- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/nls/Messages.properties
+++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/nls/Messages.properties
@@ -15,1 +15,1 @@
-SerialSettingsPage_EncryptionOptionTooltip1=Enable Flash Encryption. Ctrl+F and Ctrl+A will trigger the encrypted-flash and encrypted-app-flash commands.
+SerialSettingsPage_EncryptionOptionTooltip1=Enable Flash Encryption. Ctrl+F and Ctrl+A will trigger the flash and encrypted-app-flash commands.Audit any other references to “encrypted-flash” to ensure consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String FLASH_CMD = "flash"; //$NON-NLS-1$ | |
| String FLASH_ENCRYPTED_CMD = "encrypted-flash"; //$NON-NLS-1$ | |
| String FLASH_CMD = "flash"; //$NON-NLS-1$ | |
| String FLASH_ENCRYPTED_CMD = "encrypted-app-flash"; //$NON-NLS-1$ |
| String FLASH_CMD = "flash"; //$NON-NLS-1$ | |
| String FLASH_ENCRYPTED_CMD = "encrypted-flash"; //$NON-NLS-1$ | |
| SerialSettingsPage_EncryptionOptionTooltip1=Enable Flash Encryption. Ctrl+F and Ctrl+A will trigger the flash and encrypted-app-flash commands. |
🤖 Prompt for AI Agents
In bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFConstants.java
around lines 88 to 91 the FLASH_ENCRYPTED_CMD constant is set to the incorrect
value "encrypted-flash"; change it to the correct target "encrypted-app-flash".
After updating the constant, update the corresponding tooltip text in
Messages.properties to list the two valid targets "flash" and
"encrypted-app-flash", and search the codebase for any other occurrences of
"encrypted-flash" to replace them with "encrypted-app-flash" or adjust
references to use the new constant to keep everything consistent.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
729-738: Encrypted-flash detection will misread "1"/"y"/"yes" and similar values — use robust boolean parsing.Boolean.parseBoolean("1"/"y") returns false, which will wrongly select the regular "flash" command. Also guard SDKConfigJsonReader exceptions and optionally try an alternate key if present.
Apply:
public static boolean isFlashEncrypted() { IProject project = getProjectFromActiveLaunchConfig(); if (project == null) { Logger.log(Messages.IDFUtil_CantFindProjectMsg); return false; } - return Boolean.parseBoolean(new SDKConfigJsonReader(project).getValue("SECURE_FLASH_ENC_ENABLED")); //$NON-NLS-1$ + try { + String v = new SDKConfigJsonReader(project).getValue("SECURE_FLASH_ENC_ENABLED"); //$NON-NLS-1$ + if (v == null || v.isBlank()) { + // Optional fallback used in some setups + String alt = new SDKConfigJsonReader(project).getValue("SECURE_FLASH_ENCRYPTION_MODE"); //$NON-NLS-1$ + if (alt != null) v = alt; + } + return parseSdkconfigBoolean(v); + } catch (Exception e) { + Logger.log(e); + return false; + } }Add this helper in the class (outside the shown hunk):
private static boolean parseSdkconfigBoolean(String v) { if (v == null) return false; String s = v.trim().toLowerCase(); return s.equals("true") || s.equals("1") || s.equals("y") || s.equals("yes") || s.equals("on") || s.equals("enabled"); }Confirm the actual key names emitted by SDKConfigJsonReader in your environment:
#!/bin/bash # Where else do we reference these keys? rg -nP 'SECURE_FLASH_ENC_ENABLED|SECURE_FLASH_ENCRYPTION_MODE' -g '!**/target/**' -S
🧹 Nitpick comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
754-771: Prefer resolving project via CoreBuildLaunchConfigDelegate and add accessibility guard.This captures non-resource-mapped configs too and avoids closed/inaccessible projects. Also log project name instead of toString().
Apply:
- ILaunchConfiguration activeConfig = launchBarManager.getActiveLaunchConfiguration(); + ILaunchConfiguration activeConfig = launchBarManager.getActiveLaunchConfiguration(); if (activeConfig == null) { Logger.log("No active launch configuration."); //$NON-NLS-1$ return null; } - IResource[] resources = activeConfig.getMappedResources(); + // Try CDT helper first + IProject p = CoreBuildLaunchConfigDelegate.getProject(activeConfig); + if (p != null) { + if (!p.isAccessible()) { + Logger.log("Active project is not accessible: " + p.getName()); //$NON-NLS-1$ + return null; + } + Logger.log("Active project: " + p.getName()); //$NON-NLS-1$ + return p; + } + + IResource[] resources = activeConfig.getMappedResources(); if (resources == null || resources.length == 0) { Logger.log("No mapped resources in active launch configuration."); //$NON-NLS-1$ return null; } - IProject project = resources[0].getProject(); - Logger.log("Active project: " + project); //$NON-NLS-1$ + IProject project = resources[0].getProject(); + if (project == null || !project.isAccessible()) { + Logger.log("Mapped project is not accessible."); //$NON-NLS-1$ + return null; + } + Logger.log("Active project: " + project.getName()); //$NON-NLS-1$ return project;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/Messages.java (1)
Messages(13-63)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePlugin.java (1)
IDFCorePlugin(20-77)
⏰ 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_macos
- GitHub Check: build
🔇 Additional comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
743-777: Good centralization and null-safety in getProjectFromActiveLaunchConfig().
The defensive checks and logging are solid.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java (2)
228-229: Derive encryption from this launch’s project, not the “active” config.Calling IDFUtil.isFlashEncrypted() (which reads from the active LaunchBar config) risks a race: openSerialMonitor runs after a delayed CompletableFuture and the active config may have changed. Use the project from this configuration to avoid mismatches.
Apply this diff (paired with an overload in IDFUtil):
- map.put(SerialSettings.ENCRYPTION_ATTR, IDFUtil.isFlashEncrypted()); + map.put(SerialSettings.ENCRYPTION_ATTR, IDFUtil.isFlashEncrypted(getProject(configuration)));And add an overload in IDFUtil.java:
// IDFUtil.java public static boolean isFlashEncrypted(IProject project) { if (project == null) return false; // Reuse the same logic/source as the current no-arg version, but scoped to `project` // Example using SDKConfigJsonReader: try { String v = new SDKConfigJsonReader(project).getConfig("SECURE_FLASH_ENC_ENABLED"); return "1".equals(v) || "y".equalsIgnoreCase(v) || Boolean.parseBoolean(v); } catch (Exception e) { Logger.log(e); return false; } }
223-226: Use the defined constant and add null-safety for mapped resources.Avoid the raw key literal and protect against missing/empty mapped resources to prevent NPE/IOOBE.
- map.put("idf.monitor.project", configuration.getMappedResources()[0].getName()); //$NON-NLS-1$ + org.eclipse.core.resources.IResource[] resources = configuration.getMappedResources(); + if (resources != null && resources.length > 0 && resources[0] != null) { + map.put(SerialSettings.SELECTED_PROJECT_ATTR, resources[0].getProject().getName()); + } else { + IProject p = getProject(configuration); + if (p != null) { + map.put(SerialSettings.SELECTED_PROJECT_ATTR, p.getName()); + } + }Add import (outside this hunk):
import org.eclipse.core.resources.IResource;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java (2)
bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialSettings.java (1)
SerialSettings(22-110)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(59-1022)
⏰ 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 (1)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java (1)
221-229: No action required: ENCRYPTION_ATTR is correctly treated as a boolean. SerialLauncherDelegate.execute castsproperties.get(SerialSettings.ENCRYPTION_ATTR)toBooleanand uses it to set the encryption option, confirming it expects a boolean value.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
720-727: Document nullable return and downgrade log noise for missing projectgetCurrentTarget() can return null; please document this and reduce info-log noise.
Apply:
- Logger.log(Messages.IDFUtil_CantFindProjectMsg); + Logger.logTrace(Messages.IDFUtil_CantFindProjectMsg);Additionally (outside this hunk), add Javadoc to make nullability explicit:
/** * @return IDF_TARGET for the active project, or null if no active project/config or value missing. */Ensure downstream call sites handle null safely (guards or Optional). If you want, I can scan and patch obvious offenders.
🧹 Nitpick comments (4)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/messages.properties (1)
39-41: Trim trailing space and keep style consistent.Checkbox label has a trailing space; keep strings tidy.
Apply:
-CMakeMainTab2_FlashEncryptionCheckbox=Enable Flash Encryption +CMakeMainTab2_FlashEncryptionCheckbox=Enable Flash Encryptionbundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (1)
84-84: Docs URL is chip-specific; prefer target-agnostic or derive from selected target.Linking to “…/en/stable/esp32/…” may be wrong for non-ESP32 targets. Either switch to a generic page or build the URL from the active IDF target.
Minimal change:
- private static final String DOCS_ESPRESSIF_FLASH_ENCRYPTION_HTML = "https://docs.espressif.com/projects/esp-idf/en/stable/esp32/security/flash-encryption.html"; //$NON-NLS-1$ + private static final String DOCS_ESPRESSIF_FLASH_ENCRYPTION_HTML = "https://docs.espressif.com/projects/esp-idf/en/latest/security/flash-encryption.html"; //$NON-NLS-1$Optionally, derive per-target from the active launch configuration (can help add later if desired).
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (2)
750-776: Prefer CoreBuildLaunchConfigDelegate.getProject(configuration) over mapped resourcesUsing the delegate avoids brittle indexing into mapped resources and keeps this aligned with other code paths.
Apply:
- ILaunchConfiguration activeConfig = getActiveLaunchConfiguration(); + ILaunchConfiguration activeConfig = getActiveLaunchConfiguration(); if (activeConfig == null) { return null; } - - IResource[] resources = activeConfig.getMappedResources(); - if (resources == null || resources.length == 0) - { - Logger.log("No mapped resources in active launch configuration."); //$NON-NLS-1$ - return null; - } - - IProject project = resources[0].getProject(); - Logger.log("Active project: " + project); //$NON-NLS-1$ - return project; + IProject project = CoreBuildLaunchConfigDelegate.getProject(activeConfig); + if (project == null) { + Logger.log("No project found in active launch configuration."); //$NON-NLS-1$ + return null; + } + Logger.logTrace("Active project: " + project); //$NON-NLS-1$ + return project;
778-794: Reduce log verbosity for expected states (service/config missing)These are common/benign in many workspace states; prefer trace to avoid log noise.
Apply:
- if (launchBarManager == null) + if (launchBarManager == null) { - Logger.log("LaunchBarManager service not found."); //$NON-NLS-1$ + Logger.logTrace("LaunchBarManager service not found."); //$NON-NLS-1$ return null; } @@ - if (activeConfig == null) + if (activeConfig == null) { - Logger.log("No active launch configuration."); //$NON-NLS-1$ + Logger.logTrace("No active launch configuration."); //$NON-NLS-1$ return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
bundles/com.espressif.idf.core/OSGI-INF/l10n/bundle.properties(1 hunks)bundles/com.espressif.idf.core/plugin.xml(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(1 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java(6 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/Messages.java(1 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/messages.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.core/plugin.xml
🧰 Additional context used
🧬 Code graph analysis (2)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (3)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/Messages.java (1)
Messages(20-76)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
IDFLaunchConstants(3-24)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/Messages.java (1)
Messages(13-63)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
IDFLaunchConstants(3-24)bundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePlugin.java (1)
IDFCorePlugin(20-77)
⏰ 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 (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
22-22: Constant addition looks good.Name and namespace are consistent with existing keys.
bundles/com.espressif.idf.core/OSGI-INF/l10n/bundle.properties (1)
10-10: Rename localization key in bundle.properties
Change line 10 in bundles/com.espressif.idf.core/OSGI-INF/l10n/bundle.properties from
flash_commmand = …
to
flash_command = flash_command specifies which idf.py flash command should be executed depending on whether flash encryption is enabled (e.g., "flash" or "encrypted-flash").
Plugin.xml and UartDynamicVariable.java already reference “flash_command,” and anrgsearch confirms no other “flash_commmand” remains.bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/Messages.java (1)
56-58: LGTM: keys declared for new UI strings.Matches entries added in messages.properties.
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (3)
237-239: Good placement, but confirm scope.Flash encryption UI is added only under the UART section. If DFU flashing should also consider encryption, mirror the control or explicitly state UART-only in the UI.
566-567: Persisting encryption flag to launch config: OK.Attribute name matches the new constant; no issues spotted.
580-586: Initialize-from path wires the new group: OK.Ensures UI reflects stored config on open.
| public static boolean isFlashEncrypted() | ||
| { | ||
| ILaunchConfiguration configuration; | ||
| try | ||
| { | ||
| configuration = getActiveLaunchConfiguration(); | ||
|
|
||
| return configuration != null | ||
| && configuration.getAttribute(IDFLaunchConstants.FLASH_ENCRYPTION_ENABLED, false); | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make isFlashEncrypted resilient: fall back to sdkconfig (SECURE_FLASH_ENC_ENABLED) when launch attribute is absent
Current logic relies solely on the launch attribute; if it’s unset, encryption may be incorrectly treated as disabled. Add a fallback to sdkconfig with tolerant boolean parsing.
Apply:
public static boolean isFlashEncrypted()
{
- ILaunchConfiguration configuration;
- try
- {
- configuration = getActiveLaunchConfiguration();
-
- return configuration != null
- && configuration.getAttribute(IDFLaunchConstants.FLASH_ENCRYPTION_ENABLED, false);
- }
- catch (CoreException e)
- {
- Logger.log(e);
- }
-
- return false;
+ try {
+ ILaunchConfiguration configuration = getActiveLaunchConfiguration();
+ boolean enabled = configuration != null
+ && configuration.getAttribute(IDFLaunchConstants.FLASH_ENCRYPTION_ENABLED, false);
+ if (enabled) {
+ return true;
+ }
+ // Fallback to sdkconfig if the launch attribute is absent/false
+ IProject project = getProjectFromActiveLaunchConfig();
+ if (project == null) {
+ return false;
+ }
+ String v = new SDKConfigJsonReader(project).getValue("SECURE_FLASH_ENC_ENABLED"); //$NON-NLS-1$
+ if (v == null) {
+ return false;
+ }
+ v = v.trim();
+ return "true".equalsIgnoreCase(v) || "1".equals(v) || "y".equalsIgnoreCase(v) || "yes".equalsIgnoreCase(v);
+ } catch (CoreException e) {
+ Logger.log(e);
+ return false;
+ }
}🏁 Script executed:
#!/bin/bash
# Verify that the launch attribute is actually set somewhere.
rg -nP -C2 'FLASH_ENCRYPTION_ENABLED'
# Heuristic: look for setAttribute calls touching it
rg -nP -C2 'setAttribute\(\s*IDFLaunchConstants\.FLASH_ENCRYPTION_ENABLED'Length of output: 3862
🏁 Script executed:
#!/bin/bash
# Search for SDKConfigJsonReader class declaration
rg -n "class SDKConfigJsonReader"
# Search for any references to SDKConfigJsonReader
rg -n "SDKConfigJsonReader"
# Search for getProjectFromActiveLaunchConfig method
rg -n "getProjectFromActiveLaunchConfig"
# Verify getActiveLaunchConfiguration definition in IDFUtil.java
rg -n -C2 "getActiveLaunchConfiguration" bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.javaLength of output: 4611
Make isFlashEncrypted resilient: fallback to sdkconfig when launch attribute is absent
Add a secondary lookup in isFlashEncrypted() to read SECURE_FLASH_ENC_ENABLED via SDKConfigJsonReader(project) whenever the FLASH_ENCRYPTION_ENABLED launch attribute is false or unset. Ensure getProjectFromActiveLaunchConfig() handles null, and update IDFUtilTest to mock SDKConfigJsonReader for this new path.
🤖 Prompt for AI Agents
In bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
around lines 729-745, enhance isFlashEncrypted() so that if the
FLASH_ENCRYPTION_ENABLED launch attribute is false or absent it falls back to
reading SECURE_FLASH_ENC_ENABLED from the project's sdkconfig via new
SDKConfigJsonReader(project).getBoolean("SECURE_FLASH_ENC_ENABLED") (guarding
nulls); ensure getProjectFromActiveLaunchConfig() can safely handle a null
configuration and return null without throwing, and update IDFUtilTest to add a
mock SDKConfigJsonReader that simulates the sdkconfig lookup path (include tests
for launch attr true, launch attr false with sdkconfig true, and null project
case).
| private void createFlashEncryptionCheckbox(Composite parent) | ||
| { | ||
| Group flashGroup = new Group(parent, SWT.NONE); | ||
| flashGroup.setText(Messages.CMakeMainTab2_FlashEncryptionGroup); | ||
| flashGroup.setLayout(new GridLayout(1, false)); | ||
| flashGroup.setLayoutData(new GridData(SWT.FILL, SWT.TOP, true, false)); | ||
|
|
||
| flashEncryptionCheckbox = new Button(flashGroup, SWT.CHECK); | ||
| flashEncryptionCheckbox.setText(Messages.CMakeMainTab2_FlashEncryptionCheckbox); | ||
| flashEncryptionCheckbox.setLayoutData(new GridData(SWT.FILL, SWT.CENTER, true, true)); | ||
|
|
||
| Link flashEncryptionNote = new Link(flashGroup, SWT.WRAP); | ||
| flashEncryptionNote.setText(Messages.CMakeMainTab2_FlashEncryptionNote); | ||
|
|
||
| flashEncryptionNote.addListener(SWT.Selection, e -> Program.launch(DOCS_ESPRESSIF_FLASH_ENCRYPTION_HTML)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing selection listener and visibility logic for the note.
The note is always visible; there’s no linkage to the checkbox state. Add a listener to show/hide the note and relayout.
Apply:
- Link flashEncryptionNote = new Link(flashGroup, SWT.WRAP);
- flashEncryptionNote.setText(Messages.CMakeMainTab2_FlashEncryptionNote);
-
- flashEncryptionNote.addListener(SWT.Selection, e -> Program.launch(DOCS_ESPRESSIF_FLASH_ENCRYPTION_HTML));
+ flashEncryptionNote = new Link(flashGroup, SWT.WRAP);
+ flashEncryptionNote.setText(Messages.CMakeMainTab2_FlashEncryptionNote);
+ flashEncryptionNote.addListener(SWT.Selection, e -> Program.launch(DOCS_ESPRESSIF_FLASH_ENCRYPTION_HTML));
+ // Toggle note visibility with checkbox
+ flashEncryptionCheckbox.addListener(SWT.Selection, e -> refreshFlashEncryptionNoteVisibility(flashGroup));
+ refreshFlashEncryptionNoteVisibility(flashGroup);And declare a field:
- private Button flashEncryptionCheckbox;
+ private Button flashEncryptionCheckbox;
+ private Link flashEncryptionNote;Add helper (outside changed ranges, shown for completeness):
private void refreshFlashEncryptionNoteVisibility(Composite container) {
boolean visible = flashEncryptionCheckbox.getSelection();
flashEncryptionNote.setVisible(visible);
GridData gd = (GridData) flashEncryptionNote.getLayoutData();
if (gd == null) {
gd = new GridData(SWT.FILL, SWT.TOP, true, false);
flashEncryptionNote.setLayoutData(gd);
}
gd.exclude = !visible;
container.layout(true, true);
if (mainComposite != null) {
mainComposite.layout(true, true);
}
}🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java
around lines 241-257, the flash encryption note Link is always visible and not
tied to the checkbox state; add a private field for flashEncryptionNote, add a
selection listener on flashEncryptionCheckbox that calls a helper to show/hide
the note and relayout the parent (use the flashGroup or the composite container
you used to create the controls), ensure the initial visibility is set after
creation, and implement the provided
refreshFlashEncryptionNoteVisibility(Composite container) helper (outside these
lines) which toggles Link.setVisible, sets/excludes its GridData, and calls
container.layout(true,true) and mainComposite.layout(true,true) if non-null.
| private void updateFlashEncryptionGroup(ILaunchConfiguration configuration) | ||
| { | ||
| boolean isFlashEncryptionEnabled = false; | ||
| try | ||
| { | ||
| isFlashEncryptionEnabled = configuration.getAttribute(IDFLaunchConstants.FLASH_ENCRYPTION_ENABLED, false); | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| flashEncryptionCheckbox.setSelection(isFlashEncryptionEnabled); | ||
|
|
||
| // Update the note visibility | ||
| flashEncryptionCheckbox.notifyListeners(SWT.Selection, null); | ||
| } | ||
|
|
There was a problem hiding this comment.
notifyListeners called with null Event will throw; also no listener to handle it.
flashEncryptionCheckbox.notifyListeners(SWT.Selection, null) causes SWT.ERROR_NULL_ARGUMENT at runtime, and there’s no selection listener anyway.
Apply:
- // Update the note visibility
- flashEncryptionCheckbox.notifyListeners(SWT.Selection, null);
+ // Update the note visibility explicitly
+ refreshFlashEncryptionNoteVisibility(flashEncryptionCheckbox.getParent());Committable suggestion skipped: line range outside the PR's diff.
sigmaaa
changed the title
|
Updated the flash documantation |
|
@sigmaaa hi! Tested under: based on checkbox selection - the flash argument changes in LGTM 👍 @alirana01 @kolipakakondal please, review |
Description
Currently implemented:
Added a dynamic variable ${flash_command} that depends on the SECURE_FLASH_ENC_ENABLED sdkconfig flag.
During flashing, the variable is resolved by checking the SECURE_FLASH_ENC_ENABLED flag. If flash encryption is enabled, ${flash_command} is replaced with encrypted-flash.
If the Open serial monitor after flash option is selected and flash encryption is enabled, the monitor is opened with -E, which allows rerunning the encrypted flash from the monitor.
Fixes # (IEP-1613)
Type of change
Please delete options that are not relevant.
How has this been tested?
Change the SECURE_FLASH_ENC_ENABLED in the sdkconfig and flash -> the flash should be normal/encrypted based on the flag
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Documentation