Dongle v47 support #61
Conversation
|
Ah, another thing... I have the "bridge" dongle flashed with this firmware. Is this PR intended to support that, then? |
There was a problem hiding this comment.
Sorry for the lengthy review here... I do think there's a few more things that need to be addressed, namely: auto-discovery, and also I don't see how these new bits (particularly the keypad) get added to sensors.yaml. I'm going to do a test run of this with my spare dongle and the keypad I have laying around, and see what I can figure out. I'll make PRs towards this PR branch if I get anything worthwhile.
| @@ -288,7 +299,13 @@ def _OnSensorAlarm(self, pkt): | |||
| # is reporting way to high to actually be humidity. | |||
| sensor_type = "leak:temperature" | |||
| sensor_state = "%d.%d" % (alarm_data[5], alarm_data[6]) | |||
| e = SensorEvent(sensor_mac, timestamp, "state", (sensor_type, sensor_state, alarm_data[2], alarm_data[8])) | |||
| e = SensorEvent(sensor_mac, timestamp, "state", (sensor_type, sensor_state, alarm_data[2], alarm_data[8])) | |||
| else: | |||
There was a problem hiding this comment.
What's the value of alarm_data[0] for a climate sensor?
There was a problem hiding this comment.
Also, is event_type == 0xE8 also sent by the climate sensors? That event type was added in #32 for leak sensors, but not sure if it also applies to climate sensors.
I found a potentially relevant discussion here: #29 (comment), but there was never any answer from @jellybob 🤔
| @@ -288,7 +299,13 @@ def _OnSensorAlarm(self, pkt): | |||
| # is reporting way to high to actually be humidity. | |||
| sensor_type = "leak:temperature" | |||
| sensor_state = "%d.%d" % (alarm_data[5], alarm_data[6]) | |||
| e = SensorEvent(sensor_mac, timestamp, "state", (sensor_type, sensor_state, alarm_data[2], alarm_data[8])) | |||
| e = SensorEvent(sensor_mac, timestamp, "state", (sensor_type, sensor_state, alarm_data[2], alarm_data[8])) | |||
There was a problem hiding this comment.
There's a mismatch here between what is getting sent to SensorEvent, and what SensorEvent is listening for.
There was a problem hiding this comment.
I think 'leak' here and where I mentioned above are probably the most obvious choices.
| @@ -238,6 +239,16 @@ def __str__(self): | |||
| s += "AlarmEvent: sensor_type=%s, state=%s, battery=%d, signal=%d" % self.Data | |||
| elif self.Type == 'status': | |||
| s += "StatusEvent: sensor_type=%s, state=%s, battery=%d, signal=%d" % self.Data | |||
| elif self.Type == 'water': | |||
There was a problem hiding this comment.
This should either be 'state', or preferably we should change this and the leak portion to 'leak' or 'water'.
| @@ -238,6 +239,16 @@ def __str__(self): | |||
| s += "AlarmEvent: sensor_type=%s, state=%s, battery=%d, signal=%d" % self.Data | |||
| elif self.Type == 'status': | |||
| s += "StatusEvent: sensor_type=%s, state=%s, battery=%d, signal=%d" % self.Data | |||
| elif self.Type == 'water': | |||
| s += "WaterEvent: water=%d ext_water=%d has_ext=%d battery=%d signal=%d" % self.Data | |||
There was a problem hiding this comment.
What do the values of ext_water and has_ext represent?
| @@ -466,6 +466,103 @@ def on_event(WYZESENSE_DONGLE, event): | |||
|
|
|||
| LOGGER.debug(event_payload) | |||
|
|
|||
| state_topic = f"{CONFIG['self_topic_root']}/{event.MAC}" | |||
| mqtt_publish(state_topic, event_payload) | |||
| elif(event.Type == "water"): | |||
There was a problem hiding this comment.
I think this one should also probably be 'leak'.
| 'event': event.Type, | ||
| 'available': True, | ||
| 'mac': event.MAC, | ||
| 'device_class': 'leak', |
There was a problem hiding this comment.
I'll double-check, but I believe the device class Home Assistant uses is moisture, so probably best to use that if so.
|
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. |
| elif payloadSubType == 0x0A or payloadSubType == 0x02: | ||
| typeByte, mac = struct.unpack_from(">B8s", pkt.Payload); | ||
| mac = mac.decode('ascii') | ||
| battery = pkt.Payload[0x0C] |
There was a problem hiding this comment.
This one is consistently (for me anyways) a value of around 150... How can we figure the actual battery value from that?
|
@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.