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

Feat ph #1

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Feat ph #1

wants to merge 27 commits into from

Conversation

yurybotov
Copy link
Collaborator

PH library with calibration example

API_ru.md Outdated Show resolved Hide resolved
API_ru.md Outdated Show resolved Hide resolved
examples/calibrate/calibrate.ino Outdated Show resolved Hide resolved
examples/calibrate/calibrate.ino Outdated Show resolved Hide resolved
examples/using/using.ino Outdated Show resolved Hide resolved
src/troykaph.cpp Outdated Show resolved Hide resolved
examples/using/using.ino Outdated Show resolved Hide resolved
src/troykaph.cpp Outdated Show resolved Hide resolved
src/troykaph.h Outdated Show resolved Hide resolved
@yurybotov
Copy link
Collaborator Author

Please pay attention to the clarity of the dialog with the user in the calibrate example

// 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

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:

Suggested change
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();
...
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with small corrections

Copy link

@nomad605dis nomad605dis left a 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"

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

Copy link
Collaborator Author

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 Show resolved Hide resolved
src/troykaph.cpp Outdated
if (millis() - _nextMeasureTime > periodMilliseconds) {
_nextMeasureTime += periodMilliseconds;
// read value
(void)analogRead(_pin);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(void)analogRead(_pin);
(void)analogRead(_pin);

Why?

Copy link
Collaborator Author

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.

/*
This example demonstrate pH value reading
*/
#include "Arduino.h"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include "Arduino.h"
#include "Arduino.h"

Why are you include Arduino.h in the example?

Copy link
Collaborator Author

@yurybotov yurybotov Nov 12, 2020

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

@@ -0,0 +1,31 @@
/*

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

Copy link
Collaborator Author

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);
}
}

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?

Copy link
Collaborator Author

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.

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?

Serial.print(phMeter.read(), 1);
showingTime += INTERVAL;
}
}

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);
}

Copy link
Collaborator Author

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.

Copy link

@nomad605dis nomad605dis left a 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:

  1. There is a potentiometer on the board and why is it needed?
  2. 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TroykaPH phMeter(A4); // set used analog pin
TroykaPH phMeter(A0); // set used analog pin

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Serial.print(phMeter.read(), 1);
Serial.print(phMeter.read());
Serial.println();

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: ");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Serial.print("\nCurrent pH: ");
Serial.print("Current pH: ");

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;

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);

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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
value = value / 10;
value =/ 10;

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;

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

правые дополнить нулями.

*/

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);
}
}

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?

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.

3 participants