What is the best layout Layout for derived and base classes

Hello :slight_smile:

I am wanting to interface to some I2C sensors. being a clever dick I want to have the common functions IsConnected(), Read(int address, int length), Read(int iAddress, int iLength, uint8_t buffer) and Write(int iAddress, int iData) in a base class I2C

///////////////////////////////////////////////////////////////////
//
// I2C.h
//
///////////////////////////////////////////////////////////////////

#ifndef I2C_h
#define I2C_h

#include <inttypes.h>
#include "../Wire/Wire.h"

class I2C {
	public:
						I2C(int iDevice, int iAddress, int iID);	
		bool            IsConnected(void);
	protected:
		void			Write(int iAddress, int iData);
		uint8_t*		Read(int iAddress, int iLength);
		void			Read(int iAddress, int iLength, uint8_t buffer[]);
	private:
		uint8_t			iDeviceAddress;
		uint8_t			iRegiserAddress;
		uint8_t			iDeviceID ;
	};


#endif
///////////////////// End of I2C.H ////////////////////////////////

and the C++

///////////////////////////////////////////////////////////////////
//
// I2C.cpp
//
///////////////////////////////////////////////////////////////////

#include <I2C.h>
#include "Arduino.h"


///////////////////////////////////////////////////////////////////
//
// public functions
//
///////////////////////////////////////////////////////////////////

///////////////////////////////////////////////////////////////////
//
// constructor
//
// I2C(int iDevice, iAddress, int iID)
//
// sets the device address register address and device id
//
// returns: true if I2C is connected otherwise false if not
//
///////////////////////////////////////////////////////////////////

I2C::I2C(int iDevice, int iAddress, int iID)
{
	iDeviceAddress  = iDevice;
	iRegiserAddress = iAddress;
	iDeviceID       = iID;
}

///////////////////////////////////////////////////////////////////
//
// IsConnected(void)
//
// NOTE not compatible with error codes
//
// returns: true if I2C device is connected otherwise false if not
//
///////////////////////////////////////////////////////////////////

bool I2C::IsConnected(void)
{
	uint8_t *data = Read(iRegiserAddress, 1);
	return (*(data) == iDeviceID ) ?  true : false;
}


///////////////////////////////////////////////////////////////////
//
// protected functions
//
///////////////////////////////////////////////////////////////////
//
// Write(int iAddress, int iData)
//
// sends data iData to I2C device iAddress over I2C buss
//
// returns: nothing
//
///////////////////////////////////////////////////////////////////

void I2C::Write(int iAddress, int iData)
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.write(iData);
	Wire.endTransmission();
}

///////////////////////////////////////////////////////////////////
//
// Read(int iAddress, int iLength)
//
// reads idata from I2C device iAddress and sucessive addresses 
// over I2C  buss
//
// returns: uint8_t* to data
//
///////////////////////////////////////////////////////////////////

uint8_t* I2C::Read(int iAddress, int iLength)
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.endTransmission();
  
	Wire.beginTransmission(iDeviceAddress);
	Wire.requestFrom(iDeviceAddress, iLength);

	uint8_t buffer[iLength];
	while(Wire.available()) {
		for(uint8_t i = 0; i < iLength; i++) {
			buffer[i] = Wire.read();
		}
	}

	Wire.endTransmission();

	return buffer;
}

///////////////////////////////////////////////////////////////////
//
// Read(int iAddress, int iLength, uint8_t buffer[])
//
// reads idata from I2C device iAddress and length sucessive 
// addresses over i2C buss into the address pointed to by buffer
//
// returns: nothing
//
///////////////////////////////////////////////////////////////////

void I2C::Read(int iAddress, int iLength, uint8_t buffer[])
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.endTransmission();
  
	Wire.beginTransmission(iDeviceAddress);
	Wire.requestFrom(iDeviceAddress, iLength);

	while(Wire.available()) {
		for(uint8_t i = 0; i < iLength; i++) {
			buffer[i] = Wire.read();
		}
	}

	Wire.endTransmission();
}

///////////////////////// end of I2C.cpp ////////////////////

Ok so far so good. Now for the questions

Should the base class be stored in the same directory as the I2CDEVICE.c++ and I2CDEVICE.h ? If not where should they be stored?

Before answering that, I have some problems with your code.

First, why:

#include "../Wire/Wire.h"

Why not:

#include <Wire.h>

Second, this is wrong:

uint8_t* I2C::Read(int iAddress, int iLength)
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.endTransmission();
  
	Wire.beginTransmission(iDeviceAddress);
	Wire.requestFrom(iDeviceAddress, iLength);

	uint8_t buffer[iLength];
	while(Wire.available()) {
		for(uint8_t i = 0; i < iLength; i++) {
			buffer[i] = Wire.read();
		}
	}

	Wire.endTransmission();

	return buffer;
}

You don’t wrap Wire.requestFrom inside Wire.beginTransmission() … Wire.endTransmission();

http://www.gammon.com.au/i2c


	return buffer;

Third, in the above code, see:

http://www.gammon.com.au/forum/?id=12153#trap18


Fourth:

void I2C::Write(int iAddress, int iData)
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.write(iData);
	Wire.endTransmission();
}

Since Wire.write writes a byte, there doesn’t seem to be any point passing an int. How about:

void I2C::Write(const byte iAddress, const byte iData)
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.write(iData);
	Wire.endTransmission();
}

Fifth, since Wire.endTransmission() can fail you are probably better off passing back a return code, eg.

byte I2C::Write(const byte iAddress, const byte iData)
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.write(iData);
	return Wire.endTransmission();
}

Sixth, Wire.requestFrom does not necessarily return all the bytes you wanted. You should probably have a “pass” or “fail” boolean. eg.

        if (Wire.requestFrom(iDeviceAddress, iLength) != iLength)
          return false;   // failed to read iLength bytes

Should the base class be stored in the same directory as the I2CDEVICE.c++ and I2CDEVICE.h ?

I don’t see a major issue with that. :slight_smile:

thank you :slight_smile: this is going to take some ploughing through before I do a more detailed reply.

You are free to put class definitions where you like - all in one file even, although
you should put relevant class declarations in a .h file if something external wants
to use it. File boundaries tend to relate to the granularity of library / module, rather
than class.

Many libraries are a single class in the Arduino world, but that's not a requirement
of any sort.

Ease of finding a definition encourages a separate file for each class of course, even if
they are small (such as lots of little derived classes).

By file I may mean "pair of .cpp and .h files" above, thinking about it.

I can't quite work out what abstraction the I2C class is intended to represent. Is it intended to represent an I2C bus, or an I2C device on that bus? In the former case I'd argue that your devices should use the bus, not subclass it. In the latter case I'd suggest that the name ought to reflect that - for example, call the base class I2cDevice rather than I2C.

Point One, two and Three:

thank you, were the results of blindly copying someone else’s code fragments… The original code smith will remain un credited… to save their embarrassment, because the rest of the code was so riddled with bugs and outright misunderstandings of the operation of the device as to warrant an almost total re write from scratch just to get acceptable performance.

Yes I hated this when I first saw it coming from the old zortech/ zorland/digital mars c++ on the PC

But this works so it must be a compromise in the compiler

I’m going to replace that function with

///////////////////////////////////////////////////////////////////
//
// ReadBYTE(int address)
//
// reads 1 Byte from I2C device iAddress over I2C buss
//
// returns: BYTE* to data
//
///////////////////////////////////////////////////////////////////

BYTE I2C::ReadBYTE(int iAddress)
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.endTransmission();
  
	Wire.requestFrom((int)iDeviceAddress, 1);	//	cast coz the compiler refuses BYTE and byte

	BYTE buffer;
	if(Wire.available() == 1) {
		buffer = Wire.read();			// read data
	}
	return buffer;
}

and

///////////////////////////////////////////////////////////////////
//
// Read(int iAddress, int iLength, BYTE *buffer)
//
// reads idata from I2C device iAddress and length sucessive 
// addresses over I2C buss into the address pointed to by buffer
//
// returns: nothing
//
///////////////////////////////////////////////////////////////////

void I2C::Read(int iAddress, int iLength, BYTE *buffer)
{
	Wire.beginTransmission(iDeviceAddress);
	Wire.write(iAddress);
	Wire.endTransmission();
  
	Wire.requestFrom((int)iDeviceAddress, iLength);	//	cast coz the compiler refuses BYTE and byte

	if(Wire.available() == iLength) {
		for(BYTE i = 0; i < iLength; i++) {			// 
			*(buffer++) = Wire.read();			// read data and save in pointer location then inc pointer
	  	}
	}
}

of course prudent use the later of the two functions would requite a call to new BYTElength and delete

PeterH:
I can't quite work out what abstraction the I2C class is intended to represent. Is it intended to represent an I2C bus, or an I2C device on that bus? In the former case I'd argue that your devices should use the bus, not subclass it. In the latter case I'd suggest that the name ought to reflect that - for example, call the base class I2cDevice rather than I2C.

looking from the lazy point of view here, and believe me I am very lazy. It occurred to me that the some functions of an I2C device are common to all I2C devices. Reads() writes() and IsConnected() being good examples. So rather than rewrite these bits for each device it makes sense to have them in a base class and the device specific function in the derived class. The advantage being that I do not have to debug the I2C base class each coz I know it works. Also it makes it easier to resist the temptation to tinker with the I2C functions.

BYTE I2C::ReadBYTE(int iAddress)

{
Wire.beginTransmission(iDeviceAddress);
Wire.write(iAddress);
Wire.endTransmission();
 
Wire.requestFrom((int)iDeviceAddress, 1); // cast coz the compiler refuses BYTE and byte

BYTE buffer;
if(Wire.available() == 1) {
	buffer = Wire.read();			// read data
}
return buffer;

}

I would still prefer:

int I2C::ReadBYTE(const byte iAddress)
{
  Wire.beginTransmission(iDeviceAddress);
  Wire.write(iAddress);
  Wire.endTransmission();

  if (Wire.requestFrom(iDeviceAddress, (byte) 1) != 1)
    return -1;

  return Wire.read ();
}

You don't need a one-byte buffer, and this returns -1 if nothing is read.

Your second function has no way of detecting if you don't read all iLength bytes.

You may not care, but every now and then we get complaints about the RTC (clock) chips returning garbage. And guess what? The RTC code doesn't check if if gets the right number of bytes or not.

SuusiMB:
So rather than rewrite these bits for each device it makes sense to have them in a base class and the device specific function in the derived class. The advantage being that I do not have to debug the I2C base class each coz I know it works. Also it makes it easier to resist the temptation to tinker with the I2C functions.

Oh, I agree with that entirely. But in that case it seems to me that the base class ought to be named to indicate that it represents a device (not the bus as a whole) and the device ought to know what its address is rather than requiring that to be supplied for each operation.

Well thank you all.

I now have the BMP180 and by implication the BMP085 working as a derived class. It now works in all modes except Advanced resolution mode.

On to the HMC5833L for the same treatment :smiley:

its nice returning to C++ after a break of nearly 20 years. A few things have changed, but its nice to know that friendly help is not far away if I get really stuck.

Interesting, and thank you. My application is going to need a RTC at some point. Once I have figured out how to add a second screen, then do pitch roll and yaw.

I am not sure that returning -1 is the answer, I would tend to go with setting an error code in I2C

That's fine. Whichever design decision works best for you. :slight_smile:

Good luck with the project.

completing the BMP180 and HMC5883L derived class's has forced me into finding an answer to my original question.

In my case it turns out that the only way that it will compile is to have the base class as a separate library. Every other sane layout (and some that were less sensible - don't ask) that I tried threw up compiler errors due to multiple definitions.

Once again thank you All

BTW thats one interesting website Nick thanx

Suusi M-B