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

A generic fifo and fifosock wrapper, under telnet and http server #2650

Merged
merged 4 commits into from
Feb 16, 2019

Conversation

nwf
Copy link
Member

@nwf nwf commented Feb 6, 2019

  • This PR is for the dev branch rather than for master.
  • 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/en/*.

This pulls some things that I've had brewing in https://github.com/AcmeTensorToys/esp-lua_core for a while up to a more wide audience. It is mostly a refactoring of @TerryE's two-level fifo trick from the telnet server example into a more general module.

While here, though, move the http lua server example over to fifosock to guarantee in-order sending and waiting for all bytes sent to be queued; see #2647.

This particular repackaging is almost entirely untested; there was some churn in liberating things from my particular environment. Feedback is more than welcome, and I will be doing some testing as time allows, but I wanted to solicit early commentary. :)

Copy link
Member

@HHHartmann HHHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sure took some time to get a catch on the fifo and fifosock logic.
But now I think I like it. Will use in other projects. Especially the fifo part.

Thanks for this great contribution.

As a general remark I prefer more speaking names so I see immediately what is meant and don't need to read the code first. (which is hard without clear naming again)

lua_modules/fifo/fifosock.lua Outdated Show resolved Hide resolved
lua_modules/fifo/fifosock.lua Outdated Show resolved Hide resolved
lua_examples/telnet/telnet.lua Outdated Show resolved Hide resolved
lua_modules/http/httpserver.lua Outdated Show resolved Hide resolved
@nwf
Copy link
Member Author

nwf commented Feb 10, 2019

Thanks much for the feedback, @HHHartmann, and I'm glad to hear you think the modules are useful. :)

@nwf
Copy link
Member Author

nwf commented Feb 10, 2019

Revised commits improve documentation a whole lot (by adding .md files, too) and now include the fifosocktest example and test suite.

@marcelstoer
Copy link
Member

There's a conflict in lua_examples/telnet/telnet.lua.

The new .mds need to be registered in mkdocs.yml.

@nwf
Copy link
Member Author

nwf commented Feb 14, 2019

OK, here's another stab. This one...

  • fixes the merge conflict
  • wires the dox into the yaml file
  • has undergone some local testing
  • revises the http server in light of @HHHartmann's feedback
  • now includes a delayed-uncorking optimization in fifosock that I think will do us good but I'd like @TerryE to weigh in, if he can.

mkdocs.yml Outdated Show resolved Hide resolved
One occasionally wants a generic fifo, so here's a plausible
implementation that's reasonably flexible in its usage.

One possible consumer of this is a variant of TerryE's two-level fifo
trick currently in the telnetd example.  Factor that out to fifosock for
more general use.
If we just pushed a little bit of data into a fifosock that had idled,
wait a tick (1 ms) before transmitting.  Hopefully, this means that
we let the rest of the system push more data in before we send the first
packet.  But in a high-throughput situation, where we are streaming data
without idling the fifo, there won't be any additional delay and we'll
coalesce during operation as usual.

The fifosocktest mocks up enough of tmr for this to run, but assumes
an arbitrarily slow processor. ;)
Switch to fifosock for in-order sending and waiting for everything to be
sent before closing.

Fix header callback by moving the invocation of the handler higher
@nwf
Copy link
Member Author

nwf commented Feb 16, 2019

Some minor tweaks in response to out of band commentary. Notably, the fifo file now returns a table containing the fifo constructor as .new rather than returning the constructor function itself. The documentation has been further improved as well.

@marcelstoer marcelstoer merged commit dcc1ea2 into nodemcu:dev Feb 16, 2019
nwf added a commit to nwf/nodemcu-firmware that referenced this pull request Feb 16, 2019
@nwf nwf mentioned this pull request Feb 16, 2019
4 tasks
nwf added a commit to nwf/nodemcu-firmware that referenced this pull request Feb 16, 2019
Fixes to nodemcu#2650:

- Convert fifosock to returning tables containing ctors
- Improve docs
- Add a missed :on("sent", nil) in the http server
TerryE pushed a commit that referenced this pull request Feb 17, 2019
Fixes to #2650:

- Convert fifosock to returning tables containing ctors
- Improve docs
- Add a missed :on("sent", nil) in the http server
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