Open AK5nowman opened 2 years ago
Ah, another thing... I have the "bridge" dongle flashed with this firmware. Is this PR intended to support that, then?
And somehow I "approved" when I meant to "request changes" 😅
I'm going to make my PR with some fixes, probably tomorrow... I've got a lot of this stuff working now. I am not able to test with the leak/climate sensors, and haven't touched their code specifically much, but I've got the keypad almost totally working, including auto-discovery.
I've gone ahead and resolved the portions of my review that had more to do with refactoring and the keypad itself... but left the ones that are more directly related to the leak/climate sensors, since I don't have those available to test against.
I've nearly got the keypad "fully functional" via auto-discovery, but for integration with Home Assistant, I think there needs to be some "back and forth", which we're currently not set up for. Also, I think we need some mechanism of setting the required PIN and such... but that will likely be a bit more involved, I'm afraid. I also think that it may involve changing the structure of the messages that are sent for auto-discovery, but I'm wary of doing that for all of the sensors in case it breaks anything others are already using. I'll have to test further.
In general, I think this should be split into two PRs: one which adds support for the keypad, and one which adds support for the other sensors.
@AK5nowman the work to add the new sensor types is great and thank you. I know @drinfernoo has two other PRs in queue doing some cleanup and adding support for the keypad. I'm guessing these might not be mergeable when all is said and done. At this point since I don't have as much time, I'm letting @drinfernoo handle the merges on recent PRs, assuming willing. So maybe after the other two are merged in, check to see if this will merge cleanly, or possibly branch off the new ones.
@raetha @AK5nowman Any progress on the keypad? I'm aware of the race condition, but would love to help sort out any remaining issues.
@drinfernoo Take a look at this branch and let me know if it functions for you. Should note, this doesn't add support for the new sensors with HASS auto-discovery. I don't have the infrastructure to test any changes related to HASS so won't be making those updates.