Potential deadlock in void EnergyMonitor::calcVI(..) - 50 days

[drexamar]

Potential Deadlock

The calcVI function depends on the millis() function, to get a sequence, however the function millis() wraps, and thereby making a risk of an infinite loop. The guilty code is this.

  unsigned long start = millis();    
//millis()-start makes sure it doesn't get stuck in the loop if there is an error.

  while(st==false)                                   //the while loop...
  {
     startV = analogRead(inPinV);                    //using the voltage waveform
     if ((startV < (ADC_COUNTS/2+50)) && (startV > (ADC_COUNTS/2-50))) st=true;  
//check its within range
     if ((millis()-start)>timeout) st = true;
  }

The code above fails, if millis() returns a number very close to 4,294,967,295. E.g 4,294,967,293. This will cause millis()-start to become negative, and then it will never be bigger than the timeout value. Normally set to 2000 ms.

Minor issue - compiler warning

The code if ((millis()-start)>timeout) st = true; compares an unsigned long(positive numbers) with and int(natural numbers) It is also semantically wrong to have a timeout, that is not unsigned. These two issue have not been proven by unit test or similar.

[andreasmarkussen]

What is the normal process for contributing? Test case that shows the defect? And then a Pull request?

[lafrech]

Pull request + explanation is fine.

Test case can be part of explanation.

Thanks.

[andreasmarkussen]

Ok. Thanks! Will do a PR. and I will try with the Boost test framework :) http://www.boost.org/doc/libs/1_44_0/libs/test/doc/html/utf/user-guide/test-organization/auto-test-suite.html

On Wednesday, December 24, 2014, Jérôme Lafréchoux notifications@github.com wrote:

Pull request + explanation is fine.

Test case can be part of explanation.

Thanks.

— Reply to this email directly or view it on GitHub https://github.com/openenergymonitor/EmonLib/issues/11#issuecomment-68060302 .

Andreas Markussen Holmekrogen 26 DK-2830 Virum Mobil: (+45) 40 86 81 18 Email: andreas@markussen.dk Web: http://andreas.markussen.dk

[glynhudson]

I think this issue has bee resolved http://openenergymonitor.org/emon/node/6370, thanks for your input