G'day, folks.
I was looking over the library source code at how the Arduino digital pin manipulation functions work, and found the pinMode() function definition in the wiring_digital.c file. I am assuming that this is THE implementation of pinMode. (I used the "Agent Ransack" file search tool, and could not find any other #define or function implementation of pinMode in the source code included with Arduino 1.8.5.) If I am wrong, somebody please feel free to correct me and point out where any different implementation of pinMode would override the one I found in wiring_digital.c.
Following is the function definition of pinMode(), copied verbatim from wiring_digital.c:
void pinMode(uint8_t pin, uint8_t mode)
{
uint8_t bit = digitalPinToBitMask(pin);
uint8_t port = digitalPinToPort(pin);
volatile uint8_t *reg, *out;
if (port == NOT_A_PIN) return;
// JWS: can I let the optimizer do this?
reg = portModeRegister(port);
out = portOutputRegister(port);
if (mode == INPUT) {
uint8_t oldSREG = SREG;
cli();
*reg &= ~bit;
*out &= ~bit;
SREG = oldSREG;
} else if (mode == INPUT_PULLUP) {
uint8_t oldSREG = SREG;
cli();
*reg &= ~bit;
*out |= bit;
SREG = oldSREG;
} else {
uint8_t oldSREG = SREG;
cli();
*reg |= bit;
SREG = oldSREG;
}
}
Something bothers me about this implementation: the "default" mode is OUTPUT (i.e. if any value except INPUT or INPUT_PULLUP was passed to the mode parameter).
This is fine if the sketch author calling pinMode does not make any mistake.
But suppose the author uses a uint8_t (or byte) variable as the mode argument of the call to pinMode(), in order to dynamically switch modes between, say, INPUT and INPUT_PULLUP (but never intends it to be OUTPUT). Suppose further that this mode variable somehow gets corrupted (e.g. by a bad pointer reference or an array over-run) and is erroneously set to any value other than INPUT or INPUT_PULLUP. Then the mode will "default" to OUTPUT, and possibly damage external circuitry.
In my opinion (be it ever so humble ), the "default" value, handled by the last "else" clause of the pinMode() function, should be "INPUT" (high-impedance), and the previous, specific conditional clauses (the "if" and "else if" clauses) should specifically handle OUTPUT and INPUT_PULLUP.
That way, if the passed-in mode argument is "out of range", the pin mode will, at least, "default" to high-impedance input, and would be less likely to cause damage to external circuitry.
(Note: In the above scenario, there would still, of course, be a 1 in 256 chance that the corrupted mode variable happens to equal the valid OUTPUT value, and still possibly damage external circuitry. But this, I think, would still be much less likely to occur, than if the OUTPUT mode was handled as the "out-of-range" mode default.)
I would have written the pinMode() function like the following (changed parts shown with original code in red strike-through and new code in green):
void pinMode(uint8_t pin, uint8_t mode)
{
uint8_t bit = digitalPinToBitMask(pin);
uint8_t port = digitalPinToPort(pin);
volatile uint8_t *reg, *out;
if (port == NOT_A_PIN) return;
// JWS: can I let the optimizer do this?
reg = portModeRegister(port);
out = portOutputRegister(port);
if (mode == [color=red][s]INPUT[/s][/color][color=green]OUTPUT[/color]) {
uint8_t oldSREG = SREG;
cli();
[color=red][s]*reg &= ~bit;[/s][/color]
[color=red][s]*out &= ~bit;[/s][/color]
[color=green]*reg |= bit;[/color]
SREG = oldSREG;
} else if (mode == INPUT_PULLUP) {
uint8_t oldSREG = SREG;
cli();
*reg &= ~bit;
*out |= bit;
SREG = oldSREG;
} else {
uint8_t oldSREG = SREG;
cli();
[color=red][s]*reg |= bit;[/s][/color]
[color=green]*reg &= ~bit;
*out &= ~bit;[/color]
SREG = oldSREG;
}
}
Should this be considered for the next release of Arduino software?
I have another question about pinMode. I understand why the assignments to oldSREG and SREG are placed inside each individual if/else clause, instead of just once before and after all the clauses. (I believe they are inside each clause, in order to keep the length of the critical code down to a minimum, for each case.) But I am wondering (I really do not know) if the compiler builds and tears down the additional byte (for oldSREG) in the stack frame for each clause? Would it be more efficient, in terms of compiled code size, if the declaration:
uint8_t oldSREG;
were put at the top of pinMode(), and still keep the individual assignments:
oldSREG = SREG;
in each if/else clause just before the cli() calls?
Thanks and best regards,
DuinoSoar.