Skip to content
This repository has been archived by the owner on Dec 18, 2024. It is now read-only.

Use kuksa-client as non context-manager #526

Conversation

lukasmittag
Copy link
Contributor

This solves #476

Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling quit in kuksa-client yields exception:

Test Client> quit
Traceback (most recent call last):
  File "/Users/scs2rng/.local/share/virtualenvs/testkc-dPiaaAKY/bin/kuksa-client", line 8, in <module>
    sys.exit(main())
  File "/Users/scs2rng/Documents/Dev/kuksa.val/kuksa-client/kuksa_client/__main__.py", line 550, in main
    clientApp.stop()
  File "/Users/scs2rng/Documents/Dev/kuksa.val/kuksa-client/kuksa_client/__main__.py", line 384, in stop
    self.commThread.stop()
  File "/Users/scs2rng/Documents/Dev/kuksa.val/kuksa-client/kuksa_client/__init__.py", line 51, in stop
    self.backend.stop()
AttributeError: 'Backend' object has no attribute 'stop'

@SebastianSchildt
Copy link
Contributor

Started some testing on async side, first example (shortened), works when chaign from

async with VSSClient('127.0.0.1', 55557) as client:
        current_values = await client.get_current_values([
            'Vehicle.Speed',
        ])
        print(current_values['Vehicle.Speed'].value)

to

    myclient=VSSClient('127.0.0.1', 55557)
    await myclient.connect()
    current_values = await myclient.get_current_values([
            'Vehicle.Speed',
    ])
    print(current_values['Vehicle.Speed'].value)

Both version work, however subscription is broken, the vanilla example (complete)

import asyncio

from kuksa_client.grpc import Datapoint
from kuksa_client.grpc.aio import VSSClient

async def main():
    async with VSSClient('127.0.0.1', 55557) as client:
        await client.set_target_values({
            'Vehicle.Body.Windshield.Front.Wiping.System.TargetPosition': Datapoint(45),
        })

        async for updates in client.subscribe_current_values([
            'Vehicle.Body.Windshield.Front.Wiping.System.TargetPosition',
        ]):
            current_position = updates['Vehicle.Body.Windshield.Front.Wiping.System.TargetPosition'].value
            print(f"Current wiper position is: {current_position}")


asyncio.run(main())

fails with

 % python asynciosubscribe.py 
/private/tmp/testkc/asynciosubscribe.py:12: RuntimeWarning: coroutine 'VSSClient.check_connected_async.<locals>.wrapper' was never awaited
  async for updates in client.subscribe_current_values([
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
Traceback (most recent call last):
  File "/private/tmp/testkc/asynciosubscribe.py", line 19, in <module>
    asyncio.run(main())
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/private/tmp/testkc/asynciosubscribe.py", line 12, in main
    async for updates in client.subscribe_current_values([
TypeError: 'async for' requires an object with __aiter__ method, got coroutine

It works well with the released version for kuksa-client (may only complain a None Type is returned, if the value has not been set before)

@lukasmittag
Copy link
Contributor Author

Started some testing on async side, first example (shortened), works when chaign from

async with VSSClient('127.0.0.1', 55557) as client:
        current_values = await client.get_current_values([
            'Vehicle.Speed',
        ])
        print(current_values['Vehicle.Speed'].value)

to

    myclient=VSSClient('127.0.0.1', 55557)
    await myclient.connect()
    current_values = await myclient.get_current_values([
            'Vehicle.Speed',
    ])
    print(current_values['Vehicle.Speed'].value)

Both version work, however subscription is broken, the vanilla example (complete)

import asyncio

from kuksa_client.grpc import Datapoint
from kuksa_client.grpc.aio import VSSClient

async def main():
    async with VSSClient('127.0.0.1', 55557) as client:
        await client.set_target_values({
            'Vehicle.Body.Windshield.Front.Wiping.System.TargetPosition': Datapoint(45),
        })

        async for updates in client.subscribe_current_values([
            'Vehicle.Body.Windshield.Front.Wiping.System.TargetPosition',
        ]):
            current_position = updates['Vehicle.Body.Windshield.Front.Wiping.System.TargetPosition'].value
            print(f"Current wiper position is: {current_position}")


asyncio.run(main())

fails with

 % python asynciosubscribe.py 
/private/tmp/testkc/asynciosubscribe.py:12: RuntimeWarning: coroutine 'VSSClient.check_connected_async.<locals>.wrapper' was never awaited
  async for updates in client.subscribe_current_values([
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
Traceback (most recent call last):
  File "/private/tmp/testkc/asynciosubscribe.py", line 19, in <module>
    asyncio.run(main())
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/private/tmp/testkc/asynciosubscribe.py", line 12, in main
    async for updates in client.subscribe_current_values([
TypeError: 'async for' requires an object with __aiter__ method, got coroutine

It works well with the released version for kuksa-client (may only complain a None Type is returned, if the value has not been set before)

I see, will dig deeper into this issue.

@lukasmittag lukasmittag force-pushed the feature/use_kuksa_client_no_context_manager branch from 0788926 to 8e290b0 Compare April 4, 2023 12:47
@lukasmittag
Copy link
Contributor Author

This PR ignores tests for authorize function as this is part of #517

@lukasmittag lukasmittag force-pushed the feature/use_kuksa_client_no_context_manager branch from 8e290b0 to 8de9460 Compare April 5, 2023 07:23
@lukasmittag lukasmittag force-pushed the feature/use_kuksa_client_no_context_manager branch from ea1b0bb to c83cae8 Compare April 11, 2023 14:50
Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, but I do think it is not ideal that the CLI exposed to the user has (dis)connectThread and (dis)connect functions that do slightly different things.

I think I want to connect (and maybe disconnect, although likely I'll just call another connect instead), and I feel those functions should be doing what the "thread" versions are doing now (at least in the connect case)

@lukasmittag
Copy link
Contributor Author

It works, but I do think it is not ideal that the CLI exposed to the user has (dis)connectThread and (dis)connect functions that do slightly different things.

I think I want to connect (and maybe disconnect, although likely I'll just call another connect instead), and I feel those functions should be doing what the "thread" versions are doing now (at least in the connect case)

Agreed, what do you mean by another connect instead

Copy link
Contributor

I mean in a workflow I will likely do setServerAdress ... and then connect, I then might do another setServerAdress (to a different location) and call connect again. I would assume it disconnects the previous connection automatically before reconnecting. Therefore, It hink while having a disconnect is consistent and does not hurt, people will likely not use it very often (and should not be "required" to do so, when they decide to connect to a different location)

@lukasmittag
Copy link
Contributor Author

I mean in a workflow I will likely do setServerAdress ... and then connect, I then might do another setServerAdress (to a different location) and call connect again. I would assume it disconnects the previous connection automatically before reconnecting. Therefore, It hink while having a disconnect is consistent and does not hurt, people will likely not use it very often (and should not be "required" to do so, when they decide to connect to a different location)

Tested and works as you expected

Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, tested client and (a)sync examples

lgtm 🐢

@SebastianSchildt SebastianSchildt merged commit fc53f56 into eclipse:master Apr 12, 2023
@erikbosch erikbosch deleted the feature/use_kuksa_client_no_context_manager branch October 31, 2024 12:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants