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

Clean up Ruida driver (also remove some unnecessary optimizations) #216

Open
TheAssassin opened this issue Mar 22, 2024 · 0 comments
Open

Comments

@TheAssassin
Copy link
Contributor

The Ruida class is quite messy and hard to get into when working on support for a new device in my opinion. Apparently, it tries really hard to reduce the binary size of jobs: it caches old speed/power values and only sends them when they change, it uses some (less precise?) relative coordinates instead of just using absolute ones because that apparently saves a few bytes per command, etc. That however makes it rather hard to follow the control flow and debug changes to the code.

But there are more issues with the current implementation. First, basically no documentation is available in the code itself (i.e., there are hardly any useful comments). One can try to dissect the Git history, at least to some extent, to understand what the authors were thinking while writing their code. But that does not tell the entire story.

Then, architecture wise, the caching pollutes the class's state. All the variables' purpose isn't obvious to the user at the first glance.

Next, there are various methods to convert values into bytes the driver understands which could be consolidated or at least be based on one another. Also, some helpers ("hidden" setters like cmd_absoluteMM) which seem to have been introduced to make said resend-of-cut-data optimizations a little less painful could be removed entirely. But if they should be kept, there should be variants for all the said cached data that just set the value directly. (After all, I don't mind the caching that much, but it really needs to be refactored so that it's easier to follow the control flow; for instance, a separate class could be instantiated wherever caching is needed, the current class-wide variables are reset for every job part which hints towards the fact that they should not be in that scope; the cache object can be passed to methods that need access).

Binary size also shouldn't matter. Any LAN should provide more than enough bandwidth to transfer the jobs if using the network feature, even wireless ones. USB transfer also won't run into issues with larger job sizes. I have not tested absurdly large engravings yet, but all my testing so far shows that job transfer is near instant via USB.

I hope that with the help of the available documentation I'll be able to at least document the code as it is right now to make it a little easier to understand. I'll send a PR for that first of all, hoping for the authors to comment on whether the documentation is accurate. Also, I'll try to de-obscure some of the magic commands and the envelope so that it'll be easier to implement helpers in other languages based on the known-working VisiCut driver.

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

No branches or pull requests

1 participant