bendavid / aiopylgtv

Library to control webOS based LG Tv devices
MIT License
143 stars 47 forks source link

picture_settings only contains the last set values #45

Open nisargjhaveri opened 2 years ago

nisargjhaveri commented 2 years ago

I was trying to integrate the new picture_settings and ability to set brightness, contrast, etc in home assistant and this is what I observed.

Initially picture_settings contains all four brightness, contrast, backlight and color properties. But once I called set_current_picture_settings with a dict containing only backlight, the picture_settings now only contains the backlight property.

Is this expected? How to ensure that all the properties are correctly reported even after setting one of the properties optionally?

chros73 commented 2 years ago

Hi! Can you write down the exact steps (methods) to reproduce it, along with the expected result? I can take a look at it then.

nisargjhaveri commented 2 years ago

Here is the minimal script that I tried with the output.

import asyncio
from aiopylgtv import WebOsClient

async def on_state_change():
    print("State changed:", client.picture_settings)

async def runloop():
    global client
    client = await WebOsClient.create('<host>', '<key file>')

    await client.register_state_update_callback(on_state_change)

    await client.connect()

    print("Updating backlight")
    picture_settings = {'backlight': 100}
    await client.set_current_picture_settings(picture_settings)

    await asyncio.sleep(5)
    print("After sleep:", client.picture_settings)

    await client.disconnect()

asyncio.get_event_loop().run_until_complete(runloop())

Output:

State changed: {'contrast': 100, 'backlight': 50, 'brightness': 50, 'color': 70}
Updating backlight
State changed: {'backlight': 100}
After sleep: {'backlight': 100}
State changed: None

Expected: After calling set_current_picture_settings, it should report all the properties as before.

Actual: After calling set_current_picture_settings, it reports only the properties that were updated.

chros73 commented 2 years ago

I've fixed it in my fork, no release yet, so you have to install it from source. (I also added the ability to specify certain static states and state updates.)

What happened was: the server only sent back the modified value(s), and it has overwritten the original ones. Solution: merge the sent value(s) with original.

I'm not sure whether other state updates are also affected or not, you can see the full list here.

bendavid commented 2 years ago

I can imagine that what's happening is that the subscription is modified underneath, so I would assume that the other values will not update in this condition.

Not sure what the proper solution is, other than always calling the get api again with the full set of parameters.

chros73 commented 2 years ago

This should work fine, since we update it with values from the server, so I don't think another call is needed. Not sure about the rest.

chros73 commented 2 years ago

I released it (along with couple of other changes).

nisargjhaveri commented 2 years ago

@bendavid Any update on this? I guess merging the results with an earlier reaponse might work on client side as well, but it'd be much nicer if the case was already handled here the right way. (I'm too new to this to know if there's a better way as well)

If we know the correct solution I might also be able to help of needed. Or if some more investigations I could do.