Code efficiency

Hi,
I'm working on a project that uses 3 PCF8575 16 I/O expander boards. These return the value in two bytes each, and what I do is read each board's two bytes, save them in a variable, and than store each individual bit in a 3x16 array.
I haven't been able to test the code yet, but it really feels like this part could be coded more efficiently:

  byte read_value;
  if (Wire.requestFrom(pcf8575_1, 2) == 2) {
    read_value1_1 = (Wire.read());
    read_value1_2 = (Wire.read());
  }

  byte read_value;
  if (Wire.requestFrom(pcf8575_2, 2) == 2) {
    read_value2_1 = (Wire.read());
    read_value2_2 = (Wire.read());
  }

  byte read_value;
  if (Wire.requestFrom(pcf8575_3, 2) == 2) {
    read_value3_1 = (Wire.read());
    read_value3_2 = (Wire.read());
  }

  for (int i = 0; i < 8; i++) {
    buttonvalues[0][i] = bitRead(read_value1_1, i)
  }
  for (int i = 8; i < 16; i++) {
    buttonvalues[0][i] = bitRead(read_value1_2, i)
  }

  for (int i = 0; i < 8; i++) {
    buttonvalues[1][i] = bitRead(read_value2_1, i)
  }
  for (int i = 8; i < 16; i++) {
    buttonvalues[1][i] = bitRead(read_value2_2, i)
  }

  for (int i = 0; i < 8; i++) {
    buttonvalues[2][i] = bitRead(read_value3_1, i)
  }
  for (int i = 8; i < 16; i++) {
    buttonvalues[2][i] = bitRead(read_value3_2, i)
  }

I've always wondered how would you concatenate a variable name with a for. Let's say I have, for instance, var_1, var_2, and var_3, and I want to perform the same action for all three. In my mind would be something like:

for (int i = 1; i <= 3; i++) {
  "var_" + i = "whatever";
}

So in the above example, all 3 variables would have a value of "whatever". But how would the actual code be?

Thank you

Time to read about arrays!

2 Likes

var[3]

1 Like

Depends what you mean by "efficient" ?

In terms of reducing the amount of source lines: whenever you see a lot of copy-paste repetition like this, you should be thinking of using arrays and/or functions to take out the common repeated parts...

This may also reduce the size of the executable - so may be more "efficient" in its use of Flash space.

In terms of runtime efficiency, the code may end up running a little slower - but that's unlikely to be an issue here.

why you think you need each bit to store separated?

1 Like

Curious - are you aware of the PCF8575 libraries, or are you deliberately delving into the complexities of raw wire usage?

1 Like

Thank you all. I do know about arrays, I just didn't think of them like that, but now giving it a bit of thought, it really is the easier way. That's why this subforum is called "Project Guidance" :wink:

I might not. Now that you mention it, I think I can work with full bytes. I'll explore that possibility too.

I am, but either I'm not using them properly, they are not properly documented, or not fully working, or a combination of them (though most likely the first one). I'll play around with them a bit more, to see if I can make them work as I need.
So far, not using the libraries is the option that has worked best for me, I just wanted to improve the code.

First one I'd try would be Rob Tilaart's, his libraries have always given me satisfaction.
If you try it and fail, develop a small test app that shows what you're trying to do, and we'll take a look.
I generally use the read to uint16, followed by bit-masking as needed to test the individual bits as required. I've moved to the MCP23017, for other reasons, but the principle's the same. You can use the functions that access individual bits, but I'm usually looking for bit combinations, so masking is more general.

That's the library I have, and I don't know if he coded it expecting it to be wired differently or something, but only 1 pin works and not how you are supposed to wire it.
I believe these boards, for inputs, the pins are expected to be pulled up with a resistor to VCC, and then the input is detected when it's pulled down to ground, and that's how I've managed it to work with the code above, without library. All you need to know is that inputs are actually INPUT_PULLUP, so when idle it outputs 1 and when the button is pressed, it outputs 0. As long as you code according to that, you should be fine.
I tested it with a push button and a 10k resistor in a protoboard, and it works flawlessly. It's not that hard to code it without a library.

This is the current version of the code, though I still don't have all the resistors I need, so I haven't been able to test it yet, but in terms of efficency (in number of lines), I think it looks better thanks to your comments and it compiles without errors:

#include "Joystick.h"
#include "AS5600.h"
#include "Wire.h"

#define collectivePot A0  // select the input pin for the potentiometer
#define joyX A1
#define joyY A2
//#define pcf_1 0x21
//#define pcf_2 0x22
//#define pcf_3 0x23
int address[] = { 0x21, 0x22, 0x23 };
int buttonpressed[3][2];
int buttonvalues[3][2];

AS5600 collectiveAS5600;

Joystick_ Joystick(JOYSTICK_DEFAULT_REPORT_ID, JOYSTICK_TYPE_JOYSTICK, 38, 0, true, true, false, false, false, false, true, true, false, false, false);

const bool initAutoSendState = true;

int sensorValue = 0;  // variable to store the value coming from the sensor
int xAxis_ = 0;
int yAxis_ = 0;
int rudder_ = 0;
int throttle_ = 0;

void setup() {
  Wire.begin();

  for (int i = 0; i < 3; i++) {
    Wire.beginTransmission(address[i]);
    Wire.write(0xFF);
    Wire.write(0xFF);
    Wire.endTransmission();
  }

  Serial.begin(115200);
  Joystick.begin();

  collectiveAS5600.setDirection(AS5600_CLOCK_WISE);
}

void loop() {

  byte read_value;
  for (int i = 0; i < 3; i++) {
    if (Wire.requestFrom(address[i], 2) == 2) {
      buttonpressed[0][0] = Wire.read();
      buttonpressed[0][1] = Wire.read();
    }
  }

  for (int i = 0; i < 3; i++) {
    for (int j = 0; j < 2; j++) {
      if (buttonpressed[i][j] != buttonvalues[i][j]) {
        for (int x = 0; x < 8; x++) {
          Joystick.setButton(i + j, !bitRead(buttonpressed[i][j], x));
          buttonvalues[i][j] = buttonpressed[i][j];
        }
      }
    }
  }

  xAxis_ = analogRead(joyX);
  //xAxis_ = map(xAxis_, 0, 1023, 255, 0);
  Joystick.setXAxis(xAxis_);

  yAxis_ = analogRead(joyY);
  //yAxis_ = map(yAxis_, 0, 1023, 0, 255);
  Joystick.setYAxis(yAxis_);

  throttle_ = analogRead(collectivePot);
  //Serial.print(throttle_);
  //Serial.print("\t");
  throttle_ = map(throttle_, 0, 921, 0, 1023);
  Joystick.setRudder(throttle_);
  //Serial.print(throttle_);
  //Serial.print("\t");

  rudder_ = collectiveAS5600.readAngle();
  //Serial.println(rudder_);
  rudder_ = map(rudder_, 1558, 1984, 0, 1023);
  Joystick.setThrottle(rudder_);
  //Serial.println(rudder_);
}
#include "Joystick.h"
#include "AS5600.h"
#include "Wire.h"

#define collectivePot A0  // select the input pin for the potentiometer
#define joyX A1
#define joyY A2
//#define pcf_1 0x21
//#define pcf_2 0x22
//#define pcf_3 0x23
int address[] = { 0x21, 0x22, 0x23 };
uint16_t buttonvalues[3];

AS5600 collectiveAS5600;

Joystick_ Joystick(JOYSTICK_DEFAULT_REPORT_ID, JOYSTICK_TYPE_JOYSTICK, 38, 0, true, true, false, false, false, false, true, true, false, false, false);

const bool initAutoSendState = true;

int sensorValue = 0;  // variable to store the value coming from the sensor
int xAxis_ = 0;
int yAxis_ = 0;
int rudder_ = 0;
int throttle_ = 0;

void setup() {
  Wire.begin();

  for (int i = 0; i < 3; i++) {
    Wire.beginTransmission(address[i]);
    Wire.write(0xFF);
    Wire.write(0xFF);
    Wire.endTransmission();
  }

  Serial.begin(115200);
  Joystick.begin();

  collectiveAS5600.setDirection(AS5600_CLOCK_WISE);
}

void loop() {

  byte read_value;
  for (int i = 0; i < 3; i++) {
    if (Wire.requestFrom(address[i], 2) == 2) {
      byte h = Wire.read();
      byte l = Wire.read();
      uint16_t buttonPressed = word(h, l);
      uint16_t buttonChanged = buttonpressed ^ buttonvalues[i];
      if (buttonChanged) {
        for (int x = 0; x < 16; x++) {
           if (bitRead(buttonChanged, x)) {
             Joystick.setButton(i*16 + x, !bitRead(buttonpressed, x));
          }
        }
        buttonvalues[i] = buttonpressed;
      }
    }
  }

  xAxis_ = analogRead(joyX);
  //xAxis_ = map(xAxis_, 0, 1023, 255, 0);
  Joystick.setXAxis(xAxis_);

  yAxis_ = analogRead(joyY);
  //yAxis_ = map(yAxis_, 0, 1023, 0, 255);
  Joystick.setYAxis(yAxis_);

  throttle_ = analogRead(collectivePot);
  //Serial.print(throttle_);
  //Serial.print("\t");
  throttle_ = map(throttle_, 0, 921, 0, 1023);
  Joystick.setRudder(throttle_);
  //Serial.print(throttle_);
  //Serial.print("\t");

  rudder_ = collectiveAS5600.readAngle();
  //Serial.println(rudder_);
  rudder_ = map(rudder_, 1558, 1984, 0, 1023);
  Joystick.setThrottle(rudder_);
  //Serial.println(rudder_);
}

Hi Paul.

I actually had to make some amendments but I finally got it to work. I realized I do not need external pullup resistors, and with this code is now working like a charm:

#include "Joystick.h"
#include "AS5600.h"
#include "Wire.h"

#define collectivePot A0  // select the input pin for the potentiometer
#define joyX A2
#define joyY A1
int address[] = { 0x20, 0x21, 0x23 };
int buttonpressed[3][2];
int buttonvalues[3][2];

int buttons[3][16] = {
  { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 },
  { 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 },
  { 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47 },
};

AS5600 collectiveAS5600;

// Set i2c address

Joystick_ Joystick(JOYSTICK_DEFAULT_REPORT_ID, JOYSTICK_TYPE_JOYSTICK, 38, 0, true, true, false, false, false, false, true, true, false, false, false);

const bool initAutoSendState = true;

int sensorValue = 0;  // variable to store the value coming from the sensor
int xAxis_ = 0;
int yAxis_ = 0;
int rudder_ = 0;
int throttle_ = 0;

void setup() {
  Wire.begin();

  for (int i = 0; i < 3; i++) {
    Wire.beginTransmission(address[i]);
    Wire.write(0xFF);
    Wire.write(0xFF);
    Wire.endTransmission();
  }

  Serial.begin(115200);
  Joystick.begin();

  collectiveAS5600.setDirection(AS5600_CLOCK_WISE);
}

void loop() {

  byte read_value;
  for (int i = 0; i < 3; i++) {
    if (Wire.requestFrom(address[i], 2) == 2) {
      buttonpressed[i][0] = Wire.read();
      buttonpressed[i][1] = Wire.read();
    }
  }

  for (int i = 0; i < 3; i++) {
    for (int j = 0; j < 2; j++) {
      if (buttonpressed[i][j] != buttonvalues[i][j]) {
        for (int x = 0; x < 8; x++) {
          if (bitRead(buttonpressed[i][j], x) != bitRead(buttonvalues[i][j], x)) {
            Joystick.setButton(buttons[i][x+(j*8)], !bitRead(buttonpressed[i][j], x));
          }
        }
        buttonvalues[i][j] = buttonpressed[i][j];
      }
    }
  }

  xAxis_ = analogRead(joyX);
  //xAxis_ = map(xAxis_, 0, 1023, 255, 0);
  Joystick.setXAxis(xAxis_);

  yAxis_ = analogRead(joyY);
  //yAxis_ = map(yAxis_, 0, 1023, 0, 255);
  Joystick.setYAxis(yAxis_);

  throttle_ = analogRead(collectivePot);
  //Serial.print(throttle_);
  //Serial.print("\t");
  throttle_ = map(throttle_, 0, 921, 0, 1023);
  Joystick.setRudder(throttle_);
  //Serial.print(throttle_);
  //Serial.print("\t");

  rudder_ = collectiveAS5600.readAngle();
  //Serial.println(rudder_);
  rudder_ = map(rudder_, 1558, 1984, 0, 1023);
  Joystick.setThrottle(rudder_);
  //Serial.println(rudder_);
}

Hi Assamita. You don't like any of the changes I suggested?

Sorry, I didn't look at them, as I was coming to tell everyone that I managed it to work. I'll save my working code is a safe place and test yours.

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.