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

Add pretty bars for compile output #5929

Merged
merged 1 commit into from Jan 29, 2018

Conversation

Projects
None yet
7 participants
@geky
Member

geky commented Jan 25, 2018

Looks like this:

$ mbed compile -m ARCH_PRO -t GCC_ARM
Building project mbed-os-prettyoutput (ARCH_PRO, GCC_ARM)
Scan: .
Scan: env
Scan: mbed
Scan: FEATURE_LWIP
Text 70.5KB Data 2.72KB BSS 7.43KB                 ROM 73.2KB RAM 10.1KB
ROM [|||||||                                             ]  73.2KB/512KB
RAM [||||||||||||||||                                    ]   10.1KB/32KB
Image: BUILD/ARCH_PRO/GCC_ARM/mbed-os-prettyoutput.bin

This is an alternative to the pretty table output that provides useful information about memory consumption without using as much screen real-estate as the pretty table output. You may also note this matches what is displayed on the website, which is an added plus.

cc @theotherjimmy, @0xc0170

Returns: string containing nice looking bars
"""

# TODO add tty detection, and width detection probably

This comment has been minimized.

@kegilbert

kegilbert Jan 25, 2018

Contributor

Was this a TODO you wanted to address before merging or is it fine as is?

This comment has been minimized.

@geky

geky Jan 25, 2018

Member

Either or. You can see the current width detection below, which will only work on Linux (but doesn't hurt other OSs):

mbed-os/tools/memap.py

Lines 628 to 638 in a4bfd01

# TODO add tty detection, and width detection probably
WIDTH = 72
try:
# NOTE this only works on linux
import sys, fcntl, termios, struct
height, width, _, _ = struct.unpack('HHHH',
fcntl.ioctl(sys.stdout.fileno(), termios.TIOCGWINSZ,
struct.pack('HHHH', 0, 0, 0, 0)))
WIDTH = min(width, WIDTH)
except:
pass

I'm not planning on adding support for other OSs at this time, though I don't think that should stop this PR, they're just stuck at a 72 character width. I did search briefly, but unfortunately finding cross-platform screen width is a nightmare in python 2.7.

This comment has been minimized.

@kegilbert

kegilbert Jan 25, 2018

Contributor

Fair enough.

@kegilbert

This comment has been minimized.

Contributor

kegilbert commented Jan 25, 2018

I like it!

Sorry if I misundertood, but is there a compile flag to allow a user to specify which format they'd like (table versus graph)? Looks like the table print was removed completely.

@geky geky force-pushed the prettybars branch from a4bfd01 to a69a665 Jan 25, 2018

@geky

This comment has been minimized.

Member

geky commented Jan 25, 2018

Updated 👍

I repurposed --stats-depth to revert back to pretty table printing. So no --stats-depth = pretty bars.

@geky geky force-pushed the prettybars branch from a69a665 to 834e3d0 Jan 25, 2018

@drewcassidy

This comment has been minimized.

Contributor

drewcassidy commented Jan 25, 2018

this might be out of scope but there are unicode characters for doing progress bars at different subdivisions of one character, U+2588 █ through U+258F ▏

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 25, 2018

Looks neat 👍

This is using cmsis packs - what happens if device is not yet in the index ? From the code looks like it would fail? As I recall they are used for bootloader or exporters, with this change would be also for compilation. In case of exporter, if device is not yet in the packs, it fails for uvision to export. What is the case for compilation with this patch?

@geky

This comment has been minimized.

Member

geky commented Jan 25, 2018

@drewcassidy, Oh that's a good idea. Although it would look bad on non-unicode terminals. Do you know if there's a way to detect unicode support from a terminal? Do you think we can rely on $LANG?

@0xc0170, good point, I'll wrap all of this in a try catch based on cmsis-pack availability.

@geky geky force-pushed the prettybars branch from 834e3d0 to dc4d2bc Jan 25, 2018

@drewcassidy

This comment has been minimized.

Contributor

drewcassidy commented Jan 25, 2018

Do you think we can rely on $LANG?

$LANG appears to work, at least in iterm and terminal on macOS. You could only enable it if $LANG explicitly mentions UTF-8, and/or have a global flag to disable it like some tools have for color support. I was thinking of contributing to prettytable to add unicode block drawing support, so the flag could be used for configuring that as well

@geky

This comment has been minimized.

Member

geky commented Jan 25, 2018

@0xc0170, updated, @theotherjimmy suggested I can rely on the presence of device_name in the target object to indicate if cmsis-packs are available. CI can now verify that claim :)

Here's the output without a cmsis-pack, though I'm open to suggestions if we want to see something different:

$ mbed compile -m ARM_BEETLE_SOC -t GCC_ARM
Building project mbed-os-prettyoutput (ARM_BEETLE_SOC, GCC_ARM)
Scan: .                                                        
Scan: env                                                      
Scan: mbed                                                     
Scan: FEATURE_BLE                                              
Text 99KB Data 2.84KB BSS 13KB ROM 102KB RAM 15.8KB            
Image: BUILD/ARM_BEETLE_SOC/GCC_ARM/mbed-os-prettyoutput.bin   
@geky

This comment has been minimized.

Member

geky commented Jan 25, 2018

@drewcassidy, I think the unicode printing is a neat idea, but I don't think I'm going to be able to implement it at the moment. Feel free to add it in or include it when you add unicode support to the pretty tables.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 25, 2018

@geky Good pick of platform! that one probably won't ever have a CMSIS Pack. That output looks great! I'll review now.

@geky geky force-pushed the prettybars branch from dc4d2bc to b2a5ce1 Jan 25, 2018

@geky

This comment has been minimized.

Member

geky commented Jan 25, 2018

@theotherjimmy, I just chose the first platform that CI failed on. Speaking of CI failures, apparently the NUCLEO_F767ZI doesn't have any ram? Updated with another catch on cmsis-pack parsing errors.

memap_table = memap_instance.generate_output('table', stats_depth)

real_stats_depth = stats_depth if stats_depth is None else 2
memap_table = memap_instance.generate_output('table', real_stats_depth)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 25, 2018

Contributor

Could you hove these lines into the else statement below? that way we only generate what we need.

This comment has been minimized.

@geky

geky Jan 25, 2018

Member

memap_table is also used by the build report below, and the generate_output has side-effects I'm not sure about. You can see it was originally outside of the if not silent statement.

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 25, 2018

Contributor

memap_table is also used by the build report below

That's a good point.

generate_output has side-effects I'm not sure about

It does a compute_report followed by a reduce_depth, which will be run by the following generate_output calls anyway.

struct.pack('HHHH', 0, 0, 0, 0)))
WIDTH = min(width, WIDTH)
except:
pass

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 25, 2018

Contributor

Could you assign width to 72 here instead? and make that except not catch C-c and similar with

except Exception:
    WIDTH = 72

This comment has been minimized.

@geky

geky Jan 25, 2018

Member

WIDTH is also used on line 636 (right above). Will update the second part of your sentence.


text = sum(module['size']['.text']
for module in self.mem_report
if 'size' in module)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 25, 2018

Contributor

Could this be:

text = self.subtotal['.text']
if 'size' in module)
data = sum(module['size']['.data']
for module in self.mem_report
if 'size' in module)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 25, 2018

Contributor

Could this be:

data = self.subtotal['.data']
if 'size' in module)
bss = sum(module['size']['.bss']
for module in self.mem_report
if 'size' in module)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 25, 2018

Contributor

Could this be:

bss = self.subtotal['.bss']
bss = sum(module['size']['.bss']
for module in self.mem_report
if 'size' in module)
rom_used = text + data

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 25, 2018

Contributor

Could this be:

rom_used = self.mem_summary['total_flash']

This comment has been minimized.

@geky

geky Jan 25, 2018

Member

Why not just add them together? The math below would have problems if they differed

for module in self.mem_report
if 'size' in module)
rom_used = text + data
ram_used = data + bss

This comment has been minimized.

@theotherjimmy

theotherjimmy Jan 25, 2018

Contributor

Could this be:

ram_used = self.mem_summary['static_ram']
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jan 25, 2018

@geky Who knew? F767ZI has no RAM, huh.

Added pretty bar printing for compile output
Looks like this:

    Building project mbed-os-prettyoutput (ARCH_PRO, GCC_ARM)
    Scan: .
    Scan: env
    Scan: mbed
    Scan: FEATURE_LWIP
    Text 70.5KB Data 2.72KB BSS 7.43KB                 ROM 73.2KB RAM 10.1KB
    ROM [|||||||                                             ]  73.2KB/512KB
    RAM [||||||||||||||||                                    ]   10.1KB/32KB
    Image: BUILD/ARCH_PRO/GCC_ARM/mbed-os-prettyoutput.bin

If you build a target without a cmsis-pack it looks like this:

    Building project mbed-os-prettyoutput (ARM_BEETLE_SOC, GCC_ARM)
    Scan: .
    Scan: env
    Scan: mbed
    Scan: FEATURE_BLE
    Text 99KB Data 2.84KB BSS 13KB ROM 102KB RAM 15.8KB
    Image: BUILD/ARM_BEETLE_SOC/GCC_ARM/mbed-os-prettyoutput.bin

And the old behaviour of displaying the memap table can be brought back
by passing the --stats-depth parameter

@geky geky force-pushed the prettybars branch from b2a5ce1 to 471d99c Jan 25, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Jan 26, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 26, 2018

Build : SUCCESS

Build number : 979
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5929/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

uVisor tests are experiencing an anomaly. @0xc0170 has ping'd the team to determine how to resolve.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 29, 2018

/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jan 29, 2018

@cmonr cmonr merged commit 702eaff into master Jan 29, 2018

20 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 29, 2018

@geky geky deleted the prettybars branch Feb 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment