RobTillaart / MCP23S17

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

No way to test for connection #13

Closed BertvanRossem closed 2 years ago

BertvanRossem commented 2 years ago

I thought I would create a new issue. I have made a pastebin (https://pastebin.com/YAsagJMG) with an untested implementation. Feel free to change it to fit your syntax

RobTillaart commented 2 years ago

It might be better to create an example sketch for this functionality. The risk of someone applying it runtime by accident is real imho. Building guards around it in the library will make it unnecessary complex.

Need to think about the impact some more

RobTillaart commented 2 years ago

Have given it some thoughts, but I hesitate to embed this into the library as it increase the footprint quite a bit. That is no problem for a teensy or ESP32, but there are users on the other end of the spectrum too. So I propose to make a demo sketch out of it.

Your code design looks OK, just the way this should be done,

minor remarks on the const uint16_t _connection_test_constant = 0xAABB;

Repeating patterns should be avoided, B is better than A, AB is better than AA or BB Something like 0x74B1 = 0111 0100 1011 0001 looks more random furthermore to improve the quality of the test a loop of N (>=2) random 16 bit numbers could be done (overkill)

(to be continued)

RobTillaart commented 2 years ago

test sketch

//
//    FILE: MCP23S17_test_connection.ino
//  AUTHOR: Rob Tillaart
//    DATE: 2022-07-01
// PUPROSE: test MCP23017 library

#include "MCP23S17.h"
#include "SPI.h"

// MCP23S17 MCP(10, 12, 11, 13);  //  SW SPI address 0x00
MCP23S17 MCP(10, 4);           //  HW SPI address 4

void setup()
{
  Serial.begin(115200);
  Serial.println();
  Serial.print("MCP23S17_LIB_VERSION: ");
  Serial.println(MCP23S17_LIB_VERSION);
  Serial.println();

  SPI.begin();
  MCP.begin();

  //  test connected 
  int c = testConnection(MCP);
  Serial.print("connection: ");
  Serial.println(c);
}

void loop()
{
}

int testConnection(MCP23S17 & mcp)
{
  uint16_t magic_test_number = 0xABCD;

  //  Read the current polarity config to restore later
  uint16_t old_value;
  if (! mcp.getPolarity16(old_value)) return -1;

  //  Write the magic number to polarity register
  if (! mcp.setPolarity16(magic_test_number)) return -2;

  //  Read back the magic number from polarity register
  uint16_t temp;
  if (! mcp.getPolarity16(temp)) return -3;

  //  Write old config to polarity register
  if (! mcp.setPolarity16(old_value)) return -4;

  //  Check the magic connection test
  if (temp != magic_test_number) return -5;

  return 0;  //  OK
}

// -- END OF FILE --

The function returns an int to know HOW it failed as this test can be "destructive" for the polarity register. Bad values are -3 and -4

I will add this example to the current develop branch as it is useful and might be included in the lib in the future.

(writing the above makes me think of a derived class that may include it - and other footprint hungry stuff)

RobTillaart commented 2 years ago

Merged develop, including the new example sketch above.

BertvanRossem commented 2 years ago

Yeah sure, everybody has their preferences. My perspective is the use of the lib in my application, your perspective is as library writer. But it looks good!

RobTillaart commented 2 years ago

Very true!