HestiaPi / hestia-touch-openhab

OpenHAB2 files for HestiaPi Touch model
GNU General Public License v3.0
62 stars 17 forks source link

2nd stage heating main setting storage is volatile #8

Closed gulliverrr closed 4 years ago

gulliverrr commented 4 years ago

If set ON, it is lost on a reboot/update/restore

rkoshak commented 4 years ago

You can fix this by:

Strategies {
  everyMinute  : "0 */1 * * * ?"
  default = everyChange
}
Items {
        RestoreOnStartup* : strategy = everyChange, restoreOnStartup
}

The RestoreOnStartup* means save the state for all members of the Group RestoreOnStartup according to the defined strategies. everyChange will cause the state of the Item to be saved every time it changes. restoreOnStartup means OH will read the value stored in the database and update the configured Items to that state. Usually this occurs before Rules will start executing but sometimes not (I've a fix for this too, stay tuned) so it's important to check for NULL states in Rules that use these Items to avoid errors.

The states won't be saved to the database until the Item changes state so you might want to use the everyMinute strategy for awhile so all the Items states get saved. Or you can use the UI or REST API to change all the Items manually or write a temporary Rule to initialize the values.

rule "Initialisation"
when
    System started
then
    logInfo("initialization", "Initialization starting!")
//    MainSwitch.sendCommand("OFF");
//    HeatingMode.sendCommand("OFF");
//    CoolingMode.sendCommand("OFF");
//    FanMode.sendCommand("OFF");
    Heating2.sendCommand("OFF");
    if (TempUnit.state == "C") {
      if((TempSetpoint.state == NULL) || (TempSetpoint.state == UNDEF)) postUpdate(TempSetpoint, 18);
      if((TempSetpointC.state == NULL) || (TempSetpointC.state == UNDEF)) postUpdate(TempSetpointC, 18);
      if(TempSetpoint.state > 40) postUpdate(TempSetpoint, 18);
      if(TempSetpointC.state > 40) postUpdate(TempSetpointC, 18);
    } else {
      if((TempSetpoint.state == NULL) || (TempSetpoint.state == UNDEF)) postUpdate(TempSetpoint, 70);
      if((TempSetpointF.state == NULL) || (TempSetpointF.state == UNDEF)) postUpdate(TempSetpointF, 70);
      if(TempSetpoint.state < 32) postUpdate(TempSetpoint, 70);
      if(TempSetpointF.state < 32) postUpdate(TempSetpointF, 70);
    }
    if((HeatingBoostTime.state == NULL) || (HeatingBoostTime.state == UNDEF)) HeatingBoostTime.sendCommand(10);
    if((CoolingBoostTime.state == NULL) || (CoolingBoostTime.state == UNDEF)) CoolingBoostTime.sendCommand(10);
//    postUpdate(PreviousTempReading, 0);
//    postUpdate(PreviousHumiReading, 0);
//    Heating2Time.sendCommand(0);
//    Heating2Delta.sendCommand(0);
    Network_WLAN_IP.sendCommand(executeCommandLine("/home/pi/scripts/getwlan0ip.sh",60000));
    Network_SSID.sendCommand(executeCommandLine("/home/pi/scripts/getssid.sh",60000));
    Network_WLAN_INFO.sendCommand(executeCommandLine("/home/pi/scripts/getwifiinfo.sh",60000));
    Network_WLAN_MAC.sendCommand(executeCommandLine("/home/pi/scripts/getwlan0mac.sh",60000));
    System_CPU_TEMP.sendCommand(executeCommandLine("/home/pi/scripts/getcputemperature.sh",60000));
    System_CPU_LOAD.sendCommand(executeCommandLine("/home/pi/scripts/getcpuload.sh",60000));
    System_Used_Space.sendCommand(executeCommandLine("/home/pi/scripts/getuseddiskspace.sh",60000));
//    TempUnit.sendCommand(executeCommandLine("/home/pi/scripts/gettempunit.sh",60000));
//    SystemType.sendCommand(executeCommandLine("/home/pi/scripts/getsystemtype.sh",60000));
end

I'm not ready to submit it as a PR just yet because I think you will want to check all these Items and if they are NULL or UNDEF than apply a default value to them similar to the way the TempSetpoint Items are checked and the values sanitized. I'm also not certain of any side effects I may have introduced. Also, this is based on the released code, not the latest version checked in to this repo.

But this should be enough to get you started.

The actual database get's stored in /var/lib/openhab2/persistence/mapdb which is included when you run the backup script. So if you restore from a backup you will also restore these Item's states.

gulliverrr commented 4 years ago

I will get some more time over the weekend to review and comment. At first glance I don't see and side effect.

rkoshak commented 4 years ago

I might be blind at times. I just noticed all the lines end in ;. While that is allowed, it is only required in one case return;. In all other cases, using typical coding practices for Rules DSL, the ; is omitted.

It's not a big deal but it's something I just noticed.

gulliverrr commented 4 years ago

Developer's habits;

rkoshak commented 4 years ago

I think I'm ready to check something in for this but I have two questions.

  1. This requires the installation of a new add-on (MapDB). How is this configuration controlled? I expected to see /etc/openhab2/services/addons.cfg in use to define which add-ons to install but they are not. Is it just made part of the SD card image? How/where do I get this add-on added? Without that it will break a number of user's configs on a upgrade I think.

  2. I find that there is some sort of side effect that I am not sure where it's coming from. On a reboot, the UI at least seems to be defaulting to EU instead of US as the system type. It seems like it depends on the command to the SystemType Item to work correctly. I'm thinking that the MQTT Channel linked to SystemType needs to have the retained flag set to true. Looks like the channel already is retained, need more testing I guess. I'm testing to verify. I've also seen a few odd things regarding heating mode returning to OH but that may be unrelated.

gulliverrr commented 4 years ago

How is this configuration controlled?

We never got around this so left it purely on the SD image :( Your input here is valuable on the right way to do it.

I've also seen a few odd things regarding heating

Feel free to share so that I can comment if I know anything about them

rkoshak commented 4 years ago

OK, I'll submit a PR to move the add-ons install to the addons.cfg file, at least for now. This will let you change what add-ons are installed as part of the text based configs instead of relying on the SD card image. It should make for a smoother upgrade process.

However, it means that anyone who customizes their OH install will need to do so through PaperUI.

Ultimately I'd like to see the contents of /var/lib/userdata configuration controlled. In addition to the logger config I've already discussed to suppress that warning from Jetty, that will let us preserve REST API configs (e.g. stuff done through PaperUI). This is important because I think migrating all the configs to the JSONDB and REST API managed configuration (including Rules) will be key to reducing the boot times. I believe I can cut 5 minutes or more from the boot by doing that. Parsing those text config files is really expensive. Parsing and loading from JSONDB is really fast.

As for the weirdness about heating, I think I may have it sorted. Without restoreOnStartup, the System started Rule relies on the setting of TempUnit to set the TempSetpoint Items. But TempUnit is not initialized until the end of the Rule. I think moving it to the beginning of the Rule, or getting restoreOnStartup right will fix things.

I notice that there is a distinct lack of logging in the Rules. I find it vital to use logging to follow what the Rules are doing. Is this a policy and I should remove the logging before I submit PRs or is it OK to include the logging. Mouch of what I'm adding are debug level so won't actually be written unless someone changes the logging levels.

gulliverrr commented 4 years ago

Go ahead and add the logging you believe is right. This, as well as most of the issues you have found so far are because of the simplistic approach we have followed I'm afraid.

rkoshak commented 4 years ago

I'll get the OH parts at least as efficient as I can. I'll need help working it in to you build and SD image creation and such. I don't know much about that stuff.

gulliverrr commented 4 years ago

Once you can get the PR I can continue with the testing and of course getting it in the image v1.2. Release 1.2 may get some more issues from GitHub. I need to get up to speed to catch up with you :)

On Tue, 4 Feb 2020, 20:56 Richard Koshak, notifications@github.com wrote:

I'll get the OH parts at least as efficient as I can. I'll need help working it in to you build and SD image creation and such. I don't know much about that stuff.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/HestiaPi/hestia-touch-openhab/issues/8?email_source=notifications&email_token=ABKX3CLDKD3ST744YZCZ3DTRBG25TA5CNFSM4JCRN2EKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKYY3QI#issuecomment-582061505, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKX3CNGPZJF2UODLHHJKELRBG25TANCNFSM4JCRN2EA .

rkoshak commented 4 years ago

Here's an update. The weirdness I was seeing isn't related to the retained message. I think it is a GUI only thing. If you do not sendCommand to the SystemType, TempSetpoint, and mode Items in the System startup Rule it just shows "Off" with no other information instead of showing the three symbols at the top with the setpoint. I've added code to the initialization rule to handle that and everything seems to be working. The PR for restoreOnStartup will be submitted momentarily.

rkoshak commented 4 years ago

Given PR #27 now preserves the 2nd stage heating settings (along with all the other settings) on reboot/restart/refresh can this Issue be closed?

gulliverrr commented 4 years ago

I haven't tested this yet although it obviously seems resolved. Let me double check it and close it within the day

gulliverrr commented 4 years ago

Fixed by #27