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

Separate environment specific bindings and make esptool-js usable from Node.js environment #117

Open
cubicap opened this issue Nov 4, 2023 · 6 comments · May be fixed by #122
Open

Separate environment specific bindings and make esptool-js usable from Node.js environment #117

cubicap opened this issue Nov 4, 2023 · 6 comments · May be fixed by #122
Assignees

Comments

@cubicap
Copy link

cubicap commented Nov 4, 2023

I want to use the tool from a Node.js application to flash firmware to an ESP32.

Even though the app is used from a desktop environment, I do not want to rely on the python esptool, because, it is an external dependency, which complicates distribution and limits the possibility of the application being ported to a web app in the future.

To make this possible, I propose two changes:

  1. split the webserial Transport into two classes implementing a simple abstraction layer around the serial port and new Transport implementing packet parsing etc.
  2. change the package configuration, so that it can be imported using node.js module resolution.

This will not require extensive changes and will make it easy to write bindings for other serial port implementations (such as node serialport. It will also make it easier to port the application to completely different connection types (such as RFC 2217).

Breaking changes to the API will be required.

I have hacked an ugly version, which works both in both node.js and as a web app. I've used it as a proof of concept in my app, however, I have not made any changes to the API yet (leading to code duplication in the Transport code). I also do not have much (mostly none) experience with JavaScript/TypeScript build systems and module resolution, for which reason the changes in package configuration are especially ugly (and change things, which probably can be fixed in some other way).

I also do not want to rely on modified/own implementation of the esptool, as it would mean giving up official support and more work in the future.

If the proposed changes are accepted, I will be happy to implement the first suggested change. On the other hand, I do not have the skills to implement the second change, so I would be happy if someone else could do it or guide me through the process.

@igrr
Copy link
Member

igrr commented Nov 15, 2023

@brianignacio5 Could you please take a look at this issue?

@brianignacio5
Copy link
Collaborator

Hi @cubicap Sorry for late reply.

I've made such refactoring in #122 for separating the Transport code into a transport class. Please take a look when you can.

For using this package with Node.js, you should already be able to do so. This package is published in NPM and you can do npm install esptool-js or yarn add esptool-js and there is a module reference in the package.json. I've tested this in our Theia based IDF Web IDE project and is basically doing it with Node.js (as far as I know).

@hmeerlo
Copy link

hmeerlo commented Dec 12, 2024

So is this on a dead end? Would be really useful to be able to use this in NodeJS as well.

@brianignacio5
Copy link
Collaborator

It is not dead. I made some effort with #122 but @cubicap wanted to split slip formatting from the web serial read and we reach a point that it couldn't refactor anymore without breaking connection. Unfortunately the original esptool.py code is very tightly mixed with slip parsing and keeping functionality the same proves rather challenging.

A solution will be to just make an interface from the Transport class and implement the same for node serial port. This is relatively simple implementation I could do.

After #160 I've made some significant changes which make #122 needs some refactoring. I could give it another go but I can't guarantee the design @cubicap wish to has. This time, I think I can just add a NodeSerialPortTransport myself for you to test @cubicap @hmeerlo

@cubicap
Copy link
Author

cubicap commented Dec 13, 2024

Thanks a lot for the continued work. I've been quite busy "lately", and this got quite low on my priority list as we've temporarily moved to a browser-based flasher (but still want to move it back to Node). I'm sorry for not updating the status on my side.

I was looking at the code in main just a few days ago and saw the amount of changes and progress.

Currently, I do not have any concrete application for transport implementations other than webserial and Node SerialPort (or anything else for Node), so if it is easier for you to implement the NodeSerialPortTransport directly, it should do the trick for me.

A problem worth noting, though - Node SerialPort appears to be unmaintained since last year, and a quick search does not show any replacements. What is a bit worrying, problems are starting to appear with the library. We encountered some of them in July, but I can't recall them exactly now - I believe they were connected to the handling of RTS, DTR and possibly other signals; the problems also appeared only on some machines, but we did not find a common denominator.

The separation I wanted in the PR could simplify the implementation, as the code would consist of smaller chunks that are easier to maintain and argue about their correctness. If the transports are implemented the same way WebserialTransport is now, there would probably be a significant amount of (non-trivial) duplicate code that will have to be maintained, which could prove difficult in the future (especially to maintain functional equivalency). It could also be helpful in case the Node SerialPort library stops working in the future.

@hmeerlo
Copy link

hmeerlo commented Dec 13, 2024

Thanks guys for the update, really appreciated. I might take a stab at abstracting the Transport interface myself because I really need this. We had a proprietary nodejs implementation of esptool, but now we're moving to different ESP chips it is becoming very hard to maintain so I'd rather switch to this project.

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 a pull request may close this issue.

4 participants