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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

djGrrr
Copy link

@djGrrr djGrrr commented Sep 14, 2018

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.

Change power() and energy() functions to return int32_t instead of float, they don't support decimal values
@olehs
Copy link
Owner

olehs commented Sep 15, 2018

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
Copy link
Author

djGrrr commented Sep 15, 2018

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
Copy link
Owner

olehs commented Sep 15, 2018

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
Copy link
Author

djGrrr commented Sep 15, 2018

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
Copy link
Owner

olehs commented Sep 15, 2018

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

@djGrrr
Copy link
Author

djGrrr commented Sep 15, 2018

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.

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.

2 participants