-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Convert shell scripts to single Python script #48
Convert shell scripts to single Python script #48
Conversation
59ece31
to
3b031d4
Compare
99c9a0a
to
9f3d47e
Compare
87cb872
to
b84fabc
Compare
WOW! Just wow! This is awesome, thank you!! I will need some time to take a look at this, though. I'll have to push this to the weekend (hopefully). Very sorry about that. |
No worries @PhilippMundhenk, I already have it running on my NAS as my daily driver by overriding the scripts, so I'm in no rush 🙂 |
bff36c1
to
9ee9978
Compare
Turns out that the duplicate logs were caused by a lingering batch I had in |
I tried a few more times and this time I didn't see any disconnections 🤷 Can you please try the latest image? Normally, OCR should be the only problem left. |
9b36b70
to
665fa8c
Compare
OCR should also be fixed. |
Thank you so much for the super fast respose! I don't manage to be this fast.
I have not tested FTPS, inotify, Telegram |
@PhilippMundhenk that's strange, because OCR key is mapped to
🎉
I've tested the scenario locally and pushed a fix for it, which also logs what happened. |
OCR: Oh ok, misunderstanding. I thought I can add rear pages and it still runs through OCR. I starte with scantoocr and then ran scantoemail for rear pages. I think we should just add OCR to front and front and rear scans on the scantofile and scantoemail buttons, if OCR variables are set. This would be backward compatible behavior.
Aborts: I noticed now that it seems to be the scan command that is hanging. There is probably little we can do about that, at least not now. Same behavior in master today. Never had issues with this though, so maybe that is not even a practical situation. |
I'm not sure I follow. Right now we're calling OCR for any document that has finished scanning - regardless of which buttons were used. This seems logical, no?
@PhilippMundhenk I don't see the following lines on your log. Were they present? print(f" {side} side: Conversion and post-processing for finished.")
print("-----------------------------------") |
Yes, that would be the ideal behavior, indeed. Nope, never saw those lines... |
@PhilippMundhenk I wonder if the OCR variables are present at that point. I tested in the REPL inside the container and the check seems to work as expected. Would you be able to add some |
Ok, so the issue is not within OCR, it is within the telegram notification. We never reach OCR: print("notifying...")
notify(log, output_pdf_file, f"{job_name}.pdf ({side}) scanned")
print("notified")
print("cleaning...")
clean_job_files(log, side, job_name)
print("cleaned")
print("OCRing...")
# Check for OCR environment variables
ocr_server = os.getenv("OCR_SERVER")
ocr_port = os.getenv("OCR_PORT")
ocr_path = os.getenv("OCR_PATH")
print("OCR_SERVER: " + ocr_server)
print("OCR_PORT: " + ocr_port)
print("OCR_PATH: " + ocr_path) log:
|
Well, yeah, makes sense. We actually exit(1) there, rather than returning :) |
made sure scan doesn't exit if telegram not found
😄 yeah, that would do it! |
OCR: Something still off, but I'm looking into it. Empty pages: Are you sure that removal of empty pages works? I just scanned a bunch of empties, it also logs that the pages have been removed, but I receive a PDF with two empty pages. |
Is it a document consisting of all empties? If so, I've noticed that the file comes out as original, but I don't think that's necessarily bad, since that's not a usual scenario, and at least it allows you to see what is wrong with the document, instead of receiving a PDF with zero pages. I've tried scanning pages with some empty backs, and it worked as expected. |
Ah ok, yes, indeed, special case. I was too lazy to put paper in and just scanned a bunch of nothing. We can leave it like that. OCR: Works!! FTP: It tries to upload, although no variable set. I changed the checking condition. |
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.
lgtm
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.
Ah, just now I said in #53 that 1) there is not an issue, but here it is. If we remove the shell files altogether, we will also need to make sure the web interface is directly calling the python scripts...
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.
We probably don't need any of the php cleanups, once #32 is in...
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.
Wow! This is really neat! Much easier to handle and for users to adapt
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 feel we could make it even simpler, by getting logging and some formalities out of the way, but this is really nitpicking. Let's keep some todos for the future ;)
Oh, more of a "note-to-self": One thing I noticed in OCR is that the files are now massive. 33MB for a 14MB input, used to be that they come out much smaller. Not sure why that is... I will take a look at that another day. But this can be merged into dev, so that we can get started on the web UI integration... |
This PR converts the scripts to a single Python 3 script (
scanner.py
), making the code more readable and more maintainable. The log output was also improved in order to increase readability. Improvements are welcome, as I'm not versed in Python.NOTES:
The code should be in a good enough condition to use daily, but there might be bugs, especially in the scripts that I don't use (
trigger*.py
).There have been recent changes that aren't yet incorporated in the Python script.The remove_blank.sh script hasn't been converted fully yet, as that involves parsing the output of some command line utilities.UPDATE: fixedLater on, we might want to remove the remaining shell scripts by changing the contents of/opt/brother/scanner/brscan-skey/brscan-skey.config
to refer directly to the Python scripts:Related to #42