Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
0xc0170 opened this issue Oct 25, 2016 · 10 comments
Closed

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

0xc0170 opened this issue Oct 25, 2016 · 10 comments

Comments

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 25, 2016

Description

  • Type: Question
  • Priority: Minor

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
Copy link
Contributor Author

0xc0170 commented Oct 28, 2016

bump

@theotherjimmy
Copy link
Contributor

theotherjimmy commented Nov 2, 2016

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

@bridadan
Copy link
Contributor

bridadan commented Nov 2, 2016

Ah yes, the ever useful TARGET_TARGET 😜 (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
Copy link
Contributor

#2488 has some prior discussion of this.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 3, 2016

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
Copy link
Contributor

bridadan commented Nov 3, 2016

@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
Copy link
Contributor

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
Copy link
Contributor

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 👍

@theotherjimmy
Copy link
Contributor

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

@0xc0170
Copy link
Contributor Author

0xc0170 commented Dec 21, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants