 Notice. New forum software under development. It's going to miss a few functions and look a bit ugly for a while, but I'm working on it full time now as the old forum was too unstable. Couple days, all good. If you notice any issues, please contact me. |
Forum Index : Microcontroller and PC projects : Bug in my Code.
| Author | Message | ||||
| Phil23 Guru  Joined: 27/03/2016 Location: AustraliaPosts: 1667 |
Hi Everyone, Can anyone see where this little bug is coming from. I've seen it a few times since some code changes; Not sure when I introduced it. The Green value at the bottom of the screen; It's last Pump Run Time. It was showing -7:-1:-51 this morning, & I've seen it a few times over the last week. Not sure if it's always the same value or not. 
I stopped the code & checked a few variables:- [Code] > Print PumpOnTime 14:34:27 > Print PumpRunTime -25311 >[/code] Any thought's on where is happening? Showed a valid time last time I looked at it last night, and shouldn't update while the pump is stopped. On pump restart it should reset, which it usually seems to do Ok. Thanks Phil. 2016-09-11_224539_Spa_Controller_V1.05.zip |
||||
TassyJim Guru Joined: 07/08/2011 Location: AustraliaPosts: 6269 |
You haven't taken into account that the 'time' resets to zero at midnight. You have to include the 'date' into your running time calculations or simply use a integer counter to record the start time, you can use the miliseconds timer or roll your own. If it is only the occasional one day rollover, check for negative and add a days worth of seconds. 24*60*60 = 86400 VK7JH MMedit |
||||
| TassyJim Guru Joined: 07/08/2011 Location: AustraliaPosts: 6269 |
On about line 238, try adding the following line. It only works for one day rollover and if you need unlimited days, you will need to change the PumpRunTime variable to INTEGER Jim VK7JH MMedit |
||||
| Phil23 Guru Joined: 27/03/2016 Location: AustraliaPosts: 1667 |
Hi Jim, I figured the midnight roll-over wouldn't ever come into play, as the pump will never be started on solar in the dark, so start time & current time should never fall either side of midnight. The Pump Start condition, should not exist until after about 8:30am, When SolarVolts are above the threshold & minimum panel temperature is met. Then it's started by the 2nd option of the IF block. (That's what PumpIf=xxx is for, just monitoring & debugging; so I know which if statement is true; that's the "-1" above time on the LCD). That's when PumpOnTime is set along with PumpStat="Running". It will then only update PumpRunTime if PumpStat=Running. When I checked variables after stopping the code, the PumpOnTime was 14:34:27, 52440 Seconds AM. Sunday afternoon. The photo is Monday morning. So PumpOnTime didn't change overnight. Runtime=Time-PumpOnTime, so the value for PumpRunTime was last calculated at 7:32:09 AM this morning, 27129 Seconds. The punt should not have been running then, as the panel would not have heated to 2° above water temp. Unless the water sensor returned an invalid reading. But if it did start for that reason, it should have reset PumpOnTime. Phil. |
||||
| TassyJim Guru Joined: 07/08/2011 Location: AustraliaPosts: 6269 |
So the problem is with your PumpOnTime, not PumpRunTime. Time-PumpOnTime = 27129 - 52400 = -25311 You need to work out why your PumpOnTime didn't change when the pump started, or why your "Running" didn't change when your pump stopped. Jim VK7JH MMedit |
||||
| Phil23 Guru Joined: 27/03/2016 Location: AustraliaPosts: 1667 |
Think possibly this statement could have been True when all the ones above it were false. [Code] Else If (TmpOut-TmpInp>TmpDif) And PumpRuntime>600) Then 'Keep Pump Running while its heating PumpIf=4 PumpStat="Running" Pin(PinPump1)=0 Led2Colour=RGB(Red) [/code] That could have set the pump to run once the first IF statement became false. TmpInp & TmpOut don't give valid readings overnight, with no water flowing. TmpInp Sensor is mounted low near the concrete. TmpOut Sensor is right up neat the top or the tub, so it's fitting is in much warmer air. There's some graphs in this post that shows sensor readings overnight. Phil. |
||||
| TassyJim Guru Joined: 07/08/2011 Location: AustraliaPosts: 6269 |
I think instead of (TmpOut-TmpInp>TmpDif), you should have ((TmpOut-TmpInp)>TmpDif) This may not be the problem but the way you had it is unlikely to give sensible results. Jim VK7JH MMedit |
||||
| Phil23 Guru Joined: 27/03/2016 Location: AustraliaPosts: 1667 |
Think it should be fine according to the Operators & Precedence on Page 38. Addition & subtraction come before Logical operators. I get these results at the prompt. [code]> Print 25-24>2 0 > print 27-24>2 1 > [/code] Think it's probably the sensor outputs that's fooling it. |
||||
| Phil23 Guru Joined: 27/03/2016 Location: AustraliaPosts: 1667 |
Thanks all, Think the 4th test of the if statement is the error. If all above it return false, it could incorrectly return true. [code] '====================Start Pump & Light Red LED if conditions are met Sub SetPump If PumpStat="Running" Then PumpRunTime=TimeSecs(Time$)-TimeSecs(PumpOnTime) 'Update PumpRunTime Value If SolarVolts<SolarMin Then 'Test if Solar is Below Daylight Level PumpIf=1 PumpStat="Stopped" 'Turn Pump Off Pin(PinPump1)=1 Led2Colour=RGB(Gray) Else If (PumpStat="Stopped" And SolarVolts>=SolarThres And TmpPan-TmpCur>=TmpDifPan) Then 'Test Solar and Panel Temp then Start Pump PumpIf=2 PumpOnTime=Time$ 'Reset Pump Start Time PumpStat="Running" Pin(PinPump1)=0 Led2Colour=RGB(Red) Else If (PumpStat="Running" And PumpRunTime<600) Then 'Keep Pump Running for Minimum 10 Minutes PumpIf=3 PumpStat="Running" Pin(PinPump1)=0 Led2Colour=RGB(Red) Else If (TmpOut-TmpInp>TmpDif) And PumpRuntime>600) Then 'Keep Pump Running while its heating PumpIf=4 PumpStat="Running" Pin(PinPump1)=0 Led2Colour=RGB(Red) Else If (TmpOut-TmpInp<TmpDif) And PumpRuntime>600) Then 'Stop Pump if not heating & minimum Runtime met PumpIf=5 PumpStat="Stopped" 'Turn the Pump Off. Pin(PinPump1)=1 Led2Colour=RGB(Gray) EndIf End Sub [/code] It be correct it should only return true if the pump is already in the running state. The correct test should be:- [Code] Else If (PumpStat="Running" And TmpOut-TmpInp>TmpDif) And PumpRuntime>600) Then 'Keep Pump Running while its heating PumpIf=4 PumpStat="Running" Pin(PinPump1)=0 Led2Colour=RGB(Red) [/code] Lol I was working well as it was on most days..... Particularly as it's been a bit cloudy of a morning since my last change to the code. That meant SolarMin could be reached, then fall below it's level again with the clouds. Result would be that the pump briefly turn on then off again after a short period. On a clear day Solar volts would continuously rise, resulting in the pump running indefinitely. Lol, It's been nicely cooling my water on some mornings, either for a brief period, or just continue for the hour or more till it actually began heating. 
Cheers Phil |
||||
MicroBlocks Guru Joined: 12/05/2012 Location: ThailandPosts: 2209 |
I think the if statements contain to many tests that are similar. I see three if statements that start or keep the pump running and two if statements that stop the pump. If these are done with else if's then the logic is hard to follow. (At least for me who sees the code for the first time). It is also hard to maintain as a small change in one if can prevent another part from running. In control applications it is also best to exit a subroutine or function as quickly as possible and keep the code 'blocks' separated as much as possible. In general programming that is not considered good practice but in control applications it is i think required. It minimizes the risk of failures as you do not need to follow lots of code paths. I would write it like this (I hope i got the conditions right): [code] CONST RUNNING = 0 CONST STOPPED = 1 SUB SetPump SELECT CASE PumpStat CASE RUNNING 'Test if Solar is Below Daylight Level IF SolarVolts < SolarMin THEN PumpStat = STOPPED Pump1(STOPPED) EXIT SUB ENDIF PumpRunTime=TimeSecs(Time$)-TimeSecs(PumpOnTime) 'Update PumpRunTime Value IF PumpRunTime > 600 THEN IF TmpOut-TmpInp < TmpDif PumpStat = STOPPED Pump1(STOPPED) ENDIF EXIT SUB ENDIF CASE STOPPED IF SolarVolts>=SolarThres And TmpPan-TmpCur>=TmpDifPan THEN 'Reset Pump Start Time PumpOnTime=Time$ PumpStat = RUNNING Pump1(RUNNING) EXIT SUB ENDIF CASE ELSE PRINT 'THIS SHOULD NOT HAPPEN!!!' END SELECT END SUB SUB Pump1(NewState AS INTEGER) SELECT CASE NewState CASE RUNNING Pin(PinPump1) = 0 Led2Colour = RGB(Red) EXIT SUB CASE STOPPED Pin(PinPump1) = 1 Led2Colour = RGB(Gray) EXIT SUB END SELECT END SUB [/code] The above 'style' has very short 'code blocks' that are easy to follow. Functionality is split into logic (the SetPump routine) and actions (Pump1 subroutine). The "logic" part has a PumpStat and only in that routine PumpStat is changed. This does not need you to go find a change in that value somewhere else in the program. It might even be better to read the PIN(PinPump1) value instead of keeping the variable PumpStat in sync. I am not sure if you can read a pin that is defined as an output, but if that works then you could do the following: instead of [code] SELECT CASE PumpStat [/code] you would then do [code] SELECT CASE GetPumpStat() [/code] And a small function that reads the physical pin [code] FUNCTION GetPumpStat() AS INTEGER SELECT CASE PIN( PinPump1 ) CASE 0 GetPumpStat = RUNNING CASE 1 GetPumpStat = STOPPED END SELECT END FUNCTION [/code] The variable PumpStat can then be removed. Microblocks. Build with logic. |
||||
| Phil23 Guru Joined: 27/03/2016 Location: AustraliaPosts: 1667 |
|
||||
 Print this page |
 |
The Back Shed's forum code is written, and hosted, in Australia. | © JAQ Software 2025 |
