SPI library question

[Victor Aprea]

Hi all-

I'm not sure whether to call this a bug or not (I'm leaning toward not), but it "surprised" me a little bit this morning (glad to see Arduino can still do that for me hehe).

In the AVR implementation of SPIClass::begin these are the first few lines.

  // Set SS to high so a connected chip will be "deselected" by default
  digitalWrite(SS, HIGH);
  // When the SS pin is set as OUTPUT, it can be used as
  // a general purpose output port (it doesn't influence
  // SPI operations).
  pinMode(SS, OUTPUT);

I get why the library turns the pin to an output (so that it doesn't go into SPI Slave mode inadvertantly). But why does it bother to change the pin state. This surprised me because I externally (in setup) initialized this pin to an output set low, and it got flipped by a subsequent SPI.begin to high. That's not catastrophic for me, but I must admit it was unexpected.

I might argue that neither of these lines is the responsibility of the SPI library, but I won't go there. I'd just like to better understand the upside to the first line if possible. An acceptable answer to me is that "it's a legacy behavior, and that nobody knows what existing code would break if we took it out, so just leave it alone." But if it's truly indispensable, I have a purely academic curiosity in understanding the reason for it. 

Kind Regards,

Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

T: @vicatcu

[Cristian Maglie]

Il 06/02/2015 17:23, Victor Aprea ha scritto:
> In the AVR implementation of SPIClass::begin these are the first few lines.
>
> // Set SS to high so a connected chip will be "deselected" by default
> digitalWrite(SS, HIGH);
> // When the SS pin is set as OUTPUT, it can be used as
> // a general purpose output port (it doesn't influence
> // SPI operations).
> pinMode(SS, OUTPUT);
>
>
> I get why the library turns the pin to an output (so that it doesn't go
> into SPI Slave mode inadvertantly). But why does it bother to change the
> pin state. This surprised me because I externally (in setup) initialized
> this pin to an output set low, and it got flipped by a subsequent
> SPI.begin to high. That's not catastrophic for me, but I must admit it
> was unexpected.

Hi Victor,

I guess that the word 'legacy' is the correct answer to this one.

Looking back at the git log, on my first (very simple) implementation I
choose to set the pin LOW (I've probably borrowed that looking at other
SPI implementations I really don't remember):

https://github.com/arduino/Arduino/commit/e24b135755aee24d1eca1122a736cec20aee129d#diff-faa862c2e8f5c27d08bfb2b40c67a86aR31

afterwards David M. changed the set the default to HIGH:

https://github.com/arduino/Arduino/commit/1e0f968387f34c12401dada9e4b27d7123518a6b

because, as he explained in the commit log, "this should prevent
conflicts between an SPI device using the hardware SS pin (which
previously would have been enabled by default) and another SPI device
using another pin for its SS. It might be better to move the SPI
initialization to begin(), which could then be called by the hardware
devices which could then disable themselves"

Probably the correct way to handle this is to enable pullup on SS pin if
and only if the pin is not already set as output.

C

[Victor Aprea]

Christian,

Interesting, thanks for reviewing the origins! I guess my personal opinion is that there's no objective reason for the SPI library to affect the state of the pin beyond setting it up as an output. If setting it to some value is important, high does seem a more sensible choice, as David M. pointed out in the commit log, in the case where the Arduino is actually using the SS pin to select an external SPI device, but that is a pretty big conjecture for the SPI library to make since any GPIO pin can be used as a chip select for an external SPI device. 

My recollection is that an AVR has to have the SS pin set to an output prior to SPI hardware initialization in order to prevent it from going into SPI slave mode when the SPI hardware is engaged. If it were left as an input, bad things could happen like undefined ISR's firing and so forth. Setting the pin to some particular state (high or low) seems opinionated to me. In particular when a user's application might use the SS pin as a general purpose output, which I believe is totally legitimate usage, even in the presence of active SPI usage. I don't feel too strongly about it though, and am happy to accept legacy behavior.

In conclusion, SPI.begin() should only theoretically be called once in a sketch anyway, or at least it should not be called after setup runs, again theoretically. Recognizing that pin configuration/initialization of the SS pin should occur after all possible calls to SPI.begin (e.g. in library inits) would be a good thing; it might be a worthwhile point to explicitly call out in documentation of SPI. Some section like "caveats and pitfalls surrounding the use of the SS pin as a general purpose digital output." Or perhaps it's too subtle of a point and would just muddy the docs, not sure.

Kind Regards,

Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

T: @vicatcu

C


--
You received this message because you are subscribed to the Google Groups "Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to developers+unsubscribe@arduino.cc.

[Cristian Maglie]

Il 11/02/2015 23:25, Victor Aprea ha scritto:
> My recollection is that an AVR has to have the SS pin set to an output
> prior to SPI hardware initialization in order to prevent it from going
> into SPI slave mode when the SPI hardware is engaged. If it were left as
> an input, bad things could happen like undefined ISR's firing and so
> forth. Setting the pin to some particular state (high or low) seems
> opinionated to me. In particular when a user's application might use the
> SS pin as a general purpose output, which I believe is totally
> legitimate usage, even in the presence of active SPI usage. I don't feel
> too strongly about it though, and am happy to accept legacy behavior.

I see two cases to solve this:

- The user uses the SS pin as a GPIO output: in this case we can just
leave the pin setup unchanged.
- The user do not use the SS pin: in this case enabling the internal
pull up should be enough (or set the pin as output high as it is now).

so, as I said, the correct solution to me for SPI.begin() is to enable
pullup on SS pin (or set it as output high) if *and only if* the pin is

[Paul Stoffregen]

> - The user uses the SS pin as a GPIO output: in this case we can just
> leave the pin setup unchanged.
> - The user do not use the SS pin: in this case enabling the internal
> pull up should be enough (or set the pin as output high as it is now).

Perhaps only somewhat related to what should be done in software is what
ought to be done in hardware.

For Arduino shields and "breakout boards" sold to hobbyists and
non-engineers, a physical pullup resistor should always be placed on all
SPI chip select lines.

Some of Arduino's products fail to do this. Lack of real pullup
resistors creates incredibly difficult-to-understand problems for novices.

For example, with the Arduino Ethernet Shield, if the user runs any of
the Ethernet library examples while a SD card happens to be placed in
shield's SD socket, they sometimes mysteriously fail. The SD card's
chip select can "float" to any voltage and it sometimes happens to be
logic low, causing the SD card to hear and sometimes respond by driving
its MISO pin, while the Ethernet library is trying to use the W5100 chip!

If anyone responsible for Arduino's hardware design reads this, please,
I beg of you to consider revising your designs. Real pullup resistors
ought to be placed on those chip select lines, regardless of whatever is
done in the software.

[Victor Aprea]

Christian,

What do you think of this patch: https://github.com/vicatcu/Arduino/commit/7c3372c9a64a764b074f1d28552d27ebae4fb0d6

If you like it, I'll submit a pull request. Please let me know.

Kind Regards,

Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

T: @vicatcu

C

[David Cuartielles]

Paul: noted :-)El 15/02/2015 21:05, Paul Stoffregen <pa...@pjrc.com> escribió:
>
>
> > - The user uses the SS pin as a GPIO output: in this case we can just
> > leave the pin setup unchanged.
> > - The user do not use the SS pin: in this case enabling the internal
> > pull up should be enough (or set the pin as output high as it is now).
>

> Perhaps only somewhat related to what should be done in software is what
> ought to be done in hardware.
>
> For Arduino shields and "breakout boards" sold to hobbyists and
> non-engineers, a physical pullup resistor should always be placed on all
> SPI chip select lines.
>
> Some of Arduino's products fail to do this.  Lack of real pullup
> resistors creates incredibly difficult-to-understand problems for novices.
>
> For example, with the Arduino Ethernet Shield, if the user runs any of
> the Ethernet library examples while a SD card happens to be placed in
> shield's SD socket, they sometimes mysteriously fail.  The SD card's
> chip select can "float" to any voltage and it sometimes happens to be
> logic low, causing the SD card to hear and sometimes respond by driving
> its MISO pin, while the Ethernet library is trying to use the W5100 chip!
>
> If anyone responsible for Arduino's hardware design reads this, please,
> I beg of you to consider revising your designs.   Real pullup resistors
> ought to be placed on those chip select lines, regardless of whatever is
> done in the software.
>

> --
> You received this message because you are subscribed to the Google Groups "Developers" group.

> To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.

[Matthijs Kooijman]

Hey Christian,

> - The user uses the SS pin as a GPIO output: in this case we can
> just leave the pin setup unchanged.
> - The user do not use the SS pin: in this case enabling the internal
> pull up should be enough (or set the pin as output high as it is
> now).

I was going to say your second point is invalid, because the pin has to
be an output for master mode, but looking up things in the datasheet
shows that you are indeed correct. The datasheet says that whem master
mode is enabled:

If SS is configured as an output, the pin is a general output
pin which does not affect the SPI system. Typically, the pin
will be driving the SS pin of the SPI Slave.

If SS is configured as an input, it must be held high to ensure Master SPI operation. If the SS pin is driven low by
peripheral circuitry when the SPI is configured as a Master with the SS pin defined as an input, the SPI system
interprets this as another master selecting the SPI as a slave and starting to send data to it. To avoid bus
contention, the SPI system takes the following actions:
[snipped, essentially it switches to slave mode]

> so, as I said, the correct solution to me for SPI.begin() is to
> enable pullup on SS pin (or set it as output high) if *and only if*
> the pin is not already set as output.

That seems correct to me. Using the pullup is probably preferred - if
somehow the pin is externally connected to ground, the only effect is
that SPI stops working, instead of burning out the pin in a
short-circuit.

Gr.

Matthijs

[Victor Aprea]

I think this is probably entirely obvious to all who are participating in this thread thus far, but the only additional change to what I proposed in order to only enable the internal pull-up instead of output high would be to also delete the following line after the section I modified:

pinMode(SS, OUTPUT);

I'll wait for something resembling consensus / a decision from Christian before making that change and submitting a pull request. The only thing I anticipate any conflict over would be sketches / examples that rely on the side-effects of the current implementation. 

Kind Regards,

Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

T: @vicatcu

--
You received this message because you are subscribed to the Google Groups "Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.

[Cristian Maglie]

Il 16/02/2015 15:44, Victor Aprea ha scritto:
> I'll wait for something resembling consensus / a decision from Christian
> before making that change and submitting a pull request. The only thing
> I anticipate any conflict over would be sketches / examples that rely on
> the side-effects of the current implementation.

Hi Victor,

your implementation looks good, go on with the pull request.

C

--
Cristian Maglie <c.ma...@arduino.cc>

[Victor Aprea]

Christian,

Thanks, with or without removal of the subsequent pinMode(SS, OUTPUT);

Kind Regards,

Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

M: 607.745.1515

T: @vicatcu

--
You received this message because you are subscribed to the Google Groups "Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to developers+unsubscribe@arduino.cc.

[Matthijs Kooijman]

Hey Christian,

> your implementation looks good, go on with the pull request.

Wasn't your proposal to also only do the digitalWrite (to set the
pullup) when the mode is still input?

Gr.

Matthijs

[Victor Aprea]

Matthijs,

I think that's exactly what my modification aims to do. Please review here (I have yet to submit a pull request). The only question I have is whether the subsequent call to pinMode should also be removed.

The pseudo-code logic goes:

If you are NOT an output

   call digitalWrite(SS, HIGH); // to enable the internal pull-up resistor

End If

The actual modified implementation goes:

    uint8_t port = digitalPinToPort(SS);
    uint8_t bit = digitalPinToBitMask(SS);
    volatile uint8_t *reg = portModeRegister(port);

    // if the SS pin is not already configured as an output
    // then set it high
    if(!(*reg & bit)){
      digitalWrite(SS, HIGH);
    }

The existing code right after that sets SS to an output in these lines:

    // When the SS pin is set as OUTPUT, it can be used as
    // a general purpose output port (it doesn't influence
    // SPI operations).
    pinMode(SS, OUTPUT);

The outstanding question is whether those lines should be also removed. And also, to peer review my code to confirm it does what I say it does :-).

Kind Regards,

Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

T: @vicatcu

[Matthijs Kooijman]

Hey Victor,

> I think that's exactly what my modification aims to do. Please review here

> <https://github.com/vicatcu/Arduino/commit/7c3372c9a64a764b074f1d28552d27ebae4fb0d6>

> (I
> have yet to submit a pull request). The only question I have is whether the

> subsequent call to pinMode should *also* be removed.
Ah, I missed that. In that case, ignore my comment and carry on :-)

Gr.

Matthijs

[Cristian Maglie]

Il 16/02/2015 16:14, Victor Aprea ha scritto:
> Thanks, with or without removal of the subsequent pinMode(SS, OUTPUT);

Hi Victor,

sorry for the delay in answering this one...

Let's go without the pinMode, so we can test it.

C

[Peter Olson]

> On February 17, 2015 at 1:30 PM Victor Aprea <victor...@wickeddevice.com>

> wrote:

> Matthijs,
>
> I think that's exactly what my modification aims to do. Please review here

> <https://github.com/vicatcu/Arduino/commit/7c3372c9a64a764b074f1d28552d27ebae4fb0d6>

> (I
> have yet to submit a pull request). The only question I have is whether the

> subsequent call to pinMode should *also* be removed.

>
> The pseudo-code logic goes:
>
> If you are NOT an output
> call digitalWrite(SS, HIGH); // to enable the internal pull-up resistor
> End If

Note that if you want the internal pullup to be activated, you need pinMode
INPUT_PULLUP rather than INPUT. (This changed in Arduino 1.0.5 or so.)

Peter Olson

[Victor Aprea]

Peter, 

Thanks for the input. It's kind of a dirty little trick, but the following combination of calls is functionally equivalent to pinMode(x, INPUT_PULLUP);

pinMode(x, INPUT);

digitalWrite(x, HIGH);

Arduino 1.0.5 didn't obsolete the old way (afaik), it just gave a more intuitive way of expressing it... at the end of the day the effect just needs to be something like thollowing (for Uno):

DDRB &= ~_BV(2); // set port b.2 to an input (this is what pinMode(10, INPUT) does ultimately)
PORTB |= _BV(2); // enable pullup (this is what digitalWrite(10, HIGH) does ultimately)

pinMode(x, INPUT_PULLUP) is syntactic sugar for the above.

Regards,
Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

M: 607.745.1515

T: @vicatcu

[Victor Aprea]

Christian, 

I've submitted the pull request for testing here: https://github.com/arduino/Arduino/pull/2659

Kind Regards,

Vic

Victor Aprea // Wicked Device

victor...@wickeddevice.com

M: 607.745.1515

T: @vicatcu

[Peter Olson]

> On February 17, 2015 at 4:36 PM Victor Aprea <victor...@wickeddevice.com>

> wrote:

> Thanks for the input. It's kind of a dirty little trick, but the following
> combination of calls is functionally equivalent to pinMode(x, INPUT_PULLUP);
>
> pinMode(x, INPUT);
> digitalWrite(x, HIGH);
>

That may be true, but the documentation says otherwise:

http://arduino.cc/en/Tutorial/DigitalPins

Prior to Arduino 1.0.1, it was possible to configure the internal
pull-ups in the following manner:

pinMode(pin, INPUT); // set pin to input
digitalWrite(pin, HIGH); // turn on pullup resistors

I recall this problem because I got bit by it upgrading previously working code
to 1.0.5 (so I guess I remember the wrong release because I must have been using
1.0 previously).

I think you still need to do this:

pinMode(pin, INPUT_PULLUP); // turn on pullup resister
digitalWrite(pin, HIGH); // let output go high

The problem may be that subsequently calling pinMode(pin, INPUT) explicitly
shuts off the pullup on-chip. So if you have written your code with unnecessary
calls to pinMode(..., INPUT), not knowing about INPUT_PULLUP, your sketch will
malfunction.

Peter Olson

[Vasilis Georgitzikis]

Afaik, either of the two works:
pinMode(13, INPUT_PULLUP);

or

pinMode(13, INPUT);
digitalWrite(13, HIGH);

Vasilis

[Matthijs Kooijman]

Hey folks,

> Afaik, either of the two works:
> pinMode(13, INPUT_PULLUP);
>
> or
>
> pinMode(13, INPUT);
> digitalWrite(13, HIGH);

I haven't double-checked, but perhaps that this way doesn't work for the
Due, so the documenation recommends to use INPUT_PULLUP? I'm pretty sure
this way still works on AVR on all current Arduino versions.

Gr.

Matthijs