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

HTxx Module #222

Closed
wants to merge 8 commits into from
Closed

HTxx Module #222

wants to merge 8 commits into from

Conversation

CanyonCasa
Copy link
Contributor

Improved generic humidity and temperature sensor module. Streamlines code and supports multiple sensor types.

@gfwilliams
Copy link
Member

Thanks - sorry it's taken a while to reply.

It seems like this code is kind of compatible (or would be if it wasn't for init vs connect) with the existing DHT11 module? Is there any reason why I shouldn't merge this into there? I really like to avoid having two conflicting modules.

Also, don't know what you think, but actually appending the data to a string is faster than the shift+array access, and then there's less nastiness and shifting to get the data out.

For example it could be :

ht.data = "";

// in the watch
ht.data+=(dt>0)?1:0;

// extracting data after:
parseInt(ht.data.substr(2,5),2);

gfwilliams added a commit that referenced this pull request Feb 4, 2016
@CanyonCasa
Copy link
Contributor Author

Sorry for my delayed response as well. I've been occupied by a family emergency.

Thanks -- I adapted your suggestion for using a string and made another significant reduction in code size to ~55% of original size. (I have not yet uploaded new code to Github.)

Regarding the compatibility... I understand the conflict as I have written code for ~40 years. It's a constant balancing act between maintaining backward compatibility and not stifling innovation and improvements. I debated it myself and ended up purposely changing from *connect * to *init * because I thought the improvements outweighed the consequences as 1.) its a tiny bit smaller code (3 bytes x2, OK, that's a stretch), 2) makes more sense to me as this is not a network object that connects to anything, 3) I also changed the output object keys -- t vs temp to be consistent with rh and err vs checksumError to be more generic and added other keys as well for the DHT11 and raw data, 4) the DHTxx modules aren't interchangeable with other humidity sensors, and 5) its pretty minor code (not part of the Espruino code base) and if a user changed from one to another would not be a major impact and little effort in exchange for the amount of code space gained.

So changing init to connect, alone, won't make the module truly compatible. If code space weren't such a precious commodity I would have just embellished the existing code with new features and not sacrificed compatibility. I would not want too see a precedence that restricts all future module submissions to live with the constraints of the first submission.

Alternatively, what do you think about instead adding a disclaimer message to the beginning of the markdown declaring the code incompatible with other DHTxx modules? Then users can knowingly choose between code at the expense of compatibility.

I have only submitted code to Github once before on another project some time ago. Could you give me some direction here? Do I update the code (i.e. string changes) and make another pull request? Or do you just close this out as rejected?

@gfwilliams
Copy link
Member

No problem, hope everything is ok now!

With GitHub, it looks like you created a pull request from your the master branch of your repository (the default)... so I think all you need to do is to update your GitHub repository and this pull request will automatically get updated.

However, I don't want to waste your time on this. I'd like to update both the DHT11 & 22 modules with a lot of your changes for size/performance, and also add strings to reduce their sizes - I posted some code on the forum here.

But I really don't want to add a new module for this...

  • Whether it was the best word or not, connect is used in almost every Espruino module to date, and it's just what people expect to use now.
  • This module isn't really 'multipurpose'. If you plug a DHT11 in, the rh and t readings will just be plain wrong, and you have to know to use rh11 and t11. Similarly if you write your code for DHT11 and plug a DHT22 in, it'll just fail.
  • Because it's trying to support both, it's still not as memory efficient as it could be if it supported just DHT11 or DHT22. As I understand it, the other sensors should still work fine with the DHT22 module.

I would not want too see a precedence that restricts all future module submissions to live with the constraints of the first submission.

Well, no - some modules do change over time, but IMO a lot of what makes Espruino easy is that different modules can be used roughly the same way. You just know that you require it, connect to pins, and then do stuff with what you get back.

Also, I really want to avoid duplicates unless it's totally required. Folks can come to the Espruino site, search for some bit of hardware, find just one (working) way to control it, and use that. I think it'd be a mistake if it was totally open like NPM, where you have 15 modules to do one task, at least 10 of which are badly documented and unmaintained.

Also, I've been doing this for 4 years now. The majority of people come along, submit some stuff, and lose interest maybe a year later - and I'm stuck having to maintain what's left.

I'm not trying to restrict what people use - you can actually just use require('http://.../DHTxx.js') and use whatever modules you want, or you can change the setting in the Web IDE and use your own source for all Espruino modules. I just want to keep Espruino clean and easy to use - and to do that I've got to try and keep things simple for everyone.

Updated for changes to HTxx.js
Streamlined to support DHT22+ with more Node-like signature.
Modified version of HTxx.js that should be direct replacement for DHT22.js
@CanyonCasa
Copy link
Contributor Author

No problem and not a waste of time, just a learning experience. I appreciate the maintenance loop. Been there and it can suck the life out of you. I updated my files for more of what I wanted but also added DHTxx.js that should be a direct compatible replacement for the DHT22.js module. It checked out with my DHT22 sensor and gave the same results. Use any or all of what you want from it.

@gfwilliams gfwilliams closed this in 52752a5 Mar 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants