Using SPI_ macros from PinNames in SPIF and SD config files. #7979

yossi2le · 2018-09-04T11:41:39Z

Description

Now after PR #7774 merged the following pins add been added to mbed-os:
SPI_MOSI
SPI_MISO
SPI_SCK
SPI_PERSISTENT_MEM_CS

This PR is created in order to start using them in the new components block devices configuration file.

Pull request type

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

jeromecoutant · 2018-09-04T12:20:29Z

Why SPI_PERSISTENT_MEM_CS and not SPI_CS ?

yossi2le · 2018-09-04T12:25:08Z

Why SPI_PERSISTENT_MEM_CS and not SPI_CS ?

Because SPI_CS can point not only to storage but to any device/subdevice connected to the SPI bus.
We like that it will be clear to everyone that this Pin name is for the SPI persistent memory.

jeromecoutant · 2018-09-04T12:27:25Z

components/storage/blockdevice/COMPONENT_SD/config/mbed_lib.json

@@ -60,12 +60,6 @@
             "SPI_CLK":  "PTD5",
             "SPI_CS":   "PTD4"
        },


Please remove all NUCLEO and DISCO target_overrides configuration
as SPI_xxx define should be already defined for them.
Thx

ASFAIK, not all NUCLEO and DISCO has those pins defined. Also, some of them were defined by the data from spif and some from sd. So in case, spif is the original definition we left the sd target override as it was.

jeromecoutant · 2018-09-04T12:35:12Z

components/storage/blockdevice/COMPONENT_SD/config/mbed_lib.json

+        "SPI_CS": "SPI_PERSISTENT_MEM_CS",
+        "SPI_MOSI": "SPI_MOSI",
+        "SPI_MISO": "SPI_MISO",
+        "SPI_CLK": "SPI_SCK",
        "DEVICE_SPI": 1,


It seems that MBED_CONF_SD_DEVICE_SPI is not used

do you mean MBED_CONF_SD_SPI

jeromecoutant · 2018-09-04T13:19:46Z

Because SPI_CS can point not only to storage but to any device/subdevice connected to the SPI bus.
We like that it will be clear to everyone that this Pin name is for the SPI persistent memory.

So default setting should be
"SPI_CS": "SPI_CS",

And this should be overridden only the the targets that have some connected SPI memory.

"target_overrides": {
    "xxx": {
         "SPI_CS":   "SPI_PERSISTENT_MEM_CS"

yossi2le · 2018-09-04T13:38:41Z

So default setting should be
"SPI_CS": "SPI_CS",

And this should be overridden only the the targets that have some connected SPI memory.
"target_overrides": {
  "xxx": {
        "SPI_CS":   "SPI_PERSISTENT_MEM_CS"

No, we prefer that the configuration for the storage will be always SPI_PERSISTENT_MEM_CS. if someone like in the PinNames.h he can do
SPI_PERSISTENT_MEM_CS = SPI_CS
In our opinion it this ease the configuration

LMESTM · 2018-09-04T14:24:13Z

Hello @yossi2le @0xc0170

There have been changes in #7774 that do not look good to me at first glance - but maybe I'm wrong ..
Also we haven't been asked for review before merge so haven't asked question so far.

So my point is: adding SPI_PERSISTENT_MEM_CS in pinnames.h on D10 of for example the L475 IOT discovery board does not make sense to me:
https://github.com/ARMmbed/mbed-os/pull/7774/files#diff-6fcf86553443430596ac5ee4d817ae14R242

D10 pin is just a GPIO and this definition of SPI_PERSISTENT_MEM_CSapplies I guess in case the CI test shield is plugged in, but D10. There is one persistent memory on this target and this is the embdded QSPI.

So we think that the SPI_PERSISTENT_MEM_CS definition definitely does not belong to here.

One more question: how have you selected the list of targets that were updated in #7774 ?
It updates a subset of the STM targets, not all of them ... so I'd like to understand.

thanks in advance

dannybenor · 2018-09-04T16:31:30Z

@LMESTM @yossi2le I think we went one step too fast. We will revert the changes in the files pinnames.h and start a process of introducing the changes in synch with partners.
We will have 2 boards for example (k64f for SD, K82F for SPIF)

LMESTM · 2018-09-05T07:23:47Z

@yossi2le @dannybenor Sounds good. Just need to find out the right place where SPI_PERSISTENT_MEM_CS definition belongs to.

yossi2le · 2018-09-05T11:35:05Z

@yossi2le @dannybenor Sounds good. Just need to find out the right place where >SPI_PERSISTENT_MEM_CS definition belongs to.

PR #7995 should revert this changes.

jeromecoutant · 2018-09-24T12:26:11Z

Any update ?
Thx

0xc0170 · 2018-09-24T12:34:04Z

@yossi2le The revert was integrated, this PR is now ready as it is ? If yes, someone from @ARMmbed/mbed-os-storage should review

jeromecoutant · 2018-09-25T08:15:56Z

Why don't you use as default configuration Arduino pin ?
like in https://github.com/ARMmbed/ci-test-shield/blob/master/mbed_app.json

yossi2le · 2018-09-25T11:31:58Z

Why don't you use as default configuration Arduino pin ?
like in https://github.com/ARMmbed/ci-test-shield/blob/master/mbed_app.json

Because we aiming that SPI_ macros in PinsNames.h will be the defaults, while we keep the option to allow changes by configuration.

0xc0170 · 2018-10-15T23:58:08Z

@yossi2le Can we proceed ? Rebase to resolve latest updates, and @ARMmbed/mbed-os-storage team to review

yossi2le · 2018-10-16T09:44:05Z

Rebase done.

cmonr · 2018-10-23T01:26:26Z

Still waiting for @ARMmbed/mbed-os-storage to review.

@ARMmbed/mbed-os-maintainers When y'all get a chance, the unittests also need to be rerun. Results are not due to PR, but due to glitch from last week.

offirko

Looks fine to me

0xc0170 · 2018-10-23T08:40:02Z

@jeromecoutant @LMESTM All questions answered here, good to progress?

jeromecoutant · 2018-10-23T08:45:14Z

@jeromecoutant @LMESTM All questions answered here, good to progress?

No... I don't like the default use of SPI_PERSISTENT_MEM_CS...
I will have to make a PR for all NUCLEO and DISCO boards with :

"target_overrides": {
"xxx": {
"SPI_CS": "SPI_CS"

yossi2le · 2018-10-24T14:02:02Z

@jeromecoutant.
First of all, I don't understand why you need to update all the NUCLEO and DISCO boards because those which exists already will not be removed. for example.

"NUCLEO_L031K6": {
"SPI_MOSI": "SPI_MOSI",
"SPI_MISO": "SPI_MISO",
"SPI_CLK": "SPI_SCK",
"SPI_CS": "SPI_CS"
}

Besides if we don't have this you still need to configure every board because the global configuration now is set to NC by mistake.

Second, we think that SPI_CS cannot be set always to point on storage. if you have more component connected to the SPI bus they all need a different cheap select. Therefore, we like to have one specific pin name for the storage.

kjbracey · 2018-10-24T14:13:39Z

Just to stir the pot - yes, SPI_CS is obviously device-specific, but then so are the others, really. You can have multiple SPI buses, so there's an implied "bus for the storage" for SPI_MOSI, SPI_MISO and SPI_CLK too...

I don't think it's the end of the world if the legacy "SPI_xxx" meaning "SPI pins for storage" in PinNames.h is retained. It doesn't preclude more specific names for other devices and drivers.

But having some prefixed, some not seems odd.

…to the config files

yossi2le · 2018-10-25T09:13:16Z

I have replaced the SPI_PERSISTENT_MEM_CS macro with SPI_CS and rebased.

0xc0170 · 2018-10-25T09:50:23Z

Thank you all for quick resolution here!

/morph build

mbed-ci · 2018-10-25T11:55:11Z

Build : SUCCESS

Build number : 3462
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7979/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-10-25T17:51:00Z

Test : FAILURE

Build number : 3253
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7979/3253

cmonr · 2018-10-25T18:16:48Z

/morph test

mbed-ci · 2018-10-25T23:07:46Z

Exporter Build : SUCCESS

Build number : 3085
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7979/

mbed-ci · 2018-10-26T08:05:16Z

Test : SUCCESS

Build number : 3259
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7979/3259

adbridge · 2018-11-02T12:59:01Z

This is sitting on top of #7955 scheduled for 5.11

0xc0170 requested a review from a team September 4, 2018 12:19

0xc0170 added the needs: review label Sep 4, 2018

jeromecoutant reviewed Sep 4, 2018

View reviewed changes

yossi2le force-pushed the fix_spif_sd_config branch from 3e1c9b5 to 7fa4adf Compare September 5, 2018 11:32

jeromecoutant mentioned this pull request Sep 24, 2018

ST DISCO/NUCLEO: no default COMPONENT_SD #8231

Closed

yossi2le force-pushed the fix_spif_sd_config branch from 7fa4adf to 77eb74e Compare October 16, 2018 09:43

offirko approved these changes Oct 23, 2018

View reviewed changes

0xc0170 approved these changes Oct 23, 2018

View reviewed changes

cmonr added needs: work and removed needs: review labels Oct 23, 2018

Yossi Levy added 2 commits October 25, 2018 11:56

Using the SPI_ macros which was added to PinNames.h in PR ARMmbed#7774 …

eddd2b1

…to the config files

Remove SPI_PERSISTENT_MEM_CS macro and replace it with SPI_CS

d9a84c4

yossi2le force-pushed the fix_spif_sd_config branch from 77eb74e to d9a84c4 Compare October 25, 2018 09:11

jeromecoutant approved these changes Oct 25, 2018

View reviewed changes

kjbracey approved these changes Oct 25, 2018

View reviewed changes

0xc0170 approved these changes Oct 25, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Oct 25, 2018

cmonr added the release-version: 5.10.3 label Oct 25, 2018

0xc0170 added ready for merge and removed needs: CI labels Oct 26, 2018

0xc0170 merged commit 93a17c1 into ARMmbed:master Oct 26, 2018

0xc0170 removed the ready for merge label Oct 26, 2018

adbridge added release-version: 5.11.0-rc1 and removed release-version: 5.10.3 labels Nov 2, 2018

Using SPI_ macros from PinNames in SPIF and SD config files. #7979

Using SPI_ macros from PinNames in SPIF and SD config files. #7979

Uh oh!

Conversation

yossi2le commented Sep 4, 2018

Description

Pull request type

Uh oh!

jeromecoutant commented Sep 4, 2018

Uh oh!

yossi2le commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeromecoutant commented Sep 4, 2018

Uh oh!

yossi2le commented Sep 4, 2018

Uh oh!

LMESTM commented Sep 4, 2018

Uh oh!

dannybenor commented Sep 4, 2018

Uh oh!

LMESTM commented Sep 5, 2018

Uh oh!

yossi2le commented Sep 5, 2018

Uh oh!

jeromecoutant commented Sep 24, 2018

Uh oh!

0xc0170 commented Sep 24, 2018

Uh oh!

jeromecoutant commented Sep 25, 2018

Uh oh!

yossi2le commented Sep 25, 2018

Uh oh!

0xc0170 commented Oct 15, 2018

Uh oh!

yossi2le commented Oct 16, 2018

Uh oh!

cmonr commented Oct 23, 2018

Uh oh!

offirko left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Oct 23, 2018

Uh oh!

jeromecoutant commented Oct 23, 2018

Uh oh!

yossi2le commented Oct 24, 2018

Uh oh!

kjbracey commented Oct 24, 2018

Uh oh!

yossi2le commented Oct 25, 2018

Uh oh!

0xc0170 commented Oct 25, 2018

Uh oh!

mbed-ci commented Oct 25, 2018

Triggering tests

Uh oh!

mbed-ci commented Oct 25, 2018

Uh oh!

cmonr commented Oct 25, 2018

Uh oh!

mbed-ci commented Oct 25, 2018

Uh oh!

mbed-ci commented Oct 26, 2018

Uh oh!

adbridge commented Nov 2, 2018

Uh oh!

Uh oh!