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

python3 default support & solving python3 buffer migration problems #1284

Closed
wants to merge 2 commits into from

Conversation

supernlogn
Copy link

python3 support has problems to buffer migration issues.
By following this official guide: http://python3porting.com/problems.html#bytes-strings-and-unicode
problems with byte and str buffers like the example below are solved.

Before doing these changes the hello world example would have produced errors while monitoring like the one below:

▶ make monitor
MONITOR
--- idf_monitor on /dev/ttyUSB0 115200 ---
--- Quit: Ctrl+] | Menu: Ctrl+T | Help: Ctrl+T followed by Ctrl+H ---
ets Jun  8 2016 00:22:57

rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:2
load:0x3fff0018,len:4
load:0x3fff001c,len:5552
ho 0 tail 12 room 4
load:0x40078000,len:0

Traceback (most recent call last):
  File "/home/sniper/esp/esp-idf/tools/idf_monitor.py", line 571, in <module>
    main()
  File "/home/sniper/esp/esp-idf/tools/idf_monitor.py", line 505, in main
    monitor.main_loop()
  File "/home/sniper/esp/esp-idf/tools/idf_monitor.py", line 269, in main_loop
    self.handle_serial_input(data)
  File "/home/sniper/esp/esp-idf/tools/idf_monitor.py", line 305, in handle_serial_input
    self.handle_serial_input_line(self._read_line.strip())
  File "/home/sniper/esp/esp-idf/tools/idf_monitor.py", line 313, in handle_serial_input_line
    self.lookup_pc_address(str(m.group()))
  File "/home/sniper/esp/esp-idf/tools/idf_monitor.py", line 403, in lookup_pc_address
    if not "?? ??:0" in translation:
TypeError: 'str' does not support the buffer interface
make: *** [monitor] Error 1

Also, since all buffers are figured out for python3, I think python3 should be the default interpreter for monitoring.

@CLAassistant
Copy link

CLAassistant commented Nov 18, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

Thanks for sending these changes.

One of the long term goals for IDF was to make it work with either Python 2 or 3. esptool.py & its related tools already work this way, and it looks like you've sorted out the remaining snags.

If this PR seems to have everything working OK with Python 3 then feel free to change the comment in the root level Kconfig file from "Python 2 interpreter" to "Python interpeter" and put a note in the help section there that either 2.7 or 3.x can be used.

Before we merge this I'm going to make some internal changes so we can run CI tests with both Python 2 and 3, which should help prevent any regressions.

@@ -7,7 +7,7 @@
# SDK tool configuration
#
CONFIG_TOOLPREFIX="xtensa-esp32-elf-"
CONFIG_PYTHON="python"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not change this default here. macOS, and some Linux distros, still ship with python and no python3.

@enrikb
Copy link

enrikb commented Nov 28, 2017

Hi,
I just used your patch as basis to verify if my WSL patch would work with python3. While trying it, I found the following potential issues:

Have you tried the patch with python 2? I see very strange output here:

$ make monitor
MONITOR
--- idf_monitor on /dev/ttyS3 115200 ---
--- Quit: Ctrl+] | Menu: Ctrl+T | Help: Ctrl+T followed by Ctrl+H ---
['e']['t']['s'][' ']['J']['u']['n'][' '][' ']['8'][' ']['2']['0']['1']['6'][' ']['0']['0'][':']['2']['2'][':']['5']['7']['\r']['\n']['\r']['\n']['r']['s']['t'][':']['0']['x']['1'][' ']['(']['P']['O']['W']['E']['R']['O']['N']['_']['R']['E']['S']['E']['T'][')']
...

I further noticed that the getkey_patched function will need some tweaking, too (when actively used; the original used sets unichr = chr when running on python3, if I understand it correctly).

Last, but not least:
If I enter a non-ASCII character (example: German Umlaut ä), this is just not forwarded to the serial port with the python 2 version. With your patch and python3, I see the following:

esp32> ä

Traceback (most recent call last):
  File "/home/enrik/esp-idf/tools/idf_monitor.py", line 571, in <module>
    main()
  File "/home/enrik/esp-idf/tools/idf_monitor.py", line 505, in main
    monitor.main_loop()
  File "/home/enrik/esp-idf/tools/idf_monitor.py", line 269, in main_loop
    self.handle_serial_input(data)
  File "/home/enrik/esp-idf/tools/idf_monitor.py", line 305, in handle_serial_input
    self.handle_serial_input_line(self._read_line.strip())
  File "/home/enrik/esp-idf/tools/idf_monitor.py", line 312, in handle_serial_input_line
    for m in re.finditer(MATCH_PCADDR, line.decode('utf-8')):
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position 133: invalid continuation byte

Cheers,
Enrik

@projectgus
Copy link
Contributor

Hi @supernlogn ,

Sorry, I missed that you'd pushed more commits to this branch.

Unfortunately this PR branch doesn't work for me in either Python 2 or Python 3. (I get NameError: global name 'u' is not defined in both). Can you take another look, please?

@supernlogn
Copy link
Author

It works for me in python3, but I use python3.4.3
Please do the experiment below and tell me the outcome,
so I can fix it for other python versions:

▶ python
Python 2.7.6 (default, Nov 23 2017, 15:49:48) 
[GCC 4.8.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> u"asdasdasd"
u'asdasdasd'
>>> 

~                                                                                                                                                                                                                  
▶ python3
Python 3.4.3 (default, Nov 28 2017, 16:41:13) 
[GCC 4.8.4] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> u"asdasdasd"
'asdasdasd'
>>> 

~                                                                                                                                                                                                                  
▶ python --version
Python 2.7.6

~                                                                                                                                                                                                                  
▶ python3 --version
Python 3.4.3

Thank you very much!

@@ -288,8 +288,8 @@ def handle_key(self, key):
self.serial_reader.stop()
else:
try:
key = self.translate_eol(key)
self.serial.write(codecs.encode(key))
key = self.translate_eol(bytes(u(""+key), 'utf-8'))
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem isn't u"abc", this works fine the same as in your output.

The problem is that u("abc") is an error in Python 2 & 3, as this function doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, for example, the way to write this is bytes(u"" + str(key)) or bytes(u"" + key) if you know the type of key.

@SonnyX
Copy link

SonnyX commented Apr 26, 2018

Code below fixes issues mentioned by projectgus above.

  def handle_key(self, key):
       if self._pressed_menu_key:
           self.handle_menu_key(key)
           self._pressed_menu_key = False
       elif key == self.menu_key:
           self._pressed_menu_key = True
       elif key == self.exit_key:
           self.console_reader.stop()
           self.serial_reader.stop()
       else:
           try:
               key = self.translate_eol(bytes(key, 'utf-8'))
               self.serial.write(key)
           except serial.SerialException:
               pass # this shouldn't happen, but sometimes port has closed in serial thread
           except UnicodeEncodeError:
               pass # this can happen if a non-ascii character was passed, ignoring

   def handle_serial_input(self, data):
       # this may need to be made more efficient, as it pushes out a byte
       # at a time to the console
       try:
           for b in data.decode('utf-8'):
               s = bytes(b,'utf-8')
               self.console.write_bytes(s)
               if s == b'\n': # end of line
                   self.handle_serial_input_line(self._read_line.strip())
                   self._read_line = b""
               else:
                   self._read_line += s
               self.check_gdbstub_trigger(s)
       except UnicodeDecodeError:
           pass

@supernlogn supernlogn reopened this Jul 9, 2018
@supernlogn
Copy link
Author

@projectgus I have successfully changed the code as you suggested and also tested it.
In the tests I have used Greek and Latin characters in the terminal.
Also, I checked it in python3.5 and python2.7.
Also I updated my repo so It can be automatically merged.
I can upload the tests if you need them.

Thanks to everyone for your suggestions!

@igrr igrr closed this in 7118f47 Aug 28, 2018
catalinio pushed a commit to catalinio/pycom-esp-idf that referenced this pull request Jun 28, 2019
espressif-bot pushed a commit to espressif/esp-idf-monitor that referenced this pull request Jan 16, 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.

5 participants