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

new pwm2 module #2747

Merged
merged 13 commits into from
May 25, 2019
Merged

new pwm2 module #2747

merged 13 commits into from
May 25, 2019

Conversation

fikin
Copy link
Contributor

@fikin fikin commented May 6, 2019

Fixes #2667, follow-up to #2669

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

an implementation capable of running up to few x100kHz with varying periods, sub-Hz and fraction of frequencies, support for all gpio pins and compatible with CPU160MHz.

@fikin fikin force-pushed the pwm2 branch 7 times, most recently from 7eb5794 to 45c1dc6 Compare May 6, 2019 19:59
@marcelstoer marcelstoer added this to the Next release milestone May 7, 2019
Copy link
Collaborator

@TerryE TerryE left a comment

Choose a reason for hiding this comment

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

See comments inline. These are mostly Lua and NodeMCU Lua style issues. I leave it to @devsaurus and @pjsg to comment on the realtime aspects.

app/modules/pwm2.c Outdated Show resolved Hide resolved
app/modules/pwm2.c Outdated Show resolved Hide resolved
app/modules/pwm2.c Outdated Show resolved Hide resolved
iPin->pulseInterruptCcounter = (sPin->pulseResolutions + 1) * sPin->resolutionInterruptCounterMultiplier;
}

static uint8_t getDutyAdjustment(const uint32_t duty, const uint32_t pulse)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Conditional operator considered unhealthy 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not catching this up, what do you mean?

app/modules/pwm2.c Outdated Show resolved Hide resolved
app/modules/pwm2.c Outdated Show resolved Hide resolved
app/modules/pwm2.c Outdated Show resolved Hide resolved
app/modules/pwm2.c Outdated Show resolved Hide resolved
docs/modules/pwm2.md Outdated Show resolved Hide resolved
docs/modules/pwm2.md Outdated Show resolved Hide resolved
@fikin fikin force-pushed the pwm2 branch 2 times, most recently from 89190dc to bda634d Compare May 8, 2019 21:06
@jmd13391
Copy link

jmd13391 commented May 9, 2019

@pjsg "We had to add support for multiple users of the hw_timer as people didn't read the documentation closely enough and would complain that using module A and module B at the same time would not work."

If I recall correctly ( #2494 #2497 ), multiple users functionality was added to allow gpio.pulse and pwm modules to work at the same time (this simultaneous functionality is critical to several of my projects). During the course of you providing this functionality, you discovered and corrected at least one bug and added clarification to the gpio.pulse docs. IMO, this had little to nothing to do with "not reading the documentation closely enough".

Losing the multi-user hardware timer functionality (especially with gpio.pulse / pwm) would be detrimental to several of my ESP8266 based products already out in the field.

@fikin
Copy link
Contributor Author

fikin commented May 9, 2019

@jmd13391 : i think no one is thinking of loosing current capabilities to share timer1 between modules. not me at least.
it is more about offering ability to either share it or reserve it exclusively for given module.

@TerryE
Copy link
Collaborator

TerryE commented May 10, 2019

This has failed Travis as a consequence of #2742. Have a look at the other modules, e.g. app/modules/file.c for how to declare ROTables.

docs/modules/pwm2.md Outdated Show resolved Hide resolved
app/modules/pwm2.c Outdated Show resolved Hide resolved
@HHHartmann
Copy link
Member

Just to be sure, this Module is not supposed to replace the PWM Module, right?
Maybe it would be a good idea then to rename it to something like PWM-fast or some other fitting name to distinguish it from the original PWM Module.
Also a common section to PWM and PWM2 documentation to help the user decide which one to pick would make sense.
Sorry for being so late ...

@fikin
Copy link
Contributor Author

fikin commented May 18, 2019

@HHHartmann : i've added a small section in pwm2 doc about comparison with pwm. let me know how useful you're finding it.

@fikin
Copy link
Contributor Author

fikin commented May 18, 2019

@TerryE : apparently i'm not getting how to return values from c to lua. what i am missing in get_timer_data?

@TerryE
Copy link
Collaborator

TerryE commented May 19, 2019

apparently i'm not getting how to return values from c to lua. what i am missing in get_timer_data?

The return value to any C method called from the Lua VM is not a status; it's a return argument count. Typically this matches the number of responses that your routine has pushed onto the Lua stack, but the VM will ignore / push extra nils if needed. So lpwm2_get_timer_data() should return 3 and so on.

@fikin
Copy link
Contributor Author

fikin commented May 25, 2019

ok, now to module is more or less ready.

@marcelstoer
Copy link
Member

Nit pick: you're missing newlines at the end of most files. GitHub reports this if you look at https://github.com/nodemcu/nodemcu-firmware/pull/2747/files

@marcelstoer marcelstoer mentioned this pull request May 25, 2019
4 tasks
@fikin
Copy link
Contributor Author

fikin commented May 25, 2019

@marcelstoer : ok, i've added missing new lines.
btw how documentation is tested?

@galjonsfigur
Copy link
Member

Documentation can be tested using mkdocs. You can install it using pip (for examplepip3 install --user mkdocs) then just use mkdocs serve command in project directory. But to show up in module list you will have to add link to pwm2 documentation in mkdocs.yml file and it should work.

@fikin
Copy link
Contributor Author

fikin commented May 25, 2019

@galjonsfigur : thanks, i've opted using "docker run -i --rm -v $(pwd):/docs -p 8000:8000 melopt/mkdocs serve" ...

@marcelstoer marcelstoer merged commit 5f43a41 into nodemcu:dev May 25, 2019
@marcelstoer
Copy link
Member

Documentation was correctly published at https://nodemcu.readthedocs.io/en/latest/modules/pwm2/ and the module is available in the cloud builder (given you select the dev branch).

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.

7 participants