Enhance memap, and configure depth level #4392
Conversation
|
Looking good. A few questions:
|
Sure, the depth number can be passed to memap - see here: I'd need a bit of help to be able to pass a parameter from
I agree. Commit coming soon. |
|
@MarceloSalazar Thanks for the PR. Also, we recommend PRs follow Chris Beam’s seven rules of great commit messages. This helps us with the release notes. To match this format, please change the subject line to the imperative mood by changing it to "Enhance memap, and configure depth level" Thanks so much for your help. |
MarceloSalazar
changed the title
|
Parameter added to set depth from mbed compile. Example |
| @@ -439,7 +439,7 @@ def build_project(src_paths, build_path, target, toolchain_name, | |||
| macros=None, inc_dirs=None, jobs=1, silent=False, | |||
| report=None, properties=None, project_id=None, | |||
| project_description=None, extra_verbose=False, config=None, | |||
| app_config=None, build_profile=None): | |||
| app_config=None, build_profile=None, stats_depth=None): | |||
There was a problem hiding this comment.
Oooph. I'm not a fan of these 15+ argument functions.
There was a problem hiding this comment.
Not your fault @MarceloSalazar. Not going to block this PR.
There was a problem hiding this comment.
I have to revisit all of "build_api.py". I think that most of these arguments are passed on to constructors for the various objects that make up a project in the tools. We could move where we construct the objects elsewhere and pass them into the build_project function. That would reduce the number of arguments considerably.
That's also why I said that this was not blocking this PR. I would not expect to refactor "build_api.py" in a memap PR.
There was a problem hiding this comment.
I don't think there is an alternative that does not involve changing the way that the functions in "build_api.py" take parameters. That alternative is going to be a much bigger change than would be appropriate for this PR.
tools/make.py
Outdated
| @@ -103,6 +103,9 @@ | |||
| default=None, | |||
| help='For some commands you can use filter to filter out results') | |||
|
|
|||
| parser.add_argument("--stats-depth", type=int, dest="stats_depth", | |||
| default=2, help="Depth level for static memory report") | |||
There was a problem hiding this comment.
This indentation looks incorrect. Maybe it's a tabs vs spaces issue?
tools/memap.py
Outdated
| RE_ARMCC = re.compile( | ||
| r'^\s+0x(\w{8})\s+0x(\w{8})\s+(\w+)\s+(\w+)\s+(\d+)\s+[*]?.+\s+(.+)$') | ||
| RE_IAR = re.compile( | ||
| r'^\s+(.+)\s+(zero|const|ro code|inited|uninit)\s' | ||
| r'+0x(\w{8})\s+0x(\w+)\s+(.+)\s.+$') | ||
|
|
||
| # Pending: | ||
| # 1. Add tests | ||
| # 2. Update markdown documentation |
There was a problem hiding this comment.
Is this in the PR description? if not, let's add it.
There was a problem hiding this comment.
Looks like it's in the PR description! Good job on that one!
tools/memap.py
Outdated
| class MemapParser(object): | ||
| """An object that represents parsed results, parses the memory map files, | ||
| and writes out different file types of memory results | ||
| """ | ||
|
|
||
| print_sections = ('.text', '.data', '.bss') | ||
|
|
||
| # Info from this section can be shown as misc or startup code (.text) |
There was a problem hiding this comment.
Is this comment really needed? I think that the variable is named well enough to get the point across.
This will not block the PR
tools/memap.py
Outdated
|
|
||
| # this is required to differenciate: main.o vs xxxmain.o | ||
| key_split = key.split('/')[-1] | ||
| obj_split = object_name.split('/')[-1] |
There was a problem hiding this comment.
Should the preceding two lines use os.path.basename instead of splitting things manually?
There was a problem hiding this comment.
Yes they should. I need to be more direct. Please change the implementation to use os.path.basename or explain why the implementation must avoid using that function.
tools/memap.py
Outdated
| """ | ||
|
|
||
| # Check if object is a sub-string of key | ||
| for key, value in self.modules.items(): |
There was a problem hiding this comment.
Where is value used it this loop? I could not find it.
If value is unused, just iterate through self.modules
Further, the variable name key adds no new information about the structure of the data being processed; by definition of iterating through the dict, key must contain keys. It looks like this is the module's file name. so fname or file_name or module_path would all give me an indication that we are processing file paths in this loop.
tools/memap.py
Outdated
|
|
||
| """ | ||
|
|
||
| rex_address_line = re.compile(r'^\s+(.+\.o)\s.*') |
There was a problem hiding this comment.
This comment is the same as the other Regular expression related ones:
renotrexis common in python- Please make this a Class or global constant
tools/memap.py
Outdated
| """ | ||
|
|
||
| path = path.replace('\\', '/') | ||
|
|
||
| # check location of map file | ||
| rex = r'^(.+)' + r'\/(.+\.map)$' | ||
| #rex = r'^(.+)' + r'\/(.+\.map)$' | ||
| rex = r'^(.+)\/(.+\.map)$' |
There was a problem hiding this comment.
This comment is the same as the other Regular expression related ones:
renotrexis common in python- Please make this a Class or global constant
tools/memap.py
Outdated
| """ | ||
|
|
||
| path = path.replace('\\', '/') | ||
|
|
||
| # check location of map file | ||
| rex = r'^(.+)' + r'\/(.+\.map)$' | ||
| #rex = r'^(.+)' + r'\/(.+\.map)$' |
There was a problem hiding this comment.
Can we delete old, unused code?
tools/memap.py
Outdated
|
|
||
| for section_idx in self.all_sections: | ||
| self.short_modules[temp][section_idx] += \ | ||
| self.modules[line][section_idx] |
There was a problem hiding this comment.
Can you indent this line so that, when I read this file again in 2 months, I know that it's a continuation of the prior line?
tools/memap.py
Outdated
|
|
||
| Returns: generated string for the 'table' format, otherwise None | ||
| """ | ||
|
|
||
| self.module_remove_unused() # clean up unused modules/objects |
There was a problem hiding this comment.
I think this method is named well enough that we don't need this comment.
Not blocking this PR.
tools/make.py
Outdated
| type=int, | ||
| dest="stats_depth", | ||
| default=2, | ||
| help="Depth level for static memory report") |
There was a problem hiding this comment.
This, and the 3 prior lines, still look like they do not line up with the character after the (, which is the style we have throughout the tools.
|
Oh yeah, and write those tests! Can't forget the tests! |
|
I'm not sure if this is a user error, but with using mbed test to compile ( |
|
bumping travis. |
You are right, I'm able to reproduce this. I'll investigate further. |
|
@geky memap for mbed tests has been fixed. Could you have a look? |
|
@MarceloSalazar Thanks for the fix. Confirmed that |
|
@MarceloSalazar, looks good now, thanks for the fix! LGTM |
|
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
|
@MarceloSalazar I'm going to open a new PR with your branch to trigger Travis. |
|
Travis Triggered. |
|
@tommikas It looks like your CI forgot to report, so I retriggered it. |
Again not reported. Tommi, can you please? I am not seein there build now command |
|
Looks to me like Jenkins missed the latest reopening notification so it currently thinks the PR is closed. I'm not sure how to best fix it. @miklis, could you please have a look? |
|
@miklis noticed that the jenkins/pr-head Details link actually pointed to the wrong Jenkins job - the one for #4887. Restarting the build from the correct job seems to have fixed the issue so the update should come to the right place. Creating two PRs from the same branch probably confused Jenkins/github a bit. |
|
Hi |
|
There should be in Travis. |
mbed OS 5.5.6 release We are pleased to announce the [mbed OS 5.5.6 release](https://github.com/ARMmbed/mbed-os/releases/tag/mbed-os-5.5.6) is now available. This release includes ... Known Issues The following list of known issues apply to this release: Contents Ports for Upcoming Targets [4608](ARMmbed#4608) Support Nuvoton's new target NUMAKER_PFM_M487 [4840](ARMmbed#4840) Add Support for TOSHIBA TMPM066 board Fixes and Changes [4801](ARMmbed#4801) STM32 CAN: Fix issue with speed function calculation [4808](ARMmbed#4808) Make HAL & US tickers idle safe [4812](ARMmbed#4812) Use DSPI SDK driver API's in SPI HAL driver [4832](ARMmbed#4832) NUC472/M453: Fix several startup and hal bugs [4842](ARMmbed#4842) Add call to DAC_Enable as this is no longer done as part [4849](ARMmbed#4849) Allow using of malloc() for reserving the Nanostack's heap. [4850](ARMmbed#4850) Add list of defines to vscode exporter [4863](ARMmbed#4863) Optimize memory usage of wifi scan for REALTEK_RTW8195AM [4869](ARMmbed#4869) HAL LPCs SPI: Fix mask bits for SPI clock rate [4873](ARMmbed#4873) Fix Cortex-A cache file [4878](ARMmbed#4878) STM32 : Separate internal ADC channels with new pinmap [4392](ARMmbed#4392) Enhance memap, and configure depth level [4895](ARMmbed#4895) Turn on doxygen for DEVICE_* features [4817](ARMmbed#4817) Move RTX error handlers into RTX handler file [4902](ARMmbed#4902) Using CMSIS/RTX Exclusive access macro [4923](ARMmbed#4923) fix export static_files to zip [4844](ARMmbed#4844) bd: Add ProfilingBlockDevice for measuring higher-level applications [4896](ARMmbed#4896) target BLUEPILL_F103C8 compile fix [4921](ARMmbed#4921) Update gcc-arm-embedded PPA in Travis [4926](ARMmbed#4926) STM32L053x8: Refactor NUCLEO_L053R8 and DISCO_L053C8 targets [4831](ARMmbed#4831) Remove excessive use of printf/scanf in mbed_fdopen/_open [4922](ARMmbed#4922) bug fix: xdot clock config [4935](ARMmbed#4935) STM32: fix F410RB vectors size [4940](ARMmbed#4940) Update mbed-coap to version 4.0.9 [4941](ARMmbed#4941) Update of MemoryPool.h header file. You can fetch this release from the [mbed-os GitHub](https://github.com/ARMmbed/mbed-os) repository, using the tag "mbed-os-5.5.6". Please feel free to ask any questions or provide feedback on this release [on the forum](https://forums.mbed.com/), or to contact us at [[email protected]](mailto:[email protected]).
Generates a detailed report by analysing static memory information from the map file.
The information from the map file is combined with the information from the BUILD directory (compiled objects and path to these objects), including the main application and user libraries.
In addition, it's now possible to visualise compiler libraries and identify other miscellaneous objects introduced by the toolchain.
By default, memap generates a report with depth=2, but it's possible to specify a different depth level, and even to enable a full report (depth=0).
It has been tested with the three major toolchains: GCC, ARM and IAR compiler.
Status
READY
Migrations
NO
Todos
@sg- @theotherjimmy @screamerbg please review
Usage
Examples