Skip to content

Removed TextIOWrapper from serial.#383

Merged
felixdivo merged 5 commits intohardbyte:developfrom
gustavovelascoh:Issue-382
Aug 7, 2018
Merged

Removed TextIOWrapper from serial.#383
felixdivo merged 5 commits intohardbyte:developfrom
gustavovelascoh:Issue-382

Conversation

@gustavovelascoh
Copy link
Copy Markdown
Contributor

@gustavovelascoh gustavovelascoh commented Aug 3, 2018

I got this working with Python 3 and slcan. I'm not sure if this fix is ok.

Fixes #382.

Copy link
Copy Markdown
Collaborator

@christiansandberg christiansandberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you been able to test it with Python 2.7?

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 6, 2018

Codecov Report

Merging #383 into develop will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           develop     #383   +/-   ##
========================================
  Coverage    59.31%   59.31%           
========================================
  Files           55       55           
  Lines         4242     4242           
========================================
  Hits          2516     2516           
  Misses        1726     1726

string += '\r'
self.serialPort.write(string.decode())
self.serialPort.flush()
self.serialPortOrig.write(bytes(string,'utf-8'))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this will work for Python 2.7.

>>> bytes('O', 'utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: str() takes at most 1 argument (2 given)

Although it should work with just bytes(string) as all characters used are plain ASCII.

@felixdivo
Copy link
Copy Markdown
Collaborator

Fixes #382. (Please correct me if I'm wrong.)

@gustavovelascoh
Copy link
Copy Markdown
Contributor Author

Hi, I will test it tomorrow with python 2.7 and let you know.

@gustavovelascoh
Copy link
Copy Markdown
Contributor Author

@christiansandberg You're right. That would not work in Python 2.7. The main reason I removed TextIOWrapper is because it does not accept b'\r' as newline, it expects just a string, '\r'. And the issue is that serial object returns a bytes instance which for Python2 is interchangeable with string, and TextIOWrapper recognize b'\r' as newline.

Python 2.7:

>>> '\r' == b'\r'
True

Python 3:

>>> '\r' == b'\r'
False

Would it be ok to check in runtime the version of the interpreter and call the methods with appropriate arguments? or Do you think in another approach to solve this?

@gustavovelascoh
Copy link
Copy Markdown
Contributor Author

Ignore my last comment. I found encode() method to convert from string/unicode to bytes, making it possible to use with Python2/3.

@felixdivo felixdivo added the bug label Aug 7, 2018
@felixdivo
Copy link
Copy Markdown
Collaborator

@gustavovelascoh
Copy link
Copy Markdown
Contributor Author

gustavovelascoh commented Aug 7, 2018

Thanks @felixdivo, already checked those links. I tested the last commit with both Python 2.7.12 and Python 3.5.2 and it works.

readStr = self.serialPort.readline()

readStr = self.serialPortOrig.read_until(b'\r')
readStr = readStr.decode('utf-8')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this line should move after the if not readStr: check and inside the else: block?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@felixdivo
Copy link
Copy Markdown
Collaborator

Thanks @felixdivo, already checked those links. I tested the last commit with both Python 2.7.12 and Python 3.5.2 and it works.

Okay cool. If you manage to write some test for it, Travis CI can test it one some other versions as well, but that is probably not worth the effort.

@felixdivo felixdivo merged commit 95c5c1a into hardbyte:develop Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AttributeError: 'str' object has no attribute 'decode' on slscan.py

3 participants