config: Support MBED_ROM_START and friends #270

wernerlewis · 2021-04-13T11:28:54Z

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

This change is covered by existing or additional automated tests.
Manual testing has been performed (and evidence provided) as automated testing was not feasible.
Additional tests are not required for this change (e.g. documentation update).

codecov · 2021-04-13T11:35:48Z

Codecov Report

Merging #270 (039f372) into master (73fc6ed) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            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

Impacted Files	Coverage Δ
src/mbed_tools/build/_internal/config/config.py	`100.00% <100.00%> (ø)`
src/mbed_tools/build/_internal/config/source.py	`100.00% <100.00%> (ø)`

jeromecoutant · 2021-04-13T12:15:25Z

Hi

These attributes are now extracted from the config files

Config files are targets/custom and mbed_app json files ?
Not tools/arm_pack_manager/index.json, right ?

tests/build/_internal/config/test_config.py

rwalton-arm · 2021-04-14T08:26:17Z

src/mbed_tools/build/_internal/config/source.py

+
+ if size is not None and start is not None:
+ logger.debug(f"Extracting MBED_{mem.upper()} definitions in {namespace}: _START={start}, _SIZE={size}.")
+


The line spacing here seems a little off. We should probably remove this blank line.

src/mbed_tools/build/_internal/config/source.py

src/mbed_tools/build/_internal/config/config.py

src/mbed_tools/build/_internal/config/source.py

src/mbed_tools/build/_internal/config/config.py

LDong-Arm

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.

LDong-Arm · 2021-04-16T15:22:52Z

Another issue with tools/arm_pack_manager/index.json is, despite having memory regions specified, the memory addresses and sizes are also hardcoded in each target's linker script (.sct or .ld) in most cases. Confusions arise if they disagree. Not to mention some targets (e.g. ARM_MUSCA_S1) only have memory regions in linker scripts, not in tools/arm_pack_manager/index.json or targets.json.

rwalton-arm · 2021-04-21T11:51:01Z

Another issue with tools/arm_pack_manager/index.json is, despite having memory regions specified, the memory addresses and sizes are also hardcoded in each target's linker script (.sct or .ld) in most cases. Confusions arise if they disagree. Not to mention some targets (e.g. ARM_MUSCA_S1) only have memory regions in linker scripts, not in tools/arm_pack_manager/index.json or targets.json.

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.

LDong-Arm · 2021-04-21T12:58:57Z

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.

Agreed.

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.

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 targets.json or tools/arm_pack_manager/index.json so the hardcoded values are usually not used (even though the macro values are mostly identical to ones set by mbed-cli). Any clean-up/alignment would be a large amount of work on its own.

At minimum we need to migrate memory regions from tools/arm_pack_manager/index.json to targets.json as suggested above, I guess. Hopefully it can be scripted.

Patater · 2021-04-21T16:33:12Z

I've raised #271 for deciding what to do about tools/arm_pack_manager/index.json. Please continue discussion there.

What do we need to move this PR forward, given we are limiting its scope to only memory map information defined in targets.json?

LDong-Arm · 2021-04-21T16:39:26Z

I've raised #271 for deciding what to do about tools/arm_pack_manager/index.json. Please continue discussion there.

What do we need to move this PR forward, given we are limiting its scope to only memory map information defined in targets.json?

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?

LDong-Arm

LGTM. There's still a few comments from @rwalton-arm to be addressed?

rwalton-arm

LGTM. We need to rebase then it should be good to merge.

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.

Patater approved these changes Apr 13, 2021

View reviewed changes

rwalton-arm requested changes Apr 14, 2021

View reviewed changes

wernerlewis force-pushed the support-mbed-rom-start branch from dd7c062 to ca4eb88 Compare April 14, 2021 11:29

Patater requested a review from rwalton-arm April 15, 2021 13:31

rwalton-arm reviewed Apr 16, 2021

View reviewed changes

src/mbed_tools/build/_internal/config/config.py Show resolved Hide resolved

LDong-Arm requested changes Apr 16, 2021

View reviewed changes

Patater mentioned this pull request Apr 21, 2021

Migrate memory regions from tools/arm_pack_manager/index.json to targets.json #271

Open

LDong-Arm approved these changes Apr 21, 2021

View reviewed changes

rwalton-arm approved these changes Apr 22, 2021

View reviewed changes

wernerlewis added 2 commits April 22, 2021 09:09

config: Add memory regions to mbed_config.tmpl

039f372

wernerlewis force-pushed the support-mbed-rom-start branch from ca4eb88 to 039f372 Compare April 22, 2021 08:12

Patater merged commit 1bb2cce into ARMmbed:master Apr 22, 2021

0xc0170 mentioned this pull request May 3, 2021

Bootloader overrides from config are not processed #156

Open

2 tasks

jeromecoutant mentioned this pull request May 21, 2021

mbed_rom_size values from mbed_app.json are ignored #283

Closed

jeromecoutant mentioned this pull request Jun 1, 2021

STM32WB: improve FLASH size ARMmbed/mbed-os#14692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: Support MBED_ROM_START and friends #270

config: Support MBED_ROM_START and friends #270

wernerlewis commented Apr 13, 2021

codecov bot commented Apr 13, 2021 •

edited

Loading

jeromecoutant commented Apr 13, 2021

rwalton-arm Apr 14, 2021

LDong-Arm left a comment •

edited

Loading

LDong-Arm commented Apr 16, 2021

rwalton-arm commented Apr 21, 2021

LDong-Arm commented Apr 21, 2021 •

edited

Loading

Patater commented Apr 21, 2021

LDong-Arm commented Apr 21, 2021

LDong-Arm left a comment

rwalton-arm left a comment


		if size is not None and start is not None:
		logger.debug(f"Extracting MBED_{mem.upper()} definitions in {namespace}: _START={start}, _SIZE={size}.")

config: Support MBED_ROM_START and friends #270

config: Support MBED_ROM_START and friends #270

Conversation

wernerlewis commented Apr 13, 2021

Description

Test Coverage

codecov bot commented Apr 13, 2021 • edited Loading

Codecov Report

jeromecoutant commented Apr 13, 2021

rwalton-arm Apr 14, 2021

Choose a reason for hiding this comment

LDong-Arm left a comment • edited Loading

Choose a reason for hiding this comment

LDong-Arm commented Apr 16, 2021

rwalton-arm commented Apr 21, 2021

LDong-Arm commented Apr 21, 2021 • edited Loading

Patater commented Apr 21, 2021

LDong-Arm commented Apr 21, 2021

LDong-Arm left a comment

Choose a reason for hiding this comment

rwalton-arm left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 13, 2021 •

edited

Loading

LDong-Arm left a comment •

edited

Loading

LDong-Arm commented Apr 21, 2021 •

edited

Loading