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

Fix a number of issues with the gpio.pulse family of functions #2260

Merged
merged 9 commits into from
Feb 23, 2018

Conversation

pjsg
Copy link
Member

@pjsg pjsg commented Feb 12, 2018

Fixes #2254.

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

I'm now trying to make progress on my clock project and discovering some bugs in the existing code, and missing functionality. This PR adds the missing features and resolves the bugs.

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.

These are quite complex changes which call for corner-case testing rather than code review. Therefore I restrict myself to nitpicking 😉

// stack now contains: -1 => value; -2 => key; -3 => table
if (lua_type(L, -2) == LUA_TNUMBER) {
int pin = lua_tonumber(L, -2);
int value = lua_tonumber(L, -1);
Copy link
Member

Choose a reason for hiding this comment

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

What if value at -1 isn't LUA_TNUMBER or a convertible string?


if (strcmp(str, "delay") == 0) {
entry->delay = lua_tonumber(L, -1);
if (entry->delay < 0 || entry->delay > DELAY_LIMIT) {
Copy link
Member

Choose a reason for hiding this comment

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

I expect that this won't throw an error if value at index -1 is not convertible to a number. lua_tonumber() returns 0 in this case and the range check is happy.

luaL_checknumber() or an explicit test for lua_isnumber() if delay == 0 would detect nonsense.

Applies to further instances of lua_tonumber() below.

@pjsg
Copy link
Member Author

pjsg commented Feb 14, 2018

The changes aren't as big as they look as I moved the guts of one function into its own function so that I could use it elsewhere.

I did fix all the unchecked calls so that there is more error detection (and it actually found an error in my current application!)

@pjsg
Copy link
Member Author

pjsg commented Feb 16, 2018

This also includes a fix to the issue in https://stackoverflow.com/q/48790455/131929 -- I changed gpio.mode(, gpio.OUTPUT) to actually enable the driver.

@pjsg
Copy link
Member Author

pjsg commented Feb 18, 2018

I also addressed the comments in #2265

@marcelstoer marcelstoer merged commit 97e34ce into nodemcu:dev Feb 23, 2018
dnc40085 pushed a commit to dnc40085/nodemcu-firmware that referenced this pull request Mar 3, 2018
…cu#2260)

* Fix some subtle timing issues with gpio.pulse
* Add the pulse:update method
* Allow getstate to work on stopped pulsers
* Make gpio.mode(, gpio.OUTPUT) actually set the output mode
* Added some more documentation
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