Go Down

Topic: pinMode() function "defaults" to OUTPUT; should be INPUT (IMHO) (Read 749 times) previous topic - next topic

DuinoSoar

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 == INPUTOUTPUT) {
    uint8_t oldSREG = SREG;
    cli();
    *reg &= ~bit;
    *out &= ~bit;
    *reg |= 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;
    *reg &= ~bit;
    *out &= ~bit;

    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.
"A common mistake that people make when trying to design something completely foolproof is to underestimate the ingenuity of complete fools." from _Mostly Harmless_ by Douglas Adams (1952 - 2001)

pert

I am assuming that this is THE implementation of pinMode.
Only for Arduino AVR Boards. The core for each architecture will have its own definition of pinMode.

Also, note that if you install a different version of Arduino AVR Boards via Tools > Board > Boards Manager, it is installed to a different location on your computer (C:\Users\{user name}\AppData\Local\Arduino15\packages\arduino\hardware\avr on Windows) and the wiring_digital.c in the Arduino IDE installation folder will no longer be used.

Should this be considered for the next release of Arduino software?
I think it's a good proposal. The people who make these decisions for Arduino will likely not see this here on the forum (though this is a good place to get feedback from the community on your proposal). If you want to move this forward, the way to proceed is to submit a pull request to the Arduino AVR Boards repository:
https://github.com/arduino/ArduinoCore-avr
If accepted, the change should be made in all cores eventually, but Arduino AVR Boards can act as a model for the others.

The concern would be that some code relies on the long-established current behavior. I can't imagine any sane person relying on a specific behavior for an undefined pin mode so I can't imagine there would be much of that sort of code out there, if any.

DuinoSoar

Thanks for the feedback, Pert.

I do believe that this should "move forward" because of the reasons I mentioned above: it can be damaging to external circuitry if a pin mode is unintentionally set to OUTPUT.

The problem with your suggestion is that I am not a GitHub user, I do not have a GitHub user ID (or account, or whatever), and I have no idea how to "pull a request" (or whatever is the proper terminology).

Funny, though.  I would have thought that the Arduino.cc forums would be THE place to go for suggestiosn/requests like this.  Why should users have to go to some different web site to support or get support for Arduino, when we have a perfectly good forum right here at Arduino.cc?  That just does not make any sense at all.

Anyway, thanks again.
  DuinoSoar.
"A common mistake that people make when trying to design something completely foolproof is to underestimate the ingenuity of complete fools." from _Mostly Harmless_ by Douglas Adams (1952 - 2001)

pert

The problem with your suggestion is that I am not a GitHub user, I do not have a GitHub user ID (or account, or whatever), and I have no idea how to "pull a request" (or whatever is the proper terminology).
I understand. However, GitHub is the go to place for collaboration on open source projects. It's really an awesome website. Accounts are free and it's very fast to get signed up. There is a little bit of a learning curve to using GitHub but it's all very well documented and there are plenty of people here (including myself) who can answer any questions you might have. The main part of learning to use GitHub is learning to use Git, which is the most popular open source version control. Once you get into any relatively complex Arduino projects you're going to want to learn to use Git anyway. GitHub actually provides an extremely simplified way to use Git via their website, so it's the easiest way to get started using Git. The effort to figure out how to make a pull request will be less than it took you to hunt down the definition of pinMode and put the colored text on your forum post.

Funny, though.  I would have thought that the Arduino.cc forums would be THE place to go for suggestiosn/requests like this.  Why should users have to go to some different web site to support or get support for Arduino, when we have a perfectly good forum right here at Arduino.cc?  That just does not make any sense at all.
You're not the first to be frustrated by this. This is just the system we have to work in. The way I see it is that the forum is the first level of triage. Here, the community can point out obvious flaws, etc. in any proposals and help to polish them before they move to the next level on GitHub, where they will take up the time of the Arduino developers. This allows the developers to spend more of their time making improvements to the software, which we all benefit from.

Whandall

Changing the default from OUTPUT to INPUT still changes the pin mode on illegal parameters.

IMHO it would be better to use

Code: [Select]
  } else if (mode == OUTPUT) {
and ignore the illegal request, just like it is done with NOT_A_PIN.
Ah, this is obviously some strange usage of the word 'safe' that I wasn't previously aware of. (D.Adams)

Go Up