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

Example Lua module for coroutining #2851

Merged
merged 3 commits into from
Jul 26, 2019
Merged

Example Lua module for coroutining #2851

merged 3 commits into from
Jul 26, 2019

Conversation

TerryE
Copy link
Collaborator

@TerryE TerryE commented Jul 24, 2019

Fixes #2848

  • 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/*.

See #2848 and supplied docs for explanation.

@TerryE TerryE requested a review from nwf July 24, 2019 10:26
@TerryE
Copy link
Collaborator Author

TerryE commented Jul 24, 2019

@galjonsfigur, perhaps you might to review this as well 😄

docs/lua-modules/cohelper.md Outdated Show resolved Hide resolved
docs/lua-modules/cohelper.md Show resolved Hide resolved
docs/lua-modules/cohelper.md Show resolved Hide resolved
lua_modules/cohelper/README.md Outdated Show resolved Hide resolved
lua_modules/cohelper/cohelper.lua Show resolved Hide resolved
docs/lua-modules/cohelper.md Outdated Show resolved Hide resolved
Copy link
Member

@galjonsfigur galjonsfigur left a comment

Choose a reason for hiding this comment

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

Thanks for review invite!
The module itself looks good! I used coroutines in some of my Lua an LuaJIT projects and they seem to be even better fitted for NodeMCU constraints. I'm glad that there will be a documentation and example how to use them. Great work!

docs/lua-modules/cohelper.md Outdated Show resolved Hide resolved
lua_modules/cohelper/cohelper.lua Show resolved Hide resolved
functionality as described in the [Lua RM §2.11](https://www.lua.org/manual/5.1/manual.html#2.11) and [PiL Chapter 9](https://www.lua.org/pil/9.html).

The NodeMCU Lua VM fully supports the standard coroutine functionality. Any
interactive or callback tasks are executed in the default thread, and the coroutine
Copy link
Member

Choose a reason for hiding this comment

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

I think it is no good calling this a thread. As the PiL also points out it is only similar. A thread is executed concurrently. So taffeta will get confused. Maybe use "execution context" or whatever.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A thread is executed concurrently.

Gregor, I must disagree on this one. Until we had multi-processor CPUs. threads would never be executed concurrently, since there was only one processing unit. The WP thread article has a nice definition "a thread of execution is the smallest sequence of programmed instructions that can be managed independently by a scheduler". Features such as concurrence and pre-emption are more emergent benefits of this independence. In our Lua world the machine is the Lua VM and this must, like node.js, run in a single OS thread.

What I am trying to do is to explain how this works to the average IoT developer. If you can suggest better wording then I'll adopt this.

Copy link
Member

Choose a reason for hiding this comment

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

The term to use, I think, is "cooperative multi-threading", assuming I am correct that, on our C substrate, a Lua task will run to completion, without preemption and without concurrent access to the Lua VM and its heap, even if tasks are being dispatched by multiple threads. I'm not sure that this module's documentation is the right place to spell it out; perhaps node.task.post() is the right place and this should simply cite that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've used stack instead of thread.

statements, and since the order of SDK tasks is indeterminate, the application
must take care to handle any ordering issues. This particular example uses
the `node.task.post()` API with the `taskYield()`function to resume itself,
so the running code can simple call `taskYield()` at regular points in the
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about my English but should it be"simply"


### Release

Not required. All resoruces are release on completion of the `exec()` method
Copy link
Member

Choose a reason for hiding this comment

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

released

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And resources

Not required. All resoruces are release on completion of the `exec()` method

## `cohelper.exec()`
Execute a function which is wrapper by a coroutine handler.
Copy link
Member

Choose a reason for hiding this comment

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

wrapped

`require("cohelper").exec(func, <params>)`

#### Parameters
- `func`: Port number for HTTP server. Most HTTP servers listen at port 80.
Copy link
Member

Choose a reason for hiding this comment

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

What?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, cut and past crud. Needs removing.

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 25, 2019

I will leave it for another day or for any other feedback before pushing the updates.

@TerryE TerryE merged commit 6d9c5a4 into nodemcu:dev Jul 26, 2019
@marcelstoer marcelstoer added this to the Next release milestone Jul 27, 2019
@marcelstoer
Copy link
Member

I just aded it to the milestone...

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.

5 participants