Jump to content

Welcome to Smart Home Forum by FIBARO

Dear Guest,

 

as you can notice parts of Smart Home Forum by FIBARO is not available for you. You have to register in order to view all content and post in our community. Don't worry! Registration is a simple free process that requires minimal information for you to sign up. Become a part of of Smart Home Forum by FIBARO by creating an account.

 

As a member you can:

  •     Start new topics and reply to others
  •     Follow topics and users to get email updates
  •     Get your own profile page and make new friends
  •     Send personal messages
  •     ... and learn a lot about our system!

 

Regards,

Smart Home Forum by FIBARO Team


  • 0

Code review request - What could be done better, more appropriate?


JcBorgs

Question

Please login or register to see this spoiler.

 

Edited by JcBorgs
Minor updates
Link to comment
Share on other sites

Recommended Posts

  • 0

You have 3 loops

-stopCharging

-startCharging

-calculateCost

 

The first two would not need to loop every 30min when running as you should be able to calculate the time to call them with setTimeout (reschedule for the next stopCharging in 24h our reschedule if stopCharging time changes)

The last loops every 10min to calculate the cost during the CalcHour (20). Why not let that function schedule (or reschedule) the stopCharging and startCharging?

 

The cheapest block could be calculated something like this a bit more dynamic

Please login or register to see this code.

 

Then you could use the disable QA logic posted in another thread to pause the QA.

 

Declare your global lua variables as local in the beginning of the file

Please login or register to see this code.

 

The pauseMode == 0 is a bit unnecessary . Use booleans instead.

 

 

...otherwise - nice code.

Edited by jgab
Link to comment
Share on other sites

  • 0
  • Inquirer
  • @jgab Thanks for the input and tips, highly appreciated!

     

    Will review and implement your suggestions once I get a full grasp on how it works :-) 

    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • 21 minutes ago, jgab said:

    The first two would not need to loop every 30min when running as you should be able to calculate the time to call them with setTimeout (reschedule for the next stopCharging in 24h our reschedule if stopCharging time changes)

    The last loops every 10min to calculate the cost during the CalcHour (20). Why not let that function schedule (or reschedule) the stopCharging and startCharging?

    @jgabNot fully with you on what you mean, but perhaps I should clarify a few basic things as well.....

     

    - The first two loops actually loops even 5 minute and not every 30min....

    - The StartCarCharger loops every 5 minutes until the charger is started, then it moves to the StopCarCharger and loops there every 5min until stop time is reached.

    - Guess I need to dig into the options of the setTimeout, if it is possible to loop/wait until a specific time it would be better instead of looping for a number of minutes.

    - Prices from the Tibber Monitor QA is always available in the global variables from the current hour and the coming 10 hours. So to be able to find the best hours during evening (starts at 20:00) until the morning (last hour is 6:00) you have to do the calculation during hour 20, if done earlier you can't catch the evening, night and morning hour prices.

    - The thought of scheduling the Start and Stop from the calc function would be a good way forward, but a bit outside my competency on how to do a schedule like that.. As you already seen my knowledge of starting things at a specific time only reaches doing loops and comparing the actual time with a variable......

     

    In an ideal solution the QA would only execute 3 times per day...

    1. Calculation at 20:00

    2. Start charger at StartHour

    3. Stop charger at StopHour

     

    Ideally without a lot of loops in the code, but have no clue what so ever how one would build a code that only executes command on specific time and for the remainder of the time is silent/quiet..... 

     

    Link to comment
    Share on other sites

    • 0
    33 minutes ago, JcBorgs said:

    @jgab Thanks for the input and tips, highly appreciated!

     

    Will review and implement your suggestions once I get a full grasp on how it works :-) 

     

    Are you sure that you restart the QA in the right "StatusNow" when you enable it again?

    Time may have moved past stop time and you may start the startCharging loop.

    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • 7 minutes ago, jgab said:

     

    Are you sure that you restart the QA in the right "StatusNow" when you enable it again?

    Time may have moved past stop time and you may start the startCharging loop.

    Yes I am rather sure that it will be restarted in the right place.. 

     

    Actually during my version of "Pause" the QA continues to operate as normal with the only difference that actions (stop/start of the charger) and some label updates are not performed as long as "Pause" is active. So once the "Pause mode" is ended it should always continue where it should.

    Sure if restarted in the middle of a charging time (time between start and stop) it would not start the charging if started a different hour that the actual StartHour. i.e. if StartHour is 22 and QA pause is exited at 23 then charging would not start the way that I have implemented it now. 

    Link to comment
    Share on other sites

    • 0

    This is just an example of another logic and flow.

    The magic is the startAt function.

    It takes a label, time and function and run it at the time specified. Time is a string in HH:MM

    The label serves as debug output for when the function is going to run but it also restarts the function if we call it while it is already running.

    It's just a skeleton without the calculations etc.

     

    Please login or register to see this code.

     

    Edited by jgab
    • Thanks 2
    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • @jgab WOW! That is a really useful skeleton that will be a much better base for the QA that I am trying to build.

     

    Thank you so much for sharing this!

    • Like 1
    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • @jgab Working on rebuilding my QA with main pieces of the skeleton you provided.

     

    At the end it wont matter as the action will always be called in the very first second of the hour. But now during test I ran into an issue with your skeleton and cant find what causes it.

     

    If I as an example change the:

    local calcHour="20:00"
    to:
    local calcHour="20:35"
     
    Just to force the calculation to run a different time it does a strange remake of the time and translates the minutes to seconds and puts the following in the log:
    Will run Calulate cost at Tue Jun 28 20:00:35 2022
     
     
     
     
    Link to comment
    Share on other sites

    • 0
    7 minutes ago, JcBorgs said:

    @jgab Working on rebuilding my QA with main pieces of the skeleton you provided.

     

    At the end it wont matter as the action will always be called in the very first second of the hour. But now during test I ran into an issue with your skeleton and cant find what causes it.

     

    If I as an example change the:

    local calcHour="20:00"
    to:
    local calcHour="20:35"
     
    Just to force the calculation to run a different time it does a strange remake of the time and translates the minutes to seconds and puts the following in the log:
    Will run Calulate cost at Tue Jun 28 20:00:35 2022

     

     

    Sorry, small bug.

    In function startAt, need to multiply minutes with 60. (only tested whole hours)

    Please login or register to see this code.

    Just now, jgab said:

     

     

    Sorry, small bug.

    In function startAt, need to multiply minutes with 60. (only tested whole hours)

    Please login or register to see this code.

    Actually, make it

    Please login or register to see this code.

    to be on the safe side. Multiplying number strings with numbers sometimes produce decimal numbers (.0) that can cause issues for setTimeout...

    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • Works much better!

     

    Thank you!

    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • @jgab The Calculation routine you suggested doesnt work for me.. first tried it in the skeleton code you provided yesterday, then even tried it in a totally new empty QA.  

     

    It just gives the following in the Log:

    [QUICKAPP1247]: main.lua:7: attempt to perform arithmetic on a nil value (global 'n')

     

    Line 7 is: for i=0,n-4 do

     

     

    Short simple QA with your Calculation routine:

     

    18 hours ago, jgab said:

    Please login or register to see this code.

     

    Link to comment
    Share on other sites

    • 0
    55 minutes ago, JcBorgs said:

    @jgab The Calculation routine you suggested doesnt work for me.. first tried it in the skeleton code you provided yesterday, then even tried it in a totally new empty QA.  

     

    It just gives the following in the Log:

    [QUICKAPP1247]: main.lua:7: attempt to perform arithmetic on a nil value (global 'n')

     

    Line 7 is: for i=0,n-4 do

     

     

    Short simple QA with your Calculation routine:

     

    Yes, n should be set to number of periods evaluated - 10 in your case. Could make it a parameter to the function

    Please login or register to see this code.

    and then call cheapestTime(10) 

    or use a defined constant. Probably the 4 should a parameter too. 

    Link to comment
    Share on other sites

    • 0

    Note also that the rest of the code assumes that the startHour and stopHour are strings. The startHour and stopHour calculated are numbers.

    New version.

    Please login or register to see this code.

     

    Edited by jgab
    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • @jgab

     

    Could be totally wrong here and then I apologise.....

     

    But think there are errors in your formula and it won't achieve its purpose.

     

    From my tests it looks like your code takes the cost for one hour and then adds the same value 3 times, then it takes then next hour cost and adds the same value 3 times. etc. etc. So we end up with a table of in my case 8 different costs where each cost is the same hour *4, it should be "cost for one our"+"cost for next hour"+"cost for the following hour"+"cost for the following hour"

     

    Think the "error" are in the line:

    Please login or register to see this code.

    Should in my view be:

    Please login or register to see this code.

     

    If we have 11 data point:

    1.  1.1

    2. 1.2

    3. 1.1  

    4. 0.9

    5. 0.7

    6. 0.7

    7.  0.6

    8. 0.5

    9.  1.2

    10.  1.3

    11.  1.4

     

    The way I think your calculation works it would in block 8 have a value of 2 and say that that is the cheapest period. The accurate value of Block 8 would be the sum of data point 8,9,10&11 and should contain the value 4.4. The actual cheapest period should start with data point 5 and contain data point 5,6,7&8

     

    But doing the change from i to j in the formula, would it affect anything else in the function in regards to start/stop hour? And for clarification, Stop hour should be 4 full hours after start, so if start is 01:00 then stop should be 05:00 to get 4 full hours of charging time.) 

     

     

      

    Link to comment
    Share on other sites

    • 0
    4 minutes ago, JcBorgs said:

    @jgab

     

    Could be totally wrong here and then I apologise.....

     

    But think there are errors in your formula and it won't achieve its purpose.

     

    From my tests it looks like your code takes the cost for one hour and then adds the same value 3 times, then it takes then next hour cost and adds the same value 3 times. etc. etc. So we end up with a table of in my case 8 different costs where each cost is the same hour *4, it should be "cost for one our"+"cost for next hour"+"cost for the following hour"+"cost for the following hour"

     

    Think the "error" are in the line:

    Please login or register to see this code.

    Should in my view be:

    Please login or register to see this code.

     

    If we have 11 data point:

    1.  1.1

    2. 1.2

    3. 1.1  

    4. 0.9

    5. 0.7

    6. 0.7

    7.  0.6

    8. 0.5

    9.  1.2

    10.  1.3

    11.  1.4

     

    The way I think your calculation works it would in block 8 have a value of 2 and say that that is the cheapest period. The accurate value of Block 8 would be the sum of data point 8,9,10&11 and should contain the value 4.4. The actual cheapest period should start with data point 5 and contain data point 5,6,7&8

     

    But doing the change from i to j in the formula, would it affect anything else in the function in regards to start/stop hour? And for clarification, Stop hour should be 4 full hours after start, so if start is 01:00 then stop should be 05:00 to get 4 full hours of charging time.) 

     

     

      

    Yes, you are right, it should be j as index.

    Please login or register to see this code.

    Then we will sum

    block0 = tibber_h0dPrice_... + tibber_h1dPrice_... + tibber_h2dPrice_... + tibber_h3dPrice_...

    block1 = tibber_h1dPrice_... + tibber_h2dPrice_... + tibber_h3dPrice_... + tibber_h4dPrice_... 

    etc.

     

    Link to comment
    Share on other sites

    • 0

    Another coding practice is to have some debug function so that's it easy to turn off debugging and tracing when finnished.

    Ex.

    Please login or register to see this code.

    Here the log functions take a format like string.format. Ex

    Please login or register to see this code.

     

    The point is to be consequent when to use what. Ex. 

    • debug only for checking return values and calculation results -  useful for developing the QA - can be turned off when shipping.
    • trace for flow and status info when the QA runs -  useful for developing the QA - can be turned off when shipping.
    • log for info/status useful for the user of the QA. If log should use fibaro.debug or fibaro.trace is a matter of taste. Maybe trace.
    • warning to inform user of QA of something that's not right/optimal or unexpected error but that can be recovered from (ex. a http request failing once or twice)
    • error to inform user of a critical conditions that will stop the QA from functioning

     

    • Like 1
    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • @jgab

     

    That was what I thought :-) 

     

    Still don't understand how your code determines the stop and start.

     

    Lets for argument sake say that Block 7 is the cheapest. If the calculations are done as I plan to at 20:00 then Block 7 should include cost for hours 03, 04, 05 & 06 so StartHour should be 03 and StopHour should be 07.

     

    To be able to do that I think we need to change to the following:

      startHour = tostring((CalcHour+cheapest.block) % 24)
      stopHour = tostring((CalcHour+cheapest.block+4) % 24)

     

    14 minutes ago, jgab said:

    Another coding practice is to have some debug function so that's it easy to turn off debugging and tracing when finnished.

    Ex.

    Please login or register to see this code.

    Here the log functions take a format like string.format. Ex

    Please login or register to see this code.

     

    The point is to be consequent when to use what. Ex. 

    • debug only for checking return values and calculation results -  useful for developing the QA - can be turned off when shipping.
    • trace for flow and status info when the QA runs -  useful for developing the QA - can be turned off when shipping.
    • log for info/status useful for the user of the QA. If log should use fibaro.debug or fibaro.trace is a matter of taste. Maybe trace.
    • warning to inform user of QA of something that's not right/optimal or unexpected error but that can be recovered from (ex. a http request failing once or twice)
    • error to inform user of a critical conditions that will stop the QA from functioning

     

    Yes, that will definitely be useful!

    Edited by JcBorgs
    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • @jgab

     

    Almost works :-)

     

    Testing now and StartHour is correctly saved as 21. But StopHour is stored as 1 instead of 01.

     

    Using these lines in the Calc function to store startHour and stopHour

      startHour = tostring((14+cheapest.block) % 24)
      stopHour = tostring((14+cheapest.block+04) % 24)

     

    The interesting part here is that the log says:

    [QUICKAPP1245]: Will run Start charging at Tue Jun 28 21:00:00 2022

    and nothing more.

     

    But if I go into edit mode log says:

    [28.06.2022] [14:13:00] [DEBUG] [QUICKAPP1245]: Smart Car Charger 2 - QAid: 1245

    [28.06.2022] [14:13:00] [DEBUG] [QUICKAPP1245]: Will run Start charging at Tue Jun 28 21:00:00 2022

    [28.06.2022] [14:13:00] [ERROR] [QUICKAPP1245]: QuickApp crashed

    [28.06.2022][14:13:00] [ERROR] [QUICKAPP1245]: main.lua:12: attempt to perform arithmetic on a nil value (local 'h')

     

    Line 12 is:

    time = math.floor(3600*h+(m~="" and 60*m or 0)+os.time(t0)+0.5)

     

    So at first the QA seems to halt when it is supposed to log the Stop time, but gives no error messages. Then when going into edit mode it crashes at the same line but now also gives some information in the log to where the problem is.

     

    If I then go out and edit the QA variable StopHour from 1 to 01 and goes back to edit mode it all just work fine.

     

    So how do I get the StopHour stored with a leading 0 if hours are 0-9?

     

     

     

    Link to comment
    Share on other sites

    • 0
    10 minutes ago, JcBorgs said:

    @jgab

     

    Almost works :-)

     

    Testing now and StartHour is correctly saved as 21. But StopHour is stored as 1 instead of 01.

     

    Using these lines in the Calc function to store startHour and stopHour

      startHour = tostring((14+cheapest.block) % 24)
      stopHour = tostring((14+cheapest.block+04) % 24)

     

    The interesting part here is that the log says:

    [QUICKAPP1245]: Will run Start charging at Tue Jun 28 21:00:00 2022

    and nothing more.

     

    But if I go into edit mode log says:

    [28.06.2022] [14:13:00] [DEBUG] [QUICKAPP1245]: Smart Car Charger 2 - QAid: 1245

    [28.06.2022] [14:13:00] [DEBUG] [QUICKAPP1245]: Will run Start charging at Tue Jun 28 21:00:00 2022

    [28.06.2022] [14:13:00] [ERROR] [QUICKAPP1245]: QuickApp crashed

    [28.06.2022][14:13:00] [ERROR] [QUICKAPP1245]: main.lua:12: attempt to perform arithmetic on a nil value (local 'h')

     

    Line 12 is:

    time = math.floor(3600*h+(m~="" and 60*m or 0)+os.time(t0)+0.5)

     

    So at first the QA seems to halt when it is supposed to log the Stop time, but gives no error messages. Then when going into edit mode it crashes at the same line but now also gives some information in the log to where the problem is.

     

    If I then go out and edit the QA variable StopHour from 1 to 01 and goes back to edit mode it all just work fine.

     

    So how do I get the StopHour stored with a leading 0 if hours are 0-9?

     

    Sometimes it seems like the debug window doesn't update correctly. Anyway, the runAt function expected a time with 2 digit hour, like "05" or "05:30"

    We can fix that

    Please login or register to see this code.

    Link to comment
    Share on other sites

    • 0
  • Inquirer
  • 7 minutes ago, jgab said:

     

    Sometimes it seems like the debug window doesn't update correctly. Anyway, the runAt function expected a time with 2 digit hour, like "05" or "05:30"

    We can fix that

    Please login or register to see this code.

    @jgab I made an alternative solution to ensure to save the time in 2 digit format:

      startHour = string.format("%0.2i", tostring((14+cheapest.block) % 24))

      stopHour = string.format("%0.2i", tostring((14+cheapest.block+04) % 24))

     

    Link to comment
    Share on other sites

    Join the conversation

    You can post now and register later. If you have an account, sign in now to post with your account.

    Guest
    Answer this question...

    ×   Pasted as rich text.   Paste as plain text instead

      Only 75 emoji are allowed.

    ×   Your link has been automatically embedded.   Display as a link instead

    ×   Your previous content has been restored.   Clear editor

    ×   You cannot paste images directly. Upload or insert images from URL.

    ×
    ×
    • Create New...