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

TEST: SPI: CS is isually a DigitalOut #21

Merged
merged 1 commit into from
Jan 11, 2017

Conversation

LMESTM
Copy link
Contributor

@LMESTM LMESTM commented Jan 6, 2017

In most SPI libs, CS is controlled as a GPIO, so as a DigitalOut. This
is also how this is done in the next SPI tests. So when testing the
instance creation, better to use this type as well.

If this is not done, tests would fail on boards where Hardware CS (NSS in pin STM32) is not muxed onto D10. This would require to define all the particular mapping of those boards, whereas using D10 as a DigitalOut is valid for any MBED board.

In most SPI libs, CS is controlled as a GPIO, so as a DigitalOut. This
is also how this is done in the next SPI tests. So when testing the
instance creation, better to use this type as well.
@0xc0170
Copy link
Collaborator

0xc0170 commented Jan 6, 2017

If this is not done, tests would fail on boards where Hardware CS (NSS in pin STM32) is not muxed onto D10. This would require to define all the particular mapping of those boards, whereas using D10 as a DigitalOut is valid for any MBED board.

It should work out of the box - defining SSEL in the SPI ctor. why it fails for ST? Is it because SSEL pins are not defined properly in pinmap structure?

it should work as well as you changed it but I believe we want to test SPI class that means we might want to have both use cases, having CS outside of SPI class and part of it.

@LMESTM
Copy link
Contributor Author

LMESTM commented Jan 6, 2017

It should work out of the box - defining SSEL in the SPI ctor. why it fails for ST? Is it because SSEL pins are not defined properly in pinmap structure?

The SSEL pin of SPI IP in the STM32 of at least NUCLEO_L476RG and NUCLEO_F410RB are physically not muxed / connected to PB_6, which is the pin wired as D10 on those boards. Considering this is a preliminary test to access SDFileSystem which uses cs as a DigitalOut, I think this makes sense.

If we want to test the object creation with HW CS rather than DigitalOutput on those boards, we would need to define 2 different PINs.

The one for using SD card
"SPI_SD_CS": "D10",
Another one for testing object creation - which would vary from one boards to another and need per-board exceptions in the target_overrides section of mbed_app.json
"SPI_HW_CS": "PA_4", // actually same as A2

https://developer.mbed.org/platforms/ST-Nucleo-F410RB/

@bcostm
Copy link
Contributor

bcostm commented Jan 6, 2017

I agree. Maybe use the naming:
SPI_SSEL = SPI peripheral NSS pin used to configure the SPI in master or slave
SPI_CS = DigitalOut pin used to control a slave device

@0xc0170
Copy link
Collaborator

0xc0170 commented Jan 9, 2017

The SPI documentation states the same as we agreed above:

 * // hardware ssel (where applicable)
 * //SPI device(p5, p6, p7, p8); // mosi, miso, sclk, ssel
 *
 * // software ssel
 * SPI device(p5, p6, p7); // mosi, miso, sclk
 * DigitalOut cs(p8); // ssel
 *

D10 should be able to be sw SSEL, this change is fine with me. However, we want to test also hw ssel. In your case, D10 (this pin is wired to SD card as nCS) can't be hw SSEL. We could introduce a boolean flag to indicate that this pin is capable of hw SSEL, and test SPI with hw SSEL. Lets create an issue for this and we need to solve it. I would imagine SPI object to be able to choose, use hw ssel and drive it or fallback to digitalout if it can't, but this is not implemented yet.

Most of the test cases in spi use SDFileSystem that is using sw chip select thus it works.

@BlackstoneEngineering what do you think?

@LMESTM
Copy link
Contributor Author

LMESTM commented Jan 9, 2017

@0xc0170
In the concrete case of SDFileSystem, what I've seen is that the driver definitely wants to have a sw SSEL, not a hw one. Sometimes it sends 0xFF data on the lines while SSEL is not selected, which would not be possible with hw SSEL.

so the usage in driver would differs depending of hw or sw SSEL. In case of DIO, the SPI device driver specifically sets the cs. In case of HW there is no mention of cs in the driver as it is autonomously done in HW.

All in all I think it best to let the SPI user specifically decides between sw and hw so that the driver's code correspondingly manages (or not) cs

For this concrete test, this is a SD SPI test case and only sw CS is being used, so I think it makes sense to have the object creation with sw cs. We could have another test case, with HW object creation, but this would also be best to have a driver that makes use of this configured SPI with hw cs.

@0xc0170
Copy link
Collaborator

0xc0170 commented Jan 9, 2017

For this concrete test, this is a SD SPI test case and only sw CS is being used, so I think it makes sense to have the object creation with sw cs. We could have another test case, with HW object creation, but this would also be best to have a driver that makes use of this configured SPI with hw cs.

Agree. If this gets integrated, there should be an issue created to reflect this: Add test for SPI hw SSEL

@0xc0170
Copy link
Collaborator

0xc0170 commented Jan 11, 2017

@BlackstoneEngineering Please review and integrate

Copy link
Collaborator

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@0xc0170 0xc0170 mentioned this pull request Jan 11, 2017
@0xc0170 0xc0170 merged commit 0085ee3 into ARMmbed:master Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants