Dynamic ESP-IDF Target Discovery and Support Preview Targets #1302
Conversation
WalkthroughRemoved static toolchain entries from plugin.xml and replaced registry/extension-based loading with runtime discovery of ESP-IDF targets. Added IDFTargets and IDFTargetsReader to parse targets from ESP-IDF. Updated ESPToolChainManager, ToolsActivationJob, NewProjectCreationWizardPage, and added a guard in DefaultBoardProvider. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as ToolsActivationJob
participant IDF as IDFUtil
participant Reader as IDFTargetsReader
participant Targets as IDFTargets
participant Mgr as ESPToolChainManager
User->>UI: trigger tools activation
UI->>IDF: getIDFPath()
IDF-->>UI: idfPath
UI->>Reader: readTargetsFromEspIdf(idfPath)
Reader-->>Targets: parsed targets (supported + preview)
UI->>Mgr: configureToolChain(targetNames)
Mgr->>IDF: getIDFPath()
Mgr->>Reader: readTargetsFromEspIdf(idfPath)
Reader-->>Mgr: IDFTargets
Mgr->>Mgr: map IDFTarget -> ESPToolChainElement
Mgr-->>UI: toolchains configured
UI-->>User: setup complete
sequenceDiagram
autonumber
participant Mgr as ESPToolChainManager
participant IDF as IDFUtil
participant Reader as IDFTargetsReader
participant Targets as IDFTargets
Note over Mgr: New dynamic loading flow
Mgr->>IDF: getIDFPath()
IDF-->>Mgr: idfPath
Mgr->>Reader: readTargetsFromEspIdf(idfPath)
Reader-->>Targets: parse constants.py (SUPPORTED/PREVIEW)
Reader-->>Mgr: IDFTargets
Mgr->>Mgr: create ESPToolChainElement for each IDFTarget
Mgr-->>Mgr: log "Dynamically loaded N toolchain elements"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 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). (1)
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 |
kolipakakondal
changed the title
kolipakakondal
changed the title
kolipakakondal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java (1)
176-185: Guard against empty/invalid IDF_PATH to avoid destructive cleanupIf IDF_PATH is absent or parsing yields zero targets, this removes all launch targets and toolchains. Add safe guards.
private void setUpToolChainsAndTargets() { - //Get current active idf - String idfPath = IDFUtil.getIDFPath(); - List<String> targets = IDFTargetsReader.readTargetsFromEspIdf(idfPath).getAllTargetNames(); + // Get current active idf + String idfPath = IDFUtil.getIDFPath(); + if (StringUtil.isEmpty(idfPath) || !new File(idfPath).exists()) { + Logger.log("IDF_PATH is unset or invalid; skipping toolchain reconfiguration to preserve current setup"); //$NON-NLS-1$ + return; + } + List<String> targets = IDFTargetsReader.readTargetsFromEspIdf(idfPath).getAllTargetNames(); + if (targets == null || targets.isEmpty()) { + Logger.log("No targets discovered from ESP-IDF; skipping cleanup/configuration to avoid removing existing toolchains"); //$NON-NLS-1$ + return; + } ESPToolChainManager espToolChainManager = new ESPToolChainManager(); espToolChainManager.removeLaunchTargetsNotPresent(targets); espToolChainManager.removeCmakeToolChains(); espToolChainManager.removeStdToolChains(); espToolChainManager.configureToolChain(targets); }
🧹 Nitpick comments (5)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/DefaultBoardProvider.java (2)
59-66: Null-safety for boards list before streaming.If EspConfigParser.getBoardsForTarget can return null, Line 60 will NPE. Guard early and avoid creating an array when empty.
Apply:
- List<Board> boardsList = this.espConfigParser.getBoardsForTarget(targetName); - String[] boards = boardsList.stream().map(Board::name).toArray(String[]::new); - - if (boards.length == 0) - { - return EspTarget.enumOf(targetName).board; - } - - return boards[getIndexOfDefaultBoard(targetName, boards)]; + List<Board> boardsList = this.espConfigParser.getBoardsForTarget(targetName); + if (boardsList == null || boardsList.isEmpty()) { + return EspTarget.enumOf(targetName).board; + } + String[] boards = boardsList.stream().map(Board::name).toArray(String[]::new); + return boards[getIndexOfDefaultBoard(targetName, boards)];
23-38: Consider deriving defaults from dynamic target metadata instead of hardcoded enum.With dynamic ESP-IDF target discovery, hardcoded defaults for only ESP32C3/S3 are fragile for new targets. If feasible, read a “default board” hint alongside targets; otherwise document that unknown targets return an empty string.
Add a TODO to revisit once IDFTargetsReader exposes such metadata.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java (2)
46-54: Return unmodifiable views to prevent external mutation of internal stateExpose read-only lists to callers to avoid accidental modification.
import java.util.ArrayList; +import java.util.Collections; import java.util.List; @@ public List<IDFTarget> getSupportedTargets() { - return supportedTargets; + return Collections.unmodifiableList(supportedTargets); } @@ public List<IDFTarget> getPreviewTargets() { - return previewTargets; + return Collections.unmodifiableList(previewTargets); }Also applies to: 51-54
38-44: De-duplicate when combining supported + preview targetsIf a target appears in both lists, names and entries will be duplicated. Consider dedup at merge or name extraction.
public List<IDFTarget> getAllTargets() { - List<IDFTarget> allTargets = new ArrayList<>(); - allTargets.addAll(supportedTargets); - allTargets.addAll(previewTargets); - return allTargets; + List<IDFTarget> allTargets = new ArrayList<>(supportedTargets.size() + previewTargets.size()); + allTargets.addAll(supportedTargets); + for (IDFTarget t : previewTargets) { + if (allTargets.stream().noneMatch(x -> x.getName().equals(t.getName()))) { + allTargets.add(t); + } + } + return allTargets; } @@ public List<String> getAllTargetNames() { - return getAllTargets().stream().map(IDFTarget::getName).collect(java.util.stream.Collectors.toList()); + return getAllTargets().stream().map(IDFTarget::getName).distinct() + .collect(java.util.stream.Collectors.toList()); }Also applies to: 77-100
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargetsReader.java (1)
49-56: Consider elevating missing constants.py to warning/error in logsCurrently logged as info; raising visibility can help users diagnose misconfiguration faster.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bundles/com.espressif.idf.core/plugin.xml(0 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/DefaultBoardProvider.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargetsReader.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java(2 hunks)
💤 Files with no reviewable changes (1)
- bundles/com.espressif.idf.core/plugin.xml
🧰 Additional context used
🧬 Code graph analysis (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargetsReader.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargetsReader.java (1)
IDFTargetsReader(25-121)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(59-996)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(59-996)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargetsReader.java (1)
IDFTargetsReader(25-121)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)
⏰ 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_windows
- GitHub Check: build
🔇 Additional comments (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/DefaultBoardProvider.java (1)
61-66: Good guard — downstream callers handle empty-string defaultVerified call sites (JtagVariableResolver, ESPToolChainManager x2, NewSerialFlashTargetWizardPage): they safely accept an empty-string default (no NPE/AIOOBE); no changes required.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java (1)
177-188: Compiler pattern handling for unified layout looks goodCovers both target-specific and unified directory names and anchors on gcc executable. No issues.
Please verify on:
- ESP-IDF v5.0.x (pre-unified Xtensa)
- ESP-IDF v5.5+ (unified)
that gcc paths match the pattern for esp32, esp32s2, esp32s3 and riscv targets.bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/ToolsActivationJob.java (1)
29-29: Import looks correctUsing IDFTargetsReader directly here aligns with the dynamic discovery flow.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java (1)
135-145: Init path logging is fine; ensure stale entries don’t mask missing toolchainsWith the cache clear above, initToolChain will only add real, discoverable toolchains.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/ESPToolChainManager.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java
Show resolved
Hide resolved
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargetsReader.java
Show resolved
Hide resolved
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
LGTM. Added a few nits: we could simplify the switch cases or use a Set for the xtensa targets. Additionally, it would be good to add unit tests for IDFTargetsReader.java to improve test coverage.
| public String getArchitecture() | ||
| { | ||
| switch (name) | ||
| { | ||
| case "esp32": //$NON-NLS-1$ | ||
| case "esp32s2": //$NON-NLS-1$ | ||
| case "esp32s3": //$NON-NLS-1$ | ||
| return "xtensa"; //$NON-NLS-1$ | ||
| case "esp32c2": //$NON-NLS-1$ | ||
| case "esp32c3": //$NON-NLS-1$ | ||
| case "esp32c6": //$NON-NLS-1$ | ||
| case "esp32h2": //$NON-NLS-1$ | ||
| case "esp32p4": //$NON-NLS-1$ | ||
| default: | ||
| return "riscv32"; //$NON-NLS-1$ | ||
| } | ||
| } |
There was a problem hiding this comment.
NIT:
We could simplify the getArchitecture() method to make it more readable and avoid multiple riscv32 statements or returns. For example, using Java 21 switch expressions:
public String getArchitecture() {
return switch (name) {
case "esp32", "esp32s2", "esp32s3" -> "xtensa";
default -> "riscv32";
};
}
Alternatively, a Set-based approach could be used
private static final Set<String> XTENSA_CHIPS = Set.of("esp32", "esp32s2", "esp32s3");
public String getArchitecture() {
return XTENSA_CHIPS.contains(name) ? "xtensa" : "riscv32";
}
| public String getToolchainId() | ||
| { | ||
| switch (name) | ||
| { | ||
| case "esp32": //$NON-NLS-1$ | ||
| case "esp32s2": //$NON-NLS-1$ | ||
| case "esp32s3": //$NON-NLS-1$ | ||
| return "xtensa-" + name + "-elf"; //$NON-NLS-1$ //$NON-NLS-2$ | ||
| case "esp32c2": //$NON-NLS-1$ | ||
| case "esp32c3": //$NON-NLS-1$ | ||
| case "esp32c6": //$NON-NLS-1$ | ||
| case "esp32h2": //$NON-NLS-1$ | ||
| case "esp32p4": //$NON-NLS-1$ | ||
| default: | ||
| return "riscv32-esp-elf"; //$NON-NLS-1$ | ||
| } | ||
| } |
There was a problem hiding this comment.
NIT: same as my earlier comment on getArchitecture() — we could simplify this in the same way using a switch expression or Set-based lookup.
|
@AndriiFilippov PTAL |
|
@kolipakakondal @sigmaaa hi ! Tested under: LGTM in general. just small optimization:
should we keep it like this or not ?
|
I took some motivation from Denys's comments and further tried to enhance and generalize the class for future proofing. It adds a bit more code but the maintainability is not impacted and is much easier then trying to adjust or add targets in one class but multiple places
|
@kolipakakondal @sigmaaa can you please review the PR again I have adjusted some of the comments but in a different way
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/enums/Architecture.java (1)
39-53: XTENSA unified vs target-specific executable prefixes: provide a pattern helperFor unified toolchains, Xtensa executables may be prefixed
xtensa-esp-elf, whereasexecutablePrefixFor()returns only target-specific (e.g.,xtensa-esp32s3-elf). Consider adding a helper that returns a regex alternation including both, and let Target use it for matching compiler/GDB.Apply this addition (to be used by Target.compilerPattern()/debuggerPattern()):
public String executablePrefixFor(Target t) { return toolchainPrefixFor(t); } +/** Regex alternation to match both target-specific and unified prefixes when applicable. */ +public String executablePrefixPatternFor(Target t) +{ + String perTarget = toolchainPrefixFor(t); + if (this == XTENSA) { + return "(?:" + perTarget + "|xtensa-esp-elf)"; //$NON-NLS-1$ + } + return perTarget; +} + public String targetSpecificDirFor(Target t) { return toolchainPrefixFor(t); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/enums/Architecture.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/enums/Target.java(1 hunks)
⏰ 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)
- GitHub Check: build_macos
- GitHub Check: build
- GitHub Check: build_windows
🔇 Additional comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java (1)
74-87: Ignore preview‐targets suggestion
The project wizard (ToolsActivationJob) already calls getAllTargetNames(), so preview targets are included. No changes needed.Likely an incorrect or invalid review comment.
| public static Target fromString(String s) | ||
| { | ||
| if (s == null) | ||
| return null; | ||
| String key = s.toLowerCase(Locale.ROOT); | ||
| for (Target t : values()) | ||
| { | ||
| if (t.idfName.equals(key)) | ||
| return t; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
fromString() hard-codes known targets — limits “future-proof” discovery
Returning null for unknown target names forces callers to drop them. Given the PR’s goal (new targets appear without plugin updates), relying solely on a fixed enum is brittle.
- Either (A) expose a helper like
Target.isXtensaName(String)(e.g.,"esp32".equals(n) || n.startsWith("esp32s")) that downstream code can use as a fallback, or (B) replace enum with a dynamic model/class. - If keeping enum, ensure IDFTargets gracefully handles unknown names (see companion comment in IDFTargets.java).
Would you like me to draft a minimal helper API in this enum and wire it in IDFTargets to keep changes localized?
🏁 Script executed:
#!/bin/bash
# Locate IDFTargets.java declaration
rg -n --type java "class\s+IDFTargets" -C3
# Inspect usage of Target.fromString across Java code
rg -n --type java "Target\.fromString" -C3Length of output: 4820
Handle unknown targets gracefully
In IDFTargets.java’s addSupportedTarget/addPreviewTarget, when Target.fromString(targetName) returns null, fall back to preserving the raw name—e.g.:
Target t = Target.fromString(targetName);
if (t != null) {
supportedTargets.add(new IDFTarget(t, false));
} else {
supportedTargets.add(new IDFTarget(targetName, false));
}Alternatively, add to Target a helper like public static boolean isKnownName(String s) (e.g. Xtensa patterns) and use it for fallback. This prevents dropping new targets without plugin updates.
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/enums/Target.java
around lines 47 to 58, Target.fromString may return null for unknown/new targets
which causes callers to drop those names; update callers
(IDFTargets.addSupportedTarget/addPreviewTarget) to check the result of
Target.fromString and, if null, construct IDFTarget using the raw targetName
string (preserve the original name) OR add a Target helper like public static
boolean isKnownName(String s) and use it to decide whether to convert to a
Target or fall back to the raw name; ensure unknown names are not discarded so
new targets are preserved without plugin updates.
| /** Matches both legacy and unified layouts, ending with -gcc(.exe) */ | ||
| public String compilerPattern() | ||
| { | ||
| String exe = executablePrefix(); | ||
| String legacy = targetSpecificDirName(); | ||
| String unified = unifiedDirName(); | ||
| return "(?:" + legacy + "|" + unified + ")[\\\\/]+bin[\\\\/]+" + exe + "-gcc(?:\\.exe)?$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ | ||
| } |
There was a problem hiding this comment.
Compiler detection breaks for unified Xtensa toolchains
The pattern uses only the target-specific prefix for executables (e.g., xtensa-esp32s3-elf-gcc), but unified toolchains may use xtensa-esp-elf-gcc. This will fail to detect compilers on unified Xtensa.
Apply this diff to accept both prefixes for Xtensa executables:
public String compilerPattern()
{
- String exe = executablePrefix();
- String legacy = targetSpecificDirName();
- String unified = unifiedDirName();
- return "(?:" + legacy + "|" + unified + ")[\\\\/]+bin[\\\\/]+" + exe + "-gcc(?:\\.exe)?$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
+ String legacy = targetSpecificDirName();
+ String unified = unifiedDirName();
+ // Accept both target-specific and unified prefixes for Xtensa executables
+ String exePattern = (arch() == Architecture.XTENSA)
+ ? "(?:" + executablePrefix() + "|xtensa-esp-elf)" //$NON-NLS-1$
+ : executablePrefix();
+ return "(?:" + legacy + "|" + unified + ")[\\\\/]+bin[\\\\/]+" + exePattern + "-gcc(?:\\.exe)?$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$
}📝 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.
| /** Matches both legacy and unified layouts, ending with -gcc(.exe) */ | |
| public String compilerPattern() | |
| { | |
| String exe = executablePrefix(); | |
| String legacy = targetSpecificDirName(); | |
| String unified = unifiedDirName(); | |
| return "(?:" + legacy + "|" + unified + ")[\\\\/]+bin[\\\\/]+" + exe + "-gcc(?:\\.exe)?$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ | |
| } | |
| /** Matches both legacy and unified layouts, ending with -gcc(.exe) */ | |
| public String compilerPattern() | |
| { | |
| String legacy = targetSpecificDirName(); | |
| String unified = unifiedDirName(); | |
| // Accept both target-specific and unified prefixes for Xtensa executables | |
| String exePattern = (arch() == Architecture.XTENSA) | |
| ? "(?:" + executablePrefix() + "|xtensa-esp-elf)" //$NON-NLS-1$ | |
| : executablePrefix(); | |
| return "(?:" + legacy + "|" + unified + ")[\\\\/]+bin[\\\\/]+" + exePattern + "-gcc(?:\\.exe)?$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/enums/Target.java
around lines 97 to 104, the compilerPattern() uses only executablePrefix(),
which misses unified Xtensa executables (e.g., xtensa-esp-elf-gcc). Update the
method to allow both prefixes for Xtensa by building a regex that alternates
executablePrefix() and the unified Xtensa prefix (e.g., "xtensa-esp-elf") when
this target is an Xtensa variant; construct exeAlternatives = executablePrefix()
or the alternate prefix for Xtensa and use that in the returned pattern as
(?:exeAlternatives)-gcc(?:\.exe)?$, so both legacy and unified Xtensa compiler
names are matched.
| public String debuggerPattern() | ||
| { | ||
| return executablePrefix() + "-gdb(?:\\.exe)?$"; //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
Debugger detection misses unified Xtensa GDB prefix
Same issue as compiler: only target-specific prefix is matched. Unified Xtensa ships xtensa-esp-elf-gdb.
Apply this diff:
public String debuggerPattern()
{
- return executablePrefix() + "-gdb(?:\\.exe)?$"; //$NON-NLS-1$
+ if (arch() == Architecture.XTENSA) {
+ return "(?:" + executablePrefix() + "|xtensa-esp-elf)-gdb(?:\\.exe)?$"; //$NON-NLS-1$
+ }
+ return executablePrefix() + "-gdb(?:\\.exe)?$"; //$NON-NLS-1$
}📝 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.
| public String debuggerPattern() | |
| { | |
| return executablePrefix() + "-gdb(?:\\.exe)?$"; //$NON-NLS-1$ | |
| } | |
| public String debuggerPattern() | |
| { | |
| if (arch() == Architecture.XTENSA) { | |
| return "(?:" + executablePrefix() + "|xtensa-esp-elf)-gdb(?:\\.exe)?$"; //$NON-NLS-1$ | |
| } | |
| return executablePrefix() + "-gdb(?:\\.exe)?$"; //$NON-NLS-1$ | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/enums/Target.java
around lines 106-109, the debuggerPattern() currently only matches the
target-specific prefix and misses the unified Xtensa prefix
(xtensa-esp-elf-gdb); update the returned regex so the prefix is either
executablePrefix() or the literal "xtensa-esp-elf" (use a non-capturing
alternation group), followed by "-gdb" and the existing optional "\.exe" suffix,
preserving escaping and comments.
| public void addSupportedTarget(String targetName) | ||
| { | ||
| Target t = Target.fromString(targetName); | ||
| if (t != null) | ||
| supportedTargets.add(new IDFTarget(t, false)); | ||
| } |
There was a problem hiding this comment.
Dropping unknown targets breaks “future-proof” discovery
Target.fromString(targetName) returns null for new targets (e.g., esp32c5), so they never get added. This contradicts the PR objective of zero plugin updates for new targets.
Apply this diff to fallback to a dynamic target for unknown names:
public void addSupportedTarget(String targetName)
{
Target t = Target.fromString(targetName);
- if (t != null)
- supportedTargets.add(new IDFTarget(t, false));
+ if (t != null) {
+ supportedTargets.add(new IDFTarget(t, false));
+ } else {
+ supportedTargets.add(IDFTarget.dynamic(targetName, false));
+ }
}📝 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.
| public void addSupportedTarget(String targetName) | |
| { | |
| Target t = Target.fromString(targetName); | |
| if (t != null) | |
| supportedTargets.add(new IDFTarget(t, false)); | |
| } | |
| public void addSupportedTarget(String targetName) | |
| { | |
| Target t = Target.fromString(targetName); | |
| if (t != null) { | |
| supportedTargets.add(new IDFTarget(t, false)); | |
| } else { | |
| supportedTargets.add(IDFTarget.dynamic(targetName, false)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java
around lines 31 to 36, Target.fromString(targetName) returns null for
unknown/new targets so they are skipped; change the logic to fall back to
creating a dynamic target when t is null by constructing an IDFTarget that uses
the raw targetName and marks it as dynamic/unknown (instead of skipping), e.g.
when t != null keep existing new IDFTarget(t, false) else create a dynamic
IDFTarget using the name and a true dynamic flag so new targets (like esp32c5)
are preserved for “future-proof” discovery.
| public void addPreviewTarget(String targetName) | ||
| { | ||
| Target t = Target.fromString(targetName); | ||
| if (t != null) | ||
| previewTargets.add(new IDFTarget(t, true)); | ||
| } |
There was a problem hiding this comment.
Same issue for preview targets
Preview targets not present in the enum are silently ignored.
public void addPreviewTarget(String targetName)
{
Target t = Target.fromString(targetName);
- if (t != null)
- previewTargets.add(new IDFTarget(t, true));
+ if (t != null) {
+ previewTargets.add(new IDFTarget(t, true));
+ } else {
+ previewTargets.add(IDFTarget.dynamic(targetName, true));
+ }
}📝 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.
| public void addPreviewTarget(String targetName) | |
| { | |
| Target t = Target.fromString(targetName); | |
| if (t != null) | |
| previewTargets.add(new IDFTarget(t, true)); | |
| } | |
| public void addPreviewTarget(String targetName) | |
| { | |
| Target t = Target.fromString(targetName); | |
| if (t != null) { | |
| previewTargets.add(new IDFTarget(t, true)); | |
| } else { | |
| previewTargets.add(IDFTarget.dynamic(targetName, true)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java
around lines 38 to 43, the method addPreviewTarget currently ignores unknown
target names because Target.fromString returning null is silently skipped;
change this to the same behavior as the non-preview addTarget path by validating
the result of Target.fromString and throwing an IllegalArgumentException (or
otherwise reporting an error) if the targetName is not found so unknown preview
targets are not silently ignored.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java
Show resolved
Hide resolved
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
The goal of this PR is to include targets from the preview list even if they aren’t explicitly defined in our target list, based on the assumption that all new targets use the RICSV architecture. Therefore, returning null when a target isn’t found in the enum isn’t the behavior we want.
| if (s == null) | ||
| return null; | ||
| String key = s.toLowerCase(Locale.ROOT); | ||
| for (Target t : values()) |
There was a problem hiding this comment.
If a value isn’t present in the enum but comes from the IDF preview list, our current approach returns null and skips it. That doesn’t align with the PR’s goal
There was a problem hiding this comment.
@sigmaaa kindly take a look again the code is updated now
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/Architecture.java (1)
41-57: Consider centralizing binary prefix alternatives (per‑target vs unified) hereTargetSpec now needs to know both
toolchainPrefixFor(t)andunifiedDirName()to build robust patterns. Exposing a small helper here would reduce duplication and future mistakes.Example addition:
public enum Architecture { ... + /** + * Returns binary prefix candidates for this architecture, covering both legacy per-target + * and unified names (where applicable). + */ + public String[] executablePrefixCandidatesFor(TargetSpec t) { + String primary = executablePrefixFor(t); + String unified = unifiedDirName(); + return primary.equals(unified) ? new String[] { primary } : new String[] { primary, unified }; + } }Then TargetSpec can join candidates for gcc/gdb patterns.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/Target.java (1)
55-60: Trim input before lookup to be more forgivingHelps tolerate user/config inputs with stray spaces.
Apply this diff:
public static Target tryParse(String s) { if (s == null) return null; - return BY_NAME.get(s.toLowerCase(Locale.ROOT)); + String key = s.trim().toLowerCase(Locale.ROOT); + return BY_NAME.get(key); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/Architecture.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/Target.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/TargetSpec.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/internal/DynamicTarget.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java
🔇 Additional comments (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/TargetSpec.java (1)
54-58: Confirm toolchain file naming remains per‑target under unified toolchainsDefault
toolchain-%s.cmakeassumes per‑target files exist (e.g.,toolchain-esp32s3.cmake). If ESP‑IDF ever ships a unified toolchain file (e.g.,toolchain-esp-elf.cmake) for a subset of targets, this default will be wrong for dynamic/preview targets unless higher‑level code overrides it.Please confirm that IDFTargetsReader always supplies the effective toolchain filename when deviating from the per‑target default and that callers prefer that source over this default. If not, we should allow TargetSpec implementations to override this or extend the API to source the filename from the dynamic data.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/internal/DynamicTarget.java (1)
49-51: Java baseline check: pattern matching for instanceof requires Java 17+
if (!(o instanceof TargetSpec ts))uses pattern variables. Ensure the project’s compiler level/baseline supports it. Otherwise, fall back to classic instanceof+cast.If baseline < 17, replace with:
- if (!(o instanceof TargetSpec ts)) - return false; - return idfName.equals(ts.idfName()); + if (!(o instanceof TargetSpec)) + return false; + return idfName.equals(((TargetSpec) o).idfName());bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/Target.java (1)
28-30: Good: unmodifiable name lookup map for O(1) resolutionCreation via
Collectors.toUnmodifiableMapavoids accidental mutation and duplicate keys.
| /** | ||
| * Represents a preview target that isn't (yet) in the {@link com.espressif.idf.core.target.Target} enum. Unknown | ||
| * preview targets default to RISCV32 unless specified otherwise. | ||
| * | ||
| * Keep this class in an internal package; consumers should rely on the TargetSpec API only. | ||
| * | ||
| * @author Ali Azam Rana | ||
| */ |
There was a problem hiding this comment.
Fix Javadoc: wrong link and misleading default‑arch statement
- Link references
com.espressif.idf.core.target.Target; correct package is...toolchain.targets.Target. - The “defaults to RISCV32” claim isn’t implemented here (ctor requires an Architecture). If that default happens elsewhere (e.g., IDFTargetsReader), clarify accordingly.
Apply this diff:
/**
- * Represents a preview target that isn't (yet) in the {@link com.espressif.idf.core.target.Target} enum. Unknown
- * preview targets default to RISCV32 unless specified otherwise.
+ * Represents a preview target that isn't (yet) in the {@link com.espressif.idf.core.toolchain.targets.Target} enum.
+ * Selection of a default architecture (e.g., RISCV32) for unknown targets is handled by the dynamic reader;
+ * this class always carries an explicit {@link Architecture}.
*
* Keep this class in an internal package; consumers should rely on the TargetSpec API only.📝 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.
| /** | |
| * Represents a preview target that isn't (yet) in the {@link com.espressif.idf.core.target.Target} enum. Unknown | |
| * preview targets default to RISCV32 unless specified otherwise. | |
| * | |
| * Keep this class in an internal package; consumers should rely on the TargetSpec API only. | |
| * | |
| * @author Ali Azam Rana | |
| */ | |
| /** | |
| * Represents a preview target that isn't (yet) in the {@link com.espressif.idf.core.toolchain.targets.Target} enum. | |
| * Selection of a default architecture (e.g., RISCV32) for unknown targets is handled by the dynamic reader; | |
| * this class always carries an explicit {@link Architecture}. | |
| * | |
| * Keep this class in an internal package; consumers should rely on the TargetSpec API only. | |
| * | |
| * @author Ali Azam Rana | |
| */ |
| /** Matches legacy/unified dirs and ends with -gcc(.exe). */ | ||
| default String compilerPattern() | ||
| { | ||
| String exe = executablePrefix(); | ||
| String legacy = targetSpecificDirName(); | ||
| String unified = unifiedDirName(); | ||
| return "(?:" + legacy + "|" + unified + ")[\\\\/]+bin[\\\\/]+" + exe + "-gcc(?:\\.exe)?$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | ||
| } |
There was a problem hiding this comment.
Compiler regex likely misses unified Xtensa binary prefix; include both per‑target and unified prefixes
When Xtensa is unified, the binary prefix is typically xtensa-esp-elf, whereas executablePrefix() here resolves to per‑target (e.g., xtensa-esp32-elf). The current pattern would fail to match xtensa-esp-elf-gcc living under the unified dir. Make the binary portion accept both candidates.
Apply this diff:
/** Matches legacy/unified dirs and ends with -gcc(.exe). */
default String compilerPattern()
{
- String exe = executablePrefix();
- String legacy = targetSpecificDirName();
- String unified = unifiedDirName();
- return "(?:" + legacy + "|" + unified + ")[\\\\/]+bin[\\\\/]+" + exe + "-gcc(?:\\.exe)?$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+ String exePrimary = executablePrefix();
+ String exeUnified = arch().unifiedDirName();
+ String legacy = targetSpecificDirName();
+ String unified = unifiedDirName();
+ String exeAlt = "(?:" + exePrimary + "|" + exeUnified + ")";
+ return "(?:" + legacy + "|" + unified + ")[\\\\/]+bin[\\\\/]+" + exeAlt + "-gcc(?:\\.exe)?$"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
}🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/TargetSpec.java
around lines 60 to 67, the compilerPattern only uses executablePrefix() which
misses the unified Xtensa prefix (e.g., xtensa-esp-elf); compute a second
candidate by removing the final hyphenated target segment from
executablePrefix() (e.g., unifiedExe = exe.replaceFirst("-[^-]+$","")) and
update the regex to accept either the per-target exe or this unifiedExe when
matching the gcc binary (use a non-capturing alternation for the two prefixes in
the pattern).
| /** Matches <prefix>-gdb(.exe). */ | ||
| default String debuggerPattern() | ||
| { | ||
| return executablePrefix() + "-gdb(?:\\.exe)?$"; //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
Debugger regex should also accept unified Xtensa binary prefix
Same concern as gcc pattern; match both per‑target and unified prefixes to avoid missing xtensa-esp-elf-gdb.
Apply this diff:
/** Matches <prefix>-gdb(.exe). */
default String debuggerPattern()
{
- return executablePrefix() + "-gdb(?:\\.exe)?$"; //$NON-NLS-1$
+ String exePrimary = executablePrefix();
+ String exeUnified = arch().unifiedDirName();
+ return "(?:" + exePrimary + "|" + exeUnified + ")-gdb(?:\\.exe)?$"; //$NON-NLS-1$
}📝 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.
| /** Matches <prefix>-gdb(.exe). */ | |
| default String debuggerPattern() | |
| { | |
| return executablePrefix() + "-gdb(?:\\.exe)?$"; //$NON-NLS-1$ | |
| } | |
| /** Matches <prefix>-gdb(.exe). */ | |
| default String debuggerPattern() | |
| { | |
| String exePrimary = executablePrefix(); | |
| String exeUnified = arch().unifiedDirName(); | |
| return "(?:" + exePrimary + "|" + exeUnified + ")-gdb(?:\\.exe)?$"; //$NON-NLS-1$ | |
| } |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/targets/TargetSpec.java
around lines 69 to 73, the debuggerPattern() regex only accepts the per-target
executablePrefix and thus misses unified Xtensa prefixes like "xtensa-esp-elf";
update the pattern to accept either the per-target prefix or the unified prefix
by building an alternation between executablePrefix() and
unifiedExecutablePrefix() (e.g. use a non-capturing group
"(?:prefix|unifiedPrefix)" before "-gdb(?:\.exe)?$"), so the returned string
matches both forms.
| Target known = Target.tryParse(targetName); | ||
| if (known == null) | ||
| { | ||
| throw new IllegalArgumentException("Unknown supported target: " + targetName); //$NON-NLS-1$ |
There was a problem hiding this comment.
I don’t think this is the right approach. From my understanding, the goal of this PR is to avoid adding new targets directly in the code and instead rely fully on IDF, under the assumption that new targets will use the RISC-V architecture.
sigmaaa
left a comment
sigmaaa
left a comment
There was a problem hiding this comment.
LGTM! @AndriiFilippov please check
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
Thanks @alirana01 for the changes, LGTM
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.java (1)
IDFTargets(18-215)
⏰ 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)
- GitHub Check: build_windows
- GitHub Check: build_macos
- GitHub Check: build
|
|
||
| import com.espressif.idf.core.configparser.EspConfigParser; | ||
| import com.espressif.idf.core.logging.Logger; | ||
| import com.espressif.idf.core.toolchain.IDFTargets; |
There was a problem hiding this comment.
Use IDFTargetsReader instead of a bare IDFTargets instance
Importing IDFTargets alone is insufficient here; a freshly constructed instance has no targets. Bring in IDFTargetsReader and populate from the current ESP‑IDF installation to actually enable dynamic discovery.
Apply this import adjustment:
import com.espressif.idf.core.logging.Logger;
-import com.espressif.idf.core.toolchain.IDFTargets;
+import com.espressif.idf.core.toolchain.IDFTargets;
+import com.espressif.idf.core.toolchain.IDFTargetsReader;Committable suggestion skipped: line range outside the PR's diff.
| IDFTargets idfTargets = new IDFTargets(); | ||
| targetCombo = new Combo(mainComposite, SWT.READ_ONLY); | ||
| targetCombo.setItems(parser.getTargets().toArray(new String[0])); | ||
| targetCombo.setItems(idfTargets.getAllTargets().stream().map(t -> t.getName()).toArray(String[]::new)); | ||
| targetCombo.select(0); |
There was a problem hiding this comment.
🧩 Analysis chain
Targets combo never populated (new IDFTargets() is empty) → preview targets missing and may show no items
Creating a new IDFTargets without reading from ESP‑IDF yields empty lists, so the combo remains empty and select(0) does nothing. This contradicts the PR goal and reviewer feedback about preview targets. Read targets via IDFTargetsReader, handle empty state, and prefer getAllTargetNames() (sorted) for consistency.
Proposed fix:
- IDFTargets idfTargets = new IDFTargets();
targetCombo = new Combo(mainComposite, SWT.READ_ONLY);
- targetCombo.setItems(idfTargets.getAllTargets().stream().map(t -> t.getName()).toArray(String[]::new));
- targetCombo.select(0);
+ String[] items = new String[0];
+ try {
+ // Resolve targets from the active ESP‑IDF installation
+ IDFTargets targets = new IDFTargetsReader().readTargetsFromEspIdf(IDFUtil.getIDFPath());
+ java.util.List<String> names = targets.getAllTargetNames();
+ names.sort(String::compareToIgnoreCase);
+ items = names.toArray(String[]::new);
+ } catch (Exception e) {
+ Logger.log(e);
+ }
+ targetCombo.setItems(items);
+ if (items.length > 0) {
+ targetCombo.select(0);
+ targetCombo.setEnabled(true);
+ } else {
+ targetCombo.setEnabled(false);
+ setMessage("No ESP‑IDF targets found. Check Preferences → ESP‑IDF and re-open the wizard.", IStatus.WARNING);
+ }
targetCombo.setToolTipText(Messages.NewProjectTargetSelection_Tooltip);Verification suggestions:
- Confirm
IDFUtil.getIDFPath()(or the correct resolver) exists; adjust the call if your utility uses a different method name. - If product intent is to include preview targets at project creation,
getAllTargetNames()is correct. If not, switch togetSupportedTargetNames()or add a “Show preview targets” toggle.
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for IDFTargets class"
rg -nP --type=java 'class\s+IDFTargets\b'
echo "Searching for IDFTargets.getAllTargets method"
rg -nP --type=java 'getAllTargets\s*\(' -n
echo "Searching for IDFTargets.getAllTargetNames method"
rg -nP --type=java 'getAllTargetNames\s*\(' -n
echo "Searching for IDFTargetsReader class"
rg -nP --type=java 'class\s+IDFTargetsReader\b'
echo "Searching for IDFTargetsReader.readTargetsFromEspIdf"
rg -nP --type=java 'readTargetsFromEspIdf\s*\(' -n
echo "Searching for IDFUtil.getIDFPath"
rg -nP --type=java 'IDFUtil.getIDFPath' -nLength of output: 5967
🏁 Script executed:
#!/bin/bash
set -e
echo "Show lines 1-60 of IDFTargets.java"
sed -n '1,60p' bundles/com.espressif.idf.core/src/com/espressif/idf/core/toolchain/IDFTargets.javaLength of output: 1928
Load and display ESP-IDF targets via the reader, not the empty default
The no-arg new IDFTargets() constructor initializes empty lists, so getAllTargets() yields no items. Replace it with IDFTargetsReader.readTargetsFromEspIdf(IDFUtil.getIDFPath()), sort the resulting getAllTargetNames(), populate the combo, and disable it with a warning if no targets are found.
- IDFTargets idfTargets = new IDFTargets();
targetCombo = new Combo(mainComposite, SWT.READ_ONLY);
- targetCombo.setItems(idfTargets.getAllTargets().stream().map(t -> t.getName()).toArray(String[]::new));
- targetCombo.select(0);
+ String[] items = new String[0];
+ try {
+ IDFTargets targets = IDFTargetsReader.readTargetsFromEspIdf(IDFUtil.getIDFPath());
+ List<String> names = targets.getAllTargetNames();
+ names.sort(String::compareToIgnoreCase);
+ items = names.toArray(String[]::new);
+ } catch (Exception e) {
+ Logger.log(e);
+ }
+ targetCombo.setItems(items);
+ if (items.length > 0) {
+ targetCombo.select(0);
+ targetCombo.setEnabled(true);
+ } else {
+ targetCombo.setEnabled(false);
+ setMessage("No ESP-IDF targets found. Check Preferences → ESP-IDF and re-open the wizard.", IStatus.WARNING);
+ }
targetCombo.setToolTipText(Messages.NewProjectTargetSelection_Tooltip);📝 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.
| IDFTargets idfTargets = new IDFTargets(); | |
| targetCombo = new Combo(mainComposite, SWT.READ_ONLY); | |
| targetCombo.setItems(parser.getTargets().toArray(new String[0])); | |
| targetCombo.setItems(idfTargets.getAllTargets().stream().map(t -> t.getName()).toArray(String[]::new)); | |
| targetCombo.select(0); | |
| // Populate the targetCombo from ESP-IDF, sorted, with a warning if none are found | |
| targetCombo = new Combo(mainComposite, SWT.READ_ONLY); | |
| String[] items = new String[0]; | |
| try { | |
| IDFTargets targets = IDFTargetsReader.readTargetsFromEspIdf(IDFUtil.getIDFPath()); | |
| List<String> names = targets.getAllTargetNames(); | |
| names.sort(String::compareToIgnoreCase); | |
| items = names.toArray(String[]::new); | |
| } catch (Exception e) { | |
| Logger.log(e); | |
| } | |
| targetCombo.setItems(items); | |
| if (items.length > 0) { | |
| targetCombo.select(0); | |
| targetCombo.setEnabled(true); | |
| } else { | |
| targetCombo.setEnabled(false); | |
| setMessage( | |
| "No ESP-IDF targets found. Check Preferences → ESP-IDF and re-open the wizard.", | |
| IStatus.WARNING | |
| ); | |
| } | |
| targetCombo.setToolTipText(Messages.NewProjectTargetSelection_Tooltip); |
🤖 Prompt for AI Agents
In
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/templates/NewProjectCreationWizardPage.java
around lines 80 to 83, the code instantiates an empty IDFTargets via new
IDFTargets() which yields no targets; replace this with loading targets using
IDFTargetsReader.readTargetsFromEspIdf(IDFUtil.getIDFPath()), retrieve the list
of target names via getAllTargetNames(), sort them, call
targetCombo.setItems(...) with the sorted names, and if the resulting list is
empty disable the combo and set a visible warning label/message explaining no
targets were found.
|
@alirana01 hi ! Tested under: do see toolchains added for preview targets under: LGTM |
Description
This PR implements dynamic ESP-IDF target discovery that automatically reads both supported and preview targets from ESP-IDF's constants.py file, eliminating the need for manual plugin updates when new targets are added. The solution includes full backward compatibility and handles the toolchain unification
Key Features
Dynamic targets: Automatically discovers all targets from ESP-IDF
Preview targets: Automatically discovers all preview targets
Future-proof: New targets appear without plugin updates going forward when esp-idf supports new targets
Zero maintenance: No manual plugin.xml updates needed
Backward Compatibility: Works with ESP-IDF v5.0+ (both target-specific and unified toolchain structures)
Fixes # (IEP-XXX)
Type of change
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor