openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.88k stars 3.59k forks source link

[hdpowerview] Resolve differences between OH dual-rail shade behavior, HD remote behavior, and documentation #11593

Closed arroyoj closed 2 years ago

arroyoj commented 2 years ago

After fixing issue #11543 in PR #11552, the behavior of the secondary channel for dual-rail shades no longer matches the documentation in the README. However, there is also confusion about what the "correct" behavior should be for dual-rail shades, which is not helped by HunterDouglas's poorly worded documentation. This issue is to continue the discussion with @andrewfg and @jlaur that began in #11543 after that issue was closed. The goal is to try to resolve the differences between OpenHAB's behavior with dual-rail shades, the HunterDouglas Pebble Remote behavior, and the OpenHAB documentation.

I think there are a few questions that should be addressed, but I hope others will help to refine this list:

  1. How to map PowerView Hub raw coordinates for the primary and secondary rails to OpenHAB coordinates? That is, should both rails exist in the SAME OpenHAB coordinates with the top of the window always 0% and bottom always 100% (v3.1.0 setter behavior), or should the primary and secondary rails exist in INVERTED OpenHAB coordinates (v3.1.0 getter behavior)? See #11543 for description of setter vs getter behavior in v3.1.0.

  2. How should OpenHAB's UP/DOWN commands be mapped to primary and secondary rail motion?

  3. How to define the OPEN and CLOSED state for the primary and secondary rails?

  4. How to ensure that the dual-rail shade behavior is still compatible with venetian blinds?

Since I found it hard to understand HunterDouglas's manuals and PowerView API documentation, I made some visuals for myself to help map the coordinate systems used for single and dual-rail shades. In this first single-rail figure, grey indicates shade fabric, and the heavy line is the primary (single) rail. Here are how the raw Powerview Hub coordinates and OpenHAB coordinates map for single-rail shades, based on the HDPowerview add-on version 3.1.0 (and unaffected by PR #11552): image

Since I struggled to get this straight in my mind, "bottom-up" shades are traditional single-rail shades, where the fabric is fixed at the top of the window and the bottom (primary) rail can be moved up to open the shade. "Top-down" shades are single rail shades where the fabric is fixed at the bottom of the window and the top (primary) rail can be moved down to open the shade. The single rail in both of these shades exists in the same hub coordinate space, but the open vs closed coordinates differ because of where the fabric is located. Hopefully the above diagrams help explain this very confusing text from the PowerView API docs: "Top-down shades are in the same coordinate space as bottom-up shades. Shade position values for top-down shades would be reversed for bottom-up shades. For example, since 65535 is the open value for a bottom-up shade, it is the closed value for a top-down shade."

Next, using the version of the hdpowerview add-on from PR #11552, I made a similar figure for my dual-rail shades at various states. In this figure, grey again indicates the shade fabric, the heavy black line is the primary rail, and the heavy red line is the secondary rail (when at the same location, only the primary rail is shown). The primary rail coordinate systems are shown on the left in black, and the secondary rail coordinate systems are shown on the right in red. For each shade position (labeled A-H at the bottom), the corresponding raw and OpenHAB coordinates for both rails are provided: image

While the dual-rail shades have a single Closed state, they have two Open states (unlike single-rail shades, which have a single open state). I think this makes it hard to define "OPEN" in OpenHAB.

The HunterDouglas manual has this image describing the remote operation for dual-rail shades that I have found confusing: image

To help me understand how the HunterDouglas Remote works to control dual-rail shades, and so that I could wrap my head around how the OpenHAB commands should behave to mimic it, I set my dual-rail shades to 6 different starting positions that seemed to correspond to all the possible "equivalent" shade states (OH coordinates are as in the previous figure). I then pushed each of the 4 buttons on the remote and wrote down what happened with each rail in the table below. I reset the shade to the initial position before each button push, and waited until the shade fully stopped moving, as sometimes one rail moves and then another. The buttons are color coded by whether they are associated with the primary rail (black) or secondary rail (red), and the rail effects are similarly color coded. In italics I noted any time a rail seemed to move in the opposite direction of the arrow on the button that I pushed. Hopefully this is helpful for understanding these dual-rail shades and the remote. image

From the first table, I came up with a summary of how the buttons work (second table above). As @andrewfg has pointed out, pushing an up arrow does not always mean a rail moves up, as the secondary rail's up arrow can move the primary rail down (and vice versa, the primary rail's down arrow can move the secondary rail up). However, unfortunately Up does not always mean Open, either. The primary "Up arrow / Open" button always means Open, but the secondary "right button with up arrow" means Close. That really confused me, and was not what I was expecting.

Based on the above analysis, these are my suggestions for how OpenHAB should behave to make it match the HD Remote, and the README could be updated to match. @andrewfg and @jlaur, please let me know what you think and whether this makes sense, or whether there should be other behavior. It would also be great to get feedback from @berland and @mstroeve, who were involved in testing the original dual-rail code.

Suggestions:

Once we agree on behavior, I'm happy to help test out the code to implement it. Also, my suggestions are only for how the primary and secondary channels should behave with dual-rail shades. I don't have any venetian blinds, so I couldn't test that to try to understand how they might fit into the above model. One thing that is not clear to me with venetian blinds is whether they only use position1 in the hub JSON or whether they can be controlled with both position1 and position2 (ie, could position2 be used to control the vane, while position1 always controls the primary rail for single-rail, dual-rail, and venetian blinds?).

Sorry for cramming so much into a single issue, but I thought it would be helpful to gather everything in one place.

andrewfg commented 2 years ago

@arroyoj thank you for this.

You evidently have a done a detailed analysis of your special case Duette top down / bottom up shades. However finally the solution must be suitable for ALL types of shades as shown in the image below. I think there are possibly three 'flavours' of such blinds as follows..

  1. With primary opening panel only e.g. single rail Duette shades
  2. With primary and secondary opening panel e.g. double rail Duette shades (your case)
  3. With primary opening panel and secondary 'tilt' action e.g. Venetian, Silhouette, Pirouette, Twist, Plisse, Vertical shades, etc.

image

The solution must take into account some additional quirks as follows..

Finally below are duplicates of two images copied from the prior Issue posting..

image

image

andrewfg commented 2 years ago

PS thinking ahead to the possible solution: I guess the following..

arroyoj commented 2 years ago

@andrewfg, yes, I was also thinking that we could either use the PositionKind value or have a configuration parameter for each Shade. There is also the ShadeType parameter returned in the JSON from the Hub, although I think we would have to reverse-engineer what the values are, but we could default to single-rail/venetian blinds behavior unless the ShadeType was a known dual-rail shade. For my two different types of dual-rail shades, these are the type values returned in the JSON from a call to /api/shades/:

In terms of PositionKind, it seems like the secondary rail (2) and vane tilt (3) values can have constant behavior, since they each apply to only one of the three shade "flavors" you described. The primary rail (1) seems to be the trouble, though, because how it interacts with the other rails depends on what type of shade it is.

One question I have is whether it is valid to send a venetian blind a positions value like this: {"posKind1":1,"position1":0,"posKind2":3,"position2":32767} to move the bottom rail down and open the vanes all at once. If so, it might make it easier to integrate all three shade "flavors" by always sending the primary rail data with PositionKind set to 1 in the first JSON slot and then sending either the secondary rail or vane tilt in the second JSON slot (or nothing for single-rail non-tilt shades). @andrewfg, do you have a venetian blind to test or have any JSON data from those blinds?

andrewfg commented 2 years ago

do you have a venetian blind to test or have any JSON data from those blinds?

Sorry but I don't have such a blind :(


However (therefore), if we do make changes, we must ensure that they are "non breaking" for the majority of users. Therefore I think we should establish the following "principles" (in order of priority)..

  1. If the issue can be fully resolved by changing the ReadMe only: Do just that.
  2. For users (resp. shade types) where NO issue has been reported: Don't change the functionality.
  3. For shade types where an issue HAS been reported: Add a configuration parameter such that, a) in existing systems, the prior functionality is retained by default, b) in systems where the user explicitly wants it, the new functionality is implemented, an c) in new systems, the new functionality is always implemented.

@arroyoj / @jlaur does the above make sense? And if so, perhaps you can explicitly identify your cases of 1. above, respectively of 3. b), and 3. c) above?

jlaur commented 2 years ago

@andrewfg - it makes sense. In the scenario already changed, #11552, there was a clear inconsistency between getter and setter, so changing any of them would have to be a breaking change. Do you suggest retaining this problem with a configuration parameter for enabling the fix? Just checking. IMHO this might be too much, but perhaps it could be configurable whether to go from 0 to 100 or from 100 to 0 for both getter and setter, but of course only if each setting has a use-case.

I'm not sure I get the last question. I only ever used the getter for my Duette top down/bottom up, never the setter, so didn't notice the problem. After the fix I simply inverted my rule logic accordingly.

arroyoj commented 2 years ago

@andrewfg, yes, it makes sense, and I agree about avoiding breaking changes, especially for working shade types.

For your case 1 (issues that can be resolved by changing Readme only):

For your case 3 (issues that could be resolved with config options, defaulting to new behavior in fresh installs):

andrewfg commented 2 years ago

My preference would be that going forward, the top value for the secondary rail is 0%

I think that simply inverting the 0%..100% scale is likely to create more problems that it might "solve": The binding has to process different OH framework command types (see below), and so we need to decide how all of them should be processed (for setter and getter)..

image

image

image

arroyoj commented 2 years ago

Sorry, I probably should have been more clear about my preference. After #11552, the getter now treats 0% as the top for the primary and secondary rails, matching the setter. So, my preference is that we stay with #11552 behavior going forward, and not invert the secondary rail relative to the primary rail.

As for the commands, here are my preferences for each command type and corresponding UI:

andrewfg commented 2 years ago

Re: OnOffType, IMHO ON should be OPEN, and OFF should be CLOSED (rather than UP resp. DOWN)

arroyoj commented 2 years ago

ON/OFF being equal to OPEN/CLOSE makes sense to me. My suggestion would be that sending ON (OPEN) to the primary channel sends both rails up (ie, open up state) while sending ON (OPEN) to the secondary channel sends both rails down (ie, open down state), and sending OFF (CLOSE) to either channel sends primary rail down and secondary rail up.

However, I'm still not sure how useful a switch UI would be for direct control. For example, if you have a single rail shade, and you have it set at 50%, the switch state will be ON. Then, there is no way to use the switch to fully open the shade. You would need to toggle the switch off to start closing it, then toggle it back on to open it. That said, I could still see it being useful to interpret OnOffType commands sent by rules.

andrewfg commented 2 years ago

^ Hmm. We would still need to make sure that the mapping OnOffType => Rail Position => Setter => Getter => Rail Position => OnOffType is self consistent end to end. So your suggested mapping algorithm above might make that rather difficult...

andrewfg commented 2 years ago

Just to re-iterate, your proposed mapping (2.) is different than mine (1.) below..

  1. Slider 0% <=> Roller Blind Control UP button <=> shade physically open <=> switch is ON <=> Pebble remote Up button <=> rail is either physically up or down depending on the type of shade

  2. Slider 0% <=> Roller Blind Control UP button <=> shade physically either open or closed <=> switch is either ON or OFF <=> Pebble remote either Up or Down button <=> rail is physically up

Facit: My mapping (1.) is logically self consistent except for the physical position of the rail. Whereas yours (2.) certainly resolves the one physical position issue, but in so doing it creates three other inconsistencies; that are hard, if not impossible to resolve.

andrewfg commented 2 years ago

PS and BTW my mapping (in the current code) also applies to shades that are bottom up, left / right, or right / left. Whereas IMHO yours does not address those types at all.

arroyoj commented 2 years ago

Just to re-iterate, your proposed mapping (2.) is different than mine (1.) below..

1. Slider 0% <=> Roller Blind Control UP button <=> shade physically open <=> switch is ON <=> Pebble remote Up button <=> rail is either physically up or down depending on the type of shade

2. Slider 0% <=> Roller Blind Control UP button <=> shade physically either open or closed <=> switch is either ON or OFF <=> Pebble remote either Up or Down button <=> rail is physically up

Thanks for laying out the comparison like that. We seem to be thinking of this differently, but I think I have an idea of how to resolve it, and make mine not appear so inconsistent. Unfortunately, I don't have time right now, but will try to provide my hopefully consistent mapping tonight.

arroyoj commented 2 years ago

@andrewfg, just a quick question to make sure I understand how OpenHAB mappings work. Is it possible to have a channel with an OnOffType where ON is equal to 0% of a PercentType for both getting and setting? That seems to be required for your mapping (1.), and possibly for my mapping, too. I understand how it would work for setting, but would it round trip properly when getting? Looking at the code, it seems like the getter just returns a percent type and then relies on OpenHAB to convert it, which I think would make 0% equal OFF, like with light dimmers and switches.

andrewfg commented 2 years ago

You are correct.

arroyoj commented 2 years ago

Just to re-iterate, your proposed mapping (2.) is different than mine (1.) below..

1. Slider 0% <=> Roller Blind Control UP button <=> shade physically open <=> switch is ON <=> Pebble remote Up button <=> rail is either physically up or down depending on the type of shade

2. Slider 0% <=> Roller Blind Control UP button <=> shade physically either open or closed <=> switch is either ON or OFF <=> Pebble remote either Up or Down button <=> rail is physically up

Facit: My mapping (1.) is logically self consistent except for the physical position of the rail. Whereas yours (2.) certainly resolves the one physical position issue, but in so doing it creates three other inconsistencies; that are hard, if not impossible to resolve.

PS and BTW my mapping (in the current code) also applies to shades that are bottom up, left / right, or right / left. Whereas IMHO yours does not address those types at all.

Thanks again for laying this out. A few general comments first:

I agree that my mapping has some inconsistencies between channels, but I think they are fewer and not as bad as you implied. In some cases they are just due to the nature of dual-rail shades, as evidenced by the confusing instructions for the Pebble Remote in the Hunter Douglas manual. Here is my version of your mappings above:

While you see three inconsistencies in mapping 2, I only see two because the Pebble Remote has separate primary and secondary Up buttons, and Up always moves its corresponding rail up. I believe these are the two remaining inconsistencies, and the first is somewhat required due to the nature of dual-rail shades:

In exchange for these two inconsistencies, I think we get two worthwhile benefits:

However, I think there is a 3rd possible mapping that may be a compromise:

Like mapping 1, this mapping 3 keeps OFF equal to "Open" for both channels and has the same inconsistency where 0% is physically up for the primary rail but physically down for the secondary rail. Like mapping 2, this mapping 3 keeps Up buttons equivalent to physically moving the rail up. Basically, the differences between 2b and 3b are i) inverting the UpDownType conversion to PercentType for the secondary rail and ii) inverting the OH to hub JSON conversion for the secondary rail (so in the end UpDownType keeps its relationship to hub JSON and rail motion). I would still prefer my mapping (2.) :smile: because of the slider UI benefits with mapping 2, but I could figure out a way around that. I think @jlaur was suggesting a config option to switch between 2b and 3b for the secondary channel. If it was a config, I'd vote for 2b as default, but I'd be fine either way.

Lastly, I just wanted to add that I really appreciate you taking the time to work on this, and I'm not trying to be argumentative. And I'm sorry about the super long comments, but it feels hard to explain everything clearly. I'm hopeful that we can come to an agreement where the behavior is both as intuitive and as consistent as possible, realizing that there may be no "perfect" solution. If you think mapping 1 is the way it should be, then I will adapt to that decision, but I guess my preference would be mapping 2 > mapping 3 > mapping 1.

andrewfg commented 2 years ago

I looked into the code again, and realised that OnOffType commands are not handled for rails (only applies to vanes). So all of my above comments relating to OnOffType are irrelevant and quite wrong. I apologise for wasting your time. Furthermore I looked into the OH framework and it seems that neither i) does the framework forward OnOffType commands to RollerShutter type channels, nor ii) does it convert such commands to UpDownType or PercentType equivalents. Therefore I propose that we forget about OnOffType entirely. Apologies again.

(I realise also that for completeness the mapping table is missing one more column -- namely the "Raw Value" (value of position1/2) inside the shade; which in Mapping 1. above is 65534. Which implies that internally Hunter Douglas uses classic Cartesian coordinates.)

Given the above, I think that your alternate mapping proposals become essentially identical again (please confirm). However, in any case, before starting to write actual code, I think we should write the final correct version of this table in the ReadMe file. Specifically we should i) insert a column for "Button on Pebble Remote", and ii) insert rows for Dual Action Upper Rail for Config Param setting a) legacy [default on existing installations], and b) modern [default on new installations]. Note: it may be that the current read me is already wrong for legacy since it does not reflect the actual behaviour (please advise).

Furthermore given your observed interdependencies between the primary and secondary rail values, I think we should add a paragraph to the chapter on interdependencies

andrewfg commented 2 years ago

^ See proposed changes in ReadMe here <= NOTE Link has changed!!

arroyoj commented 2 years ago

No problem on the OnOffType confusions. I'm just glad we can ignore OnOffType for the secondary channel now. :smile:.

Given the above, I think that your alternate mapping proposals become essentially identical again (please confirm).

Taking OnOffType out of the picture, the mappings I referred to as 2b and 3b are still not identical and differ from each other in the way that PercentType maps to rail physical location. I also think 3b is (almost) what you are referring to as "legacyMode" in the draft ReadMe (see below), while 2b is the new "modern" mapping in the ReadMe (thanks!). Here are the 2b and 3b mappings again, this time without the OnOffType but I've added in the Raw JSON values the hub would use. Bold highlights the difference:

So, 3b treats 100% as the physical top of the window for the secondary rail, mapping to JSON 0, while 2b treats 0% as the top for the secondary rail, again mapping to JSON 0. Note, for the primary channel (ie, the 3a and 2a mappings, which are identical), 0% is the top of the window and maps to JSON 65535.

Note: it may be that the current read me is already wrong for legacy since it does not reflect the actual behaviour (please advise).

Yes, that is correct, the current ReadMe was not quite right for legacy and did not capture the actual legacy behavior. The ReadMe for v3.1.0 captured the intent of mapping 1b, but because the getter and setter were inverted in the code (ie, #11543), the Up/Down commands were moving the secondary rail Up/Down, respectively, while the v3.1.0 ReadMe implied the opposite relationship (moving Down/Up, respectively).

See proposed changes in ReadMe here <= NOTE Link has changed!!

Thanks for sharing the draft of the new ReadMe. I think the main table looks great, and I really like the idea of having legacy mode vs modern mode. However, I think the "legacy mode" table is still wrong, for the reasons described above for the v3.1.0 ReadMe. Here is what I think the legacy mode table should look like (my changes are in square brackets):

On older 'legacy' installations, the upper rail of dual action shades functions in a so called legacyMode. Which is different from the above table, as shown in the table below. Type of Shade Channel Rollershutter Command Motion direction Shade State Percent Pebble Remote Button
Dual action (upper rail) secondary [Up] [CLOSED] [100%]
[Down] [OPEN] [0%]

This table is the 3b mapping above. Here is why I believe this table accurately reflects the "legacy mode" of how v3.1.0 actually behaved:

One other small issue in the draft ReadMe: my computer is not showing the proper characters for the Pebble remote secondary buttons, so maybe we need to use different characters? I'm guessing it's a Unicode thing? If it helps, I'm on a Mac.

andrewfg commented 2 years ago

@arroyoj I tweaked the 'ReadMe' again based on your inputs. Can you please give me your feedback?

Notes:

image

andrewfg commented 2 years ago

PS I revised the "ReadMe" to use bitmap images for the Pebble Remote Buttons..

https://github.com/andrewfg/openhab-addons/tree/hdpowerview-wip/bundles/org.openhab.binding.hdpowerview#readme

arroyoj commented 2 years ago

Thanks for making the changes to the ReadMe, and the new bitmapped arrow images look great. I did notice one small error: the Pebble Remote buttons in the Legacy table should be reversed (ie, curved-up arrow / right.png corresponds to Up, and curved-down / left.png to down). I'm guessing this may have been my fault because I copy/pasted the previous table when I edited it, but I couldn't see the arrows, so I didn't change them.

I wrote this next section after misunderstanding what you were asking about the primary/secondary rail interactions, but leaving it in for potential interest:

For the "TBD" sections of the primary/secondary interaction table, I'm actually not sure what to add there. The text above the table is correct: "On dual action shades, the top rail cannot move below the position of the bottom rail. So the value of secondary is constrained by the value of position." The opposite is also true (bottom rail cannot move above top). But, this is really a physical constraint that is not enforced in the binding code, and surprisingly, it is not even enforced by the hub! I just did a test where I sent the following JSON directly to my hub (not through OpenHAB): {"shade": {"id": 49916, "positions": {"posKind1": 1, "position1": 65535, "posKind2": 2, "position2": 65535}}}. This tells the shade to set the bottom (primary) rail to the TOP of the shade, while setting the top (secondary) rail to the BOTTOM, physically impossible. When I sent this command, both rails were at the bottom. The result: nothing happened to the shade (just a small motor whir). However, this is what a GET of the JSON from the hub gave: "positions":{"posKind1":1,"position1":65535,"posKind2":2,"position2":65535}. Again, the hub just copied back what it had been last told, even though those positions are physically impossible.

I did some additional tests, starting with the two rails in various locations and sending the "impossible" position JSON of "positions": {"posKind1": 1, "position1": 65535, "posKind2": 2, "position2": 65535}. In all cases, the bottom (primary) rail did not move, while the top (secondary) rail moved down until it stopped at the primary rail. I thought maybe they would move toward each other and meet in the middle, but the primary never moved, only the secondary. In all cases, the hub reported back the "impossible" JSON data. So, it looks like the shade itself is handling this constraint, but not the hub.

So, I guess that raises the question of whether the OpenHAB binding should try to enforce the physical constraint by never sending positions that cannot be achieved? Or, we could stick to the current behavior, where you can send such commands, but the shade will just stop when it reaches the physical limits. I would probably just stick with current behavior for now.

End discussion of primary/secondary rail physical limits

Re-reading your question, I just realized that you probably weren't asking about the behavior of the rails trying to move past each other, so sorry about going off on that tangent. I think this table below is what you are looking for:

Case State of position State of secondary - Modern State of secondary - Legacy
Shade open, rails at top 0% = UP 0% = UP 100% = UP
Shade 70% closed from top 70% 0% = UP 100% = UP
Shade fully closed 100% = DOWN 0% = UP 100% = UP
Shade 70% closed from bottom 100% = DOWN 30% 70%
Shade open, rails at bottom 100% = DOWN 100% = DOWN 0% = DOWN
Shade 40% closed in middle 70% 30% 70%

I struggled a bit with the description for each "Case". I started by calling the first row "Shade up", to match the primary/vanes table, but then, is "Shade down" when the shade is closed, or when the shade is open at the bottom (both rails down)? Since "up" and "down" seemed potentially confusing, I went with using "open" and "closed" to describe each case, but I'm happy to change the wording if you think it could be better/clearer. I also thought it helpful to have both modern and legacy secondary position values in the table. I can now better see why some users may prefer the "legacy" mapping: with legacy, "70% closed from bottom" has the secondary rail with a value of 70%, which is intuitive. However, you could also think of that as "30% open from top", in which case the "modern" secondary value of 30% also makes sense. Anyway, I think "modern" has advantages from a UI standpoint, but having both mappings available is great because it lets users pick the one that is most intuitive to them, and I don't think there is really a right answer.

arroyoj commented 2 years ago

Slightly off-topic, but looking more at the primary/secondary rail table above, I was just thinking that maybe it makes sense to define an additional read-only channel (maybe NumberType?) to capture the shade's open/closed state. This new channel could be calculated as follows for dual-rail shades (channels treated as PercentType):

This would also help with defining open/closed state for top-down vs bottom-up single-rail shades:

With this new channel, OPEN would always be 0%, and CLOSED would always be 100%, independent of the shade type or where the rails were physically located. It could help unify both modern vs legacy as well as single vs dual rail. Of course, it wouldn't quite work fully for venetian blinds, which would still have the vanes position as a separate shade state. Anyway, just throwing it out there.

andrewfg commented 2 years ago

^ AFAICT the primary rail position relates to the full range between the physical bottom and the physical top of the window frame. Whereas the secondary position relates to the range between the position of the lower rail and the physical top of the window frame. So if the lower rail is down, then commanding the upper rail to 50% would move the upper rail to the physical middle of the window. Whereas if the lower rail is at 50% then commanding the upper rail to 50% would move the upper rail to 50% of the gap I.e. 25% from the top of the window. => is that what you see?

arroyoj commented 2 years ago

I just ran a few tests, and both the primary rail and secondary rail positions behave like absolute positions. Here is a sequence of JSON position data that I sent to one of my shades, and the resulting rail positions:

  1. {"shade":{"id":58131,"positions":{"posKind1":1,"position1":0,"posKind2":2,"position2":0}}} --> primary rail at window bottom, secondary rail at window top
  2. {"shade":{"id":58131,"positions":{"posKind1":1,"position1":32768,"posKind2":2,"position2":0}}} --> primary rail at window midpoint (half-way up), secondary rail at window top
  3. {"shade":{"id":58131,"positions":{"posKind1":1,"position1":32768,"posKind2":2,"position2":32768}}} --> primary rail at window midpoint, secondary rail also at window midpoint (right above the primary rail)

Whereas if the lower rail is at 50% then commanding the upper rail to 50% would move the upper rail to 50% of the gap I.e. 25% from the top of the window. => is that what you see?

No, that's not what I see. Sending the upper rail the "50%" command (JSON value 32768) while the lower rail is already at 50% (ie, command 3 above), the upper rail moved to the 50% point of the window, not 50% of the gap / 25% from the top.

The secondary rail also has absolute positioning behavior when using the OpenHAB binding (v3.1.0 and latest). Through OpenHAB, I can send the upper rail to an absolute position by sending the corresponding PercentType value, regardless of the primary rail position. The only exception seems to be if the secondary rail is commanded to go below the primary rail, which is impossible and leads to the secondary rail going down and stopping at the primary rail. Similarly, if the primary rail is commanded to go above the secondary rail, it moves up and stops at the secondary rail position. In that situation, the hub will return the "impossible" positions until a hard refresh. Testing with the Powerview App, it appears the App UI prevents sending impossible positions, but the hub accepts them if sent directly.

andrewfg commented 2 years ago

I also tried some tests. It seems that if you PUT a rubbish JSON payload to the hub, including (say) not existent tags and values, then the hub will echo that rubbish back in response to subsequent GET requests. But indeed, it seems that the functioning of the shades is not disturbed by this rubbish. However a GET with refresh=true will ensure that a proper and clean JSON is returned.

andrewfg commented 2 years ago

@arroyoj I am just starting to work on a draft implementation for this. And (as I expected) it is fundamentally not a trivial exercise. Actually the code changes would be quite minor. But the user experience logic is rather complex.

Background

There are essentially two parts of the code which we will need to change -- namely the Getter and the Setter (see below)..

Getter code (in Shade Thing Handler)

image

Setter code (in Shade Thing Handler)

image

Summary

So for your new mapping, we actually must decide on the two following changes, that could/should be independent of each other.

Additional Observation

For single rail top down shades (vs. the normal bottom up ones), There could/would also (logically) be a reversal of the PercentType mapping for the Setter and the Getter for the Primary rail. Plus potentially a reversal for the mapping for UpDownType. (??)

Questions

So the questions we need to answer are.

  1. Setter UpDownType mapping shall be reversed depending on a Config Param: yes/no? (answer: yes)
  2. Setter and Getter PercentType mapping shall be reversed depending on a Config Param: yes/no?
  3. In case of positive answer to 2., the Config Params for 1. and 2. shall be: identical/separate?
  4. In case of 1., 2., and/or 3., -- and bearing in mind Additional Observation above -- shall we also add analogous Config Param(s) for reversal of the primary rail: yes/no? For: setter/getter/both?

My Opinion

If we are going to add Config Params to reverse the current actions, then we should do it a) equally for Secondary and Primary rails, and b) independently for UpDownType and PercentType. So four Config Params. Each having a legacy state, a virgin state, and a legacy user overridden state.

arroyoj commented 2 years ago

Thanks for getting started on the draft code. I think we can make it a bit simpler. I'll address the secondary rail situation first, and then the "additional observation" about the primary rail. Here are my answers to your 4 questions, and then some explanation why:

So the questions we need to answer are.

1. Setter UpDownType mapping shall be reversed depending on a Config Param: yes/no? (answer: yes)

agreed, yes

  1. Setter and Getter PercentType mapping shall be reversed depending on a Config Param: yes/no? yes
  2. In case of positive answer to 2., the Config Params for 1. and 2. shall be: identical/separate? identical, explained below
  3. In case of 1., 2., and/or 3., -- and bearing in mind Additional Observation above -- shall we also add analogous Config Param(s) for reversal of the primary rail: yes/no? For: setter/getter/both? I tend toward no, but no strong feeling. If yes, then it should be done the same as for secondary rail. Explained more below.

Secondary rail

In my opinion, the mappings for UpDownType and the PercentType should be fully dependent. That is, there should only be a single config setting per rail that impacts getter/setter behavior. The way I see it, this config is really about changing the PercentType mapping and the corresponding window coordinates: is the top of the window 0% or 100% for the given rail? Once the user makes that choice (top is either 0% or 100%), then the UpDownType must follow from that to keep Up equivalent to physically moving up and Down to physically going down. If you have a second config option for the UpDownType mapping, then the user could get confused and wind up in a situation where the Up command moves their rail down. Also, I think it is important to note that in current legacy behavior (since dual-rail shades were supported) the Up command has always moved the rail physically up (as described in the draft ReadMe), so we are just maintaining that behavior while introducing a new option to swap the 0% to 100% orientation. The draft ReadMe actually captures this quite nicely: the only difference between legacy and new behavior is what window location 0%/100% correspond to, not what the UpDown buttons do or the corresponding shade state. I think if a user wants the option for the Up command to move the rail down, they should submit a feature request and explain why.

we will 1) reverse the UpDownType mapping (which is what you are asking for); and 2) we might -- or might not -- reverse the PercentType -- to correspond to whatever we decide to do with the Getter above.

This is not quite what I asked for. My request is to have the ability to reverse the PercentType mapping, but that the UpDownType mapping should follow so that UP is always physically up.

Primary rail - for top-down shades

I don't have strong feelings about whether the primary rail should have an option to flip the 0-100% orientation. If such an option is introduced, however, I strongly believe it should also be a single option as described above, which flips the 0-100% PercentType mapping but then also flips the UpDownType to match. Currently, users with top-down shades hit Up to raise their primary rail and close their shade. That behavior should not change, even if they prefer that the top of the window is remapped to 100%. My general feeling is that we should not touch the primary rail in this issue, and if users of top-down shades think such an option is beneficial, they should request that. But, if it is easy to add or we just want to be consistent with the config options, I have no problem with it. Having the primary rail config option would also allow dual-rail shade users to fully remap their windows to 0% at the bottom for both rails if desired (instead of 0% meaning both rails at the top, which is the new "modern" behavior).

Summary

I would suggest having only 1 config option per rail, either just for the secondary rail (slight preference) or for both primary and secondary rails.

Here's a rough idea of what I'm thinking for the setter (pseudocode):

case SECONDARY_RAIL:
    if (command instanceof PercentType) {
        value = the_config_option ? command : 100 - command
        moveShade(SECONDARY, value)
    } else if (command instanceof UpDownType) {
        up_value = the_config_option ? 0 : 100
        moveShade(SECONDARY, command == UP ? up_value : 100 - up_value)
    }
andrewfg commented 2 years ago

Hmm. I still see it differently..

Existing situation

In the existing code the meanings of the Arrows / UpDownType commands and the respective PercentType values are as follows.

Proposed situation

In the new situation we would change the meaning of the UpDownType commands, but not the meaning of the PercentType, as follows.

the mappings for UpDownType and the PercentType should be fully dependent

Note by the way that your proposal to lock together the meaning of both UpDownType@secondary and PercentType@secondary, would maintain the meaning of UpDownType@primary (because up=open), but it would break the lock on meaning of PercentType@primary because (100%=closed=down). So such dependency won't work

Detailed of Proposal for Config Params

So my proposal is to implement four channel configuration parameters; two on the primary channel, and two on the secondary channel, called reverseXYZ. These params would not be "required", meaning that normal users will not have to enter their values; neither via the UI nor via a Things file. So they will 'auto-magically' default to the values shown on the following table.

Note that all the default values are "no" except your single special case "yes" marked in red. Also note that on a top down single rail, the default values would also be "no". The majority of existing users and new users would not need to change the Config Params, nor even be aware of them, since a) the behaviour for legacy users would not change from before, and b) the behaviour for new users would match the "ReadMe" out of the box. Indeed the only people who would need to be aware of, and change the Config Params would be people like yourself who have a legacy (or new) set up but want to customise it. EDIT: we could even designate them as "Advanced" params.

image

Resultant Mapping per Config Params

Then I would map the functionality according to the table below. Note that the pale background colours indicate the behaviour where the Config Params remain at the "no" / legacy setting.

image

andrewfg commented 2 years ago

Oops! For the Top Down shade I broke my own rule that on a RollerShutter 100% = shut. The correct map for this one is below..

image

arroyoj commented 2 years ago

Thanks for all the details. I think the issue is that there is a difference between what was intended for the current (ie, legacy) behavior vs what is actually implemented in the legacy code, and we should be sure to maintain the currently implemented legacy behavior in the "legacy" config settings.

Existing situation

In the existing code the meanings of the Arrows / UpDownType commands and the respective PercentType values are as follows.

* Arrows / UpDownType: refer to degree of **OPEN**-ness where ▲ => opens the shade, ▼ => closes the shade.

* Slider / PercentType: refer actually to degree of **CLOSED**-ness where 100% = fully closed, 0% = not closed. Note: this is because of the convention in OH (and specifically in other bindings) that a Roller_**Shutter**_ goes (down) from 0% to 100% _**shut**_.

* Note: the above statements apply to both primary and secondary rails.

This is not a correct description of how the current (v3.1) code works for Arrows/UpDownType. As currently implemented, UP always moves the rail up (really, toward the headrail, to properly describe left/right shades too). This is captured in the current v3.1 ReadMe for top-down shades, where UP == Motion direction up == State CLOSED == 0%. For top-down shades, also note that 100% means fully OPEN, suggesting that PercenType does not always refer to degree of CLOSED-ness. Screenshot of relevant part of current v3.1 ReadMe, which also matches the code: image Moreover, as currently implemented (but not as currently documented), UP moves the secondary rail physically up, because in the current (v3.1) code, UP is converted to 0%, which the v3.1 setter (but not v3.1 getter) interpreted as the top of the window (JSON value of 0 for posKind=2)

Proposed situation

In the new situation we would change the meaning of the UpDownType commands, but not the meaning of the PercentType, as follows.

* Arrows / UpDownType: will refer to degree of **UP**-ness where ▲ => moves the shade up, ▼ => moves the shade down.

* Slider / PercentType: will **still** refer to degree of **CLOSED**-ness where 100% = fully closed, 0% = not closed. Thus maintaining the convention in OH (and in other bindings) that a Roller_**Shutter**_ goes (down) from 0% to 100% _**shut**_.

* The above would still apply to both primary and secondary rails.

My proposal is that we change the meaning of PercentType commands but not the meaning of UpDownType from current implementation. UpDownType already functions as described in your proposed situation, so therefore we do not need a legacy vs modern interpretation of UpDownType. We just have to make sure UpDownType keeps working as described here even if we change the PercentType mapping. Under my proposal, as captured in the new DRAFT ReadMe, the "modern" mapping for primary rails will keep PercentType refering to degree of CLOSED-ness, but for secondary rails "modern" mapping will have PercentType refer to the degree of OPEN-ness. In other words, with "modern", the secondary rail (and only secondary rail) would be different from canonical OH convention, but that is because OH never anticipated dual-rail shades and secondary rails are not canonical. With my proposal, the "legacy" PercentType mapping would keep OH convention for both primary and secondary rails (ie, current code getter behavior).

the mappings for UpDownType and the PercentType should be fully dependent

Note by the way that your proposal to lock together the meaning of both UpDownType@secondary and PercentType@secondary, would maintain the meaning of UpDownType@primary (because up=open), but it would break the lock on meaning of PercentType@primary because (100%=closed=down). So such dependency won't work

I probably wasn't clear what I meant by dependent. Sorry about that. I meant that if we have a config option that changes the PercentType mapping (which is a mapping between percent values and physical locations in the window) then that same config option needs to also flip the UpDownType mapping, which is a mapping between UP/DOWN commands and Percent values. Setting currently works like this: UpDownType --> PercentType --> Motion. Basically, if you change the mapping function represented by the second arrow, you need to also (and consistently) change the mapping function for the first arrow to keep the relationship UpDownType --> Motion. That's what I meant by dependent, because going from UP/DOWN to physical locations runs through the PercentType mapping. I did not mean that UpDownType should be locked to PercentType. I actually meant the opposite, but sorry about the confusion. I was hoping my pseudocode snippet would explain what I meant.

Note that all the default values are "no" except your single special case "yes" marked in red. Also note that on a top down single rail, the default values would also be "no". The majority of existing users and new users would not need to change the Config Params, nor even be aware of them, since a) the behaviour for legacy users would not change from before, and b) the behaviour for new users would match the "ReadMe" out of the box.

Again, I think the issue is that what you are calling "legacy" does not always match legacy implementation/behavior, and your red "yes" is not the special case I'm proposing. If "no" is the default for "legacy", then some legacy behaviors will break with your proposed implementation of the config parameters. I have annotated the proposed config map below to show where "no" does not match legacy behavior as implemented in current code (note, I combined the "corrected" map for top-down shades into the original mapping): image

My suggestion, to keep things simple and reduce the risk of breaking changes, is to not make any changes to the primary rail code/behavior. That way, single rail shades will not be impacted at all. We would just make changes to the secondary rail to have "legacy" vs "modern" determined by a single config option. Maybe a future update could introduce new behavior/config options for the primary rail.

andrewfg commented 2 years ago

proposal is that we change the meaning of PercentType commands but not the meaning of UpDownType from current implementation

Ok. Got it.

So that means really just one Config Param. => So can you pleas confirm that there is definitely no need for a second Config Param for independently reversing UpDownType?

Also your code snippet suggests that the one Config Param shall also effect the meaning of UpDownType, which is the opposite of what you said above. => Can you please confirm that the one Config Param shall definitely NOT effect the meaning of UpDownType?

arroyoj commented 2 years ago

So, it looks like my code snippet was buggy! Sorry about that and any further confusion it caused. When I was writing it, I think I confused myself thinking about the relationship between UpDownType and PercentType (I was thinking UpDownType would "flow through" PercentType). This should be sufficient:

case SECONDARY_RAIL:
    if (command instanceof PercentType) {
        value = the_config_option ? command : 100 - command
        moveShade(SECONDARY, value)
    } else if (command instanceof UpDownType) {
        moveShade(SECONDARY, command == UP ? 0 : 100)
    }

Where True for the_config_option would be "modern" and false would be "legacy". In moveShade, for the secondary rail, an input of 0 would always correspond to the top (JSON 0) and 100 would correspond to the bottom (JSON 65535). Then the getter would also need to use the config option as well so PercentType round trips properly.

So, yes, just one config option only affecting Percent type, and no need to independently reverse UpDownType.