Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added bounds checking on pinMode #302

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added bounds checking on pinMode #302

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d4c261
Added bounds to checking in digitalPinTo* functions
BarryPSmith Nov 30, 2019
7b336e1
Removed superflous consts. Fixed digital_pin_count checks.
BarryPSmith Dec 13, 2019
2b74a1e
Removed unnecessary declaration from pins_arduino.h
BarryPSmith Dec 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions cores/arduino/Arduino.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,24 +169,51 @@ extern const uint8_t PROGMEM digital_pin_to_port_PGM[];
// extern const uint8_t PROGMEM digital_pin_to_bit_PGM[];
extern const uint8_t PROGMEM digital_pin_to_bit_mask_PGM[];
extern const uint8_t PROGMEM digital_pin_to_timer_PGM[];
extern const uint8_t digital_pin_count;

#define NOT_A_PIN 0
#define NOT_A_PORT 0

#define NOT_AN_INTERRUPT -1

// Get the bit location within the hardware port of the given virtual pin.
// This comes from the pins_*.c file for the active board configuration.
//
// These perform slightly better as macros compared to inline functions
//
#define digitalPinToPort(P) ( pgm_read_byte( digital_pin_to_port_PGM + (P) ) )
#define digitalPinToBitMask(P) ( pgm_read_byte( digital_pin_to_bit_mask_PGM + (P) ) )
#define digitalPinToTimer(P) ( pgm_read_byte( digital_pin_to_timer_PGM + (P) ) )
//
#define analogInPinToBit(P) (P)
#define portOutputRegister(P) ( (volatile uint8_t *)( pgm_read_word( port_to_output_PGM + (P))) )
#define portInputRegister(P) ( (volatile uint8_t *)( pgm_read_word( port_to_input_PGM + (P))) )
#define portModeRegister(P) ( (volatile uint8_t *)( pgm_read_word( port_to_mode_PGM + (P))) )

#define NOT_A_PIN 0
#define NOT_A_PORT 0

#define NOT_AN_INTERRUPT -1
//
// bounds checking on these functions because they return a pointer to an address that will be modified.
// An out of bounds write can result in stack corruption and fun to track-down errors.
//
static inline const uint8_t digitalPinToPort(const uint8_t P) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for using static and inline at the same time?

If a function is inlined, then there should be no symbol for it, so there should be no link time issue. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline is only a hint, the compiler can choose to ignore it and often does for large enough functions. If the static was not there, I believe the linker will (may?) complain about duplicate definitions of digitalPinToPort(). Not 100% sure, I'm not in front of a computer where I can check this. With link-time-optimization (--lto), I think any duplicate definitions will be optimized out.

Interestingly enough, it is the first const keyword in that line that is unnecessary, since uint8_t is returned by-value. The compiler is probably printing a warning, which will show up if warnings are turned on.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline is only a hint, the compiler can choose to ignore it and often does for large enough functions [..]

I know inline is just a hint, but seems the story is complicated by the fact there were implementations before standardization:

http://www.greenend.org.uk/rjk/tech/inline.html

After reading this, and after a recollection from some other discussion I had some time ago, to have a single stand-alone copy, according to the standard, there needs to be 1 and only 1 declaration of the inline function in a .c file, while the definition can be in .h.

Given the usage here, i guess static inline is the safest and simplest choice for this code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, C++ handles the way to generate the inline standalone copy differently than C (definition/declaration rules are different).

I think I saw an explanation in some youtube video, but I can't recall which one.

Copy link
Author

@BarryPSmith BarryPSmith Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I tried it with just inline, but I got linker errors complaining about unresolved symbols. I did some searching and it seemed static inline was the most similar to what one expects a macro to do (which is what this function is replacing) - i.e. only work if the definition is included in the current compilation unit.

(Side note: I'm very surprised about the existing comment in the code that macros are faster than inline functions. Perhaps -Os is making the compiler ignore the inline hint? Seems strange, in my projects the compiler inlines half of my larger functions straight int main.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, inline is not so much a hint to inline, but it changes the linkage of the symbol, telling the linker to not complain if multiple definitions are found (just drop all but one and use that one). This is relevant since when an inline function is not not inlined every time, a definition is also emitted into the .o file. Effectively, inline should be used whenever defining a function in a header file. Defining it in the header allows inlining, while the inline keyword prevents linker errors. Also note that with link-time-optimization, also functions in .cpp files can get inlined.

When using static, I believe that means that each .o file will get is own private definition of the function, which automatically will not conflict and not produce multiple defintion errors (so adding inline is effectively pointless here). This also means that if the function is not inlined in multiple .o files, each of these will get their own private, distinct definition of the function and you will end up with multiple definitions of the same function.

So, just inline rather than static inline is better, since then any duplicate definitions gets merged.

Originally I tried it with just inline, but I got linker errors complaining about unresolved symbols.

That seems weird, maybe we should investigate and solve those?

Side note: I'm very surprised about the existing comment in the code that macros are faster than inline functions. Perhaps -Os is making the compiler ignore the inline hint? Seems strange, in my projects the compiler inlines half of my larger functions straight int main.

Well, macros are necessarily inlined, while inline functions might not be if the compiler thinks this is better. If you really want to ensure inlining, you can use the always_inline attribute (but often it is better to let the compiler decide).

After reading this, and after a recollection from some other discussion I had some time ago, to have a single stand-alone copy, according to the standard, there needs to be 1 and only 1 declaration of the inline function in a .c file, while the definition can be in .h.

This seems all wrong. A declaration has no function body and is usually in a .h file (since it can be included from multiple .c/.cpp files without conflict), while a definition has a function body and is typically in the .c/.cpp file to prevent multiple definition errors (unless the function is inline, then it can appear in a .h file and be included from multiple .c/.cpp files).

BTW, C++ handles the way to generate the inline standalone copy differently than C (definition/declaration rules are different).

I was not aware of any such differences (but that might be me). There is a difference in calling convention for all functions (C++ mangles function names to include the argument types, to allow argument overloading), maybe that is what you recall?

Interestingly enough, it is the first const keyword in that line that is unnecessary, since uint8_t is returned by-value.

Indeed, that is a pointless const.

Copy link
Author

@BarryPSmith BarryPSmith Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, inline is not so much a hint to inline, but it changes the linkage of the symbol, telling the linker to not complain if multiple definitions are found (just drop all but one and use that one). This is relevant since when an inline function is not not inlined every time, a definition is also emitted into the .o file. Effectively, inline should be used whenever defining a function in a header file. Defining it in the header allows inlining, while the inline keyword prevents linker errors. Also note that with link-time-optimization, also functions in .cpp files can get inlined.

There is information that indicates that's not how gcc behaves under std=gnu11 (what the arduino build environment runs). See https://blahg.josefsipek.net/?p=529 . Look at the row 'inline' column 'Emit (C99)' in the table 5 paragraphs down. Do I misread the blog post, or is it wrong?

When using static, I believe that means that each .o file will get is own private definition of the function, which automatically will not conflict and not produce multiple defintion errors (so adding inline is effectively pointless here). This also means that if the function is not inlined in multiple .o files, each of these will get their own private, distinct definition of the function and you will end up with multiple definitions of the same function.

So, just inline rather than static inline is better, since then any duplicate definitions gets merged.

Originally I tried it with just inline, but I got linker errors complaining about unresolved symbols.

That seems weird, maybe we should investigate and solve those?

What are the parameters you would like to investigate? What is not solved? What behaviour do you believe exists that you would like to be different?

Here's how I understand it:

  • If it's only inline and the compiler decides not to inline the function, then it generates a linker error. I find it strange that the compiler would choose not to inline this function (bounds check + array lookup seems like even if optimising for size it's less code space than a function call). Perhaps there's something with LTO also happening here that the linker error occurs?
  • If it's inline static and the compiler can inline it everywhere, then no symbol is emitted. If it cannot inline it everywhere, then a symbol is emitted. I hope that LTO takes care to avoid multiple symbols, but I'm not sure - is this what you would like to investigate?
  • If it's inline in the header with a extern inline in a .c file, then there must be a symbol emitted. Does LTO have the option to not include that symbol? I don't know that either.

If my understanding is confused, I would appreciate clarification. There does seem to be some conflicting information around given the various inline behaviours.

A final option would be to get rid of inline entirely, and make it a normal function and hope that LTO inlines it wherever its used.

Also: If you remove the static, does it compile without problems for you? Seems a simple test; if it does then I'll try figure out why it won't compile for me.

After reading this, and after a recollection from some other discussion I had some time ago, to have a single stand-alone copy, according to the standard, there needs to be 1 and only 1 declaration of the inline function in a .c file, while the definition can be in .h.

This seems all wrong. A declaration has no function body and is usually in a .h file (since it can be included from multiple .c/.cpp files without conflict), while a definition has a function body and is typically in the .c/.cpp file to prevent multiple definition errors (unless the function is inline, then it can appear in a .h file and be included from multiple .c/.cpp files).

Here is a gnu.org article that suggests the definition should be in the header file, and the "declaration" in the c file (there were a few other sources similar, but I didn't write them down): https://www.gnu.org/software/gnulib/manual/html_node/extern-inline.html

I believe this might be what you are advocating, but if so are you certain that it still gives the linker the freedom to strip the symbol if it is inlined everywhere? Also, do you think the added complexity of having to have an extern inline declaration somewhere (I assume wiring.c would make the most sense) is worth it to save code space in the edge case where the function isn't inlined and used in multiple translation units?

Copy link
Author

@BarryPSmith BarryPSmith Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the exact linker errors if I remove static:

c:/avr-gcc-9.2.0-x64-mingw/bin/../lib/gcc/avr/9.2.0/../../../../avr/bin/ld.exe: C:\Users\BARRY_~1\AppData\Local\Temp\all.obj.5rYYTU.ltrans0.ltrans.o: in function `pinMode':
  <artificial>:(.text.pinMode+0xa): undefined reference to `digitalPinToBitMask'
  c:/avr-gcc-9.2.0-x64-mingw/bin/../lib/gcc/avr/9.2.0/../../../../avr/bin/ld.exe: <artificial>:(.text.pinMode+0x12): undefined reference to `digitalPinToPort'
  c:/avr-gcc-9.2.0-x64-mingw/bin/../lib/gcc/avr/9.2.0/../../../../avr/bin/ld.exe: C:\Users\BARRY_~1\AppData\Local\Temp\all.obj.5rYYTU.ltrans0.ltrans.o: in function `digitalRead':
  <artificial>:(.text.digitalRead+0x8): undefined reference to `digitalPinToTimer'
  c:/avr-gcc-9.2.0-x64-mingw/bin/../lib/gcc/avr/9.2.0/../../../../avr/bin/ld.exe: <artificial>:(.text.digitalRead+0x10): undefined reference to `digitalPinToBitMask'
  c:/avr-gcc-9.2.0-x64-mingw/bin/../lib/gcc/avr/9.2.0/../../../../avr/bin/ld.exe: <artificial>:(.text.digitalRead+0x18): undefined reference to `digitalPinToPort'
  c:/avr-gcc-9.2.0-x64-mingw/bin/../lib/gcc/avr/9.2.0/../../../../avr/bin/ld.exe: C:\Users\BARRY_~1\AppData\Local\Temp\all.obj.5rYYTU.ltrans0.ltrans.o: in function `digitalWrite':
  <artificial>:(.text.digitalWrite+0xc): undefined reference to `digitalPinToTimer'
  c:/avr-gcc-9.2.0-x64-mingw/bin/../lib/gcc/avr/9.2.0/../../../../avr/bin/ld.exe: <artificial>:(.text.digitalWrite+0x14): undefined reference to `digitalPinToBitMask'
  c:/avr-gcc-9.2.0-x64-mingw/bin/../lib/gcc/avr/9.2.0/../../../../avr/bin/ld.exe: <artificial>:(.text.digitalWrite+0x1c): undefined reference to `digitalPinToPort'
C:\Users\Barry\collect2.exe : error : ld returned 1 exit status

I'm not running the Arduino IDE, but use the Arduino libraries. As such, perhaps I see this error because I don't run GCC exactly the same (but I have looked at what the Arduino IDE does and tried to match it). The options I pass to GCC are:

CFLAGS= -mcall-prologues -mmcu=$(MCU) -Os -fdata-sections -ffunction-sections -fno-exceptions -flto -std=c++17 -g

It still emits the same errors if I change it to std=gnu11 (to match the Arduino IDE).

if (P > digital_pin_count) {
BarryPSmith marked this conversation as resolved.
Show resolved Hide resolved
return NOT_A_PIN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me whether NOT_A_PIN is an invalid port number, or whether it is a valid port number that is returned for an invalid pin. Can you add a comment to make that more clear?

Copy link
Author

@BarryPSmith BarryPSmith Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you reviewing the latest commit? That should be a >=.

It's following the same convention already in place in pinMode, where the returned value of digitalPinToPort is compared against NOT_A_PIN to see if the input was a pin.

Do you think that it is not obvious from the code what the situation is, that a comment is needed?

} else {
return pgm_read_byte( digital_pin_to_port_PGM + P );
}
}
static inline const uint8_t digitalPinToBitMask(const uint8_t P) {
if (P > digital_pin_count) {
return NOT_A_PIN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here, it is not clear whether NOT_A_PIN is an invalid bit mask, or it's a valid mask that is returned for an invalid pin. Can you add a comment?

Copy link
Author

@BarryPSmith BarryPSmith Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you reviewing the latest commit? That should be a >=.

It's following the same convention already in place in pinMode, where the returned value of digitalPinToPort is compared against NOT_A_PIN to see if the input was a pin.

Do you think that it is not obvious from the code what the situation is, that a comment is needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was: the semantics of the NOT_A_PIN being used for multiple concepts that was confusing me.

The digitalPinToPort() function uses the digital_pin_to_port_PGM[] array. If an invalid pin is given, I expected an invalid value to be returned, which I expected to be named NOT_A_PORT_NUMBER. It seems like current valid port numbers (PA - PL) are > 0 so NOT_A_PORT_NUMBER can be 0. But it seems like only an accident that NOT_A_PORT_NUMBER is the same as NOT_A_PIN.

(The term "PORT" seems to be overloaded in pins_arduino.h. Sometimes it means "port number", sometimes it means a pointer. There is a NOT_A_PORT constant already defined, but it is used as a NULL or nullptr pointer as far as I can tell.)

The digitalPinToBitMask() function uses the digital_pin_to_bit_mask_PGM[] array. When an invalid pin is given, I expected a NOT_A_MASK value to be returned, a value that is not a valid bit mask, which I guess can be 0. It seems like it's an accident that a NOT_A_PIN is equal to NOT_A_MASK. Returning a NOT_A_PIN was confusing to me.

Similarly, the digitalPinToTimer() function uses the digital_pin_to_timer_PGM[] array. If an invalid pin is given, I expected an NOT_A_TIMER value to be returned, not a NOT_A_PIN. Again, it seems like an accident that NOT_A_PIN happens to be the same as NOT_A_TIMER. Now, there's already a NOT_ON_TIMER defined, and it's defined to be 0. So if you define NOT_A_TIMER to be 0, we can no longer distinguish between the 'NOT_A' and the 'NOT_ON'. Which raises the question, should `NOT_A_TIMER' be defined to be 255 instead?

These 3 functions are returning 3 conceptually separate things, and I got confused why all of them are returning a NOT_A_PIN. Anyway, if you choose not to create 3 new symbols (NOT_A_PORT_NUMBER, NOT_A_MASK, and NOT_A_TIMER), then I think some comments in the code would help explain why NOT_A_PIN can be used in all 3 cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please familiarise yourself with how these functions are currently used and implemented.They have no documentation of their API, so their existing use defines the API. wiring_digital.c and the various pins_arduino.h are good places to look. You will see that the new implementations are consistent with the existing API, as I explained above.

You are requesting that the existing API change. That is out of scope of this pull request. Please create a separate pull request or issue. I do not feel that a comment explaining that a function conforms to the rest of the (undocumented, uncommented) API is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's even more important to add comments to describe your assumptions when you are making changes on top of undocumented and uncommented API.

I have reviewed the usage of digitalPinToPort(), digitalPinToBitMask(), and digitalPinToTimer() in the following files (wiring_pulse.c, wiring_digital.c, Tone.cpp, SoftwareSerial.cpp, SPI.cpp). I support the use of NOT_A_PIN in digitalPinToPort(). But I disagree with its usage in digitalPinToBitMask() and digitalPinToTimer().

As far as I can tell, NOT_A_PIN means "an invalid port number for an invalid pin". If the symbol is overloaded to represent the concept of "an invalid mask for an invalid pin", and "an invalid timer for an invalid pin", I suspect that this will cause unnecessary confusion for (potentially hundreds of) programmers in the future who will try to deduce the meaning of NOT_A_PIN, without any explanatory comments.

If you choose to ignore my feedback and continue to use NOT_A_PIN in 3 different contexts, I request that you add comments to that part of the code that explains that it is being overloaded to mean 3 different things. I think this is a reasonable request.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For digitalPinToPort, the function conceptually returns:

  • For a valid pin with a port associated with it, the port
  • For all other cases, an error code about the pin. Exemplified by NOT_A_PIN, and the checks in wiring_digital.c

For digitalPinToTimer, the function conceptually returns:

  • For a valid pin with an associated timer, the timer associated with the pin.
  • For all other cases, an error code about the pin. Exemplified by NOT_ON_TIMER (if it were an error code about the return value, it would be as you suggested NOT_A_TIMER)

For digitalPinToBitmask, the function conceptually returns:

  • For a valid pin with an associated bitmask, the bitmask of the pin
  • For invalid pins, the return value is undefined (there are no coded cases of what it returns).
  • Given lack of an example, I think it is fair to deduce that digitalPinToBitmask should follow the same rules as the other digitalPinTo* functions.

So, for digitalPinToTimer, when passed a value that is not associated with a pin, the correct error code to return is a description of the input value. You cannot say that pin 215 is not on a timer, because pin 215 does not exist. So the correct error code is NOT_A_PIN. Given that NOT_ON_TIMER and NOT_A_PIN have the same value, existing code will work without having to check for the new possible return concept.

The same logic applies to digitalPinToBitmask. Returning zero might be acceptable too, but that would be inconsistent with the other similarly named functions.

Unfortunately it appears that the writer of port_to_input_PGM in the gemma and mega variants abuse NOT_A_PIN and use it to return when queries for the register of a port. I cannot understand what concept they are going for here, and given that portInputRegister never has its output checked, this appears to be a bug. (In most cases, when that function returns NOT_A_PIN the uC will attempt to read or write address 0x0000, which should cause a reset. At least that's better behaviour than pinMode or digitalWrite without this patch).

So:

  • if you want me to comment the return values here, then I'm solidifying what I induce about the API of these functions. Which requires fixing pins_arduino.h for the mega and gemma.
  • if I introduce new values, I'm changing the API and all dependent functions must be checked that they will still work. Plus that would just be inconsistent.

Both of those options are out of scope for this PR and more than I'm prepared to do. As I say, please deal with the issue in a separate PR.

Copy link
Contributor

@bxparks bxparks Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the intended limited scope of this PR. But your changes to digitalPinToBitMask() and digitalPinToTimer() went slightly beyond that intended scope. Those changes were not necessary for this PR, so I think my inquiry regarding those changes is legitimate.

The problem that you found in variants/gemma/pins_arduino.h and variants/mega/pins_arduino.h with their incorrect use of NOT_A_PIN is exactly the sort of problem that I want to avoid in this PR. It is obvious that those 2 files should be using NOT_A_PORT instead, to be consistent with all the others. And I agree it's not in the scope of this PR to fix those 2 files.

The return type of digitalPinToPort() is a port number. Therefore, NOT_A_PIN is port number type. It is a sentinel value that represents "an invalid port number representing an invalid pin". All current uses of NOT_A_PIN (except for variants/gemma/pins_arduino.h and variants/mega/pins_arduino.h) are consistent with it being a port number.

The return type ofdigitalPinToBitMask() is a bit mask, not a port number. I currently see no example of the digital_pin_to_bit_mask_PGM[] array using the NOT_A_PIN constant as a sentinel value to represent the concept of: "an invalid bit mask representing an invalid pin". This PR adds a new semantic meaning to NOT_A_PIN which is not appropriate, because the type of NOT_A_PIN is a port number not a bit mask. (They both happen to be a uint8_t, but that does not mean they can reuse constants. It would be like having a int getLength() function returning an INVALID_SECONDS which happens to be an int.)

Finally, the return type of digitalPinToTimer() function seems to be an integer enumeration of the various (Timer x Channel) combo. (A modern type-safe version would probably use a C++ scoped enum type (i.e. enum class). A timer value of NOT_ON_TIMER (a value of 0) is actually a valid value for this function, and it does not mean "an invalid timer value representing invalid pin". One can verify that NOT_ON_TIMER is a valid value because it is used in the analogWrite() function in cores/arduino/wiring_analog.c, where the last part of the switch statement is:

      case NOT_ON_TIMER:
      default:
        if (val < 128) {
          digitalWrite(pin, LOW);
        } else {
          digitalWrite(pin, HIGH);
        }

If digitalPinToTimer() returns a 0, the existing code continues to write into the pin, instead of interpreting the 0 to mean "invalid pin". Since NOT_A_PIN is also a 0, which overlaps with NOT_ON_TIMER, it cannot be correct for digitalPinToTimer() to return NOT_A_PIN for an invalid pin.

At this point, the simplest fix I can see that will implement the intended scope of this PR is to remove the checks for (P >= digital_pin_count) in digitalPinToBitMask() and digitalPinToTimer(), while keeping the check in digitalPinToPort(), which I agree is correct. In other words:

static inline uint8_t digitalPinToBitMask(const uint8_t P) {
   return pgm_read_byte( digital_pin_to_bit_mask_PGM + P );
}
static inline uint8_t digitalPinToTimer(const uint8_t P) {
  return pgm_read_byte( digital_pin_to_timer_PGM + P );
}

I cannot see how returning the NOT_A_PIN constant is correct for these 2 functions. I think the correct solution is to define 2 new constants, a INVALID_MASK_FOR_INVALID_PIN and a INVALID_TIMER_FOR_INVALID_PIN. But I agree it is beyond the scope of this PR to add 2 new constants, and add the appropriate checks for these sentinel values in the calling code.

} else {
return pgm_read_byte( digital_pin_to_bit_mask_PGM + P );
}
}
static inline const uint8_t digitalPinToTimer(const uint8_t P) {
if (P > digital_pin_count) {
return NOT_A_PIN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, is NOT_A_PIN an invalid Timer value, or the default value returned when the pin is not valid? Add comment?

Copy link
Author

@BarryPSmith BarryPSmith Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you reviewing the latest commit? That should be a >=.

It's following the same convention already in place in pinMode, where the returned value of digitalPinToPort is compared against NOT_A_PIN to see if the input was a pin. It doesn't return NOT_ON_TIMER because it's not a pin that's not on a timer, but it's not a pin at all. (convenient enough, by coincidence NOT_ON_TIMER and NOT_A_PIN have the same value so sloppy programming where only the former is checked will still pick it up.)

Do you think that it is not obvious from the code what the situation is, that a comment is needed?

} else {
return pgm_read_byte( digital_pin_to_timer_PGM + P );
}
}
// these defines are to maintain backwards compatibility with #ifdef
#define digitalPinToPort digitalPinToPort
#define digitalPinToBitMask digitalPinToBitMask
#define digitalPinToTimer digitalPinToTimer

#ifdef ARDUINO_MAIN
#define PA 1
Expand Down
2 changes: 2 additions & 0 deletions cores/arduino/wiring_digital.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
#include "wiring_private.h"
#include "pins_arduino.h"

const uint8_t digital_pin_count = sizeof(digital_pin_to_port_PGM) / sizeof(digital_pin_to_port_PGM[0]);
BarryPSmith marked this conversation as resolved.
Show resolved Hide resolved

void pinMode(uint8_t pin, uint8_t mode)
{
uint8_t bit = digitalPinToBitMask(pin);
Expand Down
2 changes: 2 additions & 0 deletions variants/circuitplay32u4/pins_arduino.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,8 @@ const uint8_t PROGMEM analog_pin_to_channel_PGM[] = {
9 // A11 D12 PD6 ADC9
};

extern const uint8_t digital_pin_count;
BarryPSmith marked this conversation as resolved.
Show resolved Hide resolved

#endif /* ARDUINO_MAIN */

// These serial port names are intended to allow libraries and architecture-neutral
Expand Down