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

Dynamic timers support #1424

Merged
merged 2 commits into from
Aug 2, 2016
Merged

Dynamic timers support #1424

merged 2 commits into from
Aug 2, 2016

Conversation

djphoenix
Copy link
Contributor

@djphoenix djphoenix commented Jul 28, 2016

Fixes #1416.

  • This PR is compliant with the contributing guidelines (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

I'll write docs soon, now will be great if anyone test this stuff.

@devsaurus
Copy link
Member

Great, I'll have a look.

@@ -135,20 +141,32 @@ static int tmr_now(lua_State* L){
return 1;
}

// Lua: tmr.register( id, interval, mode, function )
static int tmr_get( lua_State *L, int stack, timer_t *tmr ) {
if (lua_isuserdata(L, stack)) {
Copy link
Member

Choose a reason for hiding this comment

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

The code can emit a clearer error message if you query the element type first and then have luaL_argcheck() report a wrong type with something like "timer object or numerical ID expected".

@devsaurus
Copy link
Member

devsaurus commented Jul 28, 2016

The following code PANICs when the timer expires:

tim = tmr.create()
tim:register(5000, tmr.ALARM_SINGLE, function(t)
    print("timer expired")
    print(t:state())
    t:unregister()
end)

tim:start()
print(tim:state())

-- PANIC when object is destroyed before timer expired
tim = nil

Edit: No, it doesn't. My bad.

} else {
lua_rawgeti(L, LUA_REGISTRYINDEX, tmr->self_ref);
}
lua_call(L, 1, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Please stick to a consistent indentation style.
If unsure, follow the Lua style: 2 space indent + 1TBS + no trailing spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK

@djphoenix
Copy link
Contributor Author

Thanks @devsaurus for review. I will fix codestyle & logics and push it back today.

@devsaurus
Copy link
Member

Very much appreciated @djphoenix. It's been just some minor aesthetic things while functionality is great.

@djphoenix
Copy link
Contributor Author

Fixed all of your comments and began to write docs.

@djphoenix
Copy link
Contributor Author

All done. Also fixed self-unref if mode was SINGLE or SEMI and timer was not rearmed during callbeck.

Now this works OK without leaks:

function setTimeout(t, f)
    return tmr.create():alarm(t, tmr.ALARM_SINGLE, f)
end

setTimeout(1000, function()
    print("TICK!")
end)

@marcelstoer
Copy link
Member

I'm missing the deprecation notes for the old-style functions in code and docs.

@djphoenix
Copy link
Contributor Author

My propose is now only add new functions. We may think about deprecation process in another thread.

@marcelstoer
Copy link
Member

You need to think about deprecating the old/existing stuff whenever you add something new that serves as a replacement. Deprecating doesn't mean removing but adding a note to others indicating that existing functionality will or might be removed in the future. That way we give developers some slack to transition to using the new API. Also, we remove reluctance as an obstacle to actually removing old API.

@djphoenix
Copy link
Contributor Author

Ohm. OK. Can you point me to example/instruction of deprecation notes? So I'll do it some later cause AFK.

@marcelstoer
Copy link
Member

We have no guidelines for that, sorry. In essence the word "deprecated" in code and docs is all it takes e.g. http://nodemcu.readthedocs.io/en/latest/en/modules/node/#nodekey-deprecated.

@djphoenix
Copy link
Contributor Author

djphoenix commented Aug 1, 2016

@marcelstoer check it... And we really need guidelines to deprecation process, both for arguments or functions or whole modules.

@devsaurus
Copy link
Member

👍 from my side.

@marcelstoer marcelstoer merged commit f1afe7b into nodemcu:dev Aug 2, 2016
@djphoenix djphoenix deleted the dynamic-timers branch August 3, 2016 10:15
@eku
Copy link
Contributor

eku commented Aug 4, 2016

@djphoenix how long will it take till http://nodemcu.readthedocs.io/en/master/en/modules/tmr/ is updated?

@djphoenix
Copy link
Contributor Author

@eku you watching to master-branch docs, they're updating parallel with code at "master drops". Now this feature is only in dev branch, so look for dev docs ;)

@marcelstoer
Copy link
Member

@eku There's a fly-out menu in the lower left corner with which you, amongst others, can select the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants