RobTillaart / MCP23S17

Arduino library for SPI based MCP23S17 16 channel port expander
MIT License
26 stars 11 forks source link

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

Closed Spaceballs3000 closed 2 months ago

Spaceballs3000 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@Spaceballs3000

Noticed OUTPUT is defined as 0x03.

Which platform do you use?

RobTillaart commented 2 months ago

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

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

RobTillaart commented 2 months ago

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

RobTillaart commented 2 months ago

Will update the readme.md file to

In the .h file some similar comment.

RobTillaart commented 2 months ago

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

Spaceballs3000 commented 2 months ago

@Spaceballs3000

Noticed OUTPUT is defined as 0x03.

Which platform do you use?

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

RobTillaart commented 2 months ago

@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.