Repository: ARMmbed/mbed-tools

Issue:
MBED_ROM_START and friends unavailable on Mbed CLI2
### Description

<!--
A detailed description of what is being reported. Please include steps to reproduce the problem.

Things to consider sharing:
- What version of the package is being used (pip show mbed-tools)?
- What is the host platform and version (e.g. macOS 10.15.2, Windows 10, Ubuntu 18.04 LTS)?
-->

On Mbed CLI, the following symbols are generated and passed to compiler, linker, or both:

```sh
mbed compile -m NUMAKER_IOT_M487 -t ARM
```

**BUILD/NUMAKER_IOT_M487/ARM/.profile.c**:
```
{
    "flags": [
        ......
        "-DMBED_RAM_SIZE=0x28000",
        "-DMBED_RAM_START=0x20000000",
        "-DMBED_ROM_SIZE=0x80000",
        "-DMBED_ROM_START=0x0",
        ......
```

**BUILD/NUMAKER_IOT_M487/ARM/.profile.ld**:
```
{
    "flags": [
        ......
        "--predefine=\"-DMBED_BOOT_STACK_SIZE=1024\"",
        "--predefine=\"-DMBED_RAM_SIZE=0x28000\"",
        "--predefine=\"-DMBED_RAM_START=0x20000000\"",
        "--predefine=\"-DMBED_ROM_SIZE=0x80000\"",
        "--predefine=\"-DMBED_ROM_START=0x0\"",
        ......
```

But on Mbed CLI2, they are unavailable in `cmake_build/NUMAKER_IOT_M487/develop/ARM/mbed_config.cmake` or elsewhere.

```sh
mbed-tools compile -m NUMAKER_IOT_M487 -t ARM
```

### Issue request type

<!--
Please add only one `x` to one of the following types. Do not fill multiple types (split the issue otherwise).


For questions please use https://forums.mbed.com/
-->

- [ ] Enhancement
- [x] Bug

### Mbed/Tool version

**mbed-os**: 6.8.0
**mbed-cli**: 1.10.5
**mbed-tools**:: 7.2.1


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/ARMmbed/mbed-tools/pull/270?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ARMmbed) Report
> Merging [#270](https://codecov.io/gh/ARMmbed/mbed-tools/pull/270?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ARMmbed) (dd7c062) into [master](https://codecov.io/gh/ARMmbed/mbed-tools/commit/3dbc738b4f46c9b170922f9299b1650dc3ad5b9d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ARMmbed) (3dbc738) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/ARMmbed/mbed-tools/pull/270/graphs/tree.svg?width=650&height=150&src=pr&token=QFRIdRv91G&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ARMmbed)](https://codecov.io/gh/ARMmbed/mbed-tools/pull/270?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ARMmbed)

```diff
@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
+ Coverage   97.07%   97.12%   +0.04%     
==========================================
  Files          92       92              
  Lines        2769     2814      +45     
==========================================
+ Hits         2688     2733      +45     
  Misses         81       81              
```


| [Impacted Files](https://codecov.io/gh/ARMmbed/mbed-tools/pull/270?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ARMmbed) | Coverage Δ | |
|---|---|---|
| [src/mbed\_tools/build/\_internal/config/config.py](https://codecov.io/gh/ARMmbed/mbed-tools/pull/270/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ARMmbed#diff-c3JjL21iZWRfdG9vbHMvYnVpbGQvX2ludGVybmFsL2NvbmZpZy9jb25maWcucHk=) | `100.00% <100.00%> (ø)` | |
| [src/mbed\_tools/build/\_internal/config/source.py](https://codecov.io/gh/ARMmbed/mbed-tools/pull/270/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ARMmbed#diff-c3JjL21iZWRfdG9vbHMvYnVpbGQvX2ludGVybmFsL2NvbmZpZy9zb3VyY2UucHk=) | `100.00% <100.00%> (ø)` | |

jeromecoutant: 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 ?

LDong-Arm: 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: > 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: > 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](https://github.com/ARMmbed/mbed-os/blob/9738b27c7df897c29e9769911d6794ba3e5b3f19/targets/TARGET_ARM_SSG/TARGET_MUSCA_S1/device/TOOLCHAIN_ARMC6/musca_ns.sct#L24-L38)). The old mbed-cli sets memory region macros from either `targets.json` or `tools/arm_pack_manager/index.json` anyway. 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.
Patater: I've raised https://github.com/ARMmbed/mbed-tools/issues/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: > 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?

Patch:
diff --git a/news/222.bugfix b/news/222.bugfix
new file mode 100644
index 0000000..4bb5df6
--- /dev/null
+++ b/news/222.bugfix
@@ -0,0 +1,1 @@
+Add support for MBED_ROM_START, MBED_ROM_SIZE, MBED_RAM_START and MBED_RAM_SIZE in config system.
diff --git a/src/mbed_tools/build/_internal/config/config.py b/src/mbed_tools/build/_internal/config/config.py
index d93cfe4..bb493f2 100644
--- a/src/mbed_tools/build/_internal/config/config.py
+++ b/src/mbed_tools/build/_internal/config/config.py
@@ -8,7 +8,7 @@ import logging
 from collections import UserDict
 from typing import Any, Iterable, Hashable, Callable, List
 
-from mbed_tools.build._internal.config.source import Override, ConfigSetting
+from mbed_tools.build._internal.config.source import Memory, Override, ConfigSetting
 
 logger = logging.getLogger(__name__)
 
@@ -18,13 +18,15 @@ class Config(UserDict):
 
     This object understands how to populate the different 'config sections' which all have different rules for how the
     settings are collected.
-    Applies overrides, appends macros and updates config settings.
+    Applies overrides, appends macros, updates memories, and updates config settings.
     """
 
     def __setitem__(self, key: Hashable, item: Any) -> None:
         """Set an item based on its key."""
         if key == CONFIG_SECTION:
             self._update_config_section(item)
+        elif key == MEMORIES_SECTION:
+            self._update_memories_section(item)
         elif key == OVERRIDES_SECTION:
             self._handle_overrides(item)
         elif key == MACROS_SECTION:
@@ -67,6 +69,20 @@ class Config(UserDict):
 
         self.data[CONFIG_SECTION] = self.data.get(CONFIG_SECTION, []) + config_settings
 
+    def _update_memories_section(self, memories: List[Memory]) -> None:
+        defined_memories = self.data.get(MEMORIES_SECTION, [])
+        for memory in memories:
+            logger.debug(f"Adding memory settings `{memory.name}: start={memory.start} size={memory.size}`")
+            prev_defined = next((mem for mem in defined_memories if mem.name == memory.name), None)
+            if prev_defined is None:
+                defined_memories.append(memory)
+            else:
+                logger.warning(
+                    f"You are attempting to redefine `{memory.name}` from {prev_defined.namespace}.\n"
+                    f"The values from `{memory.namespace}` will be ignored"
+                )
+        self.data[MEMORIES_SECTION] = defined_memories
+
     def _find_first_config_setting(self, predicate: Callable) -> Any:
         """Find first config setting based on `predicate`.
 
@@ -89,6 +105,7 @@ class Config(UserDict):
 
 CONFIG_SECTION = "config"
 MACROS_SECTION = "macros"
+MEMORIES_SECTION = "memories"
 OVERRIDES_SECTION = "overrides"
 
 
diff --git a/src/mbed_tools/build/_internal/config/source.py b/src/mbed_tools/build/_internal/config/source.py
index 4ad7e37..59d01df 100644
--- a/src/mbed_tools/build/_internal/config/source.py
+++ b/src/mbed_tools/build/_internal/config/source.py
@@ -28,8 +28,8 @@ def prepare(
 ) -> dict:
     """Prepare a config source for entry into the Config object.
 
-    Extracts config and override settings from the source. Flattens these nested dictionaries out into lists of
-    objects which are namespaced in the way the Mbed config system expects.
+    Extracts memory, config and override settings from the source. Flattens these nested dictionaries out into
+    lists of objects which are namespaced in the way the Mbed config system expects.
 
     Args:
         input_data: The raw config JSON object parsed from the config file.
@@ -46,6 +46,11 @@ def prepare(
     for key in data:
         data[key] = _sanitise_value(data[key])
 
+    memories = _extract_memories(namespace, data)
+
+    if memories:
+        data["memories"] = memories
+
     if "config" in data:
         data["config"] = _extract_config_settings(namespace, data["config"])
 
@@ -78,6 +83,31 @@ class ConfigSetting:
         self.value = _sanitise_value(self.value)
 
 
+@dataclass
+class Memory:
+    """Representation of a defined RAM/ROM region."""
+
+    name: str
+    namespace: str
+    start: str
+    size: str
+
+    def __post_init__(self) -> None:
+        """Convert start and size to hex format strings."""
+        try:
+            self.start = hex(int(self.start, 0))
+        except ValueError:
+            raise ValueError(
+                f"Value of MBED_{self.name}_START in {self.namespace}, {self.start} is invalid: must be an integer"
+            )
+        try:
+            self.size = hex(int(self.size, 0))
+        except ValueError:
+            raise ValueError(
+                f"Value of MBED_{self.name}_SIZE in {self.namespace}, {self.size} is invalid: must be an integer"
+            )
+
+
 @dataclass
 class Override:
     """Representation of a config override.
@@ -128,6 +158,27 @@ def _extract_config_settings(namespace: str, config_data: dict) -> List[ConfigSe
     return settings
 
 
+def _extract_memories(namespace: str, data: dict) -> List[Memory]:
+    memories = []
+    for mem in ["rom", "ram"]:
+        start_attr = f"mbed_{mem}_start"
+        size_attr = f"mbed_{mem}_size"
+        start = data.get(start_attr)
+        size = data.get(size_attr)
+
+        if size is not None and start is not None:
+            logger.debug(f"Extracting MBED_{mem.upper()} definitions in {namespace}: _START={start}, _SIZE={size}.")
+
+            memory = Memory(mem.upper(), namespace, start, size)
+            memories.append(memory)
+        elif start is not None or size is not None:
+            raise ValueError(
+                f"{size_attr.upper()} and {start_attr.upper()} must be defined together. Only "
+                f"{'START' if start is not None else 'SIZE'} is defined in the lib {namespace}."
+            )
+    return memories
+
+
 def _extract_target_overrides(
     namespace: str, override_data: dict, allowed_target_labels: Iterable[str]
 ) -> List[Override]:
diff --git a/src/mbed_tools/build/_internal/templates/mbed_config.tmpl b/src/mbed_tools/build/_internal/templates/mbed_config.tmpl
index 8fb2119..7fadeb1 100644
--- a/src/mbed_tools/build/_internal/templates/mbed_config.tmpl
+++ b/src/mbed_tools/build/_internal/templates/mbed_config.tmpl
@@ -75,6 +75,10 @@ set(MBED_CONFIG_DEFINITIONS
     "-D{{setting_name}}={{value}}"
     {% endif -%}
 {%- endfor -%}
+{% for memory in memories %}
+    "-DMBED_{{memory.name}}_START={{memory.start}}"
+    "-DMBED_{{memory.name}}_SIZE={{memory.size}}"
+{%- endfor -%}
 {% for macro in macros %}
     "{{macro|replace("\"", "\\\"")}}"
 {%- endfor %}


Test patch:
diff --git a/tests/build/_internal/config/test_config.py b/tests/build/_internal/config/test_config.py
index 980ed4d..c7e2e35 100644
--- a/tests/build/_internal/config/test_config.py
+++ b/tests/build/_internal/config/test_config.py
@@ -2,10 +2,11 @@
 # Copyright (c) 2020-2021 Arm Limited and Contributors. All rights reserved.
 # SPDX-License-Identifier: Apache-2.0
 #
+import logging
 import pytest
 
 from mbed_tools.build._internal.config.config import Config
-from mbed_tools.build._internal.config.source import prepare, ConfigSetting, Override
+from mbed_tools.build._internal.config.source import prepare, ConfigSetting, Memory, Override
 
 
 class TestConfig:
@@ -24,6 +25,17 @@ class TestConfig:
         with pytest.raises(ValueError, match="lib.param already defined"):
             conf.update(prepare({"config": {"param": {"value": 0}}}, source_name="lib"))
 
+    def test_logs_ignore_mbed_ram_repeated(self, caplog):
+        caplog.set_level(logging.DEBUG)
+        input_dict = {"mbed_ram_size": "0x80000", "mbed_ram_start": "0x24000000"}
+        input_dict2 = {"mbed_ram_size": "0x78000", "mbed_ram_start": "0x24200000"}
+
+        conf = Config(prepare(input_dict, source_name="lib1"))
+        conf.update(prepare(input_dict2, source_name="lib2"))
+
+        assert "values from `lib2` will be ignored" in caplog.text
+        assert conf["memories"] == [Memory("RAM", "lib1", "0x24000000", "0x80000")]
+
     def test_target_overrides_handled(self):
         conf = Config(
             {
diff --git a/tests/build/_internal/config/test_source.py b/tests/build/_internal/config/test_source.py
index 962315a..b7f4a2a 100644
--- a/tests/build/_internal/config/test_source.py
+++ b/tests/build/_internal/config/test_source.py
@@ -2,8 +2,10 @@
 # Copyright (c) 2020-2021 Arm Limited and Contributors. All rights reserved.
 # SPDX-License-Identifier: Apache-2.0
 #
+import pytest
+
 from mbed_tools.build._internal.config import source
-from mbed_tools.build._internal.config.source import Override
+from mbed_tools.build._internal.config.source import Memory, Override
 
 
 class TestPrepareSource:
@@ -118,3 +120,48 @@ class TestPrepareSource:
         assert conf["config"][0].value == {"ETHERNET", "WIFI"}
         assert conf["sectors"] == {0, 2048}
         assert conf["header_info"] == {0, 2048, "bobbins", "magic"}
+
+    def test_memory_attr_extracted(self):
+        lib = {
+            "mbed_ram_size": "0x80000",
+            "mbed_ram_start": "0x24000000",
+            "mbed_rom_size": "0x200000",
+            "mbed_rom_start": "0x08000000",
+        }
+
+        conf = source.prepare(lib, "lib")
+
+        assert Memory("RAM", "lib", "0x24000000", "0x80000") in conf["memories"]
+        assert Memory("ROM", "lib", "0x8000000", "0x200000") in conf["memories"]
+
+    def test_memory_attr_converted_as_hex(self):
+        input_dict = {"mbed_ram_size": "1024", "mbed_ram_start": "0x24000000"}
+
+        conf = source.prepare(input_dict, source_name="lib")
+
+        memory, *_ = conf["memories"]
+        assert memory.size == "0x400"
+
+    def test_raises_memory_size_not_integer(self):
+        input_dict = {"mbed_ram_size": "NOT INT", "mbed_ram_start": "0x24000000"}
+
+        with pytest.raises(ValueError, match="_SIZE in lib, NOT INT is invalid: must be an integer"):
+            source.prepare(input_dict, "lib")
+
+    def test_raises_memory_start_not_integer(self):
+        input_dict = {"mbed_ram_size": "0x80000", "mbed_ram_start": "NOT INT"}
+
+        with pytest.raises(ValueError, match="_START in lib, NOT INT is invalid: must be an integer"):
+            source.prepare(input_dict, "lib")
+
+    def test_raises_memory_size_defined_not_start(self):
+        input_dict = {"mbed_ram_size": "0x80000"}
+
+        with pytest.raises(ValueError, match="Only SIZE is defined"):
+            source.prepare(input_dict)
+
+    def test_raises_memory_start_defined_not_size(self):
+        input_dict = {"mbed_ram_start": "0x24000000"}
+
+        with pytest.raises(ValueError, match="Only START is defined"):
+            source.prepare(input_dict)