Bluetooth-Devices / govee-ble

Manage Govee BLE devices
Apache License 2.0
19 stars 9 forks source link

H5051 in little endian #56

Closed Ernst79 closed 1 year ago

Ernst79 commented 1 year ago

Describe the bug Shouldn't this be in little endian?

https://github.com/Bluetooth-Devices/govee-ble/blob/432223ada34def7bb58a1601afdfd6954cc3a152/src/govee_ble/parser.py#L190

Additional context HCI dump from https://github.com/custom-components/ble_monitor/issues/467

> 04 3E 19 02 01 04 00 92 E4 21 59 60 E3 0D 0C FF 88 EC 00 BA 
  0A F5 0F 64 C2 01 01 C5

Will use this code (no local name)

        if msg_length == 9 and (
            "H5051" in local_name or "H5071" in local_name or mgr_id == 0xEC88
        ):
            self.set_device_type("H5051/H5071")
            (temp, humi, batt) = PACKED_hHB.unpack(data[1:6])
            self.update_predefined_sensor(
                SensorLibrary.TEMPERATURE__CELSIUS, temp / 100
            )
            self.update_predefined_sensor(
                SensorLibrary.HUMIDITY__PERCENTAGE, humi / 100
            )
            self.update_predefined_sensor(SensorLibrary.BATTERY__PERCENTAGE, batt)
            return

In little endian, this makes sense.

BA 0A --> 2746 / 100 = 27.46 deg F5 0F --> 4085 / 100 = 40.85 % 64 --> 100%

However, PACKED_hHB.unpack(data[1:6]) is using big endian PACKED_hHB = struct.Struct(">hHB")

bdraco commented 1 year ago

https://github.com/Bluetooth-Devices/govee-ble/pull/34

Need to compare with this but mobile so can't dig right now

bdraco commented 1 year ago

I think we need to remove 5071 from that block as it's actually being parsed right above it

Ernst79 commented 1 year ago

Yes, I think we can remove the H5071 from the 2nd block, but I think we can move the H5051 to the first block as well, as the H5051 is in little endian as well. The only difference between the first and 2nd block is little vs big endian. The HCI dump above is from a H5051, and it uses little endian (1st block). So, there is no device that used big endian (2nd block).

So, I think we should delete the 2nd block and change the first block to

        if msg_length == 9 and (
            mgr_id == 0xEC88
            or "H5051" in local_name  # just to be sure
            or "H5052" in local_name
            or "H5071" in local_name
        ):
            if "H5051" in local_name:
                self.set_device_type("H5051")
            elif "H5052" in local_name:
                self.set_device_type("H5052")
            elif "H5071" in local_name:
                self.set_device_type("H5071")
            else:
                self.set_device_type("H5051")  # I think if it hasn't a name, like in my HCI dump, it is a H5051
            (temp, humi, batt) = PACKED_hHB_LITTLE.unpack(data[1:6])
            self.update_predefined_sensor(
                SensorLibrary.TEMPERATURE__CELSIUS, temp / 100
            )
            self.update_predefined_sensor(
                SensorLibrary.HUMIDITY__PERCENTAGE, humi / 100
            )
            self.update_predefined_sensor(SensorLibrary.BATTERY__PERCENTAGE, batt)
            return
bdraco commented 1 year ago

Sounds good. Mind doing a PR?

Ernst79 commented 1 year ago

Yes, sure, doing a final check in BLE monitor first.

Ernst79 commented 1 year ago

I'll make a BLE monitor release first, and see if it all works out. If no-one complains, I'll create the PR.

I also have info about the H5198, another 4-probe meat sensor. If the user is confirming it works, I will add that one in the same PR. Wil probably do that at the end of the week, such that we know it works OK in BLE monitor.

https://github.com/custom-components/ble_monitor/pull/1091/

bdraco commented 1 year ago

Thanks @Ernst79