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

Additional Fix for Windows 10 IO-Error in make monitor #1567

Closed
wants to merge 2 commits into from
Closed

Additional Fix for Windows 10 IO-Error in make monitor #1567

wants to merge 2 commits into from

Conversation

FredrikFornstad
Copy link
Contributor

It appears that the fix provided to #1136 do not cover all situations: There is another write output in idf_monitor.py that also can go wrong... When running the factory lws app, it seems to trigger this one.

How to reproduce:
Compile and flash https://github.com/warmcat/lws-esp32-factory
Run make monitor, access esp32 with web browser, select an AP, enter password
make monitor (on Windows 10) will now crash within 40 seconds but normally just after 5-10 seconds. Refering to IO-Error at row 588.

Proposal for fix:
In my pull request, I have duplicated the fix (second try to write if first one fail) that was introduced to the other write statement in idf_monitor.py as the original solution to issue 1136.

Testing:
After applying my fix and have had it running for 30 minutes, I have had no crash,

@CLAassistant
Copy link

CLAassistant commented Jan 30, 2018

CLA assistant check
All committers have signed the CLA.

@projectgus
Copy link
Contributor

Thanks @FredrikFornstad , we missed that spot the first time!

Now this is needed in two places I'd prefer to do a little refactoring here, maybe add a _persistent_write(self, data) method to the ANSIColorConverter class and call that from the two places.

Is that change you'd be interested in making to this PR?

@FredrikFornstad
Copy link
Contributor Author

Fully understand that you prefer a nicer solution. Honestly think it is better if you do the refactoring as I think the result will be better then. I totally new to esp-idf...

@igrr igrr closed this in 4eab275 Feb 5, 2018
espressif-bot pushed a commit to espressif/esp-idf-monitor that referenced this pull request Jan 16, 2023
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