Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async vesync #294

Open
wants to merge 7 commits into
base: dev-2.0
Choose a base branch
from
Open

Async vesync #294

wants to merge 7 commits into from

Conversation

webdjoe
Copy link
Owner

@webdjoe webdjoe commented Feb 12, 2025

Migrate to aiohttp from requests.

pyvesync.VeSync(user,pass) is an asynchronous generator:

session = aiohttp.Session() # optional - will generate session if not passed

with VeSync(user, pass, session=session) as manager:
    await manager.login()
    await manager.update()

outlets = manager.outlets

for outlet in outlets:
    await outlet.turn_on()

call_api() method is moved to the VeSync class. A session can be passed to VeSync or it will generate a new session.

Added Vesync response codes to raise correct errors and not raise errors when there is a device issue (open door, empyt water tank, etc.). Raises aiohttp.ClientError (aiohttp errors except >400 status code, this is handled by VeSyncServerError), VeSyncRateLimit (rate limit return code) , VeSyncServerError (server code error, such as network busy) and VeSyncAPIParseError for invalid api responses. Invalid API responses will be handled by validation.

Add and remove device methods in VeSync class fixed. Used "hack" to avoid using manager._dev_list() for device modifications. I believe this is creating a copy of the device type attributes and not updating.

Fixed incorrect configuration dictionary passed to __build_config_dict() method

Fix bug that prevented stale devices from being removed and new devices from being added.
Asynchronous API call with aiohttp
Replace native JSON with orjson in anticipation of mashumaro
Move `call_api` to manager
@webdjoe
Copy link
Owner Author

webdjoe commented Feb 12, 2025

@iprak @cdnninja Any comments?

@iprak
Copy link
Contributor

iprak commented Feb 12, 2025

@webdjoe
Copy link
Owner Author

webdjoe commented Feb 12, 2025

  • On the vesync support for debug mode in library home-assistant/core#134571 issue, I could remove the default override of logging hooks so it will only override if argument/property is set.
  • Using aiohttp should resolve the putrequest error, I'm guessing it is an incomplete blocking request in a thread that gets stuck when the server does not respond and the library does not handle it properly. I tried to raise an exception for all server-side and request errors. I still need to flesh this out a bit, but the goal is any issues in connecting or receiving a response from the server will raise an exception.
  • This is something I am still thinking about, I hesitated to raise an exception on every single device error code, because there are errors for device related issues, such as door open or water tank empty. I raised exceptions on major issues that prevent the library from operating - connection error, server issue, malformed response issue. One thought is that when I implement validation, the device update methods can either return DeviceResponseStruct on code=0 or DeviceErrorStruct if code<>0 and it is a device issue instead of exception.

I am open to suggestions here, let me know what you think

@iprak
Copy link
Contributor

iprak commented Feb 13, 2025

I think it is okay to not raise an exception. The false value seems sufficient to limit operations at integration end. I think we can should prevent a refresh if the operation failed.

One other idea I had was to return the error message for the last operation as a separate property e.g. lastErrorMessage... something like windows GetLastError.

@cdnninja
Copy link
Contributor

If returning false we also need an error code to know why, the is the issue for the re-auth flow. Ideally good to know if general error or username/password failed.

As for the debug detail - yes would be great to allow the default logger commands to work. Leaving the debug=true override only when listed keeps backward computability.

@webdjoe
Copy link
Owner Author

webdjoe commented Feb 13, 2025

Maybe create a DeviceError dataclass with error code and message if the device is on and connected but has an error. This way you can test for isinstance(DeviceError) and handle it, while catching server or connection errors.

@cdnninja
Copy link
Contributor

Should the new async methods be prefixed with async_ like the HA methods?

@webdjoe
Copy link
Owner Author

webdjoe commented Feb 16, 2025

I'm thinking of returning a dataclass or other simple data structure when updating devices and calling methods, sort of like the API to indicate the device specific issue - water empty, door open, voltage error, device offline. This way the return value will allow you to handle the various scenarios appropriately. I don't want to get too granular, just indicating device offline or there is an issue preventing the device from operating.

Exceptions will be raised for API errors, reponse parsing issues, rate limit, server errors, etc. I'll detail it in the next validation PR.

@cdnninja
Copy link
Contributor

I ran a quick test - not actions on my two devices. It appears to populate data, I haven't walked through each value no exceptions.

One item I received was:

2025-02-16 14:41:07 - ERROR - asyncio - Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x00000275790A1050>
2025-02-16 14:41:07 - ERROR - asyncio - Unclosed connector
connections: ['deque([(<aiohttp.client_proto.ResponseHandler object at 0x000002757907D780>, 2803616.89), (<aiohttp.client_proto.ResponseHandler object at 0x000002757907D0F0>, 2803616.906)])']
connector: <aiohttp.connector.TCPConnector object at 0x00000275790A1090>

However, I think that is correct since I haven't unloaded the session? Does a new method have to be added to close sessions?

@webdjoe
Copy link
Owner Author

webdjoe commented Feb 16, 2025

It depends, if you passed a session in, it does not close it. Like if HA session is passed, the library should not close it, it has to be done manually. If you did not pass in a session, it should have closed.

@cdnninja
Copy link
Contributor

I didn't pass a session in.

@webdjoe
Copy link
Owner Author

webdjoe commented Feb 16, 2025

Can you post your script, this is mine without any error on exit:

async def main():
    async with VeSync(USERNAME, PASSWORD, debug=True) as manager:
        await manager.login()
        await manager.update()
        for dev in manager.outlets:
            print(dev)
            if dev.connection_status == 'offline':
                continue
            await dev.turn_on()
            await dev.turn_off()
            await dev.get_monthly_energy()
            await dev.get_weekly_energy()
            await dev.get_config()
            print(dev.power)
            print(dev.voltage)


if __name__ == "__main__":
    asyncio.run(main())

I just pushed a new commit with debug output on exit, let me know what is logged.

@cdnninja
Copy link
Contributor

cdnninja commented Feb 16, 2025

manager = VeSync("Username", "password", time_zone="zone", Debug=True)
async def main():
    await manager.login()
    await manager.update()
    device = manager.fans[1]
    print(device)
    print("config.display:" + str(rgetattr(device, "config.display")))
    print("details.display:" + str(rgetattr(device, "details.display")))


asyncio.run(main())

@webdjoe
Copy link
Owner Author

webdjoe commented Feb 16, 2025

Where is VeSync instantiated?

@cdnninja
Copy link
Contributor

cdnninja commented Feb 16, 2025

It was above this section. However wasn't with the async command. Adding that solves it. Editing the block above to show that.

@webdjoe
Copy link
Owner Author

webdjoe commented Feb 16, 2025

If you don't want to use async with, you would have to call __aenter__() and __aexit__() manually:

manager = VeSync(user, pass)
await manager.__aenter__()

...

await manager.__aexit__(None, None, None)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants