-
Notifications
You must be signed in to change notification settings - Fork 304
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
[VSC-1473/1495] update dockerfile and qemu options #1282
Conversation
Download the artifacts for this pull request: |
549494f
to
db592a3
Compare
Pull request has been marked as |
db592a3
to
5cad306
Compare
Pull request has been marked as |
test msg fix lint rm test msg
66c814e
to
2a1ba71
Compare
PTAL @radurentea @Fabricio-ESP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments related to some small text changes
src/extension.ts
Outdated
} catch (error) { | ||
const msg = error.message | ||
? error.message | ||
: "Error launching QEMU debugging"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use translation for notitfications and keep the name and capitalisation the same. Please check line 2471
src/extension.ts
Outdated
{ | ||
cancellable: true, | ||
location: ProgressLocation, | ||
title: "ESP-IDF: Starting ESP-IDF QEMU Debug", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should change the title to "ESP-IDF: Starting ESP-IDF QEMU Debugging"
Hi @brianignacio5, I've left some small comments related to consistency of the text and translations. Also, when testing I ran into the following issues:
|
2 things you need to consider:
|
Hi @brianignacio5,
After properly configuring the container settings, the issue has been resolved. The project is now working as expected. I'll create a separate ticket to investigate a more robust solution for managing configuration switches between local development and container environments as we've discussed. L.E: new task can be found under VSC-1506 in JIRA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on container connection from a project on windows and on my default WSL instance. Container creation, QEMU execution and real target connection were successful.
Description
Update Dockerfile and QEMU options.:
${idf.buildPath}/merged-qemu.bin
delete when${idf.buildPath}/.bin_timestamp
changes to trigger rebuilding the merged QEMU bin image.Fixes #1281
Fixes #1320
Type of change
Please delete options that are not relevant.
Steps to test this pull request
Provide a list of steps to test changes in this PR and required output
ESP-IDF: Monitor QEMU Device
should work for esp32 or esp32c3 and show error for other IDF_TARGET.merged-qemu.bin
should be deleted.ESP-IDF: Monitor QEMU Device
and the new image should be generated and used.Expected behaviour:
Expected output:
How has this been tested?
Manual testing in a Dev Container in Visual Studio Code running previous code on ESP-IDF project like the blink example.
Test Configuration: Using VSCode Devcontainer Docker container in either Windows or MacOS
Checklist