[hdpowerview] Update positions after triggering scene/scene group

[jlaur]

When triggering a scene or scene group, some shades will have new positions. These new positions are available in the hub immediately after triggering the scene or scene group, i.e. before the shades will reach those new positions.

Previously it would take up to one minute to have the positions refreshed. Now it will happen a few seconds after the scene or scene group has been triggered.

Fixes #11697

Signed-off-by: Jacob Laursen [email protected]

Test documentation

Before change:

2021-12-12 21:26:42.228 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'PowerViewHub_Scene_Office_Up' received command ON
2021-12-12 21:27:08.782 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Blind9_Position' changed from 100 to 0

After change:

2021-12-12 21:50:22.834 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'PowerViewHub_Scene_Office_Up' received command ON
2021-12-12 21:50:26.105 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Blind9_Position' changed from 100 to 0

And back:

2021-12-12 21:50:29.706 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'PowerViewHub_Scene_Office_Down' received command ON
2021-12-12 21:50:32.955 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'Blind9_Position' changed from 0 to 100

[jlaur]

FYI @andrewfg, @arroyoj

[andrewfg]

I think equals() and == compile to the same thing. ??

[andrewfg]

Also wondering if you should replace the pollshades() calls with scheduler.submit([ -> | pollshades()]) ??

[jlaur]

I think equals() and == compile to the same thing. ??

See https://stackoverflow.com/questions/1750435/comparing-java-enum-members-or-equals

So it's faster and safer at compile-time as well as runtime.

[jlaur]

Also wondering if you should replace the pollshades() calls with scheduler.submit([ -> | pollshades()]) ??

Done.

[arroyoj]

@jlaur, thanks for putting this together. It all looks good to me. Do you have a JAR available so that I can try using it?

[jlaur]

@arroyoj - of course: https://drive.google.com/file/d/1TYpolCxIAeO3FaMwBt6VHhnFey7F8G-w/view?usp=sharing

[arroyoj]

@jlaur, thanks for sharing the JAR. I tested it out on my machine, and it worked great. Similar to your testing, I saw that the shade positions got updated in ~4 seconds after triggering a scene, rather than up to 1 min like before. I didn't see any issues. Thanks again for this fix.

[jlaur]

Also wondering if you should replace the pollshades() calls with scheduler.submit([ -> | pollshades()]) ??

@andrewfg - please see thread from @lolodomo's comment. Decided to revert and go back to synchronous HTTP call for simplicity. Only difference is that we have two sync calls where previously we had one. With the previous changes we would have one sync and one async, and it became messy.

[andrewfg]

Decided to revert and go back to synchronous HTTP call for simplicity.

I don’t agree. I am not so much concerned about an extra HTTP call. What concerns me is that you are making re-entrant calls to the OH core; whilst you are still within handleCommand() for one channel, you run pollShades which makes multiple updateChannel callbacks to the core; in fact one call back from each channel and each shade in your system. So your handleCommand could potentially cause the OH core to block on itself. Not good.

[jlaur]

Decided to revert and go back to synchronous HTTP call for simplicity.

I don’t agree. I am not so much concerned about an extra HTTP call. What concerns me is that you are making re-entrant calls to the OH core; whilst you are still within handleCommand() for one channel, you run pollShades which makes multiple updateChannel callbacks to the core; in fact one call back from each channel and each shade in your system. So your handleCommand could potentially cause the OH core to block on itself. Not good.

Ahh - thanks for this explanation. New approach needed, marking PR as draft.

[andrewfg]

Perhaps your prior attempt was overkill; I understand why you had to wrap the asynch pollShades call in an exception handler, but perhaps it is easier to let the existing code do the heavy lifting rather than reinventing the wheel; so after handleCommand triggered the scene, you could just 'submit' a call to poll() as follows..

scheduler.submit(() -> {
    poll();
});

[jlaur]

Perhaps your prior attempt was overkill; I understand why you had to wrap the asynch pollShades call in an exception handler, but perhaps it is easier to let the existing code do the heavy lifting rather than reinventing the wheel; so after handleCommand triggered the scene, you could just 'submit' a call to poll() as follows..

scheduler.submit(() -> {
    poll();
});

poll will perform four calls in total:

• fetch shades
• fetch scenes
• fetch scene collections
• fetch scheduled events

and we need only one of them. Still, it would be much simpler like you described, so could be refactored into something like that. My concern is that if we schedule a fire-and-forget call like that, we might have a problem in case dispose() is called right after while operation is still ongoing in future, for which we don't have a hook for cancelling it?

[jlaur]

My concern is that if we schedule a fire-and-forget call like that, we might have a problem in case dispose() is called right after while operation is still ongoing in future, for which we don't have a hook for cancelling it?

Perhaps we could reschedule existing future as a compromise? We would still fetch everything, but at least we would postpone next ordinary poll for another minute:

    private void rescheduleSoftPoll() {
        ScheduledFuture<?> future = this.pollFuture;
        if (future != null) {
            future.cancel(false);
        }
        logger.debug("Rescheduling poll every {}ms", refreshInterval);
        this.pollFuture = scheduler.scheduleWithFixedDelay(this::poll, 0, refreshInterval, TimeUnit.MILLISECONDS);
    }

And we would have the future cancelled from dispose.

[andrewfg]

I think there is no need to worry about that: firstly the sheduler is part of the core, so if the core is shutting down, it will do so cleanly (it won't start un-run tasks, and it won't interrupt already running ones); and secondly the poll method is marked as synchronized so it cannot be called in parrallel.

Obviously if you unpack the poll() method, you will need to think carefully about placing the synchronized marker on any of its parts, since you would be introducing a risk of self blocking between regular polls and this special poll.

In summary I would still just submit poll; who cares if its updating other channels? It is essentially just doing another regular poll a bit earlier than the normal polling cycle would do so..

[andrewfg]

Perhaps we could reschedule existing future

Yes, that would work.

[jlaur]

Perhaps we could reschedule existing future

Yes, that would work.

Done.

[andrewfg]

LGTM

[andrewfg]

LGTM

[lolodomo]

LGTM

[andrewfg]

@lolodomo thanks for reviewing and merging the HDPowerView PR's of @jlaur (which I had approved); but could you please also review my own PR #11698 so that we can keep things in sync?

[lolodomo]

@lolodomo thanks for reviewing and merging the HDPowerView PR's of @jlaur (which I had approved); but could you please also review my own PR #11698 so that we can keep things in sync?

Really big one. Probably too much to expect a fast review from myself