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

Talk to socket with str, to serial with bytes #936

Closed
wants to merge 1 commit into from

Conversation

hroncok
Copy link
Collaborator

@hroncok hroncok commented Apr 20, 2018

Apparently, the API of socket and serial is different.

Never tested. I have no printer at hand.

Apparently, the API of socket and serial is different.
@hroncok
Copy link
Collaborator Author

hroncok commented Jun 11, 2018

Anyone for a review?

@rockstorm101
Copy link
Collaborator

Hi, I'm sorry but I'm currently unable to test anything. I'm working overseas so I have no printer nor my computer.

@Estragon2K17
Copy link

I tried this proposed fix but still cannot connect to my printer. The old Windows 1.6.0 release (Printrun-win-18Nov2017.zip) works fine (i.e. connects to the printer) and was used as the baseline for my testing.

3D Printer: FLSUN-QQ Delta
TCP IP+Port#: xxx.xxx.xxx.xxx:8080
OS: Windows 7 64-bit (Python 3.6 64-bit)

Before patch (Master branch ->2.0.0rc5 tag):
TypeError('write() argument must be str, not bytes',)

After #936 patch:
Can't write to printer (disconnected ?):
Traceback (most recent call last):
File "C:\source\Printrun\printrun\printcore.py", line 674, in _send self.printer.write(message)
io.UnsupportedOperation: not writable

I can help with further testing if someone can try to come up with a fix.

Thanks.

@Estragon2K17
Copy link

After passing the 'r' & 'w' as arguments for the socket makefile() method, the printer is able to connect now.

original:
self.printer = self.printer_tcp.makefile()

modified:
self.printer = self.printer_tcp.makefile('rw')

However, an exception with the following description was raised at the code below even though the printer appears to be working.

Description:
IndexError('tuple index out of range',)

Code:
else:
self.logError(_("SelectError ({0}): {1}").format(e.errno, decode_utf8(e.strerror)))
raise

@Estragon2K17
Copy link

Forgot to mention I had to make a change for the readline() method as well in order for the printer to connect.

Original:
try:
line = self.printer.readline().decode('ascii')

Modified:
try:
if self.printer_tcp:
line = self.printer.readline()
else:
line = self.printer.readline().decode('ascii')

I'm connecting to my printer via Wi-Fi connection. I have not tested serial connection so I'm not sure if any of these changes would break the serial connection mode. From code review, it doesn't appear so but I'll find time to test serial connection later.

@k3ns
Copy link

k3ns commented Dec 16, 2018

the default for makefile() is read, so make read & write:

self.printer = self.printer_tcp.makefile(mode='rw',buffering=None)

handle tcp and serial separately:

reading...

if (type(self.printer) is Serial):
    line = self.printer.readline().decode('ascii')
else:
    line = self.printer.readline()

writing...

if (type(self.printer) is Serial):
    self.printer.write((command + "\n").encode('ascii'))
else:
    self.printer.write((command + "\n")) 

I think this line:

if 'Bad file descriptor' in e.args[1]:

should be:

if 'Bad file descriptor' in e.args:

to fix tuple index out of range since e.args can be a list length 1

also need to increase timeout for self.printer_tcp.settimeout(self.timeout)

self.timeout = 4.25 

@rockstorm101
Copy link
Collaborator

Hi @k3ns, thanks a lot for your insight. This is really helpful. I just wanted to let you know that I've seen this and I'm working on a solution that not only fixes this but also liberates Printcore code from the "serial vs. tcp" thing. I'll let you know as soon as I have something for testing. Thanks again.

@rockstorm101
Copy link
Collaborator

I'm working on a solution that not only fixes this but also liberates Printcore code from the "serial vs. tcp" thing.

Well the bespoke solution (finally) arrived in the form of PR #1346.

Is this PR (talking about 936 now) still valid? I encountered no problems with the code as it is now so I would say we can cancel this. Thoughts?

@hroncok hroncok closed this May 9, 2023
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.

4 participants