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

JS library sometimes uploads files with 0 bytes #83

Open
czenker opened this issue Feb 14, 2021 · 5 comments · May be fixed by #84
Open

JS library sometimes uploads files with 0 bytes #83

czenker opened this issue Feb 14, 2021 · 5 comments · May be fixed by #84

Comments

@czenker
Copy link

czenker commented Feb 14, 2021

Environment

  • Operating System
    Fedora 33
  • ESP Device/Revision
    ESP8266
  • NodeMCU-Tool Version
    Please use the least recent version to validate the issue
    3.2.1
  • Node.js Version
    14.15.4
  • NodeMCU LUA Firmware Version
    3.0.0

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:

  • The device is reset between file uploads
  • files are uploaded to two different connected devices

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.

let _isTransferWriteHelperUploaded = false;

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.

czenker added a commit to wifi-tally/NodeMCU-Tool that referenced this issue Feb 14, 2021
@czenker czenker linked a pull request Feb 14, 2021 that will close this issue
@AndiDittrich
Copy link
Owner

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
Copy link
Author

czenker commented Feb 16, 2021

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
Copy link
Owner

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

@czenker
Copy link
Author

czenker commented Feb 16, 2021

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
Copy link
Owner

thanks, i've added a usage notice

czenker added a commit to wifi-tally/NodeMCU-Tool that referenced this issue Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants