Fix SPI CS pin trouble #100

ghost · 2017-09-05T15:33:14Z

I fixed a bug inside SPI library caused by the use of a CS pin other than Arduino digital pins.
Now, all pins can be used like a CS pin for the SPI.

fpistm · 2017-09-06T07:10:37Z

variants/DISCO_F100RB/variant.h

@@ -133,7 +133,7 @@ enum {

 //SPI definitions
 //define 16 channels. As many channel as digital IOs
-#define SPI_CHANNELS_NUM        16
+#define SPI_CHANNELS_NUM        PEND


Pleaser use NUM_DIGITAL_PINS instead of PEND for all
Amend the commit to avoid twice commit for this

fpistm · 2017-09-06T14:46:39Z

Thanks @fprwi6labs
But this will increase memory consumption:

Arduino_Core_STM32/libraries/SPI/SPI.h

Line 163 in 0692177

SPISettings spiSettings[SPI_CHANNELS_NUM+1];

I think, we need to study more before merge.

LMESTM · 2017-09-06T15:13:19Z

@fprwi6labs can you please describe the original root cause of the bug and how this is fixed here ?

For now please find my 2 cents as I just discussed it with @fpistm
As I read it, SPI_CHANNELS_NUM[] could be used as a table of CS pins that can be used on the SPI bus. You can have several devices on the same bus, but each of them has its own CS pin. So I would think that there is 1 device per channel and the table is used to store the CS pin.
But you should not need to increase the maximum number of channels higher than 16, you just need to be able to store the pin number in the table, and the pin number needs to be handled as a generic pin (can be D2 or PH3 or PA12 ...)

ghost · 2017-09-06T15:41:35Z

@LMESTM

Firstly, the bug is caused by this code:
if(_pin > SPI_CHANNELS_NUM) return;

So when you use a CS pin higher than SPI_CHANNELS_NUM you are out.
The solution was to increase SPI_CHANNELS_NUM to the maximum digital pin number.
And @fpistm points a side effect of my fix.

Secondly, when you look at the avr code, the CS pin is saved only one time when you call the function begin(). So I think it is not to the SPI library to save a list of CS pin but to the user if he has several devices on the same SPI bus.

Finally, spiSettings saved the SPI settings but it is 'idiot' to save multiple settings for the same instance. You should call beginTransaction() to set the SPI before to send data.

I exposed my project to @fpistm to rework the SPI library. I will propose to you soon the code.

LMESTM · 2017-09-06T15:44:10Z

One more note: I just add a look to SPI.cpp and SPI.h files and the _pin is used as an index to the spiSettings[] table, so the _pin is directly used as an channel index. To be able to use any pin as CS, the table index must be separated from the _pin ... so a deeper change is needed

LMESTM · 2017-09-06T16:01:39Z

Firstly, the bug is caused by this code:
if(_pin > SPI_CHANNELS_NUM) return;

I found out indeed

The solution was to increase SPI_CHANNELS_NUM to the maximum digital pin number.
And @fpistm points a side effect of my fix.

Yes. I agree this is due to SPI implementation which uses _pin as the index of settings.

Secondly, when you look at the avr code, the CS pin is saved only one time when you call the function begin(). So I think it is not to the SPI library to save a list of CS pin but to the user if he has several devices on the same SPI bus.

I agree the user will store and use the CS pin, which is used as a reference to a device.

Finally, spiSettings saved the SPI settings but it is 'idiot' to save multiple settings for the same instance. You should call beginTransaction() to set the SPI before to send data.

I don't think this is 'idiot'. You can have several devices on the SPI bus, on different CS pins, that have different settings. And you don't want to re-define the settings before each transfer, so the SPI library do the job of applying the right settting. This is efficient: If it's the same device as before, you keep the setting, and only when device changes you re-apply the corresponding setting. I'm not saying that you must store the _pin, but you'd need to find a way to map the table index to a given _pin ...

ghost · 2017-09-06T16:16:58Z

I don't think this is 'idiot'. You can have several devices on the SPI bus, on different CS pins, that have different settings. And you don't want to re-define the settings before each transfer, so the SPI library do the job of applying the right settting. This is efficient: If it's the same device as before, you keep the setting, and only when device changes you re-apply the corresponding setting. I'm not saying that you must store the _pin, but you'd need to find a way to map the table index to a given _pin ...

I agree but when I read the documentation of the Arduino SPI I understand that the user should follow the following sequence:

call beginTransaction() to set the bus
call transfer() to send data
call endTransaction() to release the bus

The native Arduino SPI library is simpler than our. But I can try to save several settings.

Signed-off-by: fpr <[email protected]>

fpistm · 2017-09-12T08:34:43Z

libraries/SPI/src/SPI.h

+    int16_t         _CSpin;
+    spi_t           _spi;
+
+    uint8_t getIdx(uint8_t _pin)


Merge getIdx and addIdx and use a second arg to define if a new idx could be added with a default value (no add by default)
This will avoid to loop twice if there is no idx for pin then call addIdx.

fpistm · 2017-09-12T08:36:02Z

libraries/SPI/src/SPI.cpp

-  spiSettings[_pin].bOrder = settings.bOrder;
-  if(spiSettings[_pin].bOrder == MSBFIRST) {
-    spiSettings[_pin].msb = MSBFIRST;
+  idx = getIdx(_pin);


See my comment in SPI.h for getIdx and AddIdx to avoid loop twice

fpistm · 2017-09-12T08:39:48Z

libraries/SPI/src/SPI.h

@@ -97,45 +97,59 @@ class SPISettings {
    friend class SPIClass;
 };

-
+typedef struct {
+  int16_t pinCS;


Why not add the pinCS in the SPISettings class?

fpistm · 2017-09-12T09:03:45Z

libraries/SPI/src/SPI.h

+#define SPI_TRANSFER_TIMEOUT    5000
+
+// Defines the number of settings saved per SPI instance. Must be < 254.
+#ifndef NB_SPI_SETTINGS


Add in the comment, it could be redefined in the variant.h

LMESTM

Thanks for this new solution - this looks good. I added a few comments or questions but I'm happy with the change overall that keeps the "dynamic" settings management

LMESTM · 2017-09-12T11:14:00Z

libraries/SPI/src/SPI.cpp

+  if(idx == NB_SPI_SETTINGS) {
+    idx = addPin(_pin);
+    if(idx == NB_SPI_SETTINGS)
+      return;


this case here is not clear - is this an error ?

Yes this is an error because there is no more place available for a new CS pin.

LMESTM · 2017-09-12T11:14:41Z

libraries/SPI/src/SPI.cpp

-  UNUSED(_pin);
-  end();
+  if(_pin > NUM_DIGITAL_PINS)
+    return;


same as above

This is also an error because _pin must be a defined pin (presents in digitalPin[]).

LMESTM · 2017-09-12T11:16:12Z

libraries/SPI/src/SPI.cpp

+ * The transfer function can reconfigure the SPI instance if the CS pin is
+ * different from the previous one.
+ * If the _mode is set to SPI_CONTINUE, keep the spi instance alive. That means
+ * the CS pin is not reset. Becareful in case you use several CS pin.


Good comment thx. 1 typo: Becareful => Be careful

LMESTM · 2017-09-12T11:21:04Z

libraries/SPI/src/SPI.h

+
+// Defines a default timeout delay in seconds for the SPI transfer
+#define SPI_TRANSFER_TIMEOUT    5000
+


This seems not related to CS related change and should be in a different commit.
The timeout was hardcoded to 10000 before and now is defined as 5000.
Is it really 5000 seconds ?? Do we consider that timeout can be fixed, whatever the number of bytes and frequency ?

It is not 5000 seconds but 5000 milliseconds. I took advantage of the fix to standardize the timeout value.
This timeout doesn't change whatever the number of bytes or frequency.

The comment above the definition is
// Defines a default timeout delay in seconds for the SPI transfer
so you need to change that ...
How many bytes does it allow to send @ slowest supported frequency ?

Unless I am mistaken, we can send 78kbytes at slowest frequency = 125000.

ok that sounds like a reasonable number. Even 1 sec might be enough then (~15kBytes) ... as I don't know the max supported length ...

LMESTM · 2017-09-12T11:22:41Z

variants/DISCO_F100RB/variant.h

-#define BOARD_SPI_OWN_SS        SPI_CHANNELS_NUM
-
-#define SS                      BOARD_SPI_DEFAULT_SS
+#define SS                      43


Not related to this commit, but was is 43 ? can't we use a pin name like e.g. D10 or PB13 ?

See PR #83 for more information.

I understand that D43 is same as 43 and same as PX_y ...but D43 or 43 cannot be read anywhere on my board schematics or on the board itself, so I would prefer to use a commonly used names (either D0/D16 A0/A5 or PX_y ....

Agree that on VLdisco there is no Arduino connector and so using Dx is not well appropriate while all PXn names are on the board silkscreen print. But it's not the subject of this PR. Currently, variant.h using the same naming (Dx). Probably a new PR could be done to update all variants which do not have Arduino connector or those one using a pin number > 15.

I agree, that's why I started my comment with "Not related to this commit" ;-)

fpistm

PR ok. Waiting comment correction raised by @LMESTM before merge.

Signed-off-by: fpr <[email protected]>

fpistm self-requested a review September 6, 2017 07:07

fpistm reviewed Sep 6, 2017

View reviewed changes

fpistm added the bug 🐛 Something isn't working label Sep 6, 2017

fpr added 2 commits September 8, 2017 13:47

Move SPI library source files to src/ directory

076619a

Signed-off-by: fpr <[email protected]>

Update SPI library properties

d097a55

Signed-off-by: fpr <[email protected]>

fpistm reviewed Sep 12, 2017

View reviewed changes

fpistm requested review from cparata and LMESTM September 12, 2017 08:40

fpistm reviewed Sep 12, 2017

View reviewed changes

ghost requested a review from fpistm September 12, 2017 09:34

LMESTM approved these changes Sep 12, 2017

View reviewed changes

fpistm requested changes Sep 12, 2017

View reviewed changes

ghost requested a review from fpistm September 12, 2017 12:43

Fix SPI CS pin trouble if use CS pin other than Arduino digital pins

ac933c7

Signed-off-by: fpr <[email protected]>

fpistm approved these changes Sep 12, 2017

View reviewed changes

fpistm merged commit f1b576d into stm32duino:master Sep 12, 2017

ghost deleted the remove_spics_limitation branch September 19, 2017 08:00

fpistm mentioned this pull request Jun 6, 2023

gh pr list do not show some PR cli/cli#7538

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SPI CS pin trouble #100

Fix SPI CS pin trouble #100

ghost commented Sep 5, 2017

fpistm Sep 6, 2017

ghost Sep 6, 2017

fpistm Sep 6, 2017

fpistm commented Sep 6, 2017

LMESTM commented Sep 6, 2017

ghost commented Sep 6, 2017

LMESTM commented Sep 6, 2017

LMESTM commented Sep 6, 2017

ghost commented Sep 6, 2017

fpistm Sep 12, 2017

fpistm Sep 12, 2017

fpistm Sep 12, 2017

fpistm Sep 12, 2017

LMESTM left a comment

LMESTM Sep 12, 2017

ghost Sep 12, 2017

LMESTM Sep 12, 2017

ghost Sep 12, 2017

LMESTM Sep 12, 2017

LMESTM Sep 12, 2017

ghost Sep 12, 2017

LMESTM Sep 12, 2017

ghost Sep 12, 2017

LMESTM Sep 12, 2017 •

edited

Loading

LMESTM Sep 12, 2017

fpistm Sep 12, 2017

ghost Sep 12, 2017

LMESTM Sep 12, 2017

fpistm Sep 12, 2017

LMESTM Sep 12, 2017

fpistm left a comment


		// Defines a default timeout delay in seconds for the SPI transfer
		#define SPI_TRANSFER_TIMEOUT 5000

Fix SPI CS pin trouble #100

Fix SPI CS pin trouble #100

Conversation

ghost commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpistm commented Sep 6, 2017

LMESTM commented Sep 6, 2017

ghost commented Sep 6, 2017

LMESTM commented Sep 6, 2017

LMESTM commented Sep 6, 2017

ghost commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LMESTM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LMESTM Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpistm left a comment

Choose a reason for hiding this comment

LMESTM Sep 12, 2017 •

edited

Loading