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

Update to asyncio #38

Closed
wants to merge 3 commits into from
Closed

Conversation

r-bt
Copy link

@r-bt r-bt commented Jun 12, 2023

This is a large PR so I'm marking it as draft first to get opinions / potential issues as I go through refactoring everything to use asyncio.

What it does

  • Makes most functions async
  • Removes threading

Should hopefully improve the speed & reliability of the library

Why

Bleak is not meant to be run in multiple threads (see hbldh/bleak#1191 and hbldh/bleak#242) where the maintainers recommend against having multiple threads and event loops

the design paradigm for bleak is to have multiple connections all under 1 event loop in 1 thread

Concerns

  1. This will be a breaking change – will require updating all code that uses spherov2 and updating documentation
  2. I only have Sphero Bolts – it would be great if people with other Spheros could test this with theirs

ToDo:

  • Test! Test! Test!
  • Remove left-over comments from when updating

@superfashi
Copy link
Member

As you know this is breaking, so I'm hesitant to merge it into main branch. Again, I tried https://github.com/artificial-intelligence-class/spherov2.py/tree/async async before, the API will be confusing to use.

Using asyncio just forces this library to use the pattern that bleak was intended for. Bleak is a BLE library used by this project, not that this project is built with bleak in mind. We can always swap out bleak and use other BLE libraries (but last time I checked bleak was the best cross-platform one).

Hopefully you get my point here.

@r-bt
Copy link
Author

r-bt commented Jun 13, 2023

Just to clarify you feel that the SpheroV2 API would be confusing to use if switched to asyncio? It definitely would require changes however but I'm not sure it's that more confusing i.e. any old code that be quickly changed by putting it into an async function and then awaiting calls to SpheroV2

e.g. The Bolt test script goes from

print("Testing Starting...")
print("Connecting to Bolt...")
toy = scanner.find_BOLT()

if toy is not None:
    print("Connected.")
    with SpheroEduAPI(toy) as droid:
        print("Testing Start...")
        droid.set_main_led(Color(r=0, g=255, b=0)) #Sets whole Matrix
        
        droid.reset_aim()
        droid.set_main_led(Color(r=0,g=0,b=255))
        
        print("Luminosity: " + str(droid.get_luminosity()))
        print("Accel: " + str(droid.get_acceleration()))

        droid.set_main_led(Color(r=255, g=0, b=0)) #Sets whole Matrix

to

async def main():
    print("Testing Starting...")
    print("Connecting to Bolt...")
    toy = await scanner.find_BOLT()

    if toy is not None:
        print("Connected.")
        async with SpheroEduAPI(toy) as droid:
            print("Testing Start...")
            await droid.set_main_led(Color(r=0, g=255, b=0)) #Sets whole Matrix
            
            await droid.reset_aim()
            await droid.set_main_led(Color(r=0,g=0,b=255))
            
            print("Luminosity: " + str(droid.get_luminosity()))
            print("Accel: " + str(droid.get_acceleration()))

            await droid.set_main_led(Color(r=255, g=0, b=0)) #Sets whole Matrix
            print("Testing End...")

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

It also has some advantages especially with controlling multiple Spheros at once e.g. I need to send commands to multiple Spheros at once and having this be async makes that very easy.

I see what you mean with wanting to be adapter-agnostic and being able to switch out Bleak in that case then maybe switching to asyncio isn't worth it.

However IMO given that the library uses Bleak right now and not following Bleak's patterns has issues (I've been able to trace back #35, #36, and #37 to not being async) it makes sense to align. Switching out from Bleak is a hypothetical while this issues are actual.

In the end I'm happy to close this PR if it doesn't align with your plan for the project and just maintain my own fork

@superfashi
Copy link
Member

In your example, some methods are async and some not, which is confusing to use and prone to user errors.

The issues you are having, I think might be related to specific bleak versions, since I'm not having issues connecting to multiple droids.

In any case, seems to me that these are bleak issues, not this library's issue. If you think keeping one bleak instance/asyncio loop is the solution, then you should write an adapter that only uses one global bleak instance and one asyncio loop under a single thread, not rewriting the whole API.

@r-bt
Copy link
Author

r-bt commented Jun 13, 2023

Interesting, could I ask how many droids you're able to connect to? I am able to get up to around 5 before having the pherov2.controls.PacketDecodingException: Bad response checksum error. Also what version of Bleak / Operating system are you using (I'm on Bleak 0.20.2 and Raspberry Pi OS (bullseye))?

I did try determine if it was a library vs Bleak issue by writing a minimal example which wakes up the robot just using Bleak

import asyncio
import bleak

handshake = [('00020005-574f-4f20-5370-6865726f2121', bytearray(b'usetheforce...band'))]
response_uuid = '00010002-574f-4f20-5370-6865726f2121'
send_uuid = '00010002-574f-4f20-5370-6865726f2121'

wake_command = b'\x8d\n\x13\r\x00\xd5\xd8'

async def main():
    # Find devices

    bots = []

    for index, sphero in enumerate(sphero_names):

        adapter = "hci1" if index % 2 else "hci0"

        device = await bleak.BleakScanner.find_device_by_name(sphero, 10.0, adapter=adapter)

        bot = bleak.BleakClient(device, timeout=5.0)
        try:
            print("Connecting to bot: {name} using {adapter}".format(name=device.name, adapter=adapter))
            await bot.connect()
            print("Connected to bot: {name}".format(name=device.name))
            bots.append(bot)
            # Perform handshake
            for uuid, data in handshake:
                await bot.write_gatt_char(uuid, data, True)
            await bot.start_notify(response_uuid, lambda x, y: None)
            print("Performed handshake")
            # Wake up the device
            await bot.write_gatt_char(send_uuid, wake_command, True)
        except Exception as e:
            print("Exception!")
            print(e)
        
    wait = input("Disconnect?")

    for bot in bots:
        await bot.disconnect()

sphero_names = ["SB-1B35", "SB-2175", "SB-3026", "SB-618E", "SB-6B58", "SB-9938", "SB-BFD4", "SB-C1D2", "SB-CEFA", "SB-DF1D", "SB-F465", "SB-F479", "SB-F885", "SB-FCB2"]

asyncio.run(main())

This code was able to wake up 12 robots without any errors while I've not been able to do with SpheroV2

In regards to having a combination of async functions and regular functions I could switch all the functions to being async to remove this concern.

I'm not sure writing a new Bleak adapter that only uses one asynio event loop would work. The SpheroV2 functions still wouldn't be async which means coordinating over multiple Spheros would be more difficult. For example having the functions be async allows one to set all of the Bolt matrices concurrently like

background_tasks = set()
async with asyncio.TaskGroup() as tg:
        for bot in bots:
            task = tg.create_task(bot.set_matrix_fill(2, 0, 6, 6, Color(r=int(color[0]), g=int(color[1]), b=int(color[2]))))
            background_tasks.add(task)
            task.add_done_callback(background_tasks.discard)

As opposed to one-by-one or by wrapping the bot.set_matrix_fill in a new async function

@r-bt
Copy link
Author

r-bt commented Jun 13, 2023

Anyway I'll close this PR for now – if anything changes in regards to your project plans I'd be happy to reopen

@r-bt r-bt closed this Jun 13, 2023
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.

2 participants