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.

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

pinMode1 - Silent failure using 0 vs using OUTPUT. #45

Closed
Spaceballs3000 opened this issue Jul 4, 2024 · 8 comments · Fixed by #46
Closed

pinMode1 - Silent failure using 0 vs using OUTPUT. #45

Spaceballs3000 opened this issue Jul 4, 2024 · 8 comments · Fixed by #46
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@Spaceballs3000
Copy link

Unless you check the return value, I didn't know pinMode1 was failing using 0 vs using OUTPUT. As 0 is invalid.

Noticed OUTPUT is defined as 0x03.

Also the function description is Incorrect i.e. "single pin interface mode: 0 = OUTPUT, 1 = INPUT, 1 = INPUT_PULLUP (==INPUT)"

@RobTillaart RobTillaart self-assigned this Jul 4, 2024
@RobTillaart RobTillaart added bug Something isn't working documentation Improvements or additions to documentation labels Jul 4, 2024
@RobTillaart
Copy link
Owner

From Datasheet REV_D

image

Quick test Arduino UNO constants

Serial.println(OUTPUT);
Serial.println(INPUT);
Serial.println(INPUT_PULLUP);

results in

1
0
2

So OUTPUT == 1 and INPUT == 0, ==> incorrect documentation.

Code

relevant piece of MCP23S17.cpp (about line 134)

  uint8_t mask = 1 << pin;
  //  only work with valid
  if ((mode == INPUT) || (mode == INPUT_PULLUP))
  {
    val |= mask;  <<<<<<<<  sets bit as a 1
  }
  else if (mode == OUTPUT)
  {
    val &= ~mask;   <<<<<<<<<  sets bit to 0
  }
  //  other values won't change val ....
  writeReg(dataDirectionRegister, val);

Conclusion

So the device behaves as expected when using the constants / defines INPUT and OUTPUT
but not when using 0 or 1.

Serious documentation error ==> bug level.

Will create a develop branch to start a fix.

Note to myself: check examples + MCP23008, MCP23017, MCP23S08 libraries too.

@RobTillaart
Copy link
Owner

@Spaceballs3000

Noticed OUTPUT is defined as 0x03.

Which platform do you use?

@RobTillaart
Copy link
Owner

Note to myself: check examples + MCP23008, MCP23017, MCP23S08 libraries too.

Other 3 libraries have the same documentation bug, => create issues.

@RobTillaart
Copy link
Owner

Note: bug comes from using the bit mask where 0 means output mode and 1 means input mode.

@RobTillaart
Copy link
Owner

Will update the readme.md file to

  • bool pinMode1(uint8_t pin, uint8_t mode) pin = 0..15, mode = INPUT, OUTPUT or INPUT_PULLUP.
    Do NOT use 0, 1 for mode as the 3 constants are (possibly) defined differently.
    Returns true if successful.

In the .h file some similar comment.

RobTillaart added a commit that referenced this issue Jul 4, 2024
@RobTillaart
Copy link
Owner

@Spaceballs3000
Created develop branch for version 0.5.4 with updated documentation.

@RobTillaart RobTillaart changed the title pinMode1 - Slient failure using 0 vs using OUTPUT. pinMode1 - Silent failure using 0 vs using OUTPUT. Jul 4, 2024
@RobTillaart RobTillaart linked a pull request Jul 4, 2024 that will close this issue
@Spaceballs3000
Copy link
Author

@Spaceballs3000

Noticed OUTPUT is defined as 0x03.

Which platform do you use?

In my case the define came from esp32-hal-gpio.h

@RobTillaart
Copy link
Owner

RobTillaart commented Jul 5, 2024

@Spaceballs3000
All four repo's have updated readme.md and updated comments in .h / .cpp.
It should disambiguate the use of parameters for pinMode1()

Will merge develop branch(es) later today,

(update) did not find problems in the examples.

RobTillaart added a commit that referenced this issue Jul 6, 2024
- fix #45, documentation bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants