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

Infinite asyncio tasks created for Kuando Busylights (add_task bug). #416

Open
2 tasks done
aponoran opened this issue Nov 25, 2024 · 6 comments
Open
2 tasks done
Assignees
Labels
bug Something isn't working

Comments

@aponoran
Copy link

aponoran commented Nov 25, 2024

Software Versions:

  • Operating System: MacOSx Sonoma 14.6.1
  • Python version: 3.9
  • BusyLight version: 0.32.0

General Type of Problem

  • Integrating into another project
  • other

Describe the Problem
When setting colors on Kuando Busylightrs new asyncio tasks get created every time a new color is set. There is a keepalive task that gets created each time "on" is called.

I traced it back to the def taskable add_task method which doen not correctly lookuop the task by name in the tasks dictionary.
It is added in the dictionary with the name concatenated id of the object, but looked up only byy name.

Expected Behavior
Only one keep alive task should be created per light instance.

Error Output
If there was an error displayed, paste it in here.

@aponoran aponoran added the bug Something isn't working label Nov 25, 2024
@aponoran aponoran changed the title [BUG] Inifitite asyncio tasks created for Kuando Busylights (add_task bug). Nov 25, 2024
@aponoran aponoran changed the title Inifitite asyncio tasks created for Kuando Busylights (add_task bug). Infinite asyncio tasks created for Kuando Busylights (add_task bug). Nov 25, 2024
@JnyJny
Copy link
Owner

JnyJny commented Nov 25, 2024

Thank you for the bug report and chasing down the error! I'll see about getting it fixed shortly.

JnyJny added a commit that referenced this issue Nov 26, 2024
An instance's id value was appended to a task name for reasons that
are lost in the mists of time. Presumably it was to disambiguate tasks
with the same name but different instances. However, this doesn't make
sense since tasks live in namespaces unique to instances, and the
disambiguation is not necessary.
@JnyJny
Copy link
Owner

JnyJny commented Nov 26, 2024

Unfortunately, I won't be in a position to test this code until next week. If you are so inclined, you could install the latest version of the main branch using something like:

$ python3 -m venv testing
$ source testing/bin/activate
$ python3 -m pip install git+https://github.com/JnyJny/busylight
...

You should see fewer asyncio tasks created. Thanks again for the great bug report, it is much appreciated.

@aponoran
Copy link
Author

aponoran commented Nov 26, 2024

Works perfectly, thanks a lot. I ran it for a while, changed the colors a zillion times and checked the async task list and, as expected, I see only one keep alive:

2024-11-26 11:56:23,254 - root - INFO - Task 1: <Task pending name='Task-6' coro=<_keepalive() running at busylight/lights/kuando/busylight_alpha.py:73> wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x10ff81b50>()]>>

Many thanks!

Also, as a separate note, I had to deploy on Windows and to make it work I had to send an additional 0 to the devices. Then everything was working; without it the lights were ignoring any commands. I have no idea why it works with the 0.
And I had 2 different devices, a Luxafor ORB and a Kuando, and both behaved the same.

if "WINDOWS" in platform.system().upper():

    def patched_write_strategy(instance: lightslib.Light, data: bytes) -> int:
        data = bytes([0]) + data
        return instance.device.write(data)

    lightslib.Light.write_strategy = patched_write_strategy

@JnyJny
Copy link
Owner

JnyJny commented Nov 26, 2024

Glad to hear the fix works, that is frankly an embarrassing bug :)

Moving on to Windows, that is very interesting to me! Were you running directly with Windows or using the Linux personality?

Strictly speaking, instead of calling instance.device.write, we should instead honor the strategy method ( could be instance.device.write or instance.device.send_feature_report ). Different devices require different IO strategies, hence the read and write strategy properties. A better fix might be to update the __bytes__ magic method to prepend a zero byte when preparing the data payload for it's write operation (via write_strategy). Of course, we don't want to evaluate is Windows every time we want to write so we can do some messing about in __init__. I'll play with it and see what makes the most sense. Any chance I can lean on you to test my bad ideas on Windows?

@aponoran
Copy link
Author

Yes, running directly on windows. I can absolutley test for you on Windows. What would be the better fix is to figure out why the extra byte makes it work (I don't even think it has to be a 0x0, it can be any value). I will do some digging on the hid interface when I have a bit of time.

@JnyJny
Copy link
Owner

JnyJny commented Nov 26, 2024

I've been researching the Windows behavior as well and this article seems to discuss the problem as it relates to Windows and HID devices. I think. It's honestly very murky. The "any value" makes sense in context of the linked article, which claims that the Windows USB HID stack doesn't pass along that initial byte but still requires it. I'm adding this to the big pile of reasons why I personally dislike Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants