nodemcu / nodemcu-firmware

Lua based interactive firmware for ESP8266, ESP8285 and ESP32
https://nodemcu.readthedocs.io
MIT License
7.67k stars 3.13k forks source link

uart interbyte timeout #1084

Closed Tertiush closed 8 years ago

Tertiush commented 8 years ago

Hi

I guess this is a feature request rather than an issue, but one I feel is very useful.

The current uart config allows for some flexibility in the triggering format for received data. However the problem I now have is that in selecting a predefined length for the response, my function is sometimes not called. This appears to be due to the responding device not always behaving when replying....

Would it be possible to add an argument to uart.on to specify an interbyte timeout? This is then used with some timer to wait for this silence period on the receiving end (couple ms) before signalling the completion of the received data, consequently return it.

gsker commented 8 years ago

Is this related to the fact that the fastest lua uploader I have found, https://github.com/kmpm/nodemcu-uploader no longer works with a dev branch build? A verbose run doesn't give me any clues why.

pjsg commented 8 years ago

I found this myself the other day. It turns out that it doesn't work because of a change to the filename validation. The current uploader code sends over the filename that it wants to write to followed by a null byte (which is the end-of-name marker).

function recv_name(d) d = string.gsub(d, '\000', '') file.remove(d) file.open(d, 'w') uart.on('data', 130, recv_block, 0) uart.write(0, '\006') end

The gsub is supposed to remove the null byte off the end -- but it doesn't. Then the string with the null byte is passed into the file module where the improved filename validation causes a platform PANIC. I changed the uploader code to just chop the last character off the string, and then it worked fine.

I don't know whether this is a bug in string.gsub or just in the uploader code.

gsker commented 8 years ago

I replaced d = string.gsub(d, '\000', '') with d = d:sub(1,-2) And that seemed to work for a while. I did find this: https://github.com/kmpm/nodemcu-uploader/issues/48 which pointed me to https://github.com/kmpm/nodemcu-uploader/commit/7c7026491a4a730965903d7eb848a47f4e628195 but that didn't fix it for me.

And after some playing I find that the truncation did not reliable fix it for me either. I'm starting to think that the UART code in current dev state is not stable.

I can't paste into my minicom window reliably any more either. It too often gets mis-interpreted as it gets pasted.

I'm sorry for totally hijacking this Issue.

kmpm commented 8 years ago

Since the same uploader code works for master but not dev I would say that there is a bug/change in the firmware. The uploader filename separator have been a null byte since day one.

pjsg commented 8 years ago

Yes -- the change was that zero bytes are now not valid filename characters (as determined by the file module). Having null bytes in strings is a source for endless confusion (and often security vulnerabilities). Consider windows file names like foo.dll\0.txt. Is this a txt file or a dll?

The spiffs filesystem only looked at the bytes up to the first null anyway (which is why it used to work).

Note that

d = string.gsub(d, '\000', '')

doesn't do what you think. In particular, it does not remove null bytes from a string. You could argue that this is a bug in gsub -- but fixing it will be mighty difficult (as is anything involve regexes). Happily for nodemcu, this has to be fixed way way upstream.

kmpm commented 8 years ago

so gsub does not replace "some" strings... nice

How would d = d:sub(1, string.find(d, '\000')-1) be, from performance standpoint? I do not want to remove the last character, but rather keep everything up to the null.

TerryE commented 8 years ago

@kmpm Peter, Why not just fix your bug in the uploader, rather than expecting the firmware to tolerate it?

As far as I can see, you are null char terminating the file names that you send down to the ESP. Why?? Like most languages (and unlike C) Lua doesn't treat a nullchar as special. If your add a nullchar to filename, say "file.txt" then the Python string file.open('{filename}') will evaluate to file.open('file.txt\0') which is perfectly good Lua, but also an invalid filename as far as file.open() is concerned.

Sorry but this one is not a bug as far as the firmware is concerned, and I would prefer to close this discussion.

@Tertiush, Your original request is an enhancement request that I don't think that we will action in the near future.

TerryE commented 8 years ago

Out of interest, if you RTM:

A pattern cannot contain embedded zeros. Use %z instead.

so the following will operate on any nullchars in a Lua string d:

d = d:gsub('%z', '')    -- remove all nullchars
d = d:gsub('%z.*', '')  -- truncate string at embedded nullchar
kmpm commented 8 years ago

@TerryE, I use a null char because that is what the XMODEM protocol that the uploader is based upon use and I have no real intention to change it. I'd rather implement the full protocol so that any old terminal software could transfer files.

But I do take responsibility for not RTFM and as far as I am concerned you can close the issue but it's not I who raised it.

TerryE commented 8 years ago

@kmpm Peter, I did leave the F out by the way :smile: and I could understand standardising on a nullchar as an EOL but not embedding it in a text string in quotes, but this is for you to deal with as you wish.

@gsker Gerry this highjack is a closed issue.

@Tertiush, we have to many other priorities to schedule your original request. Sorry.

kmpm commented 8 years ago

@TerryE I added the F because it's how I felt when I saw it and it's been dealt with already. Thanks.