[hdpowerview] Add support for calibrating a shade

[jlaur]

Fixes #11767

Signed-off-by: Jacob Laursen jacob-github@vindvejr.dk

This pull request adds support for calibrating a shade.

Some refactoring had to be done in order to avoid boilerplate code. While doing this I found some additional issues that have been addressed:

• When setting shade positions, response was not used to update channels, but instead a new request was made to refresh positions. This has been fixed to use the positions in the response, so new position will be updated much faster.
• Since there is now multiple code paths for updating shade positions, initialization of capabilities had to be refactored as well.
• Unused id variables in request classes removed.
• 10 second delay in refreshing positions for no apparent reason.
• Shade thing status will now initially be set to UNKNOWN until data for shade has been received. This is more transparent and also fixes situation when a shade would be configured with wrong Shade ID and initially be ONLINE, then OFFLINE after some time.
• Any pending/ongoing tasks are now killed when disposing handler.
• Shade ID is now only parsed after changing configuration.

Tests

Position update after UP/DOWN command:

2022-01-09 23:57:42.302 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'Blind9_Position' received command DOWN
2022-01-09 23:57:42.306 [INFO ] [penhab.event.ItemStatePredictedEvent] - Item 'Blind9_Position' predicted to become DOWN
2022-01-09 23:57:42.324 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Blind9_Position' changed from 83 to 100

STOP command after UP/DOWN:

2022-01-10 00:07:03.942 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'Blind9_Position' received command UP
2022-01-10 00:07:03.946 [INFO ] [penhab.event.ItemStatePredictedEvent] - Item 'Blind9_Position' predicted to become UP
2022-01-10 00:07:03.957 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Blind9_Position' changed from 100 to 0
2022-01-10 00:07:12.093 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'Blind9_Position' received command STOP
2022-01-10 00:07:17.041 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Blind9_Position' changed from 0 to 65

[andrewfg]

When setting shade positions, response was not used to update channels, but instead a new request was made to refresh positions. This has been fixed to use the positions in the response, so new position will be updated much faster.

@jlaur in one of my own recent PR's I addressed and solved this issue here, in the moveShade() method, so perhaps you trying to "double solve" it ?? => Please check and advise.

[jlaur]

in one of my own recent PR's I addressed and solved this issue here, in the moveShade() method, so perhaps you trying to "double solve" it ?? => Please check and advise.

Thanks for being vigilant! You definitely solved it. You solved it by using the calculated new position(s) to update the channels immediately. This works fine, and probably it will always work fine, unless we have a bug trying to set invalid positions which would then be rejected and positions would actually be unchanged.

Now it's slightly refactored to use positions from the response for being (also) immediately updated. This should not change anything, as the returned values should be the same as the new positions requested. This is also what I have seen in my tests - as documented in PR description. The reason for this change is that I noticed multiple REST services actually returning an updated Shade object, and we didn't use this "free information" at all. This way we can now update positions the same unified way after each REST call returning a Shade object with position information.

[andrewfg]

@jlaur you have made some quite extensive changes which are difficult to track via the GitHub browser; so instead I will have to download and build the jar in my IDE and test it on my own installation; however here a a few trivial comments concerning adding new @return declarations to the java docs.

[andrewfg]

Again, trivial, but I saw a couple of Maven build warnings (however as they refer to JSON objects, they might not be resolvable; but see Shade/ShadeData..)

[andrewfg]

@jlaur now I have studied your code in my Eclipse IDE (where it is much easier to understand your changes), and I can confirm that the changes look good to me (except the trivia noted above).

The new JAR is now running on my live system, and all the properties and channels are updating correctly; also I tried a few commands which worked too. However if you want me to try any specific tests, please let me know.

[jlaur]

you have made some quite extensive changes which are difficult to track via the GitHub browser;

Yes, first commit is kind of big because I didn't want to introduce boilerplate code in order to remove it again in next commit. So the refactoring already started in this single commit. However, after that I tried to provide explicit commits for each distinct change. It might be easier to read commit by commit?

so instead I will have to download and build the jar in my IDE and test it on my own installation; however here a a few trivial comments concerning adding new @return declarations to the java docs.

I will have a look and try to fix them. Thanks for the help!

[andrewfg]

My prior post crossed with yours.

[jlaur]

The new JAR is now running on my live system, and all the properties and channels are updating correctly; also I tried a few commands which worked too. However if you want me to try any specific tests, please let me know.

Thanks, Andrew! Besides updating the examples in the README, I plan to improve handling of dispose while there are still ongoing requests. It works correctly now, but an error is logged which could be confusing. Besides that I don't plan to do anything major, so you should be able to catch up just reading the next few commits. Hopefully I will get it done tonight, and make the pull request ready for review after that. :-)

[jlaur]

@andrewfg - and a little teaser: Of course I got some new ideas when working with this and testing it. invoke() is synchronized, so only one request at a time. A lot of calls are continuously made. So the more important calls (like "stop shade") are queued and can only happen after other calls like updating the scene list every minute. I'll create an issue, and we might brainstorm about solutions if you are interested and also see a problem.

[andrewfg]

^
Obviously invoke() has to remain a synchronised method, but it sounds like you want the scheduler to have a Double Ended Queue “DEQue”. It would mean ditching the OH built in scheduler, and implementing something else within the binding. I think there are DEQue schedulers already existing in various standard libraries..

[jlaur]

Again, trivial, but I saw a couple of Maven build warnings (however as they refer to JSON objects, they might not be resolvable; but see Shade/ShadeData..)

I'll need to dig a little deeper to find a good solution for this. To be honest, I'm relatively new to Java and I have a hate/love relationship with all this null annotation stuff. Having @NonNullByDefault for classes used solely for serialization is kind of annoying since everything within needs to be annotated as @Nullable anyway. So in this context nullable is not an exception from default, it's a rule. I will address it since I introduced it, but maybe not in context of this PR (not introduced in this PR).

[andrewfg]

find a good solution for this.

Perhaps I will make a proposal tomorrow..

[andrewfg]

I'll need to dig a little deeper to find a good solution for this. To be honest, I'm relatively new to Java and I have a hate/love relationship with all this null annotation stuff.

Attached are your de-serializer classes with @null annotations added.

HDPowerView Null Annotations.zip

Using these annotations, does in fact reveal some potential NPE failure points in your code in HDPowerViewHubHandler.updateFirmwareProperties() where additional null checks on potential de-serializer errors should be made as follows..

EDIT: use exceptions rather than return with logger.warn()

    private void updateFirmwareProperties() throws JsonParseException, HubProcessingException, HubMaintenanceException {
        if (firmwareVersions != null) {
            return;
        }
        HDPowerViewWebTargets webTargets = this.webTargets;
        if (webTargets == null) {
            throw new HubProcessingException("Web targets not initialized");
        }
        FirmwareVersion firmwareVersion = webTargets.getFirmwareVersion();
        FirmwareVersions firmwareVersions = firmwareVersion != null ? firmwareVersion.firmware : null;
        if (firmwareVersions == null) {
            throw new JsonParseException("Unable to get firmware version.");
        }
        this.firmwareVersions = firmwareVersions;
        Firmware mainProcessor = firmwareVersions.mainProcessor;
        if (mainProcessor == null) {
            throw new JsonParseException("Main processor firmware version missing in response.");
        }
        logger.debug("Main processor firmware version received: {}, {}", mainProcessor.name, mainProcessor.toString());
        Map<String, String> properties = editProperties();
        String processorName = mainProcessor.name;
        if (processorName != null) {
            properties.put(HDPowerViewBindingConstants.PROPERTY_FIRMWARE_NAME, processorName);
        }
        properties.put(Thing.PROPERTY_FIRMWARE_VERSION, mainProcessor.toString());
        Firmware radio = firmwareVersions.radio;
        if (radio != null) {
            logger.debug("Radio firmware version received: {}", radio.toString());
            properties.put(HDPowerViewBindingConstants.PROPERTY_RADIO_FIRMWARE_VERSION, radio.toString());
        }
        updateProperties(properties);
    }

[jlaur]

Using these annotations, does in fact reveal some potential NPE failure points in your code in HDPowerViewHubHandler.updateFirmwareProperties() where additional null checks on potential de-serializer errors should be made as follows..

Thanks @andrewfg, I'll have a look and provide a separate PR for resolving these existing (although recently introduced) issues tonight. I'll also test what how gson will react when having a @NonNull attribute that is omitted in the JSON to be parsed. I suppose defining the contract more clearly with some attributes being mandatory might even increase the code quality as we could have unified exception handling instead of null checks everywhere. So might extend the scope a bit, at least I need to investigate a few things to provide a clean solution.

[andrewfg]

how gson will react when having a @nonnull attribute that is omitted in the JSON to be parsed

I guess it will do the following..

1. throw JsonParseException for any @nonnull objects,
2. return a null value (with no error) for @nullable objects
3. return 0 / false for any primitives

[jlaur]

I guess it will do the following..

1. throw JsonParseException for any @nonnull objects,
2. return a null value (with no error) for @nullable objects
3. return 0 / false for any primitives

Let's continue in #12027. It will not be part of this PR as it's unrelated, and also I would like to extend the scope to fix not only the annotations, but also error handling and contract verification in general for all methods.

[andrewfg]

LGTM

[jlaur]

@openhab/add-ons-maintainers - beowulfe is listed as codeowner for the hdpowerview binding, but the user doesn't exist on GitHub anymore. What's the procedure - just remove? Secondly, @andrewfg, would you like to be added as you have been quite actively contributing to this binding? I have added myself as I'm quite into the binding at this point and feel responsibility for it.

[jlaur]

@openhab/add-ons-maintainers - beowulfe is listed as codeowner for the hdpowerview binding, but the user doesn't exist on GitHub anymore. What's the procedure - just remove?

@openhab/add-ons-maintainers - I just did a quick check, and he seems to be the only one missing as GitHub user - but listed as owner for two bindings:

#!/usr/bin/python3

import requests

github_user = 'jlaur'
github_token = ''
github_root = 'https://api.github.com'

file = open('CODEOWNERS', 'r')
lines = file.readlines()
file.close()

headers = {
        'User-Agent': github_user,
        'Authorization': 'token ' + github_token
}

for line in lines:
        if line[0:9] == "/bundles/":
                parsed_line = line.split()
                for idx, val in enumerate(parsed_line):
                        if idx == 0:
                                binding = val
                        else:
                                user = val.lstrip('@')
                                if user == "openhab/add-ons-maintainers":
                                        continue
                                url = github_root + '/users/' + user
                                response = requests.get(url, headers=headers)
                                if response.status_code == 404:
                                        print(binding + ": User " + user + " does not exist.")

Result:

$ ./check_codeowners.py
/bundles/org.openhab.binding.hdpowerview/: User beowulfe does not exist.
/bundles/org.openhab.io.homekit/: User beowulfe does not exist.

[jlaur]

CI build failed with:

/opt/hostedtoolcache/maven/3.8.4/x64/bin/mvn: 194: exec: /opt/hostedtoolcache/Java_Zulu_jdk/11.0.13-8/x64/bin/java: not found
Error: Process completed with exit code 127.

[lolodomo]

LGTM

[lolodomo]

@wborn for help with the failing CI build.
Is it safe to merge as the other build succeeded?

[jlaur]

Is it safe to merge as the other build succeeded?

I see that a new Jenkins build was scheduled, but previous one succeeded. Also CI build succeeded before last two commits. So IMHO it should be safe.