-
Notifications
You must be signed in to change notification settings - Fork 10
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
Detect printer model and media type #53
Conversation
Hi there @vulpes2. As just laid out in #51 (comment) I really appreciate the effort here. This change will be gladly welcomed into the fork. I would ask to only format new code with tools like ruff/black to keep backports of the PR as simple as possible. |
I am sorry that my intention regarding back portability was not stated explicitly, it only covers existing code. New additions can follow modern Python behaviour and styling habits. |
Would it be fine if I rebased the feature changes without the formatting changes? |
Yes! Please (at) me once this is ready for review. |
pyusb is cross-platform and works out of the box after the correct udev rules have been set up, while /dev/usb/lp0 is still typically inaccessible by regular users unless the user is in a specific user group.
@matmair All done, I've only reformatted |
I will review/test this with a few printers and report back in the next few days! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a pure code only review without functional tests, I will ping you once I had the possibility to test the code with real hardware.
I finally got time to test this with a few printers and labels; it looks great. So far I have only tested with usb-based printers. Am I understanding this right that it will only work with USB printers? |
This might also be possible on network printers, but I do not own such a device to test it, nor is |
Just for information, maybe it helps: I am currently figuring out the same thing. It seems to me PT-P700 and PT-P710BT use same command parameters for cutting as QL-800 because the cutter works for PT-P700 and PT-P710BT when running the command as model QL-800 instead of PT-P750W. |
60c869c
to
789dda6
Compare
@mkildemaa I've pushed a fix for the PT-P700, it seems to work for me. Please test on other PT printers and report back if you have access to them. I don't mind addressing other bits in the same PR if I'm able to. There are a few issues I've fixed:
|
Uncompressed data was still not printing properly on my PT-P700, I was getting garbled graphics or blank labels. Turns out the compression mode must be set with every single print job on a supported printer, otherwise the compression mode is unknown to a printer that supports both. There is no default mode specified in any of the raster command reference manuals, so this should be specified for both QL and PT printers for better reliability. |
Ran this today and get this error:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a couple of changes to the f-strings, I was able to run the code.
Tested with 3 tapes for the QL-800 and 1 tape for the QL-1110NWB!
Output from my test runs:
|
I really think you should make this code a little bit more specific to the task at hand "detect printer and media type". Cutting, auto-cutting, etc definitely seems out of scope? |
I am also able to use this code within the python scripts instead of the CLI. It only worked for pyusb backend in my testing.
|
It looks like python 3.12 has extended the f-string syntax so it was valid syntax only in python 3.12. Tested the fix with python 3.11 and everything seems to work. https://docs.python.org/3/whatsnew/3.12.html About mixing the cutter related changes, I have very limited time to work on this and this PR is moving very slowly because none of us are doing this for a living, so I'd rather put it here instead of constantly rebasing a second PR just for those changes. |
Unfortunately I also can't reproduce the other error you have been seeing:
|
@matmair I believe all the concerns have been addressed, can you review this again? Would be great if we can get this merged soon. |
Sorry for the delay in answering. Much appreciated! All my printers are in the office in which the next time I will be there is tomorrow. I see the PR is now merged so I will report back only if there are any issues and if there is silence from my part then everything worked. |
That would be ideal, thanks. |
I've implemented some rudimentary printer model and media detection stuff. This should be a good base for adding support for media verification and printer model detection before printing later on. Tested working on Linux with a QL-700 and PT-P700.
Changes made:
Fixes pklaus#123
A good staring point for addressing #39
Fixes #54