ARMmbed / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
4.68k stars 2.98k forks source link

Inheritance - name of the inheritated target is not in the lables #3129

Closed 0xc0170 closed 7 years ago

0xc0170 commented 8 years ago

Description


Question

This code snippet illustrate the problem:

`"MCU_K22F512": {
        "supported_form_factors": ["ARDUINO"],
        "core": "Cortex-M4F",
        "supported_toolchains": ["ARM", "GCC_ARM", "IAR"],
        "extra_labels": ["Freescale", "KSDK2_MCUS", "MCU_K22F", "KPSDK_MCUS", "KPSDK_CODE"],
        "is_disk_virtual": true,
        "macros": ["CPU_MK22FN512VLH12", "FSL_RTOS_MBED"],
        "inherits": ["Target"],
        "detect_code": ["0231"],
        "device_has": ["ANALOGIN", "ANALOGOUT", "ERROR_RED", "I2C", "I2CSLAVE", "INTERRUPTIN", "LOWPOWERTIMER", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "RTC", "SERIAL", "SLEEP", "SPI", "SPISLAVE", "STDIO_MESSAGES"],
        "release_versions": ["2", "5"],
        "device_name": "MK22DN512xxx5"
    },
               "K22F": {
    "inherits": ["MCU_K22F512"],
               "extra_labels_add": ["FRDM"]
               },

To make K22F work, I would need to add to extra labels MCU_K22F512 to the target MCU_K22F512. It's not defined by default. But for a regular target like K22F it is, as I recall. I could not find any relevant info in the docs.

cc @bogdanm @theotherjimmy @MarceloSalazar

0xc0170 commented 8 years ago

bump

theotherjimmy commented 8 years ago

Should we add non-public targets too? In this example, adding non-public targets would add "Target" to the list of labels.

bridadan commented 8 years ago

Ah yes, the ever useful TARGET_TARGET :stuck_out_tongue_winking_eye: (in this particular case I don't think this would be too harmful, it could easily be ignored).

One advantage of having non-public targets be added to the list of labels would be for targets like the Nordic boards. For example: https://github.com/ARMmbed/mbed-os/blob/master/targets/targets.json#L1344

It would remove the need to have MCU_NRF51_16K_BASE as the target name and also specify MCU_NRF51_16K as an extra label. You could instead just name the non-public target MCU_NRF51_16K. At least, this makes sense to me, @pan- may have some concerns about this though.

theotherjimmy commented 8 years ago

2488 has some prior discussion of this.

0xc0170 commented 8 years ago

Should we add non-public targets too? In this example, adding non-public targets would add "Target" to the list of labels.

As I understand, by default a target is set to private (public: false). Thus in my code snippet above MCU_K22F is private and its label is not polluted. Is that correct?

This taken from 2488 referenced above:

Yes, we were actually supposing that also the parent targets' name would appear within the labels of the sibling(s) ;-).

I assumed the same. When creating MCU family port, you want to have that label available but target should be private (not able to use it with scripts as it can't be built for various reasons).

I find having to add extra label as a workaround. It was not obvious to us while we were creating a new MCU family in the tree and were receiving error like "device.h" not found .and like hey, MCU target is correct, why it cant find it. Thus I created this issue.

bridadan commented 8 years ago

@0xc0170 A target is by default set to public (public: true) https://github.com/ARMmbed/mbed-os/blob/master/docs/mbed_targets.md#public

theotherjimmy commented 7 years ago

so hey everyone ( @0xc0170 @bridadan ), Is the 'add all parents except for Target' option a viable strategy? If so, I could add that to targets.py to ease new board bring up.

bridadan commented 7 years ago

I spent a few minutes trying to think of a time when this would cause problems and I honestly can't think of a case. It may even be useful to include the root Target target, since you could then add extra_labels to all targets trivially if necessary. Other people may feel differently about this though.

So seems like a good idea to me :+1:

theotherjimmy commented 7 years ago

any extra label added to Target is pretty useless, as the code that label guards will always be enabled.

0xc0170 commented 7 years ago

Is the 'add all parents except for Target' option a viable strategy? If so, I could add that to targets.py to ease new board bring up.

I would say yes. The question to answer within this ticket is how hierarchy should look like (target inheritance). The above code snippet I provided, is this a valid use case ? I would say it is, at least for now ,thats how its done for various targets.

cc @sg- @geky for opinions - for the question quoted in this post