Skip to content
This repository has been archived by the owner on Jan 7, 2019. It is now read-only.

Example: STM32F4 Discovery SPI #174

Conversation

rleh
Copy link
Member

@rleh rleh commented Aug 24, 2016

There was a typo in the SPI example.
SpiMaster2 has been initialized but SpiMaster1 used in the while loop.

@strongly-typed
Copy link
Member

@salkinium
Copy link
Member

Could you make the SpiMaster2 a typedef, so this can never happen again?

@ekiwi
Copy link
Member

ekiwi commented Aug 25, 2016

Could you make the SpiMaster2 a typedef, so this can never happen again?

I'd like to vote against that in order to keep the magic (i.e. indirection) to a minimum in the examples.

@salkinium
Copy link
Member

salkinium commented Aug 25, 2016

magic (i.e. indirection) to a minimum

A typedef is not magic, it is best practice. We'd always typedef xpcc::SoftwareSpiMaster<Sck, Mosi, Miso> SensorSpiMaster and not copy it around everywhere. Same applies for a hardware SPI masters.

@ekiwi
Copy link
Member

ekiwi commented Aug 25, 2016

Same applies for a hardware SPI masters.

I disagree:
A hardware SPI has a fixed meaning and is easily cross referenced with the data sheet. The manufacturer has given it a name and we should stick with that name in order to avoid additional complexity.
A software SPI on the other hand is the users "creation" and thus they are responsible for naming it in a sensible manner.

@salkinium
Copy link
Member

This example is so pointless (but thanks for making it less broken ;-).

We're connecting GpioOutputB12::connect(SpiMaster2::Nss);, even though we can never use it, because we don't have a SpiSlave. In fact, it's probably a very bad idea to connect Nss to a hardware SPI, because if that pin goes low, the peripheral switches to slave mode, and our driver can't deal with it.

And then we're sending 0xF0, but not printing what we read? This can't even be used as a loop-back test without modification.

What is this example supposed to show exactly? That our documentation isn't lying? Shouldn't this rather be part of the documentation of the SpiMaster interface?

It's not even specific to the STM32F4, certainly not to the SPI2 of the STM32F4. This could (and should) work on the AVR, or the STM32F1 or the F3.

The manufacturer has given it a name and we should stick with that name in order to avoid additional complexity.

We'd be advocating hardcoding the instance of the SPI driver. How is that useful?

The typedef would be a good opportunity to explain that all SPI instances share the same interface, even the bit-banged one, but that you must change the Gpio connections and perhaps the baudrate, otherwise you'd get a compile error.
That simple comment would already make this example like 9001% more useful.

I think it's obvious enough that SpiMaster2 refers to SPI2 on all the STM32, but it may not be obvious that you can use the SoftwareSpiMaster as a drop-in replacement.

I'd even go a step further and advocate that you should ALWAYS use the bit-banged masters for SPI and I2C especially when developing an external chip driver, since they are more stable than the hardware drivers (see #127). You can then switch to the hardware implementations and see if it still works.

@strongly-typed
Copy link
Member

ALWAYS is ALWAYS wrong ;-)

I'd even go a step further and advocate that you should ALWAYS use the bit-banged masters for SPI [...]

Well, that's what I thought until last year. I discovered the known anomality that the ENC28J60 to run the SPI at frequencies of at least 8 MHz. (at least ist Rev. B4 Silicon).

@salkinium
Copy link
Member

salkinium commented Aug 25, 2016

I discovered the known anomality that the ENC28J60 to run the SPI at frequencies of at least 8 MHz. (at least ist Rev. B4 Silicon).

That's it. I'm becoming a farmer.

@salkinium
Copy link
Member

Merged and modified manually in b327877.

@salkinium salkinium closed this Sep 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants