New feature: Serial Terminal #650

screamerbg · 2018-03-27T01:08:37Z

Mbed CLI already has a -f option to automatically flash the program to the connected board after compiling. This PR takes the next natural next step to add serial terminal via -s/--sterm option ("s" for serial) to mbed compile and mbed detect.

Workflow
This PR extends mbed compile and mbed detect by adding -s/--sterm option. The option can be chained with -f/--flash (with mbed compile) or -r/--reset (with mbed detect), meaning that Mbed CLI will first apply the new firmware image, reset the MCU/target/board and then open serial terminal.

The serial terminal offers few commonly used shortcuts:

Quit: CTRL+C or CTRL+]
Reset: CTRL+B or CTRL+R
Echo toggle: CTRL+E
Terminal info: TAB or CTRL+I
Help: Ctrl+H
Menu: Ctrl+T (PySerial)

There are much more options behind the terminal menu (Ctrl+T).

The implementation is based on PySerial, which is one of the fundamental libraries for Mbed Greentea (mbed test). Therefore there are no additional dependencies.

Example usage with mbed compile

$ mbed compile -t GCC_ARM -m NUCLEO_F401RE -f -s
Building project mbed-os-example-blinky (NUCLEO_F401RE, GCC_ARM)
Scan: .
Scan: mbed
Scan: env
Compile [100.0%]: main.cpp
Link: mbed-os-example-blinky
Elf2Bin: mbed-os-example-blinky
+------------------+-------+-------+------+
| Module           | .text | .data | .bss |
+------------------+-------+-------+------+
| [fill]           |   102 |     7 |   11 |
| [lib]/c.a        | 27166 |  2204 |   56 |
| [lib]/gcc.a      |  3752 |     0 |    0 |
| [lib]/m.a        |    88 |     0 |    0 |
| [lib]/misc       |   236 |    16 |   28 |
| main.o           |   267 |     4 |  276 |
| mbed-os/drivers  |  1301 |     4 |  100 |
| mbed-os/hal      |  1395 |     4 |   66 |
| mbed-os/platform |  3096 |     4 |  354 |
| mbed-os/rtos     |  9114 |   168 | 5989 |
| mbed-os/targets  |  7017 |     5 |  388 |
| Subtotals        | 53534 |  2416 | 7268 |
+------------------+-------+-------+------+
Total Static RAM memory (data + bss): 9684 bytes
Total Flash memory (text + data): 55950 bytes

Image: ./BUILD/NUCLEO_F401RE/GCC_ARM/mbed-os-example-blinky.bin
[mbed] Detected "NUCLEO_F401RE" connected to "/Volumes/NODE_F401RE" and using com port "/dev/tty.usbmodem1413"
[mbed] --- Terminal on /dev/tty.usbmodem1413 - 9600,8,N,1 ---
[mbed] --- Quit: CTRL+C | Reset: CTRL+B | Echo: CTRL+E ---
[mbed] --- Info: TAB    | Help:  Ctrl+H | Menu: Ctrl+T ---
$$$07200221061A657F395EF362start
something 1
something 2
something 3
something 4
something 5
something 6
something 7
something 8
something 9

Example use with mbed detect

$ mbed detect -rs
[mbed] Detected "NUCLEO_F401RE" connected to "/Volumes/NODE_F401RE" and using com port "/dev/tty.usbmodem1413"
[mbed] --- Terminal on /dev/tty.usbmodem1413 - 9600,8,N,1 ---
[mbed] --- Quit: CTRL+C | Reset: CTRL+B | Echo: CTRL+E ---
[mbed] --- Info: TAB    | Help:  Ctrl+H | Menu: Ctrl+T ---
$$$07200221061A657F395EF362start
something 1
something 2
something 3
something 4
something 5
something 6
something 7

Inspired by @yennster's request (#639).

@yennster Do you think you could look into adding help for this feature in the README.md?

…s are present

sg-

Seems slick. @SenRamakri @deepikabhavnani Have a look and see if there is a natural way to plugin or incorporate the necessary logic for upcoming binary trace expansion.

theotherjimmy · 2018-03-27T14:32:37Z

It seems odd to me that compile would open a serial terminal. I fear we are turning mbed compile into mbed do. That being said, I can't think of a verb that means build it then run it.

theotherjimmy · 2018-03-27T14:34:27Z

mbed/mbed.py

@@ -34,6 +34,7 @@
 import zipfile
 import argparse
 import tempfile
+from mbed_cdc import mbed_cdc


This import should probably be scoped in some way. maybe from mbed.mbed_cdc or from .mbed_cdc. Further, this seems very redundant: from thing import thing.

This is not a must have, this works in python 2

theotherjimmy · 2018-03-27T14:35:59Z

mbed/mbed.py

+                try:
+                    from mbed_host_tests.host_tests_toolbox import flash_dev
+                except (IOError, ImportError, OSError):
+                    error("The '-f/--flash' option requires that the 'mbed-greentea' python module is installed.\nYou can install mbed-greentea by running 'pip install mbed-greentea'.", 1)


This is not strictly true. The -f/--flash option requires mbed-host-tests.

I'd rather encourage the user to install mbed-greentea which will pull in all the needed modules behind it. In this case, the product is mbed-greentea, not host-tests.

Sure, that works. If mbed-host-tests is not something the user should ever install standalone, I'll merge it with greentea.

In case this was not clean: this is resolved.

theotherjimmy · 2018-03-27T14:37:51Z

mbed/mbed_cdc.py

@@ -0,0 +1,111 @@
+


Added. Thanks

theotherjimmy · 2018-03-27T14:38:41Z

mbed/mbed_cdc.py

@@ -0,0 +1,111 @@
+
+def mbed_cdc(port, reset=False, sterm=False, baudrate=9600, timeout= 10, print_term_header=True):


Could we drop mbed from the name of this function? it adds no meaning here. I think this is more a run_serial_term or something similar.

But it's not a standard serial terminal and it's rather for Mbed needs. So I'd rather not use a generic name. Also the whole file would rather go in https://github.com/ARMmbed/mbed-sterm, and this function name will be the contract.

Understood. This is resolved.

theotherjimmy · 2018-03-27T14:39:29Z

mbed/mbed_cdc.py

+    except (IOError, ImportError, OSError):
+        return False
+
+    def get_instance(*args, **kwargs):


This function does not capture any state. Could you move it outside of the body of the mbed_cdc function?

It does - see port in printed message.

port is the first parameter. It seems weird to capture it and pass it in.

This is just a minor style comment. Not a blocker.

theotherjimmy · 2018-03-27T14:40:36Z

mbed/mbed_cdc.py

+            except:
+                result = False
+
+    def cdc_term(serial_instance):


I'm pretty sure that this function does not capture any state either. Could you move this function out of mbed_cdc?

It uses other variables from the wrapper function, e.g. imports and cdc_reset()

theotherjimmy · 2018-03-27T14:41:35Z

mbed/mbed_cdc.py

+                    console_print('[QUIT]')
+                    term.stop()
+                    term.alive = False
+                    break


This break is not needed; the loop already checks for term.alive

theotherjimmy · 2018-03-27T14:45:12Z

mbed/mbed_cdc.py

+    def cdc_reset(serial_instance):
+        try:
+            serial_instance.sendBreak()
+        except:


This except will catch ctrl+c. You probably did not mean to do that.

Then perhaps you could fix it in mbed-host-tests as well? :) https://github.com/ARMmbed/htrun/blob/master/mbed_host_tests/host_tests_plugins/module_reset_mbed.py#L77

Yep! Just a second.

theotherjimmy · 2018-03-27T14:45:23Z

mbed/mbed_cdc.py

+        except:
+            try:
+                serial_instance.setBreak(False) # For Linux the following setBreak() is needed to release the reset signal on the target mcu.
+            except:


This except will catch ctrl+c. You probably did not mean to do that.

See previous comment

theotherjimmy · 2018-03-27T14:46:00Z

mbed/mbed_cdc.py

+        serial_port.reset_input_buffer()
+        if reset:
+            cdc_reset(serial_port)
+            result = True


cdc_reset may set result to False. This statement ignores that assignment, effectively throwing away the reset failure.

theotherjimmy · 2018-03-27T15:29:14Z

mbed/mbed_cdc.py

+                        term.console.write(echo_text)
+        term.writer = input_handler
+
+        if print_term_header:


~~If you're referring to this by port, why not just pass it in?~~ Nope something else.

Not sure what you meant. Could you elaborate?

Please ignore, I was mistaken.

theotherjimmy · 2018-03-27T16:11:44Z

@screamerbg All that remains to be fixed is the two except:s

SenRamakri · 2018-03-27T16:44:04Z

Why is this only an option to detect/compile? Can we start it by itself? like, "mbed startserial [other options if multiple targets are detected]" OR "mbed sterm".
Also from a developer standpoint, people tend to keep separate terminal to run the builds. So it would be useful if I can do "mbed sterm" separately. And yes, as @sg- mentioned we can potentially expand on this to be used with Binary/ID-based tracing so that it automatically pick the right elf file to do the ID-string mapping.

theotherjimmy · 2018-03-27T16:57:21Z

@SenRamakri How is mbed sterm different from mbed detect -s? both have to detect attached targets before attaching the terminal.

SenRamakri · 2018-03-27T17:00:49Z

If "mbed detect -s" take options in case of multiple targets, then yes, we don't need "mbed sterm".

theotherjimmy · 2018-03-27T17:04:46Z

@SenRamakri What do you mean by "take options in case of multiple targets"? What options would it take? how do they apply to multiple targets?

SenRamakri · 2018-03-27T17:12:34Z

Offline discussion with @theotherjimmy here confirmed that "mbed detect" does pick one of many targets if required. So, "mbed detect -s" does support what I was looking for. Thanks for confirming, @theotherjimmy

yennster · 2018-03-29T04:29:37Z

So glad this is taking off @screamerbg ! I'll take a look at the README tomorrow 😄

yennster · 2018-03-29T05:09:08Z

@screamerbg Is there a way to change the baud rate for the serial monitor? Right now it looks like it defaults to 9600 (which is used for a lot of programs, but I vaguely remember needing to use 115200 for various applications, like Mbed Cloud apps)

jupe · 2018-03-30T07:39:52Z

Does this feature support terminal codes (ANSI/VT100) ?

screamerbg · 2018-03-31T12:51:16Z

@yennster You can change the baudrate form within the terminal using Ctrl+T + B combo. There's no commandline flag (yet)

screamerbg · 2018-03-31T12:56:58Z

@jupe The implementation invokes PySerial miniterm. This is from miniterm's documentation:

This is a console application that provides a small terminal application.

Miniterm itself does not implement any terminal features such as VT102 compatibility. However it may inherit these features from the terminal it is run. For example on GNU/Linux running from an xterm it will support the escape sequences of the xterm. On Windows the typical console window is dumb and does not support any escapes. When ANSI.sys is loaded it supports some escapes.

http://pyserial.readthedocs.io/en/latest/tools.html#module-serial.tools.miniterm

On Mac terminal it provides a fully featured ANSI support. Had a chance to try it with an ANSI embedded program @stevew817 wrote.

jupe · 2018-04-05T06:42:04Z

I tried this with mbed-client-cli which provides ANSI support. Couple comments / proposals:

could echo be disabled by default ?
could there be cli parameter for baudrate ?
When serial port dies it raises SerialException - could it be handled more carefully ?
If I press backspace help is printed unexpectedly -> cannot remove characters from cli interface

when calling mbed detect -s it prints following logs:

[mbed] WARNING: The mbed OS tools were not found in "/tmp". 
[mbed] WARNING: Limited information will be shown about connected mbed targets/boards

What this Limited information means in practice ?

CTRL+T CTRL+B causes IOError: [Errno 25] Inappropriate ioctl for device and application stuck (tried with mac)
This combination is not mentioned in help, but it's easy to do this mistake because CTRL+T b is used to change baudrate.

…dback

screamerbg · 2018-04-29T00:30:44Z

Follow up PR #664

screamerbg added 2 commits March 27, 2018 02:50

Add initial support for PySerial Miniterm

22e3ec7

Allow reset and sterm switches for 'mbed detect' even if mbed OS tool…

f8c2170

…s are present

screamerbg requested review from theotherjimmy, sg- and MarceloSalazar March 27, 2018 01:10

Fix help messages

653eb85

sg- reviewed Mar 27, 2018

View reviewed changes

screamerbg added 2 commits March 27, 2018 13:56

Move CDC code to separate python file

a5e0d75

Catch missing serial module (but don't depend on it)

c4e98ff

theotherjimmy reviewed Mar 27, 2018

View reviewed changes

Address comments in PR ARMmbed#650

e9d77ea

cmonr requested a review from SenRamakri March 27, 2018 16:35

yennster mentioned this pull request Apr 3, 2018

Added README info for Serial Terminal #652

Closed

Reimplement Serial Terminal feature and documentation based on PR fee…

5c65097

…dback

screamerbg closed this Apr 29, 2018

screamerbg added a commit to screamerbg/neo that referenced this pull request Apr 29, 2018

Address comments in PR ARMmbed#650

c4c4c01

screamerbg mentioned this pull request Apr 29, 2018

New feature: Serial Terminal (revised) #664

Merged

		@@ -0,0 +1,111 @@

		def mbed_cdc(port, reset=False, sterm=False, baudrate=9600, timeout= 10, print_term_header=True):

New feature: Serial Terminal #650

New feature: Serial Terminal #650

Conversation

screamerbg commented Mar 27, 2018 • edited Loading

sg- left a comment

Choose a reason for hiding this comment

theotherjimmy commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

screamerbg Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherjimmy Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherjimmy Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

theotherjimmy Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherjimmy commented Mar 27, 2018

SenRamakri commented Mar 27, 2018 • edited Loading

theotherjimmy commented Mar 27, 2018

SenRamakri commented Mar 27, 2018

theotherjimmy commented Mar 27, 2018

SenRamakri commented Mar 27, 2018

yennster commented Mar 29, 2018

yennster commented Mar 29, 2018 • edited Loading

jupe commented Mar 30, 2018

screamerbg commented Mar 31, 2018

screamerbg commented Mar 31, 2018

jupe commented Apr 5, 2018 • edited Loading

screamerbg commented Apr 29, 2018

screamerbg commented Mar 27, 2018 •

edited

Loading

screamerbg Mar 27, 2018 •

edited

Loading

theotherjimmy Mar 27, 2018 •

edited

Loading

theotherjimmy Mar 27, 2018 •

edited

Loading

theotherjimmy Mar 27, 2018 •

edited

Loading

SenRamakri commented Mar 27, 2018 •

edited

Loading

yennster commented Mar 29, 2018 •

edited

Loading

jupe commented Apr 5, 2018 •

edited

Loading