olehs / PZEM004T

Arduino communication library for Peacefair PZEM-004T Energy monitor
MIT License
226 stars 114 forks source link

Replace usages of long with int32_t / uint32_t and make power()/energy() return int32_t #50

Open djGrrr opened 6 years ago

djGrrr commented 6 years ago

Every other integer type is specified with intXX_t / uintXX_T, but the 32bit type is using "long", this should be changed so that it is consistent, and properly support potentially differing sizes on different platforms.

It also make sense to change the return types of the power() and energy() functions to return int32_t instead of float, they don't support decimal values.

olehs commented 6 years ago

Hi, @djGrrr. Thanks for your PR. I can accept your changes to'timeout' type, but I can't accept energy()/power() changes. My opinion is that these are physical values that do have fractional part in real world, so it's better to keep them float, as they can be a part of other calculations (like power factor etc.).

djGrrr commented 6 years ago

Hi @olehs I actually don't agree that it's better to keep them float for a number of reasons.

  1. You will generally define any variable such as power factor as float and therefore the result will still be a float.
  2. It doesn't actually matter, if you don't define or cast the type, if you do calculations with a float var in it, the result will be a float, regardless of if there are integers in the calculation.
  3. This is more about the energy value only, but floats are 32bit, and loose precision above 24bits, and actually fail completely if adding small values to them.

Here's a quick sketch to demonstrate the above points:

int32_t test_int = 0xFFFFFF;
float test_float = 0xFFFFFF;

void setup() {
  // put your setup code here, to run once:
  Serial.begin(57600u);
  Serial.println();
  // volts
  float v = 120.0;
  // amps
  float i = 10.0;
  // volt-amps
  float va = v * i;
  // watts
  int32_t p = 1050;

  // power factor
  float pf = p / va;

  Serial.print("PF (float var): ");
  Serial.println(pf);
  Serial.print("PF (calculation): ");
  Serial.println(p / va);
}

void loop() {
  // put your main code here, to run repeatedly:
  test_int++;
  test_float++;
  Serial.print("int32_t: ");
  Serial.print(test_int);
  Serial.print(" int32_t(float): ");
  Serial.print((float)test_int);
  Serial.print(" float: ");
  Serial.println(test_float);
  delay(1000);
}

Which will output:

PF (float var): 0.88
PF (calculation): 0.88
int32_t: 16777216 int32_t(float): 16777216.00 float: 16777216.00
int32_t: 16777217 int32_t(float): 16777216.00 float: 16777216.00
int32_t: 16777218 int32_t(float): 16777218.00 float: 16777216.00
int32_t: 16777219 int32_t(float): 16777220.00 float: 16777216.00
int32_t: 16777220 int32_t(float): 16777220.00 float: 16777216.00
int32_t: 16777221 int32_t(float): 16777220.00 float: 16777216.00

As you can see, even with the power/watts as int32_t, it has no problem with the power factor calculation. You can also see how floats loose precision, and even totally break above 24bits, both of which would be very bad if recording a large watt-hour total over time. The precision loss and small values issue gets worse the larger the number gets.

olehs commented 6 years ago

I was rather thinking about human mistakes... This change may be non-backward compatible.

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  float f = 10123.;
  int32_t i = 10123;

  Serial.print("float: ");
  Serial.println(f / 100);
  Serial.print("uint32_t: ");
  Serial.println(i / 100);
  Serial.print("fixed uint32_t: ");
  Serial.println(i / 100.);
}

void loop() {
  // put your main code here, to run repeatedly:

}

Output

float: 101.23
uint32_t: 101
fixed uint32_t: 101.23
djGrrr commented 6 years ago

You are certainly correct about that particular error, but I think the likelihood of such an error with this library is slim to none, because the "i" variable in this case would be float, otherwise the error would have already existed, since the float value from the library would already have been cast to int32_t.

float e = pzem.energy(ip);

e would be cast to float, even if the return type from energy() is int32_t, therefor no bug would exist.

on the other hand, if the code was:

int32_t e = pzem.energy(ip);

Then the bug would already exist, as the float from the energy() function would be cast to int32_t

It seems highly unlikely that pzem.energy(ip), or any of the other functions would be used directly in a calculation without first assigning it to a variable.

olehs commented 6 years ago

Going back to timeout. unsigned long was chosen because of return type of the millis() function.

djGrrr commented 6 years ago

uint32_t is technically just an alias for unsigned long on most (all?) platforms, that change is more just for consistency with the other integer variables.