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

Expunge integer timers #2603

Merged
merged 1 commit into from
Jan 22, 2019
Merged

Expunge integer timers #2603

merged 1 commit into from
Jan 22, 2019

Conversation

nwf
Copy link
Member

@nwf nwf commented Jan 8, 2019

Fixes #2601.

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

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

Copy link
Contributor

@djphoenix djphoenix left a comment

Choose a reason for hiding this comment

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

Code LGTM, but can't test right now

docs/en/modules/tmr.md Outdated Show resolved Hide resolved
Copy link
Member

@devsaurus devsaurus left a comment

Choose a reason for hiding this comment

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

The functions in tmr_map[] should be removed as well - tmr.alarm() and friends don't exist anymore.

Regarding the docs: I'd suggest to group the timer functions as a module subsection. They're currently mixed with tmr.wdclr() etc. This was ok for the old API, but a dedicated section would improve clarity (e.g. https://nodemcu.readthedocs.io/en/dev/en/modules/net/#netsocket-module).

}
return 0;
timer_t t = (timer_t)luaL_checkudata(L, stack, "tmr.timer");
if (t == NULL)
Copy link
Member

Choose a reason for hiding this comment

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

Cosmetic: Not needed since luaL_checkudata() errors out if supplied with a wrong/no userdata.

Copy link
Member Author

Choose a reason for hiding this comment

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

The documentation at https://www.lua.org/pil/28.2.html disagrees:

Finally, luaL_checkudata checks whether the object at the given stack position is a userdatum
with a metatable that matches the given name. It returns NULL if the object does not have the
correct metatable (or if it is not a userdata); otherwise, it returns the userdata address.

@nwf
Copy link
Member Author

nwf commented Jan 10, 2019

I suppose we could eliminate tmr.alarm() in favor of the t:alarm() form only, but I didn't see any particular reason to require it. If there's strong opinion, I'll go ahead and do that.

As to reformatting the docs, sure, makes sense.

@marcelstoer
Copy link
Member

We have been "promoting" the use of tmr.create():alarm() through the examples in the documentation ever since OO timers were introduced. However, I'm indifferent as to whether to drop tmr.alarm() now.

@jmd13391
Copy link

jmd13391 commented Jan 10, 2019

We have been "promoting" the use of tmr.create():alarm() through the examples in the documentation ever since OO timers were introduced. However, I'm indifferent as to whether to drop tmr.alarm() now.

Expunging the static timers is already forcing Lua Developers to rework existing application code if we want to take advantage of the upcoming firmware version. Might as well drive the final tmr.alarm() nail into this coffin now and be done with it in a single pass. There is a significant time and labor cost incurred each time you take an existing design through this rework process.

@devsaurus
Copy link
Member

We have been "promoting" the use of tmr.create():alarm()

This is not affected.
And note that I referred to alarm() just as the representative of all the timer tmr.* functions.

However, I'm indifferent as to whether to drop tmr.alarm() now.

Which benefit do you see for supporting

local tobj = tmr.create()
tmr.alarm(tobj, 1000, tmr.ALARM_SINGLE, function() print("ping") end)

in addition to

local tobj = tmr.create()
tobj:alarm(1000, tmr.ALARM_SINGLE, function() print("ping") end)

Easier migration of application code?

The firmware doesn't offer the former variant for other object types (e.g. sockets).

@marcelstoer
Copy link
Member

And note that I referred to alarm() just as the representative of all the timer tmr.* functions.

Ahh, got it.

Which benefit do you see for supporting
[...]
Easier migration of application code?

No benefit at all, a misunderstanding, sorry. If it's a breaking change anyhow it doesn't matter much if that variant is gone. So, 👍 for removing those functions.

@marcelstoer
Copy link
Member

I think once the admonition is updated and the conflicts in tmr.md are resolved we're good for merge.

@nwf
Copy link
Member Author

nwf commented Jan 18, 2019

I think this makes us good to go; let's see what travis says.

@marcelstoer marcelstoer merged commit 0e89fb2 into nodemcu:dev Jan 22, 2019
@nwf nwf deleted the no-int-tmr branch January 22, 2019 23:00
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Feb 10, 2019
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Feb 10, 2019
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Feb 10, 2019
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Feb 10, 2019
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Feb 10, 2019
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Feb 10, 2019
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Feb 12, 2019
vsky279 pushed a commit to vsky279/nodemcu-firmware that referenced this pull request Feb 12, 2019
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