AndiDittrich / NodeMCU-Tool

:wrench: Upload + Manage Lua files on NodeMCU
https://www.npmjs.com/package/nodemcu-tool
MIT License
316 stars 54 forks source link

JS library sometimes uploads files with 0 bytes #83

Open czenker opened 3 years ago

czenker commented 3 years ago

Environment


Issue Description

When using NodeMCU-Tool as library for JS, the upload function will create files with 0 byte size if any of the following conditions is met:

Expected Behavior

Files are created on the board...

Current Behavior

... in some cases they have a size of 0 bytes.

Steps to Reproduce

  1. create example.txt with a non empty content
  2. write a JS program that does the following
    1. connect to a device and upload a file
    2. reset the device
    3. connect to the same device and upload a file
  3. the second file will have a file size of 0 bytes

Example

const nodemcu = require('nodemcu-tool')

const dev = "/dev/ttyUSB0";

(async () => {
  try {
    await nodemcu.connect(dev)
    await nodemcu.upload("./example.txt", "example.txt", {}, () => {})

    console.log(await nodemcu.fsinfo())

    await nodemcu.hardreset()
    await nodemcu.disconnect()

    await new Promise(r => setTimeout(r, 2000)) // wait for reboot

    await nodemcu.connect(dev)

    await nodemcu.upload("./example.txt", "example.txt", {}, () => {})
    console.log(await nodemcu.fsinfo())
  } catch (e) {
    console.error(e)
  }
})()

Detailed Description

Core problem is that _isTransferWriteHelperUploaded is tracked as a global variable and is never reset. It should handle device changes, reboots, disconnections, etc.

https://github.com/AndiDittrich/NodeMCU-Tool/blob/f160e0abbbe89927fbdc8c6a308b4d2713009fc8/lib/connector/upload.js#L9

Side note: The download function does not show this behavior.

Possible Solution

Remove the check, if transferWriteHelper was written and write it before every single file upload.

An alternative approach is to reset _isTransferWriteHelperUploaded whenever a connection to a new device is established or it is reset. But this would create dependencies between unrelated modules. And it would not cover the case when the user or a script issues a reset through the execute function.

A different alternative is to check if _G.__nmtwrite exists on NodeMCU before every write. But I would disregard that, because if we need to execute a lua script, we might as well just write the transferWriteHelper.

Pull-request inbound.

AndiDittrich commented 3 years ago

HI @czenker

thanks for your report.

a reset during connection is not projected to be handled by the library.

imho the only reliable solution is to wrap the file transfer helper within an instance of the current connection but this require stateful components within the library.

at the moment i would mark it as bug and wont-fix since the use-case is very limited

czenker commented 3 years ago

Hi @AndiDittrich.

Thanks for the fast response. I think the main issue here is that it is very easy to get nodemcu-tool (the JS library) into a state where the upload does not work. If there was a way to programatically recover from it, I would not consider it a too big issue. But it is not possible to recover from that state without restarting the NodeJS(!) application.

AndiDittrich commented 3 years ago

ok. the underlying library was never intended to be used as regular library.

czenker commented 3 years ago

Alright. This decision makes sense then.

ProgrammaticUsage.md should be updated, because it states

It's possible to use the underlying "NodeMcuConnector" in your own Node.js projects

AndiDittrich commented 3 years ago

thanks, i've added a usage notice