New Feature: Flash Target Board #466

screamerbg · 2017-03-30T00:27:15Z

Added feature support for -f/--flash to mbed compile to flash (and reset) connected target board after successful compile.

The implementation uses greentea/htrun framework and therefore relies on the same routines as testing.

To use:

$ mbed compile -t <toolchain> -m <mcu|DETECT> -f|--flash

Example output:

Total Static RAM memory (data + bss): 22732 bytes
Total RAM memory (data + bss + heap + stack): 22732 bytes
Total Flash memory (text + data + misc): 30266 bytes

Image: .\BUILD\NUCLEO_F429ZI\ARM\mbed-os-example-blinky.bin
[mbed] Detected "NUCLEO_F429ZI" connected to "E:" and using com port "COM6"
        1 file(s) copied.

…reset) the target after successful compile

theotherjimmy

A few nits.

theotherjimmy · 2017-03-30T00:54:42Z

mbed/mbed.py

+        elif len(targets) > 1:
+            error("Multiple targets were detected.\nOnly 1 target board should be connected to your system.")
+        elif len(targets) == 0:
+            error("No targets were detected.\nPlease make sure a target board is connected to this system.")


On linux, it's not enough to connect the target board to the system. You must also mount the fake file system of the board somewhere. Should we mention this in the error message?

theotherjimmy · 2017-03-30T00:56:34Z

mbed/mbed.py

@@ -2221,6 +2231,23 @@ def compile_(toolchain=None, target=None, profile=False, compile_library=False,
                  + (['-v'] if verbose else [])
                  + args,
                  env=env)
+
+            if flash:


Before trying to flash, should you also check that the return code of the compile job indicates success?

theotherjimmy · 2017-03-30T00:57:05Z

mbed/mbed.py

+            if flash:
+                fw_name = artifact_name if artifact_name else program.name
+                fw_fbase = os.path.join(build_path, fw_name)
+                fw_file = fw_fbase + ('.hex' if os.path.exists(fw_fbase+'.hex') else '.bin')


You only check that the .hex file exists. I think you should check for .bin as well.

theotherjimmy

I don't think that any of my aforementioned nits should block this PR.

screamerbg · 2017-03-30T08:53:30Z

@theotherjimmy Thanks for your comments. All of them make sense (and now addressed) 👍

bridadan

LGTM. I mentioned a few optional usability notes below.

bridadan · 2017-03-30T14:24:16Z

mbed/mbed.py

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


This is really part of the mbed-host-tests module, not mbed-greentea. mbed-greentea will correctly bring in this dependency though. So it's ok to leave this as is I think, but just thought I'd mention it.

The developer never installs mbed-host-tests manually, neither is familiar with this module. Generally we would like to point developer at installing mbed-greentea as foundation of mbed OS testing, not specifically mbed-host-tests.

Makes sense to me!

bridadan · 2017-03-30T14:25:16Z

mbed/mbed.py

+                    error("The '-f/--flash' option requires that the 'mbed-greentea' python module is installed.\nYou can install mbed-ls by running 'pip install mbed-greentea'.", 1)
+
+                if not flash_dev(detected['msd'], fw_file, program_cycle_s=2):
+                    error("Unable to flash the target board connected to your system.", 1)


You might also prompt the user to check for a FAIL.TXT on the board for more information. Not necessary though.

Why not have that check that in htrun instead, and have the top flash_dev() function handle it?

Sure, also a possibility. Like I said, not necessary and should't block this PR.

bridadan · 2017-03-30T14:32:07Z

mbed/mbed.py

@@ -1447,6 +1440,22 @@ def ignore_build_dir(self):
            except IOError:
                error("Unable to write build ignore file in \"%s\"" % os.path.join(build_path, '.mbedignore'), 1)

+    def detect_target(self, info=None):


There is a valid case where you have multiple platforms plugged in, but all of them are different. This could be supported by supplying an optional target name to this function. Shouldn't be too hard to implement:

def detect_target(self, target_name=None, info=None): targets = self.get_detected_targets() if target_name: targets = [t for t in targets if t['name'].upper() == target_name.upper()] ...

(^Note that actual code is totally untested, but the idea should be sound)

But this can always be added in another PR. @screamerbg totally up to you on the priority of this.

What's the use case for detecting multiple targets? Also what if I have multiple targets of the same kind mixed with different ones?

I guess I'm missing the point.

Say I have a LPC1768, a K64F, and an NRF51_DK plugged into my machine. I could run mbed compile -m NRF51_DK ... --flash and mbed CLI could correctly figure out which board to flash. Can be useful when developing for multiple platforms, but maybe not important as of right now.

Also what if I have multiple targets of the same kind mixed with different ones?

In this case it would have to error like you do above since there are multiple boards (for example, 2 LPC1768s plugged in. You don't know which one to flash just by using the target name).

I'm afraid I'm hindering this PR more than I'm helping it with this comment. I'm happy to submit the change I mentioned above as a PR after this is merged for more discussion.

…ly caused by an old interface firmware

bridadan · 2017-03-31T15:13:57Z

mbed/mbed.py

@@ -1447,6 +1440,22 @@ def ignore_build_dir(self):
            except IOError:
                error("Unable to write build ignore file in \"%s\"" % os.path.join(build_path, '.mbedignore'), 1)

+    def detect_target(self, info=None):


Say I have a LPC1768, a K64F, and an NRF51_DK plugged into my machine. I could run mbed compile -m NRF51_DK ... --flash and mbed CLI could correctly figure out which board to flash. Can be useful when developing for multiple platforms, but maybe not important as of right now.

Also what if I have multiple targets of the same kind mixed with different ones?

In this case it would have to error like you do above since there are multiple boards (for example, 2 LPC1768s plugged in. You don't know which one to flash just by using the target name).

I'm afraid I'm hindering this PR more than I'm helping it with this comment. I'm happy to submit the change I mentioned above as a PR after this is merged for more discussion.

Added feature support for -f/--flash to mbed compile to flash (and …

9630304

…reset) the target after successful compile

screamerbg requested review from sg-, theotherjimmy and bridadan March 30, 2017 00:27

screamerbg changed the title ~~New Feature: Flash Target~~ New Feature: Flash Target Board Mar 30, 2017

screamerbg added 2 commits March 30, 2017 01:38

Improve error message for when mbed-greentea is not installed

5610e35

Add documentation for mbed compile -f|--flash

6763c85

theotherjimmy reviewed Mar 30, 2017

View reviewed changes

theotherjimmy approved these changes Mar 30, 2017

View reviewed changes

Fixed handling errors regarding target detection and flashing

2f0ac78

screamerbg force-pushed the f/flasher branch from 2a54f45 to 2f0ac78 Compare March 30, 2017 08:58

bridadan approved these changes Mar 30, 2017

View reviewed changes

screamerbg added the enhancement label Mar 30, 2017

Improve error message for when failing to reset target. This is usual…

558a20e

…ly caused by an old interface firmware

bridadan approved these changes Mar 31, 2017

View reviewed changes

screamerbg added the in progress label Apr 4, 2017

screamerbg merged commit ae86dc3 into ARMmbed:master Apr 13, 2017

New Feature: Flash Target Board #466

New Feature: Flash Target Board #466

Uh oh!

Conversation

screamerbg commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

screamerbg commented Mar 30, 2017

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

screamerbg Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

screamerbg Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

screamerbg commented Mar 30, 2017 •

edited

Loading

screamerbg Mar 30, 2017 •

edited

Loading

screamerbg Mar 30, 2017 •

edited

Loading