Scene fixes

[tstabrawa]

This change fixes a handful of problems that I identified while attempting to use the scene import/sync capabilities of insteon_mqtt with my existing Insteon network that includes an Insteon Hub (2245-222) and that has several preexisting scenes, some of which involve the hub.

These are being submitted in a single pull request because some of the changes depend on changes made in earlier commits in the same chain. Please let me know if you feel it is important to split these into multiple PRs.

@krkeegan I would very much appreciate your code review of these changes, since you wrote much of the content that is being changed here.

Each of the issues/fixes is summarized below.

Issues fixed:

Issue 1: Multiple dimmer scenes are being combined by compress_controllers() despite having different ramp rates for responders.

• Added test (commit 16b561f) - Add scene tests for Dimmers with ramp rates
• Fix (commit 60cc79c) - Make link_data_from_pretty match "to" variant for ramp_rate

Issue 2: "TypeError: 'int' object is not iterable" in KeypadLinc.py / link_data_from_pretty.

• Added test (commit 103341a) - Add scene tests for FanLinc & KeypadLinc with ramp rates
• Fix (commit b545497) - Iterate on Dimmer.ramp_pretty.items(), add missing break

Issue 3: Scene sync tries to replace group-0 entries with group number changed to 1.

• Added test (commit 7b25c9d) - Add scene tests for preserving group-0 scenes
• Fix (commit c49791b) - Don't treat group-0 and group-1 as equivalent

Issue 4: Scenes created by mini-remotes are being changed to have group # in data_3 field.

• Added test (commit 8634bf6) - Add simple scene tests for Remotes
• Fix: (commit 5e0e032) - Change default data_3 value for mini remotes

Issue 5: Typos in insteon_mqtt/Scenes.py comments.

• Fix: (commit c9d9464) - Fix typos in comments

Issue 6: Identical DB entries being removed/re-added by scene sync when scenes only differ in responder group number.

• Added test: (commit 8ff32ba) - Add scene test involving multiple groups from same device
• Fix: (commit b611ce3) - Force match on local_group in Device.diff()

Issue 7: FanLinc scenes read from yaml would all have ramp rate set to 0x1f.

• Added test: (commit 0a4e936) - Add scene test for preserving FanLinc dimmer ramp rates
• Fix: (commit 4bd65dd) - Allow FanLinc to parse ramp_rate if group is default

Tests performed:

• Confirmed that all new pytest tests fail before fix and pass after fix (see attached test log - scene-fixes-testing.txt)
• Confirmed no new flake8 errors (see attached test log - scene-fixes-testing.txt)
• Confirmed no new pylint errors (see attached test log - scene-fixes-testing.txt)
• Confirmed all old & new pytest tests are passing (see attached test log - scene-fixes-testing.txt)
• Reviewed test coverage report for substantial new gaps (see attached test log - scene-fixes-testing.txt)
  • Untested lines of code reduced by ~135 lines.
• Dog-food tested with HA version 0.118.3 and 0.118.4 for ~1 day without problems.

Attachments:

• scene-fixes-testing.txt

[tstabrawa]

@krkeegan Would you please code-review these changes? I think you wrote much of the scene support, so you'd probably be most familiar with what is being changed here. (Sorry for the nag, especially if you've already started looking at this.)

[krkeegan]

Yes, I will look at it. Sorry it is a bit more involved so it may take some time.

[krkeegan]

First, thank you for doing this. The scene code is not some of my finer work, in part because all of these variations make my brain hurt.

I am looking over the following and will check them off once I have reviewed them.

• Issue 1: Multiple dimmer scenes are being combined by compress_controllers() despite having different ramp rates for responders.

Ouch, it looks like this was bad coding on my end that I should have caught. The fix looks correct, and the tests are a nice addition.

• Issue 2: "TypeError: 'int' object is not iterable" in KeypadLinc.py / link_data_from_pretty.

Again, more mistakes on my part. The fix looks good.

• Issue 3: Scene sync tries to replace group-0 entries with group number changed to 1.

Hmm. This one is more complex. The idea here is that group 0x00 and 0x01 are the same group and for most devices they are interchangeable. Older devices tended to use 0x00 and the newer standard seems to be using group 0x01.

What I was trying to accomplish here, was to prevent the adding a link when a group 0x00 link was already present on the devices and the scenes file contained a 0x01 definition.

If I have bad logic does fixing it still work with your devices? Or is this concept just broken and you have devices where the group 0x00 difference matters?

• Issue 4: Scenes created by mini-remotes are being changed to have group # in data_3 field.

Again, this one is interesting. How were the links you are comparing this too created? Did you create them with manual button pressing?

This hasn't been a big deal deal in the past, my recollection is that even with wrong data3 values things still work. Did they work with the group in data3? This would be a deviation from how keypadlincs use data3.

• Issue 5: Typos in insteon_mqtt/Scenes.py comments.

• Issue 6: Identical DB entries being removed/re-added by scene sync when scenes only differ in responder group number.

I made a comment in the code here. I think this should still be group <= 0x01 as 0x00 and 0x01 have been used interchangeably.

Can you explain the issue more? It looks like the issue may have been, you have multiple buttons on a kpl set to respond to the same scene command. And if I am reading this right, because the search was ignoring the the group on the responder device, it was deleting all but one of the responder links and then re-adding them?

I think this is a good fix, as long as it isn't causing issues with 0x00 and 0x01 groups being used interchangeably.

• Issue 7: FanLinc scenes read from yaml would all have ramp rate set to 0x1f.

[tstabrawa]

Thanks for taking the time to review these changes (and for implementing the scene support in the first place). (Sorry in advance for writing a novel here.) :-)

 * [ ]  Issue 3: Scene sync tries to replace group-0 entries with group number changed to 1.

Hmm. This one is more complex. The idea here is that group 0x00 and 0x01 are the same group and for most devices they are interchangeable. Older devices tended to use 0x00 and the newer standard seems to be using group 0x01.

What I was trying to accomplish here, was to prevent the adding a link when a group 0x00 link was already present on the devices and the scenes file contained a 0x01 definition.

If I have bad logic does fixing it still work with your devices? Or is this concept just broken and you have devices where the group 0x00 difference matters?

The device that I have that uses group 0 is actually a Hub 2 (model # 2245-222) that co-exists on my Insteon network. (I'm using a USB PLM (model 2448A7) with Insteon-MQTT and trying to transition away from the hub as I replace the functionality we've gotten used to with the hub.) The only scenes I see for group 0 on this device all have the hub as the controller. For example:

- controllers:
    - aa.bb.cc: 0
  responders:
    - kitchen cabinet lights:
        on_level: 70.2
        ramp_rate: 2
    - lr corner lamp:
        on_level: 0.0
        data_3: 0

Since there is no Device class in Insteon-MQTT for hubs on the network (aside from the support you have in #201 for using a Hub as a PLM), I assume that changes to any scenes that include this device wouldn't be applied to the hub itself. That then leads me to assume that if I let Insteon-MQTT change the all-link DB's on all of my devices to expect to use group 1 with the hub instead of group 0, then all of those devices would stop working with the hub. Is this interpretation correct?

That said, it seems like my changes would break the behavior that you intended to implement (not adding a group 1 scene when a corresponding group 0 scene already exists in the device's DB). I wonder, though, wouldn't it be better for the scenes.yaml file to specify group 0 explicitly, if that is what needs to be used with the older device for it to work? Also, prior to the changes in this PR, wouldn't it be impossible to create a scene for group 0 using scenes.yaml? Or maybe you're saying that all devices don't actually care about group 0 versus 1, so you don't want to end up deleting the group 0 scene and replacing with a group 1 scene, when it otherwise doesn't matter?

* [ ]  Issue 4: Scenes created by mini-remotes are being changed to have group # in data_3 field.

Again, this one is interesting. How were the links you are comparing this too created? Did you create them with manual button pressing?

This hasn't been a big deal deal in the past, my recollection is that even with wrong data3 values things still work. Did they work with the group in data3? This would be a deviation from how keypadlincs use data3.

The scenes that Insteon-MQTT was wanting to change (to add the group number into the data_3 field), were created on a 4-scene mini-remote (model 2342-232) the old-fashioned way (using the buttons on the remote & devices). I haven't tried overwriting them with what Insteon-MQTT wanted yet, but if you think that putting the group number into data_3 is more correct for these devices, I could give it a try. Are there other devices that enumerate with the Remote class that put the group number into data_3? Or, more importantly, are there any other devices that use the Remote class that require the group number to be in data_3? My understanding is that KeypadLinc devices use the "KeypadLinc" class.

It may be worth noting, that in changing the default value for data_3 to 0x00, I was careful not to cause it to overwrite any existing scenes that do have the group number in data_3. In fact, test_mini_remote_button_config_with_data3() confirms that the existing data_3 values would be preserved when imported.

* [ ]  Issue 6: Identical DB entries being removed/re-added by scene sync when scenes only differ in responder group number.

Can you explain the issue more? It looks like the issue may have been, you have multiple buttons on a kpl set to respond to the same scene command. And if I am reading this right, because the search was ignoring the the group on the responder device, it was deleting all but one of the responder links and then re-adding them?

That's right. The set up I have is basically four scenes set up in the Hub 2 for "Fan Off", "Fan Low", "Fan Med", and "Fan High", where each scene sets the appropriate speed in a FanLinc device and enables/disables the appropriate buttons of a KeypadLinc device in the same room. It's basically the same scheme described by TFitzpatri8 here (@ 12/08/2015 11:41:08 AM). Having since familiarized myself some with how Insteon-MQTT works, I see that I could probably accomplish the same thing using set-flags to set up the buttons as a radio group, but I don't remember the Hub 2 / Insteon app having that option at the time I set it up.

My existing scenes look basically like what test_foreign_hub_keypad_button_backlights_scene() sets up, and the scene-sync wants to delete and re-add the scenes for groups 4, 5, and 6 (leaving group 3 alone).

For example, if I check out commit 8ff32ba and un-comment-out the following lines in test_foreign_hub_keypad_button_backlights_scene():

     # Uncomment the next two lines to see what sequence would do:
     #IM.log.initialize()
     #seq.run()

I get the following output from the test (which would need to be run with the -rP options, because the assertion passes when the sequence is emptied by running it):

----------------------------- Captured stderr call -----------------------------
2020-12-08 23:15:25 UI Base: Would Delete 201b: aa.bb.cc grp: 19 type: RESP data: 0x00 0x1f 0x04:
2020-12-08 23:15:25 UI Base: Would Delete 207f: aa.bb.cc grp: 19 type: RESP data: 0x00 0x1f 0x05:
2020-12-08 23:15:25 UI Base: Would Delete 20e3: aa.bb.cc grp: 19 type: RESP data: 0x00 0x1f 0x06:
2020-12-08 23:15:25 UI Base: Would Add 0ff7: aa.bb.cc grp: 19 type: RESP data: 0x00 0x1f 0x04:
2020-12-08 23:15:25 UI Base: Would Add 0fef: aa.bb.cc grp: 19 type: RESP data: 0x00 0x1f 0x05:
2020-12-08 23:15:25 UI Base: Would Add 0fe7: aa.bb.cc grp: 19 type: RESP data: 0x00 0x1f 0x06:

Of course, in my case, again it's a Hub 2 that's the controller, but you could imagine the same problem could happen if you had a scene that would turn on a FanLinc's fan and its associated light when a button was pressed on another device.

* [ ]  Issue 7: FanLinc scenes read from yaml would all have ramp rate set to 0x1f.

I made a comment in the code here. I think this should still be group <= 0x01 as 0x00 and 0x01 have been used interchangeably.

I think this is a good fix, as long as it isn't causing issues with 0x00 and 0x01 groups being used interchangeably.

Your response comments around issue 6 & 7 seemed to get a bit mixed up (either that, or I just got confused). I've re-ordered them in the quote above to match how I think you meant them. (Please correct me if I misunderstood your intention.)

As I'll also write in response to your comment in the code, I agree with your point here. I think I may have changed it to == 0x01 because I was unaware that some old FanLinc's might use group 0 for the light and I was trying to be consistent with the other changes I made related to group 0 / 1 scenes being treated interchangeably. By using data_3 <= 0, this code should apply for either group number. (Note your comment in the code says data_3 <= 0, but your comment in the above-quoted response for Issue 7 says group <= 0x01. I believe the former is the correct change to make, since it addresses the scenario where 'group' is not in the dict.)

[krkeegan]

I checked out my remote and tested it. I agree it sets d3 to 0x00 on controller entries. So Issue 4 is good.

[krkeegan]

I also agree with Issue 6. My initial concern was that group 0x00 and group 0x01 are the same thing. By searching specifically for the group, we may inadvertently cause an update to a responder entry that isn't needed.

But you problem is a bigger issue and that is the correct fix.

[krkeegan]

And issue 7 is fine. If there is any future problems that is confined.

Issue 3 is outstanding. I just want to make sure this is correct. I think I understand your problem now. I just want to make sure that someone else with group 0x01 is going to post an issue later saying that it is trying to change everything to 0x00.

[tstabrawa]

Issue 3 is outstanding. I just want to make sure this is correct. I think I understand your problem now. I just want to make sure that someone else with group 0x01 is going to post an issue later saying that it is trying to change everything to 0x00.

As I understand things, in the old code, you'd basically have a situation where, on import, any links with a group # of 0 or 1 would get represented in scenes.yaml as using the default group number (by having the group value omitted). So, then when trying to sync scenes, the SceneDevice.group property for each of these would return 1, leading it to want to have a group-1 scene instead of the existing group-0 scenes.

The primary difference in the new code is that, duing import, existing links with group 0 would have the group number explicitly saved as such in scenes.yaml. As a result, the SceneDevice.group property for these links would return 0, since it is specified as such in the yaml data. Any scenes.yaml entries without a specified group number should still have the SceneDevice.group property return 1, as before, so I wouldn't expect any scenes to get changed from 1 to 0 due to these code changes. Furthermore, for anyone who's been using scenes with the old implementation, I wouldn't expect there to be any entries in their scenes.yaml with the group # specified as 0, since the old code would have removed this detail (and overwritten the scenes with the group number changed to 1).

Are there any specific scenarios you're concerned about? If so, I could try adding additional tests to cover the cases you think could be problematic. For what it's worth, though, there seem to be a number of tests that use group 1 already, so maybe your specific concerns are already covered by some of the tests?