Purpose of Sd2PinMap.h ?

111 views
Skip to first unread message

Phillip Stevens

unread,
Feb 12, 2016, 6:59:46 AM2/12/16
to Developers
As part of searching out for compatibility concerns for the ATmega1284p device,
I've found that there is pin mapping contained in SD2PinMap.h relates to a VERY limited set of Arduino hardware platforms.

~/Arduino/libraries/SD/src/utility/Sd2PinMap.h

The data seems to approximately replicate the information contained in pins_arduino.h for each Variant.

I can't find anywhere that uses the functions or data defined in this file, except for SD2Card.cpp, and then only the standard SPI defines.
Am I missing something? Or, is this file just there for legacy compatibility?

Thanks for any feedback on whether this is worth fixing, or can be safely ignored.

Massimo Banzi

unread,
Feb 12, 2016, 7:01:50 AM2/12/16
to Arduino Developers
Phillip

This library predates some of the improvements we added to the core in the last years..

Feel free to propose any refactoring that cam modernise the code

m




--
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.

Phillip Stevens

unread,
Feb 12, 2016, 8:08:19 AM2/12/16
to Developers
Done. For discussion.


This commit removes the #include "Sd2PinMap.h" from Sd2Card.h
and redefines the SD SPI interface to pin names provided by "pins_arduino.h"

I am not sure whether preferred style is to include "Arduino.h" or "pins_arduino.h".
The commit includes pins_arduino.h as it is sufficient for the purpose, but both work.

The legacy file Sd2PinMap.h should also be removed as it is no longer relevant.

Massimo Banzi

unread,
Feb 12, 2016, 8:16:42 AM2/12/16
to Arduino Developers
Thanks for the update.

We would need somebody to test it on multiple boards before we accept the patch officially
(any takers in exchange for a t-shirt or some swag?)



> I am not sure whether preferred style is to include "Arduino.h" or "pins_arduino.h".



Cristian what is your preference?



m

Cristian Maglie

unread,
Feb 12, 2016, 9:17:13 AM2/12/16
to devel...@arduino.cc
Il 12/02/2016 12:59, Phillip Stevens ha scritto:
> I can't find anywhere that uses the functions or data defined in this
> file, except for SD2Card.cpp, and then only the standard SPI defines.
> Am I missing something? Or, is this file just there for legacy
> compatibility?

That's not legacy, it's currently used for software emulating SPI on
pins where a real SPI master is not available. If you are wondering,
someone else already tried to convert the SD library into a
"board-agnostic SD library" using the macros from pins_arduino.h...

https://github.com/arduino/Arduino/pull/121

unfortunately this patch has a very bad impact on performance when using
software-SPI, see a bit more of discussion here:
https://github.com/arduino/Arduino/commit/05a9750747fcc3d5218acba8ae6a4fdacc6d017a#commitcomment-15657902

BTW I think that we can use your patch to at least use pins_arduino.h
when using the real hardware SPI, the only doubt is that removing the
#include "Sd2PinMap.h" will make the build fail when SOFTWARE_SPI is
enabled.

May you try to build the library SOFTWARE_SPI is defined? A very common
case is changing #define MEGA_SOFT_SPI 0 to 1 and building for the
Mega2560.


C

Victor Aprea

unread,
Feb 12, 2016, 9:57:04 AM2/12/16
to Arduino Developers
Cristian,

Pull request 121 looks fundamentally sound to me logically, but could be substantially improved (i.e. performance) by "caching' the results of portModeRegister(digitalPinToPort(pin)), portInputRegister(digitalPinToPort(pin)), portOutputRegister(digitalPinToPort(pin)), and digitalPinToBitMask(pin) in static class member variables at the point where pin gets assigned, and then using the (one-time) computed values. Surely that would execute much more quickly than laboriously looking up the (effectively) runtime constants every time. What do you think?

Kind Regards,
Vic

Victor Aprea // Wicked Device



C

Paul Stoffregen

unread,
Feb 12, 2016, 10:25:35 AM2/12/16
to devel...@arduino.cc
That's still a big performance hit, and it (probably) also needs interrupt disable to be safe on AVR when libraries like Servo are used.

Phillip Stevens

unread,
Feb 12, 2016, 10:37:43 PM2/12/16
to Developers
I have closed the pull request, as I didn't understand the software emulated SPI application.

Looking at the code for the _arm_ Due board found in the Sd2PinMap.h



#if defined(__arm__) // Arduino Due Board follows

#ifndef Sd2PinMap_h
#define Sd2PinMap_h

#include <Arduino.h>

uint8_t
const SS_PIN = SS;
uint8_t
const MOSI_PIN = MOSI;
uint8_t
const MISO_PIN = MISO;
uint8_t
const SCK_PIN = SCK;

#endif // Sd2PinMap_h


I think that something similar should be in Sd2Card.h, referencing Arduino.h
(so that either Variant.h or pins_arduino.h gets included as appropriate for the architecture).
Something like below, for discussion.

//------------------------------------------------------------------------------
// SPI pin definitions
//
#ifndef SOFTWARE_SPI
// hardware pin defs

#include <Arduino.h>

/**
 * SD Chip Select pin
 *
 * Warning if this pin is redefined the hardware SS will pin will be enabled
 * as an output by init().  An avr processor will not function as an SPI
 * master unless SS is set to output mode.
 */

/** The default chip select pin for the SD card is SS. */
uint8_t
const  SD_CHIP_SELECT_PIN = SS;
// The following three pins must not be redefined for hardware SPI.
/** SPI Master Out Slave In pin */
uint8_t
const  SPI_MOSI_PIN = MOSI;
/** SPI Master In Slave Out pin */
uint8_t
const  SPI_MISO_PIN = MISO;
/** SPI Clock pin */
uint8_t
const  SPI_SCK_PIN = SCK;
/** optimize loops for hardware SPI */
#ifndef USE_SPI_LIB
#define OPTIMIZE_HARDWARE_SPI
#endif



Having this code would reference the wider list of pins_arduino.h layouts, where hardware _avr_ hardware SPI is used, and that would be enough most cases.
Fixing software SPI is too hard for me, and also not that important.

If that is sufficient, then I'll make a simple pull request for the above code.
 
On Fri, Feb 12, 2016 at 9:17 AM, Cristian Maglie <c.ma...@arduino.cc> wrote:
> Am I missing something? Or, is this file just there for legacy
> compatibility?

That's not legacy, it's currently used for software emulating SPI on
pins where a real SPI master is not available.

BTW I think that we can use your patch to at least use pins_arduino.h
when using the real hardware SPI.

Phillip Stevens

unread,
Feb 18, 2016, 11:08:35 PM2/18/16
to Developers
I opened a new pull request #4576 that only applies when using hardware SPI.

The change references pins_arduino.h or variant.h via the Arduino.h file. That is the most architecture neutral way to do it, I think.
This ensures that all of the devices that provide a pins_arduino.h or variant.h file will be compatible with the SPI hardware interface.

No change to the software SPI implementation.
Reply all
Reply to author
Forward
0 new messages