kizniche / Mycodo

An environmental monitoring and regulation system
http://kylegabriel.com/projects/
GNU General Public License v3.0
3k stars 500 forks source link

Modularize sensor/device code #60

Closed kizniche closed 8 years ago

kizniche commented 9 years ago

This is an effort to reduce the bloat of mycodo.py and allow fewer requirements for its use, based on what is being used by it (modules). I can foresee, with many supported sensors/devices, Mycodo getting unmanageable and requiring the installation of extraneous software that will never be utilized by some users.

For instance, functions related to reading the AM2315 temperature/humidity sensor would all be in AM2315.py and the TCA9548A multiplexer in TCA9548A.py. They would only be loaded if that sensor/device is in use. That way software will not need to be installed if the device is not used, such as symbus- only used by the TCA9548A.

I'm struggling to find the best way to do this. Often, when I take on big tasks such as rewriting my code for better operability/modularity/readability, my first choice of a method is not necessarily the best course (but seldom is it the worst choice).

kizniche commented 9 years ago

@Cabalist What are your thoughts on this?

kizniche commented 9 years ago

I've done something similar for MycodoLog.py, MycodoGraph.py, and MycodoPID.py, but I'm not sure if this is the best approach. Additionally, these modules are always loaded by the daemon. Being able to selectively load modules is key to the proposed functionality.

kizniche commented 9 years ago

Some questions:

kizniche commented 9 years ago

More questions:

kizniche commented 9 years ago

It seems as though the only functions that should be modularized (or broken down to smaller modules from each) are read_t(), read_ht(), read_co2(), and read_press(). The reference to breaking down to modules from the function refers, for instance, to read_ht(), for which there are multiple different sensors (the other classes of sensors only have one sensor each) and would potentially need a module for the DHT and a module for the AM2315.

Cabalist commented 9 years ago

These are all good questions and a lot of what I'd like to help contribute. I don't have the answer to most of them at the moment.

I think the way to move forward is:

  1. Rename common code snippets to illustrative variable names. This will help us identify repeated code.
  2. Remove globals and add more functions to reduce repeated code.
  3. Add Classes for certain data types. I believe that we have probably 2 or 3 classes that will eventually form from the way that the data is accessed.
  4. Write tests. So we don't break anything along the way.
  5. Possibly move to SQLAlchemy to handle DB stuff. Could simplify some things.
  6. Decouple threading from functions and put it in a different module.

For example: read_t(), read_ht(), and read_co2() are all (sorta) the same function. They read from a sensor and return a value. I think we can combine them and then have some other functions that makes sense of the value they return. This will reduce code complexity by a great deal.

mycodograph.py is... unique. It probably would have been a better place for me to start since it is self contained. It is great that it is it's own module because it has a well defined start and end. We know exactly what It should do. No globals! However I think it is the largest single function I've seen. It definitely has the most arguments I've ever seen! haha. Breaking that up into pieces would be the next exercise. I bet that is 20-30 functions that all construct a small part of the whole and then you use another function to assemble those parts and return it to the caller.

I'm still trying to get all the code in my head. Does that all sound in line with what you are envisioning?

kizniche commented 9 years ago

That all sounds great. You made me laugh with

mycodograph.py is... unique.... No globals! I think it is the largest single function I've seen. It definitely has the most arguments I've ever seen!

I'm both honored and appalled

Seriously, though. mycodograph.py was the last big thing I created, to replace (but now complements) the static graph (server-side generated), as a client-side generated graph (with more features). It was my first attempt at moving completely away from using any globals. I just wanted to get it working at all costs, then go back and make it more efficient and readable. Slowly but surely I'll learn, and when I create things in the future, they'll be (more) logical from the start!

kizniche commented 9 years ago

Also, If you want to have a Mycodo login on one of my Pis (w hum/temp, co2, and pressure sensors), let me know how I can send you the connection/auth details.

Edit: Here's an easy way to contact me

kizniche commented 9 years ago

What's your background with programming/Python, if you don't mind me asking?

Cabalist commented 9 years ago

Email sent!

Eh. I own a company in Portland. We do python development with Point of Sale and Retail (Lightspeed POS and SAP) and Web Development (Flask mostly). I need some side projects that don't make me go crazy and I've been watching this one for awhile. It's fun. One of our programmers is a Biology Major at PSU (@etiology) and that makes it easier to work on this.

I've been programming for about 3-4 years. I read a lot of books and really like the Pythonic way of doing things. I didn't go to school for it but learned it out of necessity. Before this I worked as a screwdriver jockey at a bunch of random computer shops all over the US

Book Recommendation: You obviously know how to program so I'd recommend this book. It has a lot of very small tips that demonstrate good code usage. I think it helps writing better code in general. :)

Honestly I picked this project because it seems like you have been consistently working on it for awhile and didn't just get sick of it. It means that I am not screaming into the void when I make pull requests. :)

kizniche commented 9 years ago

Thanks! And I'm glad to see you like this project. Can you give an example of this so I might begin working on these?

Rename common code snippets to illustrative variable names

Cabalist commented 9 years ago

We already did it once with the relay active code. I'm looking to attack some of the other conditionals to see if we can find more examples. I'll probably send you some questions about what different pin combinations mean.

Cabalist commented 9 years ago

A good example:

if ((pid_ht_temp_set_dir[sensor_id] == 0 and pid_ht_temp_relay_high[sensor_id] != 0 and pid_ht_temp_relay_low[sensor_id] != 0) or (pid_ht_temp_set_dir[sensor_id] == -1 and pid_ht_temp_relay_high[sensor_id] != 0) or (pid_ht_temp_set_dir[sensor_id] == 1 and pid_ht_temp_relay_low[sensor_id] != 0)) and pid_ht_temp_or[sensor_id] == 0 and pid_ht_temp_down == 0 and sensor_ht_activated[sensor_id] == 1:
kizniche commented 9 years ago

That's a fun one!

kizniche commented 9 years ago

set_dir defines if the PID is going to regulate up (1), down (-1), or up and down (0). relay_high is the relay used to lower a condition (e.g. cooling device to lower temperature). relay_low is the relay used to raise a condition. This is making sure that whatever direction the PID regulation is going to go, there is a relay or relays set (not 0, set to the relay number) to do so.

temp_or is the override. When set to 1, the PID is not active. temp_down is used to shut down the PID thread (when set to 1). activated determines if the sensor is actively logging (1) or not (0).

Cabalist commented 9 years ago

Real quick PID means Pin ID or Process ID?

kizniche commented 9 years ago

Proportional Integral Derivative, as in PID controller, which is used to determine the amount of time the relay turns on for, based on the input condition (e.g. temperature), set point (i.e. desired temperature), and Kp, Ki, and Kd gains.