Skip to content
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

SD default configuration for ST boards is already defined in TARGET_STM #9386

Merged
merged 1 commit into from Jan 18, 2019

Conversation

Projects
None yet
7 participants
@jeromecoutant
Copy link
Contributor

commented Jan 15, 2019

Description

Default configuration is:
"SPI_CS": "SPI_CS",
"SPI_MOSI": "SPI_MOSI",
"SPI_MISO": "SPI_MISO",
"SPI_CLK": "SPI_SCK",

As all these SPI_xx are defined for all STM NUCLEO and DISCO targets,
we don't need extra configuration to support component_sd

If some user uses other SPI pins, better way is to configure it at application side.

Pull request type

[ ] Fix
[ ] Refactor
[x] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jan 15, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

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

@yossi2le
Copy link
Contributor

left a comment

@jeromecoutant Thanks for the cleanup.
LGTM

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 18, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Jan 18, 2019

@cmonr cmonr merged commit 309d9be into ARMmbed:master Jan 18, 2019

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-8 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9345 cycles (-1174 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 18, 2019

@jeromecoutant jeromecoutant deleted the jeromecoutant:PR_SDJSON branch Jan 18, 2019

@dannybenor

This comment has been minimized.

Copy link

commented Jan 21, 2019

@jeromecoutant there are issues in this PR, for exmple
Removed from components/storage/blockdevice/COMPONENT_SD/config/mbed_lib.json in #9386

"DISCO_F429ZI": {
"SPI_MOSI": "PC_12",
"SPI_MISO": "PC_11",
"SPI_CLK": "PC_10",
"SPI_CS": "PA_15"

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F429xI/TARGET_DISCO_F429ZI/PinNames.h
SPI_MOSI = PA_7,
SPI_MISO = PA_6,
SPI_SCK = PA_5,
SPI_CS = PB_6,

@jeromecoutant

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

@dannybenor
This PR is approved, merged, and explained in the description:

As all these SPI_xx are defined for all STM NUCLEO and DISCO targets,
we don't need extra configuration to support component_sd

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

@jeromecoutant we found runtime issues to initialize SD component in our CI tests over the weekend for nucleo boards. @dannybenor will provide more details

@dannybenor

This comment has been minimized.

Copy link

commented Jan 21, 2019

Removed from components/storage/blockdevice/COMPONENT_SD/config/mbed_lib.json in #9386
"NUCLEO_F411RE": {
"SPI_MOSI": "PC_3",
"SPI_MISO": "PC_2",
"SPI_CLK": "PC_7",
"SPI_CS": "PB_9"

         https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F4/TARGET_MTS_MDOT_F411RE/PinNames.h             
SPI_MOSI = PA_7,
SPI_MISO = PA_6,
SPI_SCK = PA_5,

SPI_NSS = PA_4,

@dannybenor

This comment has been minimized.

Copy link

commented Jan 21, 2019

@jeromecoutant which setting is correct in your view for DISCO_F429ZI
"SPI_MOSI": "PC_12" or SPI_MOSI = PA_7
Because after the change tests fail

@jeromecoutant

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

@0xc0170
If your application (CI tests) is using other SPI pins than the one defined in TARGET_STM,
better way is to configure your own pins at application side.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

If some user uses other SPI pins, better way is to configure it at application side.

We did not realize these fix is changing the pins . As I understand , this is a fix but it's breaking current applications. Applications use what was defined before in the component - in most cases ppl were not aware of default config in targets.

"SPI_MOSI": "PC_12" or SPI_MOSI = PA_7

@jeromecoutant what do you suggest? Which config is correct? As apps use the SD config, we should keep it as it is within 5.11.x and can do clean up for 5.12?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

I removed 5.11.3 label for now until we get a solution

@dannybenor

This comment has been minimized.

Copy link

commented Jan 22, 2019

The right definitions are in the TARGET_ST folder, according to the spec. The removed definitions are related to special wiring done for the CI. See
https://github.com/ARMmbed/sd-driver#wiring-instructions-for-target-nucleo_f429zi-with-ci-test-shield.
I think the best will be to move the PIN definitions used in the CI to the CI tests, in the same way that CI is adding COMPONENT SD.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

I think the best will be to move the PIN definitions used in the CI to the CI tests, in the same way that CI is adding COMPONENT SD.

+1, means this patch is fine and 5.11.3 label should be added back (update: 5.12 as this breaks the config so rather goes to the next feature release). We will fix this on our side, correct?

@dannybenor

This comment has been minimized.

@dannybenor

This comment has been minimized.

Copy link

commented Jan 22, 2019

@jeromecoutant also for NUCLEO-F411RE I see in the documentation
PB6 TIM4_CH1 or SPI1_CS
But in the file https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F4/TARGET_MTS_MDOT_F411RE/PinNames.h I see SPI_NSS = PA_4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.