IEP-1669 Automatically Run IDF Activation Script on Terminal Startup #1388
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new IDF terminal process connector that emits ESP‑IDF activation script commands on connect, refactors the console launcher to simplify terminal creation, introduces selectable/persistent terminal color themes and supporting classes, updates plugin manifest to require Gson, and adjusts a terminal test's encoding detection. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Terminal as Eclipse Terminal\n(ITerminalControl)
participant Connector as IDFTerminalProcessConnector
participant Config as EIM JSON\n(eim_idf.json)
participant Out as Terminal Output\n(OutputStream)
User->>Terminal: open terminal
Terminal->>Connector: connect(control)
activate Connector
Connector->>Config: read/parse JSON (getActivationScriptPath)
Config-->>Connector: script path or empty
alt script path found
Connector->>Connector: buildActivationCommand(path)
Connector->>Out: sendCommand(activation command)
Out-->>User: command executed in shell
else no script
Connector->>Out: sendCommand(error message)
Out-->>User: show error line
end
deactivate Connector
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In
`@bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java`:
- Around line 42-45: The current else branch sends a shell comment string ("#
Error: ...") which is invisible with local echo disabled; change the else lambda
in the getActivationScriptPath().ifPresentOrElse call so it sends a visible
error string to process.getOutputStream() instead of a comment. Use a
platform-appropriate command: send "echo 'Error: ESP-IDF activation script is
missing.'" for POSIX shells and "Write-Host 'Error: ESP-IDF activation script is
missing.'" for PowerShell/Windows (detect platform via existing OS check or the
same logic used by buildActivationCommand()), keeping the
sendCommand(process.getOutputStream(), ...) and message formatting consistent
with buildActivationCommand().
- Around line 80-88: The parsing code in IDFTerminalProcessConnector currently
calls getAsString()/getAsJsonArray() on potentially missing keys
(KEY_SELECTED_ID, KEY_INSTALLED, KEY_ID, KEY_SCRIPT) which can throw NPEs; fix
by guarding access: check root.has(KEY_SELECTED_ID) and root.has(KEY_INSTALLED)
(or root.get(...) != null) and verify types (isJsonPrimitive/isJsonArray) before
calling getAs*(), and in the StreamSupport mapping/filtering ensure each item
has KEY_ID and KEY_SCRIPT and those values are the expected JSON
primitive/string types; if any check fails return Optional.empty() (or a safe
default) instead of dereferencing nulls.
🧹 Nitpick comments (1)
bundles/com.espressif.idf.terminal.connector/META-INF/MANIFEST.MF (1)
17-18: Pin a Gson bundle version range to avoid incompatible resolution.Without a version range, OSGi may resolve to a newer incompatible major on different Eclipse targets. Consider constraining to the supported major/minor range used by the IDE platform.
♻️ Suggested manifest tweak
- com.google.gson + com.google.gson;bundle-version="[2.8.0,3.0.0)"
...connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java
Show resolved
Hide resolved
...connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java
Show resolved
Hide resolved
....connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java
Outdated
Show resolved
Hide resolved
| org.eclipse.terminal.connector.process;bundle-version="[1.0.0,2.0.0)", | ||
| org.eclipse.terminal.control;bundle-version="[1.0.0,2.0.0)" | ||
| org.eclipse.terminal.control;bundle-version="[1.0.0,2.0.0)", | ||
| com.google.gson |
There was a problem hiding this comment.
Check if this is already a pre-requisite plugin during the update site?
There was a problem hiding this comment.
Hi Kondal, we are already using com.google.gson and have it in the update site dependencies
|
I opened the existing workspace and see that terminal has errors. Please check below. When I closed and restarted the IDE, I don't see this issue again, is this due to terminal plugin name changes? 
|
|
There is a discrepancy between what ESP-IDF shows in the terminal and what is activated in the ESP-IDF Manager. steps:
|
|
Another issue is that I'm unable to open multiple terminals. Users may be working on different projects and would prefer to keep multiple terminals open. |
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
Thanks for the PR. Please check my findings.
Hi @kolipakakondal, thanks for the review. Yes, this is a problem caused by the name change. Basically, the memento stores the outdated plugin name and tries to restore this window. I’ll check whether it’s possible to fix this on our side. But I saw the same problem reported on the terminal side Update: I've added an assert with a user-friendly message that explains the need to close and reopen the terminal process. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java`:
- Around line 182-188: The loop that reads themeButtons currently calls
btn.getSelection() without checking whether the Button is disposed, risking
SWTException if the UI was closed; update the loop in the method that saves the
selection (the block iterating themeButtons and using PREF_THEME_SELECTION and
theme.getId()) to skip disposed buttons by checking !btn.isDisposed() before
calling btn.getSelection(), mirroring the pattern used in extractData; ensure
the break behavior remains the same once a selected, non-disposed button is
found.
- Around line 202-210: The loop over themeButtons may access disposed Button
widgets and performs a potentially unsafe null-sensitive comparison; update the
loop that iterates themeButtons in IDFConsoleWizardConfigurationPanel to skip
any Button where btn == null or btn.isDisposed() before calling btn.getData() or
btn.setSelection(), and replace theme.getId().equals(savedId) with a null-safe
comparison such as savedId != null && savedId.equals(theme.getId()) or use
Objects.equals(savedId, theme.getId()) when comparing the ITerminalTheme.getId()
to savedId; ensure found is still set to true only when a matching, non-disposed
button is selected.
🧹 Nitpick comments (1)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/themes/CustomTheme.java (1)
18-23: Avoid calling overridableconfigure()from the base constructor.
This can run subclass code before subclass initialization; safer to invoke after subclass construction.♻️ Suggested refactor (base class)
public CustomTheme(String id, String label) { this.id = id; this.label = label; loadDefaults(); - configure(); } + +protected final void init() { + configure(); +}You’d then call
init()at the end of each subclass constructor.
...or/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java
Show resolved
Hide resolved
...or/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java`:
- Line 78: selectedId obtained via new
IDFEnvironmentVariables().getEnvValue(IDFEnvironmentVariables.ESP_IDF_EIM_ID)
can be null, so avoid calling selectedId.equals(...); update the comparison in
IDFTerminalProcessConnector to be null-safe by either checking selectedId !=
null before equals or by using Objects.equals(selectedId, "<expectedValue>")
(import java.util.Objects) so the comparison (previously using
selectedId.equals(...)) cannot throw a NullPointerException.
🧹 Nitpick comments (1)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java (1)
29-29: Unused constantKEY_SELECTED_ID.This constant is declared but never used. The code at line 78 reads
selectedIdfromIDFEnvironmentVariables.ESP_IDF_EIM_IDinstead of from the JSON using this key. Consider removing it to avoid confusion.- private static final String KEY_SELECTED_ID = "idfSelectedId"; //$NON-NLS-1$
...connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java
Show resolved
Hide resolved
fixed |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/messages.properties`:
- Around line 2-3: Remove the duplicate empty properties entry for the key
IDFConsoleWizardConfigurationPanel_IDFConsoleWizardConfigurationPanel_ProjectLabel
so the intended label "Project name:" is preserved; locate both occurrences of
IDFConsoleWizardConfigurationPanel_IDFConsoleWizardConfigurationPanel_ProjectLabel
in the properties file and delete the second (empty) definition or consolidate
them so only the non-empty value remains.
In
`@bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleMementoHandler.java`:
- Around line 34-37: The null-check order is wrong: Assert.isTrue calls
memento.getID() before verifying memento is not null, which can cause NPE;
update the assertions in IDFConsoleMementoHandler so Assert.isNotNull(memento)
runs before the Assert.isTrue(...) that references memento.getID(), keeping the
existing Messages.IDFConsoleMementoHandler_TerminalNameOutdatedMsg and leaving
Assert.isNotNull(properties) as-is.
In
`@bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java`:
- Around line 74-75: The code in IDFTerminalProcessConnector uses
Files.newBufferedReader(path) which relies on the platform default charset;
change it to open the reader with an explicit UTF-8 charset (use
Files.newBufferedReader(path, StandardCharsets.UTF_8)) so
JsonParser.parseReader(reader) always reads JSON as UTF‑8; update imports to
include java.nio.charset.StandardCharsets if necessary.
- Around line 48-50: buildActivationCommand currently always returns a POSIX
dot-source string which breaks when the launcher uses PowerShell; update
buildActivationCommand to detect the target shell (consult
IDFConsoleLauncherDelegate.getShellExecutable or the configured shell type) and
emit shell-appropriate activation strings: keep the POSIX ". \"<scriptPath>\""
for sh/bash, but for PowerShell return the PowerShell-compatible sequence
including the PSReadLine preamble noted in the PR (the PowerShell invocation and
dot-sourcing semantics) so the activation script runs correctly on Windows.
Ensure the method branches on shell type and references buildActivationCommand
and IDFConsoleLauncherDelegate.getShellExecutable so callers get the correct
command for the configured shell.
🧹 Nitpick comments (2)
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (1)
99-105: Consider falling back ifSHELLpoints to a non-executable path.This avoids terminal launch failures in misconfigured environments.
🔧 Suggested guard for invalid SHELL paths
String shell = System.getenv("SHELL"); //$NON-NLS-1$ - return (shell != null && !shell.isBlank()) ? shell : "/bin/sh"; //$NON-NLS-1$ + if (shell != null && !shell.isBlank() + && java.nio.file.Files.isExecutable(java.nio.file.Path.of(shell))) { + return shell; + } + return "/bin/sh"; //$NON-NLS-1$bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/IDFConsoleWizardConfigurationPanel.java (1)
56-58: Avoid repeating the connector ID literal.Centralize the ID in a constant to prevent drift.
♻️ Suggested refactor
+ private static final String CONNECTOR_ID = "com.espressif.idf.terminal.connector.espidfConnector"; //$NON-NLS-1$ private static final String PREF_THEME_SELECTION = "IDF_CONSOLE_THEME_SELECTION"; //$NON-NLS-1$ private static final String TERMINAL_PREF_NODE = "org.eclipse.terminal.control"; //$NON-NLS-1$ @@ - data.put(ITerminalsConnectorConstants.PROP_TERMINAL_CONNECTOR_ID, - "com.espressif.idf.terminal.connector.espidfConnector"); //$NON-NLS-1$ + data.put(ITerminalsConnectorConstants.PROP_TERMINAL_CONNECTOR_ID, CONNECTOR_ID);Also applies to: 133-135
...idf.terminal.connector/src/com/espressif/idf/terminal/connector/controls/messages.properties
Outdated
Show resolved
Hide resolved
...al.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleMementoHandler.java
Outdated
Show resolved
Hide resolved
...connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java
Show resolved
Hide resolved
...connector/src/com/espressif/idf/terminal/connector/launcher/IDFTerminalProcessConnector.java
Show resolved
Hide resolved
|
@sigmaaa hi! Tested under: able to work in multiple terminals ✅ able to build / flash / monitor / Ctrl A + T ✅ LGTM 👍 |
Description
The goal of this PR is to make the experience of using the ESP-IDF terminal in the IDE as close as possible to the ESP-IDF terminal outside the IDE. To achieve this, we use the activation script, which also makes our terminal more robust, since it ensures that all required variables are initialized.
To run the activation script after terminal startup, a new class, IDFTerminalProcessConnector, was created. After starting PowerShell, I use
"Remove-Module PSReadLine -ErrorAction SilentlyContinue; . \"" + scriptPath + "\"\r\n";to avoid the default PowerShell color scheme, which makes the terminal output hard to read in Eclipse. n the future, this approach can be replaced by providing an Espressif-specific color scheme that works best with PowerShell.
Also, since variables are now initialized via the activation script, we are able to simplify IDFConsoleLauncherDelegate. As a result, this PR includes significant cleanup.
Fixes # (IEP-1669)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test esp-idf terminal in the IDE and compare it to normal esp-idf terminal
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.