Check for valid pin properly (fixes #4605)

[cousteaulecommandant]

Current behavior: look up pin number in digital_pin_to_port_PGM[] and then check if it magically returned 0.
Proposed behavior (from issue #4605): check if pin is within the valid range, and THEN look it up. Don't check afterwards.

IMPORTANT NOTE: This change assumes that digital_pin_to_port_PGM[] doesn't skip any pin, which until now could have been done by assigning NOT_A_PIN to the position to skip. If it is planned to have boards where the pin numbering behaves like this, the if (port == NOT_A_PIN) return; part should be kept as well (assuming that NUM_DIGITAL_PINS will count the "holes" too).

[mikaelpatel]

Seems more than pinMode that had the same issue. Good job!

[sandeepmistry]

@cousteaulecommandant thanks for submitting.

Do you have an example of how this improves things from a user perspective?

Some other notes:

• The (port == NOT_A_PIN) should probably be left in, 3rd party variants may have "holes".
• There will also be a (small) performance hit at runtime, as well as increase in code size if an additional check is added. So, we should make sure the advantages of introducing this change outweigh the disadvantages.

[NicoHood]

I also think that this is not a good fix.
Since a board may have lets say 10 pins. And another board has only 8 of them broken out (aka number of pins is 8) but the number of the actual pin is still possible to be higher then 8 (if 3&4 are not broken out.

And while the check @sandeepmistry mentioned still has to be there, this just adds more code, not less.

[cousteaulecommandant]

There will also be a (small) performance hit at runtime, as well as increase in code size if an additional check is added. So, we should make sure the advantages of introducing this change outweigh the disadvantages.

Well, right now the check doesn't work at all; it doesn't provide any useful protection against invalid pins. So if code size is an issue I'd just remove both checks. And if I had to choose I'd prefer this check to the other.

Also, I think that an alternative to having the second check (the one I removed) is that boards with "holes" leave a valid port in digital_pin_to_port_PGM (for example, the previous or next one), but store a 0 in the corresponding digitalPinToBitMask instead of _BV(x). This way the rest of the function will still run, setting/clearing no bits on the corresponding registers, and returning LOW for digitalWrite :) (This makes the code simpler, the execution slightly faster for valid pins, and slower but still valid for invalid pins)

@NicoHood I'm not sure I understood, but if what you mean is that there might be boards where the highest pin is larger than NUM_DIGITAL_PINS-1 (for example if NUM_DIGITAL_PINS doesn't count holes) then I guess the only way around would be to add another macro.

Or just use sizeof digital_pin_to_port_PGM / sizeof *digital_pin_to_port_PGM instead. Should I do that?

[NicoHood]

Your suggestion still does not work. It does still not count holes. I guess the only real working check is the one we have right now. And I see no clue why the addition could help in any way, rather than breaking stuff.

[cousteaulecommandant]

Rewrote this pull request as #4649 since it needed some editions but I had deleted the branch.