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

Pwm2 #2669

Closed
wants to merge 3 commits into from
Closed

Pwm2 #2669

wants to merge 3 commits into from

Conversation

fikin
Copy link
Contributor

@fikin fikin commented Feb 14, 2019

Fixes #2667.

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

Lets review the code and think of changes if needed.

@pjsg
Copy link
Member

pjsg commented Feb 14, 2019

Can you restage this to the dev branch please?

@marcelstoer marcelstoer changed the base branch from master to dev February 14, 2019 22:33
@galjonsfigur
Copy link
Member

The only problem I can see is that ESP8266_new_pwm project is GPL licensed and nodemcu-firmware is MIT. I don't know much about licenses but this could be a potential problem. Other than that this PR is very good idea - better PWM can be used to make more advanced things.

@fikin
Copy link
Contributor Author

fikin commented Feb 17, 2019

huh, that would certainly become a bother.
i ignored it until now but looking at some previous discussions for license change (StefanBruens/ESP8266_new_pwm#16), feel kind of not very optimistic...
is rewrite the only way out of this?

@marcelstoer
Copy link
Member

ESP8266_new_pwm project is GPL licensed

Yep, that's a no-go :(

@nwf
Copy link
Member

nwf commented Feb 18, 2019

IANAL, IANYL, TINLA, &c, but: because nodemcu is licensed MIT, it can always be mixed in to GPL code, and the result shared under the GPL (just as it can be used as part of proprietary products). There is nothing stopping a developer or end-user from mixing ESP8266_new_pwm and nodemcu, as this PR does, but the result will be that the whole thing is GPL and must be distributed as such.

None of this is meant to say that we should merge this PR -- indeed, I think the project will only accept such a module if it can also be MIT licensed -- merely to say that if the lack of interesting PWM support is a problem for down-stream users right now, they are free to do the integration themselves and take on the different licensing regime.

@fikin
Copy link
Contributor Author

fikin commented Feb 19, 2019

i'll re-write it under mit license so that it can make it to the master. would that be ok with you guys?

@nwf
Copy link
Member

nwf commented Feb 19, 2019

Be careful when rewriting that you do not implicitly create a derivative work; just typing in new code with the old code on screen is probably not legally sufficient. I think past efforts on these lines have involved two people, one of whom documented the old thing's behavior as pertained to the wider system, and the other looked only at that documentation to produce the new software. Again, though, I am not an IP lawyer.

@fikin
Copy link
Contributor Author

fikin commented Feb 19, 2019

fair enough. i've seen the code but i't rather do it my own way.
btw, just to confirm, is using hw_timer1 for pwm purposes ok from nodemcu pov?

@fikin fikin deleted the pwm2 branch March 10, 2019 14:17
This was referenced May 6, 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