Update control.py by limbotempo · Pull Request #7 · jaydenleong/piponic

limbotempo · 2021-02-24T05:17:40Z

No description provided.

cbrrrry

I like what you've done here!
A few general comments -

Please add more descriptive comments to each of your functions and modules.
Please also remove the unnecessary components from each object (for the pH component, please remove the leak sensor stuff, etc).
Can this code run when there are separately running sensor modules? For example, if the piponic.py loop is reading the pH, is there no problem running both? For both threading and duplicate sensor instantiation reasons.

Before we merge, it's probably a good idea to rebase this branch to main. We can meet to talk about what this looks like once you've addressed my comments. Thanks :)

cbrrrry · 2021-03-15T22:12:36Z

src/control.py

+        self.init_leak()
+        self.pH_sensor = 0
+        self.init_pH()
+	self.x = 4.7


What does the variable self.x represent?

It should be a debugging variable, I will remove this.

cbrrrry · 2021-03-15T22:13:15Z

src/control.py

+    def __init__(self):
+        self.ads=0
+        self.init_i2c()
+        self.leak_sensor = 0


Does the ph_control() object need to have the leak sensor defined? Can this be removed?

Yes, I believe this can be removed. I will remove it for now and comment on the code what's removed, since the threading sometimes getting wired in init functions.

cbrrrry · 2021-03-15T22:14:23Z

src/control.py

-
-
+    def calibrate(self):
+	x = read


This is a placeholder function right?

cbrrrry · 2021-03-15T22:15:37Z

src/control.py

+            self.pH_sensor= AnalogIn(self.ads,ADS.P2)
+            pH_voltage = self.pH_sensor.voltage
+            pH = 4.7 +(pH_voltage-1.65)*(-3.3)		#test self.x here
+            if (pH<=8.2):				#test self.desired_ph here


Shouldn't this be self.desired_pH?

Yes! This value was used for Lynes testing, I will fix it.

cbrrrry · 2021-03-15T22:16:29Z

src/control.py

+            time.sleep(2)
+            print('loop start')
+            self.pH_sensor= AnalogIn(self.ads,ADS.P2)
+            pH_voltage = self.pH_sensor.voltage


Getting the pH could be done in a cleaner way by calling self.read_pH()

I 've edited it! correct me if I'm wrong

cbrrrry · 2021-03-15T22:20:06Z

src/control.py

-#    sensors.read_pH()
-# Reference provided at: https://learn.adafruit.com/adafruit-4-channel-adc-breakouts/python-circuitpython
+
+#----------------------------------------------Template--------------------------------------------------#


I like this Template description! Would you please be able to add all of the example code together at the end of it?

cbrrrry · 2021-03-15T22:22:48Z

src/test.py

+            cycle = cycle + 0.5
+            print ('5 Second Thread cycle+1.0 - ')
+
+#Create Class


Cool! I like this threading approach! Is there a way to modularize the delay? For example, so that the user can control how often the system is polling the sensors? How quickly do you guys think the pH control system needs to update/operate for good response?

I believe our client mentioned this in meeting, and the pH controlling system should operate every 24hrs. The delays here are used for testing right now

cbrrrry · 2021-03-15T22:23:56Z

src/control.py

+        pH = 7.7 +(pH_voltage-1.65)*(-3.3)
+        return pH
+
+    def test_wl(self):


So test_wl(self) is the control loop for the water level?

Yes exactly!

Great! If you can just add a brief description, so that future users know what this module's function is.

cbrrrry · 2021-03-15T22:31:06Z

src/test.py

+#Start Thread 
+FiveSecondThread.start()
+
+#Create Class


So will we be able to use the threading approach in the main loop of Piponic.py without blocking things up?

Yes, but not in test.py. The one that's working should be in control.py in control branch.

cbrrrry · 2021-03-15T22:33:49Z

src/control.py

    def init_leak(self):
        self.leak_sensor= AnalogIn(self.ads,ADS.P0)
-
-    def water_level_init(self):


The water level sensor does not use the ADC. Please review the recently merged water_level.py on the main branch for code.

cbrrrry · 2021-03-15T22:43:20Z

src/control.py

+            self.pH_sensor= AnalogIn(self.ads,ADS.P2)
+            pH_voltage = self.pH_sensor.voltage
+            pH = 4.7 +(pH_voltage-1.65)*(-3.3)		#test self.x here
+            if (pH<=8.2):				#test self.desired_ph here


Have we considered what a more complex control system could look like here? Maybe at least a PI system?

I'm not sure what do you mean by the Pi system. However, I do play to optimize this equation here. Also, I think the users cannot avoid adjusting the pH system themselves.

What I meant by "PI" system is a "proportional-integral" control system. This is basically a controller that turns the pump on more if the current pH is far away from the desired pH, and less if it is close to the desired pH.

I think what you suggest are possible, I will try to work on this!

Version after reviewing Carson's comments

cbrrrry · 2021-03-17T16:50:19Z

src/control.py

 # Notice that the FUNCTION_NAME should be the function in the class (defined before), and is the one that you would like it to iterate
 # 4.3. Start the thread, for example: NAME_OF_THREAD.start()
+#-----------------------------------------------CODE COMPLETE---------------------------------------------#
+# class ph_control(object):


Isn't ph_control() already defined as a class? Correct me if I'm wrong, but don't we need to instantiate an object of this class similar to you do below. I'm guessing an example might look like this:
import src.control as control pH_controller = control.ph_control() pH_thread = Thread(target = pH_controller.test()) pH_thread.start()
I like that you've made your example more general than mine, I just wanted to try and show the most important use case.

Do threads only work for single functions?

Here the aim of define ph_control(object) should just be an example for the users indicate that they should create a class. I will edit and try to make my sample code more obvious.
For your last question, I think yes, I test before and the threads can only work for one single function within one class

limbotempo added 30 commits February 23, 2021 21:17

Update control.py

20cca21

Delete control_phpump.py

77985ab

Update control.py

b681e36

Update control.py

62571c3

Update control.py

194a2fd

Update control.py

930133e

Update control.py

7eca15f

Update control.py

20f13b4

Update control.py

9a0837e

Update control.py

a9b0779

Update control.py

0ff6716

Update control.py

818126c

Update control.py

f223974

Update control.py

7e4bf2e

Update control.py

71bf83c

Update control.py

f466cae

Update control.py

7433728

Update control.py

d93df0c

Update control.py

b1e98ea

Update control.py

69c5f5f

Update control.py

33697e1

Update control.py

3b34878

Update control.py

74893dd

Update control.py

b60fc14

Update control.py

ec362bd

Update control.py

b07d4ac

Create test.py

86ecabc

Update test.py

7945c3f

Update test.py

a9bbfc8

Update control.py

8f4dcd8

limbotempo and others added 20 commits March 4, 2021 20:00

Update control.py

381f790

Update control.py

af6a823

Update control.py

0967cda

Update control.py

ee49ede

Update control.py

35a5f50

Update control.py

aa983d9

Update control.py

20b8b7f

Update control.py

7156d02

Update control.py

d7434e1

Update control.py

97d31b6

Update control.py

08529df

Update control.py

ebbf598

Update control.py

6e67b7b

Update control.py

21de782

Update control.py

02d9df3

Update control.py

899bf4f

calibrated ph sensor

888e77b

Update control.py

9488f7a

Update control.py

25e2b8b

Update control.py

ca7626c

limbotempo requested a review from cbrrrry March 12, 2021 16:24

LynesChan added 2 commits March 14, 2021 15:24

Update control.py

191e79e

Update control.py

c9286ea

cbrrrry reviewed Mar 15, 2021

View reviewed changes

Update control.py

f98237c

Version after reviewing Carson's comments

cbrrrry reviewed Mar 17, 2021

View reviewed changes

cbrrrry and others added 3 commits March 17, 2021 11:24

added Calibration function

343855f

Update control.py

3316656

Update control.py

8c20930

Conversation

limbotempo commented Feb 24, 2021

Uh oh!

cbrrrry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants