andrewfg
commented
2 years ago Please consider also Venetian blinds, where the secondary position (vane twist angle) is (must be) always UNDEFINED until the primary shade is fully closed.
Closed arroyoj closed 2 years ago
andrewfg
commented
2 years ago Please consider also Venetian blinds, where the secondary position (vane twist angle) is (must be) always UNDEFINED until the primary shade is fully closed.
andrewfg
commented
2 years ago I wonder if this is related to the getter function returning UnDefType.UNDEF; as shown in the code extract below?
Note: these returns are intentionally present to identify the unexpected edge case of an unknown coordinate system, or position kind.
arroyoj
commented
2 years ago It is related to the getter function returning UNDEF on line 225, but the reason it returns UNDEF is because the hub stops sending position2 and posKind2 after it receives a JSON command with only position and posKind. I think this is really a bug in the Powerview Hub, but I'm guessing there is little to no chance Hunter Douglas will fix it, so I'm hoping that the OpenHAB binding can work around it.
Maybe one solution would be a config option that enables the get --> update --> set operation for the primary rail, just like is done now for the secondary rail. This option could be disabled by default, so no change in current behavior, but users with dual-rail shades could manually enable it. We could also potentially automate enabling this behavior by checking for the presence of the secondary rail right after a hard refresh (ie, posKind2 would be set to 2 in the return JSON for that shade).
andrewfg
commented
2 years ago Ok, that is definitely a bug. It is made more complicated by my following observation. Namely if I send a notional position2 command to my shades that do NOT have a second rail, the hub continues to include that position2 value in its JSON response, until the next hard refresh occurs.
Therefore/anyway I think the solution is that the binding's moveShade() method should pass a different instantiation of the ShadePosition class depending on whether the shade actually supports a secondary position -- even in cases where only the primary position is being moved.
ShadePosition with a primary position only (i.e. secondary is NULL)ShadePosition having both primary and secondary positionsSo the question is how to determine between the two cases? I think there are several possible ways to do this as follows. @arroyoj can you please give me your thoughts on this?
Option A
Execute either 1. or 2. above depending depending on some attribute of the Shade.
I think we could use either the type or capabilities parameter. I think type might be quite difficult to support because the list might be long (your shades are type 9 but mine are 43 resp. 44) and it might be extended whenever Hunter Douglas adds new products. So I am interested in capabilities (yours have 7 and mine have 0 resp. 1) because perhaps this is a bit flag (yours 111 and mine 000 resp. 001) - where maybe we can ask HD for the meaning of each bit in the flag?
Option B Execute either 1. or 2. above depending on a Config Param. The advantage of this is that users could override it. But really the Param should be initialized automatically when the Shade is first discovered or the Thing is first instantiated. => So see Option C below.
Option C Essentially this is Option B where the Config Param is initialized via Option A.
Option D
Remember if a position2 was returned after the hard refresh, and depending on that, we would execute either 1. or 2. above.
Option E
To hell with the consequences! Always execute with an instance of ShadePosition having both a primary and a secondary position -- even if the shade does not have a second rail. In my observations sending a position2 does not seem to do any obvious harm to my shades. And after the next hard refresh the value disappears anyway by itself. However my only (slight) concern is that it might do some hidden harm to my shades, perhaps over time, or perhaps to shades of another type than yours or mine??
arroyoj
commented
2 years ago @andrewfg, thanks for confirming the buggy behavior of the hub. I also did some additional tests. Like you, I found that I could send the hub JSON positions that made no sense for my shades (ex: setting a vane position on my dual-rail shades) and the hub would just returned the nonsense JSON object on the next GET. I also discovered that the hub doesn't actually require both rail positions to be set at the same time to move the secondary rail. It is valid to just set the secondary rail (posKind=2), as long as it is sent in position1: {"shade": {"id": 58131, "positions":{"posKind1":2,"position1":0}} is valid and moves the secondary rail appropriately, and annoyingly leads the hub to "forget" the primary rail position.
In terms of the binding implementation and the strategy for fixing this issue, these results demonstrate that the binding cannot assume that position1/posKind1 is the primary rail and position2/posKind2 is the secondary rail. Instead, when parsing the shade position JSON data from a GET, it needs to check the value of posKind1, and use that to determine which rail it refers to, then (if present) check the value of posKind2. In fact, it is even valid for posKind1 and posKind2 to both refer to the same rail (ie, a single-rail shade can actually return a value for position2/posKind2 as you observed).
Therefore/anyway I think the solution is that the binding's
moveShade()method should pass a different instantiation of theShadePositionclass depending on whether the shade actually supports a secondary position -- even in cases where only the primary position is being moved.1. Secondary **_NOT_** supported: instantiate a `ShadePosition` with a primary position only (i.e. secondary is NULL) 2. Secondary **_IS_** supported: instantiate a `ShadePosition` having both primary and secondary positions
I really like your solution above, and I think it is probably the best way forward.
In terms of your Options for determining whether to do 1 vs 2 for a given shade, I think Option E should be possible, but it might be hard to implement properly without access to other shade types for testing. My hunch is that shades with vanes may also use position2, just with posKind2 = 3, and sending irrelevant values in position2/posKind2 could lead to a different version of this current issue when those values are retrieved from the hub.
Option A would be my preference, but I worry there could be times when it fails, especially given the incomplete documentation from Hunter Douglas. Therefore, I think Option C is probably the best. I think we should be able to properly detect the correct shade type most of the time, but giving the user the ability to override it at the Thing config level if necessary would be great. Also, I would suggest the config option be some sort of Enum type, as I could imagine case 1 vs 2 expanding in the future, especially if we can properly separate out shades with vanes behavior from single rail behavior (they are currently both treated as case 1).
In terms of how to initialize the config option for 1 vs 2 behavior, I think the best would be if we can decipher the capabilities parameter. I confirmed that both of my types of dual-rail shades (which have types 8 and 9) have capabilities 7.
If we can't decipher the capabilities parameter, than I would suggest this sequence during Thing discovery: hard refresh, get shades JSON, check both posKind1 and (if present) posKind2, if either posKind1 or posKind2 is equal to 2, then assume the shade has a secondary rail. Just going off the presence of position2/posKind2 (as in Option D) could be wrong if vane shades return posKind2 for the vane control or a future shade type uses position2/posKind2 for something else.
Thanks so much for working on this.
andrewfg
commented
2 years ago I will contact Hunter Douglas to see if they can give me the details of the capabilities flag. It might take a few days, so let’s pause the discussion until they respond.
andrewfg
commented
2 years ago I did not yet get a response from Hunter Douglas yet. However from doing some extensive Googling of shade JSON payloads, I was able to build the following (tentative) table of capabilities. @arroyoj any further inputs would be appreciated.
arroyoj
commented
2 years ago That's an impressive table! I wish I had more to add to it. My only thought, though, is that figuring out the relationship between capabilities and shade behavior looks pretty challenging without guidance from Hunter Douglas. I tried to see if I could infer some sort of bitmapping, but my hunch is they are just using the numbers as is. Using the capabilities parameter definitely seems like the right way to define different shade behaviors, but I'm wondering if searching for a secondary position (posKind == 2) in the JSON might end up being more reliable.
I'd be interested to know if the "Duolite Lift" and "Duolite Lift and Tilt 90" really have a secondary rail, or if they have repurposed poskind == 2 for something else? I couldn't find a description of those shades when I searched.
andrewfg
commented
2 years ago my hunch is they are just using the numbers
Indeed. And I found that list here -- although subject to direct confirmation yet.
wondering if searching for a secondary position (posKind == 2) in the JSON might end up being more reliable.
I think I will do both. And issue a logger.warn() (or info()) if the two do not match. And furthermore I intend to add a shade Thing Property showing the current values of the type and capabilities attributes.
if the "Duolite Lift" and "Duolite Lift and Tilt 90" really have a secondary rail
I got the JSON data here and looking again at the JSON it seems to only have position1 values. => So tentatively, I will delete the second rail from my table.
andrewfg
commented
2 years ago I think I will do both
Apparently shades can return capabilities:-1 in case of error. Or a NULL value on older hub firmware. So definitely in those cases I would fall back on looking for position2 with posKind:2..
andrewfg
commented
2 years ago @arroyoj my plan is to resolve your two issues with two separate pull requests. First I will resolve Issue #11557 because it will include preparations to set the foundations for solving Issue #11593.
The Issue #11557 solution will in fact contain three parts..
type and capabilities. The purpose is to allow different functionality depending on the respective capabilities. It will also produce a logger.warn() message to identify if a shade has a type or capabilities which is not in the database, and prompt the user to inform developers, so that the feature set can be adapted/extended.type and capabilities. (And display 'unknown' if not in the database). Also detect if a shade has posKind=2 and produce a logger.warn() message if the secondary shade support thus indicated is different than that indicated by capabilities.The Issue #11593 solution will build on items 1. and 2. above in order to implement the differentiated functionality that you require. And probably implement something similar for shades with reversed primary rails.
arroyoj
commented
2 years ago @andrewfg, that sounds like a great plan. Thanks for taking on both of these issues. Please let me know if you need me to do any testing.
andrewfg
commented
2 years ago ^ FYI
andrewfg
commented
2 years ago @arroyoj I made a start in creating the database of known shade types and their respective capabilities. And as you can see from the screenshot above, I have implemented those as properties of the respective shade Things. If you are interested, you can see the new code in ShadeCapabilitiesDatabase.java -- in particular the CAPABILITIES_DATABASE and TYPE_DATABASE which should hopefully be self explanatory.
Note: I have not (yet) implemented the functionality to fix your bug..
arroyoj
commented
2 years ago The new Thing properties for type and capability look good, along with the corresponding databases. In the CAPABILITIES_DATABASE, I noticed that 0 ("Bottom Up"), 3 ("Vertical"), and 8 ("Duolite Lift") are listed as having vane support. Just from the descriptions of those (ie, not including "Tilt" in the description), I'm not sure if they truly support vanes in the JSON (ie, posKind=3). I have no way to test that, though. I'm guessing you kept vanes() for those to ensure that the current behavior for those shade types is maintained. That makes sense and seems like the best/safest approach, especially since there doesn't seem to be a real downside to sending posKind=3 to shades that don't support it. Maybe it is something to look into in the future if we can collect more JSON data from a wider variety of shades.
I'm also interested in how you are planning to use the primaryInverted property with Top-Down shades (capability 6). While those shades do have an inverted sense of Open vs Closed state, the rail positions are still in the same coordinates as with Bottom-Up shades. That is, the getter and setter for Top-Down shades should still work the same as for Bottom-Up shades. Maybe a different name for this property should be used? Perhaps primaryStateInverted to help indicate that the positions are not inverted?
andrewfg
commented
2 years ago I noticed that 0 ("Bottom Up"), 3 ("Vertical"), and 8 ("Duolite Lift") are listed as having vane support
@arroyoj on @bujvary's gitHub repo there is a table (see below) which certainly indicates that shades with capabilities:0 do not have vanes; so perhaps I will delete that functionality from our binding database; .. and see if anyone complains..
andrewfg
commented
2 years ago ^ so it now has two properties jsonHasSecondary & jsonHasVanes (which are detected by parsing JSON responses (when refresh=true)), and it compares those values with the database capabilities, and issues a logger.warn() if the two don't match..
andrewfg
commented
2 years ago @arroyoj I made a new branch on my repository HERE which has the probable fix for this issue. I have uploaded the respective new JAR file to that link; so could you please download it to your addons folder, and confirm that a) it solves the bug, and b) it did not break anything else? => Many thanks in advance :)
In addition to fixing the bug, this version also adds a few other things. I think you may be interested in item 5. in particular.
arroyoj
commented
2 years ago @andrewfg, thanks for the link to the updates and JAR for testing. I'll test it out tonight and get back to you soon. Thanks for including item 5 as a bonus. It will definitely improve the behaviour for dual rail shades.
arroyoj
commented
2 years ago @andrewfg, I tested out the JAR from the link above, and it is great! I ran through several tests, and the shades and OpenHAB behaved as expected, and matched the behavior with the PowerView App. The bug from my initial report was fixed, my attempts to set rails past each other worked as expected (a few comments below), and as far as I could tell, nothing broke. A few times, when I was using a slider UI, I moved the slider and nothing happened, but when I checked the logs, OpenHAB had not registered a command being sent, so I think that was an issue with the web UI perhaps. I could never consistently reproduce it, but if I can figure it out, I will let you know.
After removing the old JAR and putting in the new JAR, I checked my shade properties, and everything looked correct. I checked both my type 8 and type 9 shades, and both returned the correct properties. I then went about testing a single shade by issuing commands through the slider UI and roller shutter UI, as well as sending "swapped" position data (ie, sending primary rail data in position2 and vice versa). I've attached a TRACE log for your information, with some annotations added in: shades_trace_2021-12-02.txt.
Using the slider UI, the bug is now gone (setting the primary rail did not unset the secondary rail) and trying to move the lower rail above the upper rail resulted in the lower rail stopping at the upper rail, and vice versa. I do have one suggestion, though. When moving past a rail, the binding does not learn that the expected position was not set until after the normal soft refresh. I think it would make sense to immediately update the shade's channels after sending a command. One thought would be to do this using the returned JSON response from the PUT, but if not, maybe triggering a refresh of just that shade right after sending any command would be appropriate. It's not a big deal since the normal refresh happens within a minute by default, but you can see in the log the OpenHAB states do bounce around a bit when overshooting or using the roller shutter UI (see below). Also, interestingly, my very first command to item LivingRoom_Shade5_Top was sent right as the refresh cycle was occurring, so the item state went from 0 --> 30 (from my command) --> 0 (from the refresh that was in progress when command was sent) --> 30 (after additional refresh)
After the slider tests, I manually sent "swapped" position data directly to the hub with my web browser, to make sure the new binding would handle that properly (the old binding did not). After sending the swapped data and waiting for the soft refresh of the hub, the positions for each rail were interpreted correctly. I then sent a command to make sure the setter wouldn't get messed up, and it correctly set the position and corrected the "swap", returning position1 to primary rail and position2 to secondary rail.
Lastly, I tested a roller shutter UP/DOWN UI. This worked as expected, but also suffers from the jumpiness mentioned with the slider. I set the upper rail into the middle of the window, and sent the UP command to the lower rail. The item immediately registered a "0%" state as expected, even though the binding sent a JSON command that told it to move to the upper rail's position, and not past it. After the next refresh, the position was correct.
While the behavior seems to be perfect, I had one small suggestion about the code that I think could make it clearer. In CoordinateSystem.java, I would suggest calling the position kind enum values just PRIMARY and SECONDARY. For the primary rail, JSON 0 can mean closed for a bottom-up single-rail shade, but open for a top-down single-rail shade, while OpenHAB 0% is the opposite, so it's a bit confusing. And for the secondary rail, JSON 0 (and OpenHAB 0%) would mean the rail is up / closed. Along with that, the comments could probably use some fixing up. For example, the comments on lines 54-60 for top-down and bottom-up appear to be reversed (ie, when fully up, the top-down shade is closed and the bottom-up shade is open). And the comments on lines 70-76 are confusing where "ZERO_IS_OPEN" has position: 0 linked to "closed".
Thanks so much for implementing all these changes. I'll keep this version running and let you know if I encounter any issues with daily use, but so far, it looks great!
arroyoj
commented
2 years ago Quick follow-up to my comment about updating state after sending commands: I would also suggest doing a soft refresh after any scene is triggered. As far as I can tell from my testing, the hub responds to a GET /shades call with the correct JSON data for all the shade states immediately after a scene is triggered, even while the shades are still moving, so a soft refresh should be sufficient.
andrewfg
commented
2 years ago Many thanks @arroyoj for the testing and the feedback. I have some comments to your comments as follows..
it would make sense to immediately update the shade's channels after sending a command. do this using the returned JSON response from the PUT .. if not, maybe triggering a refresh
Yes, that makes sense. I will do it by calling requestRefreshShadePosition() within moveShade() in the case that the positions will have been over-ridden (or swapped). It makes the code far simpler. => I will shortly make a new build JAR for you to test.
one small suggestion about the code that I think could make it clearer
Yeah. Actually Andy Lindter's original code contribution did just that e.g. 'PRIMARY' and 'VANE'. But when I added the secondary rails, I modified the enum names to make it easier (at least for me) to understand the meanings UP=OPEN resp. DOWN=CLOSED. However in the near future, when Issue #11593 will be addressed, the meanings will change again to UP=UP resp. DOWN=DOWN, so the present enum value names would indeed no longer be appropriate. (This is the prime reason why I am decoupling the fixes of Issue #11557 from Issue #11593). Therefore I want to defer changing the enum value names until the fix for the latter Issue. => Is that Ok with you?
the comments could probably use some fixing up
Ditto above.
also suggest doing a soft refresh after any scene is triggered
This is certainly an excellent idea. But I think your request also applies to @jlaur 's Issue #11533 concerning 'Scene Collections' and his respective PR #11534 that solved it. So I suggest two things: 1) to NOT address your suggestion in this current Issue resp. my soon to be coming PR to solve it, but instead 2) open a new Issue (resp. new future PR to solve it), and 3) therefore get @jlaur 's feedback resp. participation thereon. => Is that Ok with you?
andrewfg
commented
2 years ago calling requestRefreshShadePosition() within moveShade() in the case that the positions will have been over-ridden (or swapped)
The new JAR is HERE (same link as before).
andrewfg
commented
2 years ago PS I just realised requestRefreshShadePosition() does a 'hard' refresh; which takes some time to execute so maybe too slow; and perhaps 'overkill' too. => I think I will try another build with only a 'soft' refresh..
andrewfg
commented
2 years ago The new JAR is HERE (same link as before). NOTE: it does not even do a soft refresh; as your original suggestion, it just updates the channels with the PUT data..
arroyoj
commented
2 years ago @andrewfg, deferring the enum and comment cleanups to be part of fixing #11593 sounds good. I also agree that it makes sense to do a separate issue to address the idea of a soft refresh after scenes or scene collections are triggered.
I'm going to download and test out your new JAR, but I was looking at the code, and I was wondering if updating the binding state after a position change could be even simpler. It looks like you are identifying cases where the positions sent to the hub are different than the commands received, but is there any downside to just always updating the binding state with the newPosition data? That way, the code in ShadePosition.java would stay simple, and if other work in the future also resulted in overriding the percent values somehow, there would be less chance of re-introducing this problem. Sorry if there is something I'm not seeing.
andrewfg
commented
2 years ago is there any downside to just always updating the binding state
Good point. I just uploaded a new JAR (same link as above) that contains the (re-) simplified code.
openhab-bot
commented
2 years ago This issue has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/hd-powerview-binding-issue-openhab-3-3-0/136921/4
Expected Behavior
Moving/setting the primary rail of a dual-rail shade should not change the binding's state for the secondary rail position.
Current Behavior
When the primary rail of a dual-rail shade is moved (either using up/down commands or by sending a PercentType command), the secondary rail position becomes UNDEF within OpenHAB. As expected, the secondary rail itself does not move. However, the secondary rail state is lost. Eventually, the binding will tell the hub to do a "hard refresh" of each shade, and then the secondary rail state will be properly restored.
This bug seems to be due in part to a quirk of the Powerview Hub. When a shade position command is PUT to the hub, it seems to cache that exact position object, and returns it on future polling. So, if a command is sent with only the primary rail position set, the hub will return only the primary rail position with each poll until the hard refresh is performed.
I captured a TRACE level log showing this behavior. To make the logging a bit easier, I disabled the Thing for all but one of my shades, so only shade ID 58131 is getting updated. In addition, to speed up the state changes, I set the hard refresh interval to 5 minutes. I also set the polling interval to 2 minutes to reduce the number of log lines. The full log is attached (powerview_secondary_undef_trace.txt), but an annotated and truncated version is included below. Also, this log was captured using the JAR file that fixed issue #11543, but I confirmed that this bug existed in the 3.1.0 version of the add-on.
I started capturing the log the log after a hard refresh. I then issued a command to move the secondary rail (
LivingRoom_Shade5_Top) to position 49. This worked as expected, and the JSON response included both rail positions.I then issued a command to move the primary rail (
LivingRoom_Shade5_Bottom) to position 80. This time, only a primary rail position was sent, and only a primary rail position was received in the JSON response:Then, the binding polled for state. For simplicity, I'm truncating the JSON response of all 11 shades to show only 2 shades: the shade with id 58131 that was just moved and another shade that was not recently moved for comparison. Shade 58131 is only returning the primary rail position that was just set, not both rails, and the update leads to the secondary rail (
LivingRoom_Shade5_Top) being set to UNDEF. I've removed some lines for simplicity, but see attached for full log:Additional polling continues to return only the primary position for this shade. Eventually, a hard refresh is triggered, and the secondary rail position is once again returned on the next polling. Response and logs are truncated as above for simplicity:
Possible Solution
I think there are a few approaches to fix this. One workaround would be to increase the hard refresh frequency, but hard refreshes are expensive/slow because they involve RF communications with all shades on the network. Another option would be to perform a hard refresh before polling whenever the binding knows a shade has recently been moved, but I'm not sure how easy that would be to implement. The approach that seems cleanest to me is to make setting the primary rail similar to setting the secondary rail in this shade handler code: https://github.com/openhab/openhab-addons/blob/062f45440663b431d6213d2854c17027787c9269/bundles/org.openhab.binding.hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/handler/HDPowerViewShadeHandler.java#L204-L227 The binding could first get the current state of the shade regardless of which rail was being moved, then only update the position of the desired rail and send the move shade command with both positions set. This would ensure the hub keeps the state of both rails in its cache. In the case of single rail shades, I think the get operation would already have the secondary rail position recorded as null, so updating the primary rail position and resending should still work, but I don't have any single rail shades to test that on.
Steps to Reproduce (for Bugs)
Context
I have been setting up slider UIs for my dual-rail shades, so that I have vertical sliders for the top and bottom rails that visually show their physical position. When I use the slider to change the bottom (primary) rail, the slider for the top (secondary) rail will move to 0 (the location when the value is UNDEF), so the UI no longer reflects the physical position of my shades.
Your Environment
powerview_secondary_undef_trace.txt