Skip to content

add detect chip with get security info - #197

Open
brianignacio5 wants to merge 8 commits into
mainfrom
feature/detect-chip
Open

add detect chip with get security info#197
brianignacio5 wants to merge 8 commits into
mainfrom
feature/detect-chip

Conversation

@brianignacio5

@brianignacio5 brianignacio5 commented Apr 22, 2025

Copy link
Copy Markdown
Collaborator

Description

Fix #248

Add get security info detect chip to read IMAGE_CHIP_ID from devices before using magic number as fallback.

This should minimize maintenance of magic number for new chips.

This pull request introduces significant improvements to chip detection, security information handling, and support for new ESP32 chip variants. The changes enhance the robustness of chip identification, add support for secure download mode, and expand compatibility with additional ESP32 chips. There are also updates to stub loader file naming and the firmware image loader to accommodate new devices.

Chip detection, security, and communication improvements:

  • Refactored chip detection logic in ESPLoader to use the GET_SECURITY_INFO command for identifying chips, with a fallback to the magic register method for older or unsupported chips. This makes chip detection more reliable and future-proof.
  • Added support for handling secure download mode: the loader now caches security info, exposes parsed security flags, and disables baud rate changes when secure download mode is active, both in ESPLoader and ESP32C5ROM. [1] [2] [3] [4]
  • Exported new types and constants related to security info (SecurityInfo, SECURITY_INFO_FLAG_MAP, ParsedSecurityFlags) for use by downstream consumers. [1] [2]

Expanded chip and firmware image support:

  • Added support for new ESP32 chip variants (esp32s31, esp32e22, esp32h21, esp32h4) in the firmware image loader and implemented corresponding firmware image classes. [1] [2] [3] [4] [5]
  • Updated the ROM definitions for various chips to include the new USES_MAGIC_VALUE property, clarifying detection capabilities. [1] [2]

Stub loader and file naming updates:

  • Standardized stub loader JSON file names for all chip variants, removing the stub_flasher_ prefix and updating import logic accordingly.
  • Added stub loader support for new chips where available, and handled missing stubs gracefully for chips not yet supported.

Firmware image header handling:

  • Improved robustness in ESP32FirmwareImage by handling cases where IMAGE_CHIP_ID may be undefined, preventing potential errors when working with new or custom chip variants. [1] [2]

Developer experience and documentation:

  • Enhanced JSDoc comments for several methods, providing clearer documentation for chip detection, security info retrieval, and flash reading operations. [1] [2]

These changes collectively make the codebase more maintainable, extensible, and ready for future ESP32 chip variants.

Testing

Ideally you could try testing to connect a board which magic number is not defined in esptool-js magic2Chip function.

Another test is esp32 or esp32s2 will try to connect and run the get security info command, which will fail, and fallback to use the magic number to identify ROM class.


Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@brianignacio5 brianignacio5 self-assigned this Apr 22, 2025
@github-actions

github-actions Bot commented Apr 22, 2025

Copy link
Copy Markdown

Download the artifacts for this pull request:

@brianignacio5

Copy link
Copy Markdown
Collaborator Author

PTAL @RushikeshPatange We need this to support ESP32 S31

@RushikeshPatange

Copy link
Copy Markdown
Contributor

PTAL @RushikeshPatange We need this to support ESP32 S31

Sure will review the changes

@RushikeshPatange RushikeshPatange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The points below are based on a comparison of logs between esptool-js (current branch) and esptool.py (main), as reviewed by Claude. Please consider them accordingly, as per the scope of this MR.

A few points that might be worth considering:

  1. main() may still encounter an issue with chips in Secure Download Mode (SDM). While the detection now handles SDM correctly, esploader.ts:1480 subsequently calls getChipDescription / readMac unconditionally, which use READ_REG (unsupported in SDM), followed by runStub. In Python, run_stub handles this gracefully by warning and continuing without the stub. It might be worth considering skipping the register-read information and stub upload when this.secureDownloadMode is true.

  2. H21/E22 flashing may still not work end-to-end. getStubJsonByChipName returns undefined for these chips, and runStub (esploader.ts:1417) currently throws "Error loading Stub json" instead of falling back to ROM flashing. Python handles this by warning that the stub flasher is not yet supported on ESP32-H21/E22 and continuing without the stub. It might be helpful to have a similar graceful fallback here; otherwise, the newly added targets may still not be usable for these two chips.

  3. securityInfoCache does not appear to be invalidated on reconnect. A reconnect, including the retry inside identifyChip, may therefore reuse the previously parsed information. This is harmless with the current flow, but it might be safer to either call getSecurityInfo(cache=false) after reconnect or clear the cache in connect().

  4. Minor / pre-existing: The usesUsbOtg-style checks appear to compare only the USB PID rather than the VID + PID (this is also mentioned in the S31 comment). Additionally, Python's verification in the non-detecting connect() flow — "This chip is X, not Y — wrong chip argument?" — does not appear to be ported. Since esptool-js always autodetects the chip, I believe this is relatively low priority.

@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2025 Espressif Systems (Shanghai) CO LTD

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider 2026 instead of 2025 ?

same "printed page" as the copyright notice for easier
identification within third-party archives.

Copyright 2025 Espressif Systems (Shanghai) CO LTD

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider 2026 instead of 2025 ?

Comment thread src/targets/esp32p4.ts
// throw new Error("Powering on flash in secure download mode");
// }
if (loader.secureDownloadMode) {
throw new Error("Powering on flash in secure download mode");

@RushikeshPatange RushikeshPatange Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not getting the semantics of "Does this operation allowed or not in SDM ?" from the error message
Should it be Powering on flash in secure download mode not allowed or something ?

Comment thread src/esploader.ts
*/
export type FlashReadCallback = ((packet: Uint8Array, progress: number, totalSize: number) => void) | null;

export { SecurityInfo, SECURITY_INFO_FLAG_MAP, ParsedSecurityFlags } from "./types/securityInfo.js";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This re-export seems to be redundant, should be removed from here and incorporated in the index barrel file

// esploader.ts — delete line 26 entirely

// index.ts
export { ESPLoader, FlashReadCallback } from "./esploader.js";
export { SecurityInfo, SECURITY_INFO_FLAG_MAP, ParsedSecurityFlags } from "./types/securityInfo.js";
// SecurityInfo and ParsedSecurityFlags are pure types, so export type { ... } would be more precise to have

Comment thread src/esploader.ts Outdated
* @param {Before} mode Reset mode used if a reconnect is required
* @param {number} attempts Connection attempts used if a reconnect is required
*/
private async identifyChip(mode: Before, attempts: number) {

@RushikeshPatange RushikeshPatange Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few thoughts on identifyChip method breakdown for better modularity, readability and reusability

  1. identifyChip() is doing too many things. It currently handles chip-ID detection, magic-value fallback, the S2-in-SDM case, reconnect/retry, and updating loader state. It may be cleaner to split these into small helpers such as:

    • romFromChipId(chipId)
    • readSecureDownloadMode()
    • identifyChipByMagic()
    • applyDetectedChip(chip)
  2. connect() and identifyChip() are somewhat coupled. connect() calls identifyChip(), which can call connect() again for retry. This is currently safe because of detecting=false, but moving the retry logic to the caller would make the flow easier to understand.

  3. There is some duplicated SDM/security-info logic. A common readSecureDownloadMode() helper could avoid duplication and also provide a single place to handle security-info cache invalidation after reconnect.

  4. The S2-SDM path creates another ESP32S2ROM instance. Since CHIP_DEFS.esp32s2 already exists in the target registry, it would be better to reuse that instance instead of creating a new one. This keeps all detection paths consistent.

  5. The biggest concern for me is using error messages for control flow. For example, checking error.message === "unsupported command error" or startsWith("Unexpected chip ID value"). A small change to the error message could break the detection flow. It would be more robust to introduce typed errors such as UnsupportedCommandError and UnexpectedChipIdError and check them using instanceof, similar to Python.

  6. The catch block currently treats any failure as "GET_SECURITY_INFO not supported". An actual timeout or connection failure could therefore incorrectly fall back to magic-value detection. Typed errors would make it possible to fall back only for the expected unsupported-command case and propagate unexpected errors.

  7. Returning the detected ROM instead of modifying loader state inside identifyChip() could make the flow clearer. Something like identifyChip(): Promise<ROM> would make it easier to see what the method actually produces and could also make the logic easier to reuse later.

Overall, I think typed errors (5) would be the most important change to consider in this MR because it affects correctness. The remaining points are mostly structural improvements and can reasonably be deferred if we want to keep this MR focused on parity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ESP32-S31 misdetected as ESP32-P4: magic-register-only detection can't identify newer chips

2 participants