Copy memory regions and sectors from tools/arm_pack_manager/index.json to targets.json (On hold) #14975

rajkan01 · 2021-08-03T12:02:48Z

Summary of changes

fixes ARMmbed/mbed-tools#271
Previously Mbed CLI1 parses index.json to get the memory information and sectors information for a target which has bootloader_supported with true in targets configuration in targets.json as going further with CLI2 like to get mem and sectors info from target.json as part of targets configuration, so this PR changes copy mem regions and sectors info into targets JSON and retain the existing mem and sectors info in index.json until CLI1 is deprecated

Impact of changes

None.

Migration actions required

As going further new targets gets added or existing targets have to add/update mem and sectors info in targets.json instead of index.json

Documentation

None.

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2021-08-03T12:30:15Z

@rajkan01, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

ciarmcom · 2021-08-06T11:50:13Z

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please complete review of the changes to move the PR forward. Thank you for your contributions.

targets/targets.json

Patater · 2021-08-09T12:51:29Z

targets/targets.json

@@ -1689,7 +1787,11 @@
            "SERIAL_ASYNCH",
            "FLASH",
            "MPU"
-        ]
+        ],
+        "mbed_rom_start": "0x8000000",


Which device in index.json did these come from?

Should we add a "device_name": "somedevice" entry here?

MCU_STM32xxxx represents a "family" of devices with RAM and ROM common values.
But you can't define a specific device_name here, this has to be defined in the board target.

device_name: STM32F303RETx and target NUCLEO_F303RE & device name : STM32F303ZETx and target NUCLEO_F303ZE.

Both device_name has similar memory configuration and both targets inherited from MCU_STM32F303xE

Patater · 2021-08-09T12:58:59Z

targets/targets.json

@@ -1834,7 +1946,11 @@
        ],
        "extra_labels_add": [
            "STM32F411xE"
-        ]
+        ],
+        "mbed_rom_start": "0x8000000",


Which device in index.json are these from? Should we add "device_name"?

Targets: NUCLEO_F411RE, MTS_DRAGONFLY_F411RE, MTS_MDOT_F411RE has from the same device_name: STM32F411RETx and all targets are inherited from MCU_STM32F411xE family device

Patater · 2021-08-09T12:59:21Z

targets/targets.json

@@ -1906,7 +2022,11 @@
        "device_has_add": [
            "CAN",
            "TRNG"
-        ]
+        ],
+        "mbed_rom_start": "0x8000000",


Which device in index.json are these from? Should we add "device_name"?

Targets: NUCLEO_F412ZG, WIO_EMW3166 has the same device_name: STM32F412ZGTx and all targets are inherited from MCU_STM32F412xG family

Patater · 2021-08-09T13:00:34Z

targets/targets.json

+        "mbed_rom_start": "0x8000000",
+        "mbed_rom_size": "0x100000",
+        "mbed_ram_start": "0x20000000",
+        "mbed_ram_size": "0x40000"
    },
    "NUCLEO_F412ZG": {


Should we add rom and ram entries for the NUCLEO_F412ZG target directly? I see it inherits from MCU_STM32F412xG to get these, but we are putting "device_name" here, not under MCU_STM32F412xG. Shouldn't we keep "device_name" entries next to the mbed_rom_* and mbed_ram_* entries to make it easy to verify which device the values came from in index.json?

I think it is better to keep memory info in the device family instead of duplicating the same configuration.

And In this case Targets: NUCLEO_F412ZG, WIO_EMW3166 has the same device_name: STM32F412ZGTx and both targets are derived from the same MCU_STM32F412xG family

Patater · 2021-08-09T15:33:37Z

targets/targets.json

@@ -7601,6 +8028,8 @@
            "CYBSP_WIFI_CAPABLE"
        ],
        "device_name": "CY8C6247BZI-D54",
+        "mbed_rom_start": "0x10000000",
+        "mbed_rom_size": "0x100000",
        "mbed_ram_start": "0x08002000",
        "mbed_ram_size": "0x00045800",


Are these RAM values correct? They seem to have similar issues to the CY8C624ABZI-D44 issues.

Patater · 2021-08-09T15:33:44Z

targets/targets.json

@@ -7642,6 +8071,8 @@
            "PSOC6"
        ],
        "device_name": "CY8C6347BZI-BLD53",
+        "mbed_rom_start": "0x10000000",
+        "mbed_rom_size": "0x100000",
        "mbed_ram_start": "0x08002000",
        "mbed_ram_size": "0x00045800",


Are these RAM values correct? They seem to have similar issues to the CY8C624ABZI-D44 issues.

Patater · 2021-08-09T15:33:53Z

targets/targets.json

@@ -7686,6 +8117,8 @@
            "CYBSP_WIFI_CAPABLE"
        ],
        "device_name": "CY8C6247BZI-D54",
+        "mbed_rom_start": "0x10000000",
+        "mbed_rom_size": "0x100000",
        "mbed_ram_start": "0x08002000",
        "mbed_ram_size": "0x00045800",


Are these RAM values correct? They seem to have similar issues to the CY8C624ABZI-D44 issues.

Patater · 2021-08-09T15:33:57Z

targets/targets.json

@@ -7741,6 +8174,8 @@
            "CYBSP_WIFI_CAPABLE"
        ],
        "device_name": "CY8C6247FDI-D52",
+        "mbed_rom_start": "0x10000000",
+        "mbed_rom_size": "0x100000",
        "mbed_ram_start": "0x08002000",
        "mbed_ram_size": "0x00045800",


Are these RAM values correct? They seem to have similar issues to the CY8C624ABZI-D44 issues.

Patater · 2021-08-09T15:37:44Z

targets/targets.json

+                65536,
+                32768
+            ]
+        ],


How is this change tested in CI? What tests would fail in our CI if we got these sector values incorrect?

If I am not wrong, currently, we don't have any test available to test the sector values.

Patater · 2021-08-09T15:55:33Z

I'm curious to know which greentea tests require adding this sectors and ram_start stuff. Could you add that in the commit message as part of the "why" behind these changes?

jeromecoutant

This is a big change, please don't merge it just before a new release version...

You could also remove arm_pack_manager/index.json to validate the changes...?

targets/targets.json

jeromecoutant · 2021-08-16T13:39:37Z

targets/targets.json

+        "mbed_ram_size": "0x40000",
+        "sectors" : [
+            [ 134217728, 4096 ],
+            [ 134221824, 4096 ],


Only 1 line is needed ?

ciarmcom · 2021-08-19T15:50:19Z

This pull request has automatically been marked as stale because it has had no recent activity. @rajkan01, please carry out any necessary work to get the changes merged. Thank you for your contributions.

Previously mbed CLI1 parses index.json to get the memory information as going further with CLI2 like to get mem info from target.json as part of targets configuration, so this PR changes copy all memory regions into targets JSON and retain the existing mem info in index.json until CLI1 deprecate

Previously mbed CLI1 parses index.json to get the sector information for a target which has "bootloader_supported" with "true" in targets configuration in targets.json as going further with CLI2 like to get sectors info from target.json as part of targets configuration, so this PR changes copy sectors info into targets JSON and retain the existing sectors info in index.json until CLI1 deprecate

Pull request has been modified.

jeromecoutant · 2021-09-15T14:57:30Z

Removing index.json file will break the export feature which is well appreciated by customers

rajkan01 · 2021-09-21T11:18:46Z

When we copy the mbed_rom_size, mbed_ram_size configs from index.json to targets.json causes the build failure for non-bootloader supported targets as MBED CLI1 expects that if any of the mem (ROM, RAM) configs are present in targets.json should have bootloader support, so putting these PR changes on hold until MBED CLI1 is deprecated.

Meanwhile introducing new rom-size and ram-size using config in targets.json to generate MBED_CONF_TARGET_ROM_SIZE and MBED_CONF_TARGET_RAM_SIZE macros by Mbed CLI1 and Mbed CLI2 which can be used in the greentea test.

mergify · 2021-10-01T14:01:37Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

0xc0170 · 2022-02-09T14:52:37Z

We are closing this pull request due to inactivity

ciarmcom added the release-type: patch label Aug 3, 2021

ciarmcom requested a review from a team August 3, 2021 12:30

ciarmcom added needs: review stale labels Aug 3, 2021

ciarmcom added stale and removed stale labels Aug 6, 2021

Patater previously requested changes Aug 9, 2021

View reviewed changes

mergify bot added needs: work and removed needs: review labels Aug 9, 2021

ciarmcom removed the stale label Aug 9, 2021

jeromecoutant reviewed Aug 16, 2021

View reviewed changes

ciarmcom added the stale label Aug 19, 2021

jeromecoutant mentioned this pull request Aug 20, 2021

STM32G0B0XX not in ARM Pack manager #15014

Closed

ciarmcom added stale and removed stale labels Aug 20, 2021

ciarmcom added stale and removed stale labels Aug 27, 2021

ciarmcom added stale and removed stale labels Sep 3, 2021

rajkan01 mentioned this pull request Sep 7, 2021

Refactor hal stack_size_unification greentea cmake #15058

Closed

ciarmcom added stale and removed stale labels Sep 10, 2021

rajkan01 added 3 commits September 14, 2021 16:09

Incorporated partial review comments

9ceccca

rajkan01 force-pushed the copy_mem_sectors_config branch from f99b803 to 9ceccca Compare September 14, 2021 15:10

ciarmcom removed the stale label Sep 14, 2021

rajkan01 added stale and removed needs: work release-type: patch labels Sep 17, 2021

rajkan01 changed the title ~~Copy memory regions and sectors from tools/arm_pack_manager/index.json to targets.json~~ Copy memory regions and sectors from tools/arm_pack_manager/index.json to targets.json (On hold) Sep 21, 2021

0xc0170 added do not merge and removed stale labels Sep 29, 2021

0xc0170 mentioned this pull request Sep 29, 2021

Add ram and rom size config to targets.json (Copy mem reg info from index.json) #15094

Closed

mergify bot added the needs: work label Oct 1, 2021

0xc0170 closed this Feb 9, 2022

mergify bot removed needs: work do not merge labels Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy memory regions and sectors from tools/arm_pack_manager/index.json to targets.json (On hold) #14975

Copy memory regions and sectors from tools/arm_pack_manager/index.json to targets.json (On hold) #14975

rajkan01 commented Aug 3, 2021 •

edited

Loading

ciarmcom commented Aug 3, 2021

ciarmcom commented Aug 6, 2021

Patater Aug 9, 2021

jeromecoutant Aug 16, 2021

rajkan01 Sep 8, 2021

Patater Aug 9, 2021

rajkan01 Sep 8, 2021

Patater Aug 9, 2021

rajkan01 Sep 8, 2021

Patater Aug 9, 2021

rajkan01 Sep 8, 2021

Patater Aug 9, 2021

Patater Aug 9, 2021

Patater Aug 9, 2021

Patater Aug 9, 2021

Patater Aug 9, 2021

rajkan01 Sep 8, 2021

Patater commented Aug 9, 2021

jeromecoutant left a comment

jeromecoutant Aug 16, 2021

ciarmcom commented Aug 19, 2021

jeromecoutant commented Sep 15, 2021

rajkan01 commented Sep 21, 2021

mergify bot commented Oct 1, 2021

0xc0170 commented Feb 9, 2022

+,
+
+                          ]
+                      ],

Copy memory regions and sectors from tools/arm_pack_manager/index.json to targets.json (On hold) #14975

Copy memory regions and sectors from tools/arm_pack_manager/index.json to targets.json (On hold) #14975

Conversation

rajkan01 commented Aug 3, 2021 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Aug 3, 2021

ciarmcom commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patater commented Aug 9, 2021

jeromecoutant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciarmcom commented Aug 19, 2021

jeromecoutant commented Sep 15, 2021

rajkan01 commented Sep 21, 2021

mergify bot commented Oct 1, 2021

0xc0170 commented Feb 9, 2022

rajkan01 commented Aug 3, 2021 •

edited

Loading