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

Feat/asyncio #1828

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Feat/asyncio #1828

wants to merge 5 commits into from

Conversation

SamsonBox
Copy link

This pull request soves issues #1824. It uses asyncio to handle the serial communication. This can be done as python 3.8 is mandatory for bCNC. Now the reset command is synchonized with the respons. Also the clear message ist synchonized.

@Harvie
Copy link
Collaborator

Harvie commented Mar 28, 2023

To be honest, i am not really familiar with this async/await concept. Guess i will have to read something about it first :-)
Maybe other members of bCNC community will help me with the review in the meantime...

@SamsonBox
Copy link
Author

Actually this is pretty cool. Is a kind of cooperative scheduling in an eventloop within one thread. So the loop is started in the originally serialIo-Thread. within this loop tasks can be started and the can give back control to the schedular by the await key word. But take you time to review the changes.

@Harvie
Copy link
Collaborator

Harvie commented Mar 29, 2023

OK, i did some research. And while i agree that this is kinda cool.
It also feels like it might significantly impact readability and debugability of the code, since execution of individual routines will now be interdependent to some extent.

While i am not completely opposed to the idea, i would like to see what are the gains when compared to "synchronous" code in our use-case and whether they acutally outweigh the increase in maintenance complexity.

Also we might consider if it would make sense to split bCNC into several separate threads.
For instance i can image separating GUI, Serial RX, TX and CAM subsystems, so they do not block each other.
I would not be as lightweight as asyncio, but might fix even more issues.

@SamsonBox
Copy link
Author

So the problem I found is metioned in issue #1824. The basic problem is, that the controller reset is send from the serialIO thread but can not be synchronized to the execution end (which is the reading the initial test the controller sends). So my basic idea was to have two indevidual tasks (a reading and writing task) running in one thread. So one can wait in the writing task for the response from the reading task. You are right, this can be also solved by running these two tasks in seperat threads, but I think that is a overkill for the serial communication as it is relatively slow. Using two thread will not reduce the higher maintenance complexity. The GUI is allread in the mainthread and the CAM subsystem can be moved to it's one thread in both cases.

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