config: Support MBED_ROM_START and friends #270
Conversation
Codecov Report
@@ Coverage Diff @@
## master #270 +/- ##
==========================================
+ Coverage 97.07% 97.12% +0.04%
==========================================
Files 92 92
Lines 2769 2813 +44
==========================================
+ Hits 2688 2732 +44
Misses 81 81
|
|
Hi
Config files are targets/custom and mbed_app json files ? |
|
|
||
| if size is not None and start is not None: | ||
| logger.debug(f"Extracting MBED_{mem.upper()} definitions in {namespace}: _START={start}, _SIZE={size}.") | ||
|
|
There was a problem hiding this comment.
The line spacing here seems a little off. We should probably remove this blank line.
dd7c062
to
ca4eb88
Compare
There was a problem hiding this comment.
Config files are targets/custom and mbed_app json files ?
Not tools/arm_pack_manager/index.json, right ?
This is the question raised by @jeromecoutant above.
This change only works if a target's mbed_ram_start and friends are directly defined in targets.json, and there are only 15 such targets.
But the vast majority of targets (116 of them, incl. NUMAKER_IOT_M487 mentioned in #222 which this PR is created for) only have a device_name in targets.json, and the actual memory regions are defined in tools/arm_pack_manager/index.json (a huge file of 517706 lines) and looked up based on device_name. The old tool does such look up, to support all targets. This huge file was imported from CMSIS according to ARMmbed/mbed-os@167ed2b and modified whenever a new target was added.
@Patater In the long run what should we do? Keeping and using that huge file, or trying to migrate a small amount of useful info into targets.json and removing it?
Before such refactoring happens (or if it won't happen), we might need to parse tools/arm_pack_manager/index.json too.
|
Another issue with |
I would suggest the best course of action is to migrate the necessary information to targets.json, and ignore pack_manager/index.json, as it seems to be out of date or incorrect in a number of cases. If the memory regions aren't configurable for certain targets, because they're hardcoded in the linkerscripts, then we should probably not define the properties in targets.json for those targets. That would mean the tool would at least emit a warning if a user attempts to override the memory region for a target where that isn't possible, which may reduce the potential other "confusions" later on. |
Agreed.
Most targets' linker scripts have hardcoded values, but they can be overridden with macros (example). The old mbed-cli sets memory region macros from either At minimum we need to migrate memory regions from |
|
I've raised #271 for deciding what to do about What do we need to move this PR forward, given we are limiting its scope to only memory map information defined in |
Thanks. I will approve this PR as it's sufficient for the current scope. Maybe we can't close #222, because the target mentioned there is one that depends on #271? |
There was a problem hiding this comment.
LGTM. There's still a few comments from @rwalton-arm to be addressed?
Memory regions may be defined in config sources with MBED_ROM_START and MBED_ROM_SIZE or MBED_RAM_START and MBED_RAM_SIZE. These were not extracted by the config system, and so were ignored when generating a config in mbed-tools. These attributes are extracted from config files, adding defined memory regions to the Config. Both SIZE and START are required together to add a region, and repeated definition of a memory region is ignored.
ca4eb88
to
039f372
Compare
Description
MBED_ROM_START/SIZE and MBED_RAM_START/SIZE may be included in config files, to define active memory regions. However, these are currently ignored in the config system, and so are not passed to the compiler.
These attributes are now extracted from the config files, and the definitions are added to
mbed_config.cmake. Both SIZE and START are required together to define a region, and repeated definition of a memory region is ignored.Fixes #222
Test Coverage