-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat ph #1
base: main
Are you sure you want to change the base?
Conversation
red-faced Co-authored-by: AlexErAmp <[email protected]>
Co-authored-by: AlexErAmp <[email protected]>
Co-authored-by: AlexErAmp <[email protected]>
Co-authored-by: AlexErAmp <[email protected]>
Co-authored-by: AlexErAmp <[email protected]>
Co-authored-by: AlexErAmp <[email protected]>
Please pay attention to the clarity of the dialog with the user in the calibrate example |
examples/using/using.ino
Outdated
// phMeter.begin(correction,zeroShift); // if you have it (use calibrate.ino) for it | ||
|
||
Serial.begin(9600); | ||
showingTime = millis() + INTERVAL; // show result once per 3 seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't mean it:) Use the classic millis() pattern:
showingTime = millis() + INTERVAL; // show result once per 3 seconds | |
constexpr uint32_t interval = 3000; | |
void setup() { | |
... | |
showingTime = millis(); | |
} | |
void loop() { | |
... | |
if (millis() - showingTime >= interval) { | |
showingTime = millis(); | |
... | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with small corrections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made edits and comments
src/troykaph.cpp
Outdated
* License: GPLv3, all text here must be included in any redistribution. | ||
*/ | ||
|
||
#include "TroykaPH.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arduino IDE does not compile example:
error: troykaph.h: No such file or directory
Rename files from troykaph.cpp
and troykaph.h
to TroykaPH.cpp
and TroykaPH.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/troykaph.cpp
Outdated
if (millis() - _nextMeasureTime > periodMilliseconds) { | ||
_nextMeasureTime += periodMilliseconds; | ||
// read value | ||
(void)analogRead(_pin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(void)analogRead(_pin); | |
(void)analogRead(_pin); |
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Googling showed that in cases where greater accuracy is needed, you must first make an empty read so that the internal analog multiplexer switches to the desired channel, and only after a pause, when the internal capacitor voltage value stabilizes, read real data.
examples/using/using.ino
Outdated
/* | ||
This example demonstrate pH value reading | ||
*/ | ||
#include "Arduino.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "Arduino.h" | |
#include "Arduino.h" |
Why are you include Arduino.h
in the example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My compiler (under linux) swore at millis
examples/using/using.ino
Outdated
@@ -0,0 +1,31 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an example should be called simpleReadPH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done.
Serial.println("Recalibration is needed if you change Arduino board to another.\n\n"); | ||
delay(5000); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you use microcontroller registers? After all, the calibration example won't work with the Arduino Due and ESP8226 boards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the standard Arduino API, there is no way to read the voltage at the internal Vref. There is only a way to use it instead of Aref. Now, as you can see, there is a way only for different AVRs. The options for other controllers can be worked on in the next version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to write from which boards support the calibration process. And there are no other options easier than using Vref?
examples/using/using.ino
Outdated
Serial.print(phMeter.read(), 1); | ||
showingTime += INTERVAL; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How the example works? Why not do it in our standard way?
void loop() {
// Read the sensor data values PH
phMeter.read();
// Receive the stored PH
float valuePH = phMeter.getPH();
// Print results
Serial.print("PH Value = ");
Serial.println(valuePH);
// Wait 1000 ms
delay(1000);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes no sense to constantly poll the pH sensor. First, its meaning changes very slowly. Secondly, it's a long time, there are a lot of delays when reading. Therefore, it makes sense to read from time to time, and return the last read value upon request. As here.
Co-authored-by: Igor <[email protected]>
Git dont want rename capital change only
This reverts commit f2fc42d.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a new batch of edits.
Please also add:
- There is a potentiometer on the board and why is it needed?
- For calibration, you need two buffer solutions, one of which should be about 7PH.
/* | ||
* This file is a part of TroykaPH library. | ||
* | ||
* Product page: https://amperka.ru/product/zelo-folow-line-sensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Product page: https://amperka.ru/product/zelo-folow-line-sensor | |
* Product page: https://amperka.ru/product/troyka-ph-sensor |
// Include library | ||
#include "TroykaPH.h" | ||
|
||
TroykaPH phMeter(A4); // set used analog pin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TroykaPH phMeter(A4); // set used analog pin | |
TroykaPH phMeter(A0); // set used analog pin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using pin A0, it is used everywhere by default.
if (currentTime - lastShowingTime > INTERVAL) { | ||
lastShowingTime = currentTime; | ||
Serial.print("\nCurrent pH: "); | ||
Serial.print(phMeter.read(), 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serial.print(phMeter.read(), 1); | |
Serial.print(phMeter.read()); | |
Serial.println(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two decimal places is normal for solutions of PH.
|
||
if (currentTime - lastShowingTime > INTERVAL) { | ||
lastShowingTime = currentTime; | ||
Serial.print("\nCurrent pH: "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serial.print("\nCurrent pH: "); | |
Serial.print("Current pH: "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Arduino users, it is better to do a newline at the end of the output:
Serial.println()
constexpr float idealVcc = 5.0; | ||
constexpr float minPh = 0.0; | ||
constexpr float maxPh = 14.0; | ||
constexpr float phHalfRangeInVolts = 0.82; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest putting all the constants into a file TroykaPH.h
public: | ||
TroykaPH(uint8_t pin); | ||
void begin(float correction = 1.0, float zeroLevel = 2.0); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there an empty line between functions?
for (uint8_t i = 0; i < 10; i++) { | ||
value += (float)analogRead(_pin) * 5.0 / 1024.; | ||
} | ||
value = value / 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value = value / 10; | |
value =/ 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduce the expression?
constexpr float adcResolution = 1024.; | ||
constexpr float meanReferenceVoltage = 1.1; | ||
constexpr long toMillivolts = 1000L; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, I suggest putting all the constants into a file TroykaPH.h
правые дополнить нулями. | ||
|
||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using only one language in comments.
Serial.println("Recalibration is needed if you change Arduino board to another.\n\n"); | ||
delay(5000); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to write from which boards support the calibration process. And there are no other options easier than using Vref?
PH library with calibration example