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

Remove moribund C ds18b20 module #2492

Merged
merged 1 commit into from
Apr 5, 2019
Merged

Remove moribund C ds18b20 module #2492

merged 1 commit into from
Apr 5, 2019

Conversation

nwf
Copy link
Member

@nwf nwf commented Sep 17, 2018

Just use Lua speaking OW (via C) instead.

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

@TerryE
Copy link
Collaborator

TerryE commented Sep 18, 2018

Moribund is dying rather than dead. The problem with not being actually dead is that some apps developers out there are still using it and if we withdrawn the module, and they then update their firmware -- for example to use LFS -- then they will suddenly discover that their applications no longer work unless the do some coding effort.

I feel what we really need is a retirement process for modules if they have been superseded or don't work and are no longer being maintained. This should include a warning / notice period of, say, 6 months between initial notice and any retirement that includes this caveat in the documentation.

In the case of the C version of the DS18B20 module, it is slow but it does work and if you are an application developer with a couple of thermos and it is fast enough, then why ask them to change their code?

@nwf
Copy link
Member Author

nwf commented Sep 18, 2018

I am content to leave this PR open for whatever retirement period is wished, but I object, aesthetically, to arbitrarily preserving and continuing to advertise no-longer-recommended solutions, especially when they appear to be completely superseded by new alternatives. Anyone updating to master to get LFS is already acknowledging that they will have to make changes to their code; I think the same is plausible for any update to master, but I suppose a cycle with deprecation warnings is fair.

Let me propose, then, that the next master drop include updated docs for the ds18b20 module pointing at this PR (I'll open another PR, I guess) and that the master drop after that include this removal of old code.

@marcelstoer
Copy link
Member

marcelstoer commented Sep 18, 2018

I feel what we really need is a retirement process for modules if they have been superseded

We kind of have that or had that. When we removed functions from C modules in the past we
a) added a "Deprecated, will be removed soon" console statement to the function body
b) then removed the function body except for the console statement
c) removed it entirely

over the course of a few months. And adjusted the docs accordingly of course.

@marcelstoer
Copy link
Member

Now would be last opportunity to agree on whether to eventually remove the C module and, therefore, to add a note to the documentation before the upcoming master drop. At the same time task a) from my above list could be tackled.

👍 for deprecating & removing from my side

@nwf nwf mentioned this pull request Dec 4, 2018
4 tasks
@nwf
Copy link
Member Author

nwf commented Feb 12, 2019

Now that #2581 has been merged for some months, should I revisit this for this upcoming master drop or leave it for the next one?

@marcelstoer
Copy link
Member

I personally favor the latter.

@marcelstoer marcelstoer added this to the Next release milestone Apr 5, 2019
@marcelstoer
Copy link
Member

@nwf could you please resolve the conflicts so we can add this to dev?

Just use Lua speaking OW (via C) instead.
@nwf
Copy link
Member Author

nwf commented Apr 5, 2019

Should be better now. Yes?

@TerryE TerryE merged commit b6cd2c3 into nodemcu:dev Apr 5, 2019
@nwf nwf deleted the rm-ds18b20-c branch July 5, 2019 12:08
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