error in computing room_temp_adj

[uspass]

I reckon there's an error in computing room_temp_adj, line 645 and following.

data['room_temp_adj'] = ((payload[13] << 8) + payload[14])/2.0

Suppose stored adj is negative, that is, for example
payload[13] =0xFF
payload[14] = 0xFA

Thus
adj = 0xFFFA

You divide it by 2.0, and obviously the result is always less than 0x7FFF = 32767.
When the stored adj is positive it is always less than 0x7FFF = 32767

Consequently the next line:

if data['room_temp_adj'] > 32767:

will always produce False and the next line

data['room_temp_adj'] = 32767 - data['room_temp_adj']

will never happen.

A correct version of the code would be:

data['room_temp_adj']  = (_response[13] << 8) + _response[14]
if data['room_temp_adj']  > 0x7FFF:
    data['room_temp_adj']  =data['room_temp_adj']  - 0x10000
data['room_temp_adj']  = data['room_temp_adj']  / 2.0

cheers,
us

[ptd006]

I skimmed quickly and looks like you're right.

Cheers, Peter

[Danielhiversen]

Please open a pull request

[uspass]

I will PR, but before doing it I want to share few things about the pyton-broadlink's hysen class.

As far as I know, Hysen company does 3 types of controllers (from the firmware point of view), sold directly by them and also as OEM for Beok, Floureon, Beca Energy and others.
The first type is a heating controller (class hysen in python-broadlink code) and identified by (devtype ) 0x4EAD.
The second type is a 2 pipe fan coil identified by 0x4F5B.
The third type is a 4 pipe fan coil. As I don't own one, I can't tell what is its devtype.

I have spent lots of hours testing first two types and understanding how the data is structured and how it can be properly retrieved and changed. Btw, the cli was a great help.

The result of that work can be found, for python-broadlink's class hysen (I call it hysenheating)
here:
https://github.com/uspass/hysenheating/blob/master/config/custom_components/hysenheating/hysenheating_device.py
and for the 2 pipe fan coil class (hysen2pfc) here:
https://github.com/uspass/hysen2pfc/blob/master/config/custom_components/hysen2pfc/hysen2pfc_device.py
As one can see, I have commented every routine.
The code is in production for several months now and I couldn't see any problem.

There are few important things that have to be mentioned:

1. The CRC16 problem. I have raised an issue about it Error in hysen device, send request, CRC16 #223 )
   I was wrongfully thinking the issue can be avoided by changing the location of the "from PyCRC.CRC16 import CRC16" . The real solution is:

        for i in range(1, 3):
            crc = CRC16(modbus_flag = True).calculate(bytes(input_payload))
            if crc == None:
                _LOGGER.error("[%s] CRC16 returned None, step %s.", self._host, i)
            else:
                break

Real live testing shows it never goes beyond the second loop to get the proper value.

2. In send_request, when the device answers and an error appears in the answer, the device has to be "reset".
   A call to self.auth() is needed:

        # check if return response is right
        if (input_payload[0:2] == bytearray([0x01, 0x06])) and \
           (input_payload != return_payload):
            _LOGGER.error("[%s] request %s response %s",
                self.host,
                ' '.join(format(x, '02x') for x in bytearray(input_payload)),
                ' '.join(format(x, '02x') for x in bytearray(return_payload)))
            self.auth()
            raise ValueError('hysen_response_error','response is wrong')
        elif (input_payload[0:2] == bytearray([0x01, 0x10])) and \
             (input_payload[0:6] != return_payload):
            _LOGGER.error("[%s] request %s response %s",
                self.host,
                ' '.join(format(x, '02x') for x in bytearray(input_payload)),
                ' '.join(format(x, '02x') for x in bytearray(return_payload)))
            self.auth()
            raise ValueError('hysen_response_error','response is wrong')
        elif (input_payload[0:2] == bytearray([0x01, 0x03])) and \
             ((input_payload[0:2] != return_payload[0:2]) or \
             ((2 * input_payload[5]) != return_payload[2]) or \
             ((2 * input_payload[5]) != len(return_payload[3:]))):
            _LOGGER.error("[%s] request %s response %s",
                self.host,
                ' '.join(format(x, '02x') for x in bytearray(input_payload)),
                ' '.join(format(x, '02x') for x in bytearray(return_payload)))
            self.auth()
            raise ValueError('hysen_response_error','response is wrong')
        else:
            return return_payload

3. Splitting the set mixed commands to basic commands.
   User is not always aware of the fact that calling a routine may change several variables, instead of the very one he actually wants to be changed.
   For example, in python-broadlink code:

    def set_power(self, power=1, remote_lock=0):
        self.send_request(bytearray([0x01, 0x06, 0x00, 0x00, remote_lock, power]))

Here if one wants to change power, one has to know the actual status of the remote lock and vice-versa.
The hysenheating equivalent code:

    # set lock and power
    # 0x01, 0x06, 0x00, 0x00, 0x0r, 0xap
    # r = Remote lock, 0 = Off, 1 = On
    # a = Manual over Auto, 0 = Off, 1 = On
    # p = Power State, 0 = Power off, 1 = Power on
    # confirmation response:
    # response 0x01, 0x06, 0x00, 0x00, 0x0r, 0x0p
    def set_lock_power(self, remote_lock, power_state):
        _request = bytearray([0x01, 0x06, 0x00, 0x00])
        _request.append(remote_lock)
        _request.append(power_state)
        self.send_request(_request)

    def set_remote_lock(self, remote_lock):
        if remote_lock not in [
            HYSEN_HEAT_REMOTE_LOCK_OFF, 
            HYSEN_HEAT_REMOTE_LOCK_ON]:
            raise ValueError(
                'Can\'t set remote lock (%s) outside device\'s admitted values (%s), (%s).' % ( \
                remote_lock,
                HYSEN_HEAT_REMOTE_LOCK_OFF,
                HYSEN_HEAT_REMOTE_LOCK_ON))
        self.get_device_status()
        self.set_lock_power(
            remote_lock, 
            self.power_state)

    def set_power(self, power_state):
        if power_state not in [
            HYSEN_HEAT_POWER_OFF, 
            HYSEN_HEAT_POWER_ON]:
            raise ValueError(
                'Can\'t set power state (%s) outside device\'s admitted values (%s), (%s).' % ( \
                power_state,
                HYSEN_HEAT_POWER_OFF,
                HYSEN_HEAT_POWER_ON))
        self.get_device_status()
        self.set_lock_power(
            self.remote_lock, 
            power_state | (self.power_state & 0xFE))

One can see that self.get_device_status() ensure we have the real time status of the variable(s) we don't want to change.

4. Manual over auto
   The hysen class does not deal with this situation.
   On the phone app, if one is in auto mode and changes the target temp a variable is set (see the code above):

   # a = Manual over Auto, 0 = Off, 1 = On

That tells the device that the target temp is temporarily manually changed. The manually changed value in auto mode is different than the manual target temp which remains untouched.

Please take a look to the hysenheating and hysen2pfc.
If you think this two classes can be incorporated into the python-broadlink, please let me know.

Thank you for your time,
Cheers,
us

[felipediel]

A correct version of the code would be:

data['room_temp_adj']  = (_response[13] << 8) + _response[14]
if data['room_temp_adj']  > 0x7FFF:
    data['room_temp_adj']  =data['room_temp_adj']  - 0x10000
data['room_temp_adj']  = data['room_temp_adj']  / 2.0

Hi @uspass. Thank you for pointing this out. Indeed, that block of code will never be reached. I am improving the hysen class and came to this issue, which is being very useful. Let's see if we can solve the problem together. Your suggestion brings us closer to the solution, but it seems to have a precision error. I don't have the device, could you please test this formula?

data["room_temp_adj"] = int.from_bytes(response[13:15], "big", signed=True) / 10.0

[felipediel]

This is the PR in case someone wants to help with tests: #572.

[felipediel]

Thank you! 🚀

[ptd006]

Looks nice improvements/fixes, thanks both (I was the original author and no longer have the device but it's great to see the code still being developed)