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

Refactor to clean up and fix console handling #3666

Merged
merged 9 commits into from
Dec 10, 2024

Conversation

jmattsson
Copy link
Member

Fixes #3657.

  • This PR is for the dev-esp32 branch rather than for the release branch.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

In the interest of backwards compatibility I/we have in the past shoehorned the USB-Serial-JTAG and CDC-ACM console types into the uart module via the platform layer. That has kept adding complications and differences in behaviour depending on what the underlying hardware interface is, plus it has been fighting against the IDF's own abstractions in regard to the console.

This PR finally "bites the bullet" and moves the system console handling into its own module (console), and tidies up the platform layer to no longer attempt to special-case things. It also sets the stage for possibly also including a fancier interactive console based on linenoise, though that requires further investigation and work if so.

The downside of course is that various IDEs are accustomed to manipulating the uart module in order to implement file uploads and downloads, and this refactoring obviously breaks that. That said, @serg3295 has already implemented support in the nodemcu-tools VS Code extension and NodeMCU-Tool CLI utility. I've also provided both an example of how to implement a framed transmission in the module documentation, as well as a helper utility (scripts/upload-file.py) which can be used to upload files using console module interactions, using said framing scheme.

I'm not happy about breaking backwards compatibility, but I'm even less happy about the number of issues that have cropped up due to trying to avoid it when it's clearly time for it. We don't have the person-power here to fight against the direction the IDF is pulling, so imho it's time to follow its lead in the console area.

Testing has been performed on each of UART, USB-Serial-JTAG and CDC-ACM, and additional testing by @serg3295 over in the #3657 thread.

jmattsson and others added 5 commits October 22, 2024 19:00
A breaking change, but should finally see us move away from the chronic edge
cases and inconsistent behaviour we have while trying to shoe-horn the
usb-serial-jtag and cdc-acm consoles into uart behaviour and assumptions.
Added example on using framed data transmission over the console.
Plus, it can serve as a reference for any IDEs which may need/want
updating.
@jmattsson jmattsson force-pushed the dev-esp32-console-module branch from f108d65 to f9875d0 Compare October 27, 2024 03:47
@jmattsson
Copy link
Member Author

Announcing intent to merge despite lack of peer review — I'd rather be told off than have the project stagnate into obsolescence 🙃

@marcelstoer
Copy link
Member

marcelstoer commented Dec 9, 2024

Thank you Jade! Breaking backwards compatibility is ok IMO when the alternatives are worse. Going with the flow wrt IDF is definitely sensible.

@serg3295 can I ask you to once again make a PR for the upstream NodeMCU-Tool project such that your changes can be baked into a release by the maintainer. Thank you!

@serg3295
Copy link

serg3295 commented Dec 9, 2024

@marcelstoer Sure. I have already prepared PRs that provide support for the new esp32 "console" module for the upstream NodeMCU-Tool and ESPlorer projects.
I'll push them as soon as the PR "Refactor to clean up and fix console handling" is merged.

Changes have already been made to the NodeMCU-Tool fork (the dev-esp32 branch), but no release has been released.

Of course, the nodemcu-tools extension for VS Code will also be released 😃

@jmattsson
Copy link
Member Author

Thank you @marcelstoer (and @serg3295 )!

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

Successfully merging this pull request may close these issues.

3 participants