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

STM32 : targets.json file simplification #4610

Merged
merged 1 commit into from Jun 28, 2017

Conversation

Projects
None yet
5 participants
@jeromecoutant
Contributor

jeromecoutant commented Jun 22, 2017

Description

"FAMILY_STM32" has been created with all common features.
All STM32 devices can now inherit from it.
This will simplify readability and maintainability.

Status

READY

@theotherjimmy

Could you get rid of the core and progen lines? I really like this diff so far!

targets/targets.json Outdated
@@ -668,367 +668,321 @@
"extra_labels_remove": ["FRDM"],
"supported_form_factors": []
},
"FAMILY_STM32": {
"inherits": ["Target"],
"core": "",

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 22, 2017

Contributor

You should not add this line. Assigning core like this can cause problems if a child incorrectly does not define it's core attribute. Further, each child must define the core attribute to function, overriding this one.

This comment has been minimized.

@jeromecoutant

jeromecoutant Jun 22, 2017

Contributor

no, I can't. It seems that core is mandatory in the parse script ?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 22, 2017

Contributor

Replace it with "public": false, maybe

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 22, 2017

Contributor

It's required for public targets, but you don't really want to be able to build for the non-target "FAMILY_STM32".

This comment has been minimized.

@jeromecoutant

jeromecoutant Jun 22, 2017

Contributor

OK, it's working.
I think it's even better to add this public false

targets/targets.json Outdated
"is_disk_virtual": true,
"macros": ["HSE_VALUE=26000000", "TRANSACTION_QUEUE_SIZE_SPI=2"],
"macros_add": ["HSE_VALUE=26000000"],
"progen": {"target": "mts-mdot-f405rg"},

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 22, 2017

Contributor

I know that you did not change this, but could you get rid of this progen key? It's not used anymore.

This comment has been minimized.

@jeromecoutant

jeromecoutant Jun 22, 2017

Contributor

I can :-)

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 22, 2017

Contributor

Thanks!

@jeromecoutant jeromecoutant force-pushed the jeromecoutant:PR_TARGETJSON branch Jun 22, 2017

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jun 22, 2017

Could you get rid of the core and progen lines?

done

@theotherjimmy

Awesome! Thanks for addressing my comments so quickly.

@0xc0170

Just one missing macro, the rest looks good, +1 for simplifying the stm32 targets

targets/targets.json Outdated
"macros": ["TRANSACTION_QUEUE_SIZE_SPI=2", "RTC_LSI=1", "HSE_VALUE=12000000", "GNSSBAUD=9600"],
"inherits": ["Target"],
"device_has": ["ANALOGIN", "ANALOGOUT", "I2C", "I2CSLAVE", "INTERRUPTIN", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "SERIAL", "SERIAL_FC", "SLEEP", "RTC", "SPI", "SPISLAVE", "STDIO_MESSAGES", "TRNG"],
"macros_add": ["RTC_LSI=1", "HSE_VALUE=12000000"],

This comment has been minimized.

@0xc0170

0xc0170 Jun 26, 2017

Member

I dont see here GNSSBAUD=9600

@jeromecoutant jeromecoutant force-pushed the jeromecoutant:PR_TARGETJSON branch 2 times, most recently Jun 26, 2017

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jun 26, 2017

Hi
Missing macro added and rebase done
Thx

STM32 : targets.json file simplification
"FAMILY_STM32" has been creeated with all common features.
All STM32 devices can now inherit from it.
This will simplify readability and maintainability.

@jeromecoutant jeromecoutant force-pushed the jeromecoutant:PR_TARGETJSON branch to 8e28d9e Jun 26, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

👏 Thanks @jeromecoutant

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 27, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 641

Test failed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

Clearly not related.

/morph test

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

@mazimkhan Did this one fail to report as well?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

retest uvisor

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 28, 2017

@jeromecoutant could there be an additional layer of inheritance for the processor family? for example L0,L1(soon),L4 support Flash but the changes would have to be made to all targets individually.

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 28, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 668

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 28, 2017

@jeromecoutant could there be an additional layer of inheritance for the processor family? for example L0,L1(soon),L4 support Flash but the changes would have to be made to all targets individually.

Can you elaborate? Is this required for this patch or can be done on top of this one?

@chrissnow

This comment has been minimized.

Contributor

chrissnow commented Jun 28, 2017

It could be done on top of this,

My thoughts were that a lot of things are common to a processor family and splitting out what the HAL for the family supports from the individual targets would tidy it up further.

My example being I have implemented the flash HAL for all L1 devices but currently would need to go through any L1 target and modify them all to enable flash.

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jun 28, 2017

@chrissnow
I am not in favor of this additional level... 2 levels are enough to see what is common for all STM32, and what makes the difference.

@0xc0170
Please merge, I am waiting for this PR for #4421

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jun 28, 2017

@theotherjimmy theotherjimmy merged commit 4fc4405 into ARMmbed:master Jun 28, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment