[venstarthermostat] Venstar thermostat away mode enhancement

[raveydavies]

<[venstarthermostat] Add away mode channel in binding>

<This PR adds the Away Mode available via the thermostat local API to the Venstar thermostat binding. The Away Mode is a String channel. It allows the Away Mode to be seen in the UI and also changed as a setting with two options (away or home). It has been tested using the demo app in Eclipse IDE. https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.binding.venstarthermostat/3.1.0-SNAPSHOT/>

Closes #10707

[digitaldan]

Suggested change

	updateIfChanged(CHANNEL_AWAY_MODE_RAW, new StringType("" + value));
	updateIfChanged(CHANNEL_AWAY_MODE_RAW, new StringType(value.toString()));

[raveydavies]

yep. I had copied the previous few lines of code. I will update mine and the previous one.

[digitaldan]

This seems to duplicate the updateThermostat method, would be cleaner if we could break this out and have a "updateControls" and "updateSettings" method that then in turn calls "updateThermostat" passing int the path string (setting or control) and the parameter map.

[raveydavies]

Fair comment. I was also thinking there must be a cleaner way to do it. I'll give it a try.

[raveydavies]

For future reference - the thermostat probably needs "updateControls", "updateSettings", "updateInfo", and possibly other functions as well, to replicate the thermostat local API. I'm planning on adding more functionality later, so there will be a few things to do here. I'll work on these things after this first "easy" (not easy :-)) piece of code gets included.

[digitaldan]

Suggested change

	public VenstarInfoData(double cooltemp, double heattemp, VenstarSystemState state, VenstarSystemMode mode,VenstarAwayMode away) {
	public VenstarInfoData(double cooltemp, double heattemp, VenstarSystemState state, VenstarSystemMode mode, VenstarAwayMode away) {

[digitaldan]

You might want to run mvn spotless:check in the venstartthermostst directory, it will catch formatting issues like this for you .

[raveydavies]

Done. Thanks for the advice.

[digitaldan]

Suggested change

	this.away=away;
	this.away = away;

[raveydavies]

fixed by mvn spotless:apply

[digitaldan]

Thanks for the PR! Looks pretty good! I left a few initial comments, i'll try checking it out later today and look over it again in more depth.

[digitaldan]

Hi, just tried the changes out , they look good to me! there are a number of formatting errors that will need to be fixed, fortunately running mvn spotless:apply from the venstarthermostat directory will fix all of them for you. Now that i look at the code, i notice a number of other issues that need to be fixed, i'll do that in my other PR at #10740 and ask to have that merged after your PR so you don't have to deal with merge conflicts .

[raveydavies]

Thanks @digitaldan for reviewing. Yes, I'd noticed the int temperature values as well - probably a good idea to change it, although the thermostat itself only allows 0.5degree steps. I will fix the formatting problems with my new code.

[raveydavies]

By the way @digitaldan I had a couple of questions about versioning, as this is my first project. 1. Do I need to give an updated version number for the java files? How does the version numbering work (I see 3.1.0-SNAPSHOT, what is that??)? It's confusing to me at least as an amateur programmer. 2. Can the new binding be made available outside of an official new release of openHAB or does it need to wait until the next release to be included?

[digitaldan]

There is nothing you need to do about versioning, the 3.1.0 tag in the pom file will get automatically incremented by our build system when we move to the next release. So you should not need to touch the pom file unless you are working on the binding in the middle of the project moving to a new release (and our scripts can't do the increase for you). Every build of the jar gets its own automatic version applied as well (like 202105241010).

Can the new binding be made available outside of an official new release of openHAB or does it need to wait until the next release to be included?

Our system will build the jar every time you push your changes to the PR, that Jar is available for some amount of time online and can be downloaded by anyone. Once the PR is merged into master, it becomes available as part of our nightly builds, which can be used by anyone. If a user is running an older version of OH (so not the nightlies) and wants to use the new Jar, they can download it and add it to the addons folder in the config directory, some times this is ok if there has not been any major changes to our core between their version and current, but lately thats not been the case due to library upgrades going on right now. But we release milestone release every couple weeks, which a lot of users use.

[jferfolia]

I'd like to give it a go adding fanstate and fansetting to this too. If the mentioned updateControls and updateSettings functions are developed, is it the intention to simply add everything possible with the API?

[raveydavies]

yes, I was planning to eventually add everything. I'm working on updateControl and updateSettings now.

[digitaldan]

Any update on this? I was holding off on submitting #10740 so you did not have to deal with merge conflicts, but we are getting close to doing a release and i would like to get it in soon.

[raveydavies]

@digitaldan I have just completed the code for separate /control and /settings functions (was a bit more difficult than I was expecting). I have just committed it so you will see it soon. I've spotted some problems with the existing binding as well, but they are not very simple to fix....one example is that the System mode on the thermostat cannot be changed if the schedule is On, but the binding does not reflect this. Probably something to fix in the next upgrade.

[raveydavies]

@digitaldan I assume that my new committed code is visible in the pull request. Let me know if I need to do something else.

[digitaldan]

So all the logic for settings should be scoped to the updateSettings method , no need to have it spread out in 2 functions.

[digitaldan]

all the logic for controls should be scoped to the updateControl method , no need to have it spread out in 2 functions.

[digitaldan]

Hi, what about something like this?

    private void updateSettings(VenstarAwayMode away) {
        // this function corresponds to the thermostat local API POST /settings instruction
        // the function can be expanded with other parameters which are changed via POST /settings
        Map<String, String> params = new HashMap<>();
        params.put("away", String.valueOf(away.mode()));
        VenstarResponse res = updateThermostat("/settings", params);
        if (res != null) {
            log.debug("Updated thermostat");
            // update our local copy until the next refresh occurs
            infoData.setAwayMode(away);
            // add other parameters here in the same way
        }
    }

    private void updateControls(int heat, int cool, VenstarSystemMode mode) {
        // this function corresponds to the thermostat local API POST /control instruction
        // the function can be expanded with other parameters which are changed via POST /control
        Map<String, String> params = new HashMap<>();
        if (heat > 0) {
            params.put("heattemp", String.valueOf(heat));
        }
        if (cool > 0) {
            params.put("cooltemp", String.valueOf(cool));
        }
        params.put("mode", String.valueOf(mode.mode()));
        VenstarResponse res = updateThermostat("/control", params);
        if (res != null) {
            log.debug("Updated thermostat");
            // update our local copy until the next refresh occurs
            infoData.setCooltemp(cool);
            infoData.setHeattemp(heat);
            infoData.setMode(mode);
        }
    }

    /**
     * Function to send data to the thermostat and update the Thing state if there is an error
     *
     * @param path
     * @param params
     * @return VenstarResponse object or null if there was an error
     */
    private @Nullable VenstarResponse updateThermostat(String path, Map<String, String> params) {
        try {
            String result = postData(path, params);
            VenstarResponse res = gson.fromJson(result, VenstarResponse.class);
            if (res != null && res.isSuccess()) {
                return res;
            } else {
                String reason = res == null ? "invalid response" : res.getReason();
                log.debug("Failed to update thermostat: {}", reason);
                goOffline(ThingStatusDetail.COMMUNICATION_ERROR, reason);
            }
        } catch (VenstarCommunicationException | JsonSyntaxException e) {
            log.debug("Unable to fetch info data", e);
            String message = e.getMessage();
            goOffline(ThingStatusDetail.COMMUNICATION_ERROR, message != null ? message : "");
        } catch (VenstarAuthenticationException e) {
            goOffline(ThingStatusDetail.CONFIGURATION_ERROR, "Authorization Failed");
        }
        return null;
    }

[raveydavies]

Makes sense. Not sure how I didn't arrive at that myself :-(. I'll update and commit again later.

[raveydavies]

@digitaldan I updated as per above and made a new commit. Thanks for the help!!

[digitaldan]

LGTM! One last thing for my review, you will need to sign off on your code commits as noted in https://www.openhab.org/docs/developer/contributing.html#sign-your-work , basically this is just adding something like

Signed-off-by: Joe Smith <joe.smith@email.com>

To your commit messages. You can see on this page that the DCO check is failing because of this, if you click on the Details link, it will give you instructions on how to retroactively do this.

I'm not sure why the build is failing in the checks, i don't think it has anything to do with this PR, so we can ignore that for now.

For future reference you can sign commits in real time by passing in the -s flag, so 'git commit -s ./something`

[digitaldan]

Acutally can you update the README with the new channel ? Just an FYI, When dealing with markdown tables, i use http://markdowntable.com/ to format them so i don't need to deal with manually spacing stuff so it looks ok in source form.

[raveydavies]

I will update the README. I tried to retroactively sign off the commits, but I think I completely failed (can you check?). Really have no idea what I'm doing.

[raveydavies]

When I follow the instructions, after I use "git rebase HEAD~10 --signoff", I get this:

error: cannot rebase: You have unstaged changes.
error: Please commit or stash them.

[digitaldan]

What does git status print out, are there modified files that have not been committed ?

[raveydavies]

yes there were 2 files in a different binding that have not been committed (no idea why). I have corrected that, but it still does not work....still trying

[raveydavies]

OK I think I did it, not sure though!!

[raveydavies]

I will update the README later today also.

[digitaldan]

Awesome! Thank you!

[raveydavies]

@digitaldan I updated the README. Still having problems with signoff, even though I am actually signing off my commits in Eclipse IDE. I always get a DCO error which needs to be resolved. I think it might be because I am trying to use my real name and e-mail address (as per OpenHAB guidance) instead of my GitHub ID. Anyway, I know how to fix it now, so I'll try to figure out how to stop it happening systematically.

[digitaldan]

LGTM!

[raveydavies]

@digitaldan Thanks!! Do I need to do anything else to close this PR (Do I need to "close with comment")? How / when can I install the new binding? Thanks for your coaching and help on the PR. Steep learning curve for me - my first ever contribution to an open source project and also my first ever use of git/github/eclipse. I'm better placed for the next round of work on the binding now.

[digitaldan]

This will still get reviewed by one other project member before getting merged. Once merged, our build system will package it up and it will be available as part of our nightly distribution and of course future milestone and stable releases. The review process is what keeps the openHAB code base secure, stable and of commercial quality ( or much better in my experience) .

Steep learning curve for me - my first ever contribution to an open source project and also my first ever use of git/github/eclipse. I'm better placed for the next round of work on the binding now.

You adapted very quickly through a pretty complicated system between java, maven, eclipse, git and github, so well done! Looking forward to more contributions from you !

[fwolter]

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

[digitaldan]

Thanks @fwolter , I did not see any blocking high checkstyle errors, and i believe the warnings were there before and not caused by this PR. Is there something else i missed? I have another PR I was going to submit after @raveydavies 's PR was approved where i can look at addressing those existing ones if thats the case.

[fwolter]

Newly introduced are at least these:

These should be fixed. If you want to fix them in a follow-up PR soon, I can merge this, though.

[digitaldan]

These should be fixed. If you want to fix them in a follow-up PR soon, I can merge this, though.

Yeah, let's do that, thanks! I'll submit the other PR today or tomorrow after i rebase with the merged PR and fix those checkstyles issues.

[raveydavies]

Thanks @digitaldan and @fwolter I appreciate your help as a first time contributor. I learned a lot and the next one will go smoother for sure. What should I have done to find that list of warnings posted by @fwolter ?

[fwolter]

After invoking mvn clean install, you can find the list here target/code-analysis/report.html.

[raveydavies]

thanks @digitaldan @fwolter . I am using Eclipse IDE and was (mistakenly) thinking all the maven stuff would be done automatically because I installed the openHAB environment there. Is it possible to make it automatic in Eclipse, or do I always need to remember to do mvn spotless:apply and mvn clean install? Maybe this conversation and a few of the conversations above belong in the openHAB forum rather than here, so I'd be happy to move it there.

[fwolter]

The code formatter in eclipse will produce a very similar result, than spotless. I set it as a save action.

I never tried that, but maybe the checkstyle and spotbugs plugin for eclipse work with the OH checks.

Actually this belongs to the developer documentation. Feel free to extend it!