Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix display of monitor the device when flash fails #1286

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

radurentea
Copy link
Collaborator

@radurentea radurentea commented Aug 22, 2024

Description

  • Changed the following message You can monitor with ESP-IDF: Monitor your device command to:
    You can monitor your device with ESP-IDF: Monitor command
  • Change behaviour so that we don't display the message if flash fails

JIRA: https://jira.espressif.com:8443/browse/VSC-1431

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Steps to test this pull request

Preparation:

  1. Ensure you have the latest version of the VS Code Extension source code:

    • Install Node.js
    • Clone this repository git clone --recursive https://github.com/espressif/vscode-esp-idf-extension.git
    • Install all the dependencies using yarn. (you might need to install yarn)
    • Open the project in Visual Studio Code and press F5 to run with the debugger. This will launch a new VS Code Extension Development Host window, allowing you to debug the extension. (Note: This method allows you to test changes directly, as an alternative to installing the extension via a .vsix file.)
  2. Set up a test ESP project that can be flashed.

Test Scenarios:

A. Normal Behavior (Baseline):

  1. Attempt to flash a project that should succeed.
  2. Verify that the flashing process completes successfully.
  3. Confirm the message "You can monitor your device with ESP-IDF: Monitor command" appears.

B. Simulated Flash Failure:

  1. Modify the file src\flash\uartFlash.ts in the VS Code Extension source code.
  2. Add the following code beneath the try{ inside async function flashCommand:
    throw new Error("TEST_UNUSUAL_ERROR");

Like this:

try {
    throw new Error("TEST_UNUSUAL_ERROR"); // <-- Add this line
    const model = await createFlashModel(
      flasherArgsJsonPath,
      port,
      flashBaudRate
    );

How has this been tested?

As described above

Test Configuration:

  • ESP-IDF Version: 5.1.4
  • OS (Windows,Linux and macOS): Windows 11

Checklist

  • PR Self Reviewed
  • Applied Code formatting
  • Added Documentation
  • Added Unit Test
  • Verified on all platforms - Windows,Linux and macOS

@radurentea radurentea self-assigned this Aug 22, 2024
Copy link

github-actions bot commented Aug 22, 2024

Download the artifacts for this pull request:

@radurentea
Copy link
Collaborator Author

Hi @brianignacio5, PTAL

Copy link
Collaborator

@brianignacio5 brianignacio5 left a comment

Choose a reason for hiding this comment

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

LGTM

@radurentea
Copy link
Collaborator Author

HI @AndriiFilippov, PTAL

src/extension.ts Outdated Show resolved Hide resolved
@AndriiFilippov
Copy link
Collaborator

@radurentea hi !

Tested under:
OS - Windows 11 / MacOS

do see "success" message:
image

do see "fail" message":
image

LGTM 👍

Only question related to output:
Why don't we clean the console ?
I mean if I do successful flash and the next flash will fail - I will get something like this. Where you could see the previous message about successful flash and then the notification about failed flash
image

Nothing critical but should we somehow separate this messages from each other with space ? or simply clean console

@radurentea
Copy link
Collaborator Author

Hi @AndriiFilippov,

Regarding the messages that don't clean themselves, I think it's ok the way it is. The user can use the clear console feature if they want it.

We could add a feature to add timestamps for the ESP-IDF output channel in the future and disable it by default and offer users the possibility of enabling it by using an idf paramater.

@radurentea radurentea merged commit 98a4d94 into master Sep 10, 2024
6 checks passed
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.

3 participants