Undefined reference to `myFunction(unsigned char, unsigned char)'

Chris_abc · January 22, 2022, 1:43pm

Hi, I am quite new to PlatformIO and migrating from the ArduinoIDE, so please bear with me.

I am moving over a project I did in the ArduinoIDE to PlatformIO, but have an error that I cannot get fixed. I made a minimal reproducible example shown in the code below. I am trying to distribute the functions I am using in different files, and that’s probably where the error is in. The error states:
< artificial>(.text.startup+0xc0): undefined reference to `readByte(unsigned char, unsigned char)'

Of course I want the error fixed, but I also want to learn how to have the program in multiple files in a ‘proper’ way. From the numerous guides on the internet I found and studied, this is what I came up with, but I suspect that there is tonnes of things to improve/change, so please do not hesitate to point me to anything that can be improved.

The files:
main.cpp:

#include "main_head.h"

void setup() {
  // put your setup code here, to run once:
}

void loop() {
  bool succes = readAccel(&xMeas,&yMeas,&zMeas);
}

main_head.h:

#ifndef MAIN_HEAD_H
#define MAIN_HEAD_H
#include <Arduino.h>

#include <Wire.h>

//global var:
float xAccel, yAccel, zAccel;
float xMeas, yMeas, zMeas;
int KXTJ3 = 0x0E;

//other functions:
extern bool readAccel(float *,float *,float *);

#endif

accel_readout.cpp:

#include "accel_read_head.h"

bool readAccel(float *xAccel,float *yAccel,float *zAccel){
  bool succes = false;
  byte x0Data, x1Data, y0Data, y1Data, z0Data, z1Data;
  if(
    readByte(byte(0x06), x0Data)&&
    readByte(byte(0x07), x1Data)&&
    readByte(byte(0x08), y0Data)&&
    readByte(byte(0x09), y1Data)&&
    readByte(byte(0x0A), z0Data)&&
    readByte(byte(0x0B), z1Data)
  ){
    succes = true;
  }
  *xAccel = x0Data+x1Data;
  *yAccel = x0Data+x1Data;
  *zAccel = x0Data+x1Data;
  return succes;
}

accel_read_head.h:

#ifndef ACCEL_READOUT_H
#define ACCEL_READOUT_H

#include "main_head.h"

extern bool readByte(byte, byte);

#endif

read.cpp:

#include "read_head.h"

bool readByte(byte register_address, byte &data){
  bool success = false;                     // set default to no success
  Wire.beginTransmission(KXTJ3);
  Wire.write( register_address);            // select register
  int error = Wire.endTransmission( false); // false for repeated start
  if( error == 0){                          // no error ? then data can be requested
    int n = Wire.requestFrom(KXTJ3, 1);
    if( n == 1){                            // received the bytes that were requested ?
      data = (byte) Wire.read();
      success = true;                       // only now success is true
    }
  }
  return(success);
}

read_head.h:

#ifndef READ_H
#define READ_H

#include "main_head.h"

#endif

Any help is greatly appreciated.

maxgerhardt · January 22, 2022, 1:45pm

Your function declaration isn’t matching you’re function implementation.

Chris_abc · January 22, 2022, 2:43pm

Wow that’s great. I changed
extern bool readByte(byte, byte);
to
extern bool readByte(byte, byte &);
And now the error is gone. (I assume this is what you were getting at?) Thanks.

Now there are other errors that I hope someone here can also help me with:
All of the global variables from the main_head.h file have multiple definitions:
.pio\build\ATmega4809\src\read.cpp.o (symbol from plugin): In function 'initVariant': (.text+0x0): multiple definition of 'zAccel' Is the error. (There is one for each of the global variables.)

I don’t understand why this is, I thought that using the

#ifndef MAIN_HEAD_H
#define MAIN_HEAD_H
//contents
#endif

would prevent this from happening: when MAIN_HEAD_H is defined the first time, it will prevent the contents from being implemented a second time.

maxgerhardt · January 22, 2022, 2:46pm

Oh right, I overlooked that mistake.

Doing

in a header file is wrong, because it defines / creates these global variables. So every .cpp file that includes this header will, when compiled, want to create the shown global variables. If more than one does that, you have a double definition.

The way to solve that is to declare these global variables as extern in the header and then only define them in one .cpp file, so that only one instance of the global variable is created, but everyone that includes the headers knows about the existance of these variables (= declaration). Change the header file to

//global var:
extern float xAccel, yAccel, zAccel;
extern float xMeas, yMeas, zMeas;
extern int KXTJ3;

and add in one .cpp file of your choice the original expression

float xAccel, yAccel, zAccel;
float xMeas, yMeas, zMeas;
int KXTJ3 = 0x0E;

See c++ - How do I use extern to share variables between source files? - Stack Overflow for reference.

The include guards only guard against multiple inclusions of the same file in one compilation unit (= one .cpp file), it does not prevent anything in regards to a second compilation unit, they’re all stand-alone until the linking phase.

Chris_abc · January 23, 2022, 8:53am

Thank you for the concise explanation! I got it working with your help, and the following is what I now have for this example, I still have to remake the bigger project into this structure.
But before I do that, I would like to ask if there are any things you spot that are not ‘best practice’? I would like to prevent problems in the future by learning this stuff the right way

main.cpp:

#include "main_head.h"

void setup() {
  // put your setup code here, to run once:
}

void loop() {
  bool succes = readAccel(&xMeas,&yMeas,&zMeas);
}

main_head.h: (I put my global variables in here because I found it ‘cleaner’. This file will no longer be included anywhere except for the main.cpp file. Is this okay, or can it cause problems later on?)

#ifndef MAIN_HEAD_H
#define MAIN_HEAD_H

#include <Arduino.h>
#include <Wire.h>

//global var:
float xAccel, yAccel, zAccel;
float xMeas, yMeas, zMeas;
int KXTJ3 = 0x0E;

//other functions:
extern bool readAccel(float *,float *,float *);

#endif

accel_readout.cpp:

#include "accel_read_head.h"

bool readAccel(float *xAccel,float *yAccel,float *zAccel){
  bool succes = false;
  byte x0Data, x1Data, y0Data, y1Data, z0Data, z1Data;
  if( 
    readByte(byte(0x06), x0Data)&&
    readByte(byte(0x07), x1Data)&&
    readByte(byte(0x08), y0Data)&&
    readByte(byte(0x09), y1Data)&&
    readByte(byte(0x0A), z0Data)&&
    readByte(byte(0x0B), z1Data)
  ){
    succes = true;
  }
  *xAccel = x0Data+x1Data;
  *yAccel = x0Data+x1Data;
  *zAccel = x0Data+x1Data;
  return succes;
}

accel_read_head.h:

#ifndef ACCEL_READOUT_H
#define ACCEL_READOUT_H

#include <Arduino.h>
extern bool readByte(byte, byte &);

#endif

read.cpp:

#include "read_head.h"

extern int KXTJ3;

bool readByte(byte register_address, byte &data){
  bool success = false;                     // set default to no success
  Wire.beginTransmission(KXTJ3);
  Wire.write( register_address);            // select register
  int error = Wire.endTransmission( false); // false for repeated start
  if( error == 0){                          // no error ? then data can be requested
    int n = Wire.requestFrom(KXTJ3, 1);
    if( n == 1){                            // received the bytes that were requested ?
      data = (byte) Wire.read();
      success = true;                       // only now success is true
    }
  }
  return(success);
}

read_head.h:

#ifndef READ_H
#define READ_H

#include <Arduino.h>
#include <Wire.h>

#endif

Kind regards,
Chris

maxgerhardt · January 23, 2022, 12:28pm

The logic looks sound. This should compile & work.

From a code-style perspective, I could say…

In general you should avoid global state (=variables) as much as possible (see here why). You’re using KXTJ3 here as the target I2C address. Why not have have this go in as a parameter too, forwarded by the readAccel function? Further, too much of the variables are global although they don’t need to be. In this example, xMeas, yMeas, zMeas could be local variables of the loop() function, or static global variables of the main.cpp function. static prevents the leakage of the global variables into other files, making them file-scope global. Further, float xAccel, yAccel, zAccel; shouldn’t be global variables at all, they are local parameters to the readAccel function are not accessed outside in any other file.
We usually name header and implementations file with the same base name, so e.g., read.cpp would match with read.h. But your names are all different – you have accel_read_head.h and read_head.h as headers and their functionality is implemented in accel_readout.cpp and read.cpp. This is unusual and confusing naming.
The headers don’t even match their cpp files. E.g., the header main_head.h declares the function readAccel which is implemented in accel_readout.cpp that by name looks to be more associated with accel_read_head, but isn’t. The accel_readout.cpp functions should be declared in a accel_readout.h file where readAccel is exposed.

This is bad, the header exposes no functions implemented by this header. You include it in #include "read_head.h" because this way you get the headers needed for implementation. Usually, you only include the bare minimum of headers that are needed to make the function declarations in the header valid. The implementatino can have additional headers. E.g.,

#ifndef MY_LIB_H_
#define MY_LIB_H

#include <stdint.h> //uint32_t, int16_t

uint32_t my_cool_library_func(int16_t arg);

#endif /* MY_LIB_H */

coupled with

#include <mylib.h>
#include <stdio.h>
#include <vector>
#include <string>
#include <other_lib_needed_for_implementation.h>
#include <yet_other_lib_needed_for_implementation.h>

uint32_t my_cool_library_func(int16_t arg) {
  /* ... */
}

The exposed header stays clean, all headers only needed for the implementation stay inside the implementation file. If your header does not expose functions of a compilation unit, do not write it all. Of course, the correct way would have been to declare bool readByte(byte register_address, byte &data); in the heaer because it’s impelmented in read.cpp, but you moved that accel_read_head.h, which again is weird.

I would argue that you split the logic in too many parts – in basically 1 function per file. The readByte() logic needed by readAccel could well go into one file named after the sensor, KXTJ3. This is however very subjective.
Avoid magic values. You do readByte(byte(0x06), x0Data)&& readByte(byte(0x07), x1Data)&& ... to read out the accelerometer data. No one besides you or people who read the datasheet will know why in the world there is that 0x06 that goes up to 0x0B. One usually creates macros to give these constants a more meaningful name, like, #define KXTJ3_REG_X_ACCEL_LOW_BYTE 0x06 and then use that name. One can also use enums, as e.g. here.
Your C++ writing style is very C-like. Except for the usage of a refernce (byte&) and calling into some classes, you write all code in global functions, no classes created by yourself. This is valid, but not the usual implementation style when it comes to Arduino libraries and sensors. They all encapsulate the needed functionality in a class. Take a look at e.g. matmunk/DS18B20. Or adidax/dht11. Or adafruit/Adafruit_TSL2561. All the same concept – you create your sensor as an object of a provided C++ class with the needed parameters (like, I2C address, the I2C bus object / Wire), and then you have convenient read functions. You can do exactly the same thing here. This simplifies your files down to two for the sensor and one for the user.

I would write:
main.cpp

#include <Arduino.h>
#include <KXTJ3.h>

// 7-bit address is always either 0001110 or 0001111 (=0x0e, 0x0f) 
// depending on ADDR pin.
#define KXTJ3_SENSOR_ADDR 0x0e

// create sensor object from I2C address
KXTJ3 kxtj3(KXTJ3_SENSOR_ADDR);

void setup() {
}

void loop() {
    // happily read data
    float xAccel, yAccel, zAccel;
    kxtj3.readAccel(&xAccel, &yAccel, &zAccel);
}

KXTJ3.h

#ifndef _KTXTJ3_H_
#define _KTXTJ3_H_

#include <Arduino.h> // byte

class KXTJ3 {
public:
    KXTJ3(int i2c_address);
    bool readAccel(float *xAccel,float *yAccel,float *zAccel);
private:
    bool readByte(byte register_address, byte &data);
    int m_i2c_address;
};

#endif /* _KTXTJ3_H_ */

KXTJ3.cpp

#include "KXTJ3.h"
#include "Wire.h"

KXTJ3::KXTJ3(int i2c_address) {
  this->m_i2c_address = i2c_address;
}

bool KXTJ3::readAccel(float *xAccel,float *yAccel,float *zAccel) {
  if(xAccel == nullptr || yAccel == nullptr || zAccel == nullptr) 
    return false;
  bool succes = false;
  byte x0Data, x1Data, y0Data, y1Data, z0Data, z1Data;
  if( 
    readByte(byte(0x06), x0Data) &&
    readByte(byte(0x07), x1Data) &&
    readByte(byte(0x08), y0Data) &&
    readByte(byte(0x09), y1Data) &&
    readByte(byte(0x0A), z0Data) &&
    readByte(byte(0x0B), z1Data)
  ){
    succes = true;
  }
  *xAccel = (int16_t) x1Data;
  *yAccel = (int16_t) y1Data;
  *zAccel = (int16_t) z1Data;
  return succes;
}

bool KXTJ3::readByte(byte register_address, byte &data){
  bool success = false;                     // set default to no success
  Wire.beginTransmission(m_i2c_address);
  Wire.write( register_address);            // select register
  int error = Wire.endTransmission( false); // false for repeated start
  if( error == 0){                          // no error ? then data can be requested
    int n = Wire.requestFrom(m_i2c_address, 1);
    if( n == 1){                            // received the bytes that were requested ?
      data = (byte) Wire.read();
      success = true;                       // only now success is true
    }
  }
  return(success);
}

Also notice how there is only 1 global variable in this code, the sensor object. Other than that, there don’t need to be any. The sensor object has a member variable that stores its I2C address, and the returned acceleration data is stored in function-local variables.

maxgerhardt · January 23, 2022, 12:46pm

And, in hindsight, even that

maxgerhardt:

  if( 
    readByte(byte(0x06), x0Data) &&
    readByte(byte(0x07), x1Data) &&
    readByte(byte(0x08), y0Data) &&
    readByte(byte(0x09), y1Data) &&
    readByte(byte(0x0A), z0Data) &&
    readByte(byte(0x0B), z1Data)
  ){

maxgerhardt:

    int n = Wire.requestFrom(m_i2c_address, 1);
    if( n == 1){                            // received the bytes that were requested ?
      data = (byte) Wire.read();
      success = true;                       // only now success is true
    }

is bad. These will cause individual I2C request to get each byte. Per datasheet

Reading from a KXTJ3 8-bit Register
Note that the KXTJ3 automatically increments through its sequential registers, allowing data to be read from multiple registers following a single SAD+R (slave-address and read) command as shown below in Sequence 4.

Note Accelerometer’s output data should be read in a single transaction using the auto-increment feature to prevent output data from being updated prior to intended completion of the read transaction.

Meaning instead of a 1-byte request for each register (Wire.requestFrom(m_i2c_address, 1)), you can a single 6-byte I2C request at the starting register, 0x06, to get all data at once returned in one I2C operation, atomically. This improves data throughput and latency of a sensor reading and is even needed for correctness. I leave this optimization as an exercise for the reader.

Many sensors implement this auto-increment address feature.

maxgerhardt · January 23, 2022, 12:57pm

And as a second correction, this is also wrong. First of all, x0Data and x1Data are the only variables used in all x,y and z variable computation, when it should have been y0Data, etc. Second of all, the x0Data, x1Data describe low + high bytes of a 16-bit dataword. For combining them, you cannot just add them, the byte needs to be shifted by the amounts noted in the datasheet. If you have two 8-bit values e.g. as the high and low byte, to produce the 16-bit value, you should write uint16_t x = (high << 8) | (low);. In this case, bitwise | and + are equivalent.

The datasheet says

And the default resolution of the sensor is 8-bit (page 32), is 8-bit, so since you don’t alter that, the sensor stays in 8-bit mode and XOUT_L is 0 all thetime and XOUT_H will have the actual data.

Still, for valid conversion, the datasheet says that these create a signed value (page 28), not unsigned so they should first be casted into a int16_t (int8_t must however be used in the 8-bit case since the source data is only 8 bits).

For the 8-bit case then, it would be more correct to do

  //only valid for 8-bit mode
  *xAccel = (int8_t) x1Data;
  *yAccel = (int8_t) y1Data;
  *zAccel = (int8_t) z1Data;

For 12-bit mode, we could write

  //only valid for 12-bit mode
  *xAccel = (int16_t)((x1Data << 4) | (x0Data >> 4));
  *yAccel = (int16_t)((y1Data << 4) | (y0Data >> 4))
  *zAccel = (int16_t)((z1Data << 4) | (z0Data >> 4))

Note

a = llll0000
b = hhhhhhhh

b << 4 = hhhhhhhh0000
a >> 4 = 00000000llll
- OR'ed-------------
         hhhhhhhhllll

Chris_abc · January 24, 2022, 9:10pm

Hello Maxgerhardt, Thank you again for taking your time to answer me so extensively, it’s truly very helpful.

question:
I have been trying to implement your suggestions, and it all makes a lot of sense. However I still have a question:

The headers don’t even match their cpp files. E.g., the header main_head.h declares the function readAccel which is implemented in accel_readout.cpp that by name looks to be more associated with accel_read_head, but isn’t. The accel_readout.cpp functions should be declared in a accel_readout.h file where readAccel is exposed.

In main_head.h, I declare the function as an ‘extern’, is that not right? If I omit the line extern bool readAccel(float *,float *,float *);, PIO tells me identifier "readAccel" is undefined. I thought using extern was the whole point here; you tell the compiler that the function will be used here, but is declared somewhere else.

Of course, the correct way would have been to declare bool readByte(byte register_address, byte &data); in the heaer because it’s impelmented in read.cpp , but you moved that accel_read_head.h , which again is weird.

I think this is the same point (or I am misunderstanding it): don’t I need the extern bool readByte(byte, byte &); in the header of accel_readout.cpp, so that the compiler knows that the function will be declared somewhere else (read.cpp in this case)?

C++ classes:
I will try to do this in the way you implemented it (in a couple of days I will probably have more questions )! My C/C++ programming knowledge is mainly based on the Arduino forums, so I have no doubt a lot to learn in doing it properly, but with people like you willing to help, I will get better!

I2C readout
Wow I was just happy I finally got something working… but what you describe is such an obvious improvement (necessity even)… I will 100% get that implemented.

Data Adresses
Sorry! I should have stated this! the x0Data+x1Data were bogus lines to make a ‘working’ example of my error that was not unnecessarily big.
However, it’s still great you showed how it can be done, because your implementation is 20x cleaner and shorter than my own, so I will further improve my code with it.

Thanks for all the help, it’s greatly appreciated.
I’m wondering, do you have a paypal or something similar so I can buy you a virtual beer/coffee? (please DM if so)