Error in 'if' statement

Only one of the two relays will be activated (LOW) at any one time, and this code is supposed to increase the respective value of the variable 'heaterH' and 'HeaterL' depending how long the relay is activated. The first 'if' statement works great and the variable 'heaterH' increases in value as expected. However, the second 'if' never seems to increase the value of 'heaterL', despite 'RelayTwo' being activated. I can't help but think that I have made a error in the way I have written this, but can't see the wood for the trees!! Does this look OK?

if (digitalRead(RelayOne) == LOW) { heaterH = heaterH + (1090/1000 * 1.0/3600.0 * (t_diff/1000.0)); } if (digitalRead(RelayTwo) == LOW) { heaterL = heaterL + (545/1000 * 1.0/3600.0 * (t_diff/1000.0)); }

545/1000 == 0, 1090/1000 == 1

Perhaps you mean 0.545 and 1.09 respectively - integer constants have integer type and integer division truncates...

MarkT: 545/1000 == 0, 1090/1000 == 1

Perhaps you mean 0.545 and 1.09 respectively - integer constants have integer type and integer division truncates...

Ok, I wasn't being lazy in writing it this way, it was so I had a clear audit trial of how this works, but I'll rewrite it and see if it sorts the problem. Does the '1.0/3600.0' also need resolving? Thanks Mark

Sorry, I still don’t get this…
I have declared ‘HeaterL’ and ‘HeaterH’ as ‘double’.
Do I need to simplify the whole statement or just the 0.545 and 1.09?
(sorry if I’m a little slow here, it’s a steep learning curve for me!)

OP, I don't think I understand what you intend to do. A relay should only be written to but can't be read so why are you reading a pin that is called relay? If it is the same pin that actually controls the relay, you could be deactivating the relay by performing this read. You need to tell a more complete story.

liudr:
OP, I don’t think I understand what you intend to do. A relay should only be written to but can’t be read so why are you reading a pin that is called relay? If it is the same pin that actually controls the relay, you could be deactivating the relay by performing this read. You need to tell a more complete story.

‘Relay’ is a value, and is declared as;
int RelayOne = 4; //Arduino pin 4 goes to the opto isolator (full power)

If the relay's are 100% disjunct one could optimize it to :

  float cap =  1.0/3600.0 * (t_diff/1000.0);
  if (digitalRead(RelayOne) == LOW) 
  {
    heaterH += 1090.0/1000.0 * cap;
  } else {
    heaterL += 545.0/1000.0 * cap;
  }

or even

  float cap = 1090.0/1000.0 * 1.0/3600.0 * (t_diff/1000.0);
  if (digitalRead(RelayOne) == LOW) 
  {
    heaterH += cap;
  } else {
    heaterL += cap/2;
  }

pauldreed:

liudr: OP, I don't think I understand what you intend to do. A relay should only be written to but can't be read so why are you reading a pin that is called relay? If it is the same pin that actually controls the relay, you could be deactivating the relay by performing this read. You need to tell a more complete story.

'Relay' is a value, and is declared as; int RelayOne = 4; //Arduino pin 4 goes to the opto isolator (full power)

And what is this opto isolator indicating? Whether the relay is engaged? Just want to be completely clear before I can help.

liudr:

pauldreed:

liudr: OP, I don't think I understand what you intend to do. A relay should only be written to but can't be read so why are you reading a pin that is called relay? If it is the same pin that actually controls the relay, you could be deactivating the relay by performing this read. You need to tell a more complete story.

'Relay' is a value, and is declared as; int RelayOne = 4; //Arduino pin 4 goes to the opto isolator (full power)

The opto isolator is connected to pin 4 and serves to isolate the arduino from the external components, ie Relay And what is this opto isolator indicating? Whether the relay is engaged? Just want to be completely clear before I can help.

Just to clarify the int v. float division... If both the numerator and denominator are of an integer type, integer division (yielding an integer result) is performed. It doesn't matter what the context is, 5/2 is always 2 and never 2.5, even in this case:

  float f = 5/2 ;

The value of 5/2 is 2, and this is then converted to a float (2.0) to store in f... Also integer division truncates towards zero, it doesn't round to the nearest integer, 15/16 gives 0.

If either or both of the numerator/denominator are float or double then float division is performed. So 5/2.0 or 2/5.0 or 2.0/5.0 will yield 2.5.

On the Arduino setup double means the same as float, a 32bit IEEE floating point type. On most larger computers double means 64 bit IEEE floating point type.

@Liudr,

OP, I don't think I understand what you intend to do. A relay should only be written to but can't be read so why are you reading a pin that is called relay? If it is the same pin that actually controls the relay, you could be deactivating the relay by performing this read. You need to tell a more complete story.

The pin that controls the relay can be read safely, you just get the value last written. Check this sketch:

void setup()
{
  pinMode(13, OUTPUT);
}

void loop()
{
  digitalWrite(13, 1 - digitalRead(13));
  delay(100);
}

There are 2 drawbacks: 1) you are in fact checking the state of the pin not the relay . If the relay fails the pin doesn't know. 2) if you are reading directly after a write a (mechanical) relay hasn't had time to switch properly. Timing is crucial.

robtillaart: If the relay's are 100% disjunct one could optimize it to :

  float cap =  1.0/3600.0 * (t_diff/1000.0);
  if (digitalRead(RelayOne) == LOW) 
  {
    heaterH += 1090.0/1000.0 * cap;
  } else {
    heaterL += 545.0/1000.0 * cap;
  }

or even

  float cap = 1090.0/1000.0 * 1.0/3600.0 * (t_diff/1000.0);
  if (digitalRead(RelayOne) == LOW) 
  {
    heaterH += cap;
  } else {
    heaterL += cap/2;
  }

Rob, they are not disjunct, both are independent and either one could be activated at any one time, or neither. So shouldn't it be;

  float cap =  1.0/3600.0 * (t_diff/1000.0);
  if (digitalRead(RelayOne) == LOW) 
  {
    heaterH += 1090.0/1000.0 * cap;
  } 
  if (digitalRead(RelayTwo) == LOW) 
  {
    heaterL += 545.0/1000.0 * cap;
  }

I still can't get my head around this, for example how can it perform the 1090/1000 calculation without float, yet 1/3600 needs float to return the correct number? Also, how do I declare HeaterH and HeaterL, I currently have double HeaterH, HeaterL ; and does 'cap' need declaring? I take your point about the timing, but in this instance it is not crucial as each of the relays maybe activated for hours at a time.

Just when I thought I was doing well.....

OK, both are independent then your code is correct.

for example how can it perform the 1090/1000 calculation without float

It can't.... but the outcome 1 differs only 9% from what you expect, so it looks like a good value (these are the worst bugs!)

I think you are doing math in KWH (dividing by 3600 = sec -> Hour and 1000 == Kilo)

While typing this I had a far better idea. The best way to catch these rounding errors is to sum the t_diff's and do the heat math later

  unsigned long timeH = 0, timeL = 0, heaterH = 0, heaterL = 0;
  float KWH_H = 0, KWH_L = 0;

  if (digitalRead(RelayOne) == LOW) timeH += t_diff; 
  if (digitalRead(RelayTwo) == LOW) timeL += t_diff;

  heaterH = (1090 * timeH)/ 1000;    // units Ws;  (long math) 
  heaterL = (545 * timeL)/ 1000;      
  KWH_H = heaterH / 3.6e6;     // units KWH    3.6e6 = (3600.0 * 1000.0)
  KWH_L = heaterL / 3.6e6;

This way the timeH and timeL will be exact (unsigned longs have 9-10 digits!). heaterH and heaterL are not exact but the precision increases fast after timeH and timeL become greater than a few seconds. idem for KWH_H and KWH_L.

Of course you can calculate the KWH directly from time:

KWH_H = timeH * (1090/3.6e9); KWH_L = timeL * (545/3.6e9)

Even if you assign say 7/8 to a float you will still get 0.0

The computer doesn't look at the left side of the equal (assignment) sign until the very last step when the right side is entirely calculated. It also only does one calculation step at a time on the right side, keeping the order of operation straight. A CPU can only calculate operations between identical data types. When two numbers of different types are supplied for a calculation, forced type cast is done in a pre-defined way, typically in favor of precision, so if one of them is float, the other one is integer, the integer is converted to float to maintain float precision. If both are integer, there is no reason to convert to float since both are integer and the CPU can calculate with two integers. There are countless needs/uses for such operations so don't think the CPU is stupid since it is not using a higher precision than integer:)

float precision

The precision of floats is lower than the precision of longs (in the overlapping part) however the range of floats is far greater. Do not overestimate float's precision, 6 or 7 digits and with every float operation you may loose significant digits....

Arduino does support the datatype long long or uint64_t (sssllloooooowwww) but exact from 0 - 2^63 approx(10^20). Still not the range of floats but 20 digits of precision! For processing national debts, etc ;)

I should have stuck with "forced type cast is done in a pre-defined way".

Thank you. I have amended and uploaded the sketch, but as this is Solar Energy project, I wont know if it works until tomorrow, although I have every confidence!

but as this is Solar Energy project, I wont know if it works until tomorrow

You can put bright white LEDS above your solar panels ... to test. (or just fake the input)

robtillaart:

but as this is Solar Energy project, I wont know if it works until tomorrow

You can put bright white LEDS above your solar panels ... to test. (or just fake the input)

Don't fancy the LED option, I'm afraid of heights! Did consider faking the inputs, but because it's a stand-alone PCB, it's a pain to upload sketches. But.. tried it today, together with a power meter on the feed cable and it appears to have recorded OK, as both readings are very similar.

Thanks again Rob.