Skip to content

pinMode does not accept NC / unused pin #693

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.

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

Closed
MarcelRobitaille opened this issue Jun 8, 2023 · 3 comments · Fixed by #698
Closed

pinMode does not accept NC / unused pin #693

MarcelRobitaille opened this issue Jun 8, 2023 · 3 comments · Fixed by #698

Comments

@MarcelRobitaille
Copy link
Contributor

Normally, if a library takes a pin that is not used, I would pass in -1 as the pin number. The equivalent in Mbed would be NC. However, this -1 crashes the MCU with the mbed core. (I'm not sure if crashes is the right term. My Portenta H7 is not responding and the red LED is blinking).

The reason is that libraries made for arduino use pinMode with this signature:

void pinMode(pin_size_t pin, PinMode mode)

There is also a version with PinName, but most arduino libraries use uint8_t or something like that for their pin numbers, not PinName.

This definition of pinMode looks up the GPIO object inside of g_APinDescription without doing any checks that the pin is in range.

The equivalent definitions of digitalRead and digitalWrite check if the pin is in range before doing anything, but pinMode does not.

  if (pin >= PINS_COUNT) {
    return LOW;
  }  

I think pinMode should have the exact same guard as digitalRead and digitalWrite. I can open a PR for this. Is there anything I am missing as to why this is implemented this way?

@facchinm
Copy link
Member

Hi @MarcelRobitaille , thanks for reporting this. In fact, there's no specific reason why pinMode() doesn't have that check, so a PR is very welcome 🙂

@MarcelRobitaille
Copy link
Contributor Author

Thanks @facchinm. I am working on a PR. I also noticed this for pulseIn (and friends), shiftIn, and shifOut. As a rule, should every function that takes uint8_t pin or pin_size_t pin have this guard? I can add them to my PR.

@facchinm
Copy link
Member

As a rule of thumb, we should protect the most used APIs from misusage, even at the cost of slowing them down due to the additional check.
pulseIn, shift* and so on are quite specific and very unlikely to be used without any additional check from the library writers, so I think it's ok to leave them as is.

MarcelRobitaille added a commit to MarcelRobitaille/ArduinoCore-mbed that referenced this issue Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants