Arduino I2C implementation doesn't check for device register validity.

[SFE-Brudnerd]

There is no checking done to ensure that devReg is not nullptr. IMO this could be accomplished by adding a check above the write on line 168 like so.

if (devReg != nullptr && regLength > 0)
    _i2cPort->write(devReg, regLength);

SparkFun_Toolkit/src/sfTkArdI2C.cpp

Lines 143 to 174 in 2aa0fde

	sfTkError_t sfTkArdI2C::readRegister(uint8_t *devReg, size_t regLength, uint8_t *data, size_t numBytes,
	size_t &readBytes)
	{
	// got port
	if (!_i2cPort)
	return ksfTkErrBusNotInit;
	// Buffer valid?
	if (!data)
	return ksfTkErrBusNullBuffer;
	readBytes = 0;
	uint16_t nOrig = numBytes; // original number of bytes.
	uint8_t nChunk;
	uint16_t nReturned;
	uint16_t i; // counter in loop
	bool bFirstInter = true; // Flag for first iteration - used to send devRegister
	while (numBytes > 0)
	{
	if (bFirstInter)
	{
	_i2cPort->beginTransmission(address());
	_i2cPort->write(devReg, regLength);
	if (_i2cPort->endTransmission(stop()) != 0)
	return ksfTkErrFail; // error with the end transmission
	bFirstInter = false;
	}

[PaulZC]

I need this fix too - but for a different reason:

I need to be able to set the register address, then wait 2ms, then perform the read.

Adding an optional delay between the devReg write and the requestFrom read would be ugly.

My proposal is to wrap the the whole devReg write in the if statement. I will implement the delay externally.

Let me know if you want this as a Pull Request.

    bool bFirstInter = true; // Flag for first iteration - used to send devRegister

    while (numBytes > 0)
    {
        if (bFirstInter)
        {
            if (devReg != nullptr && regLength > 0)
            {
                _i2cPort->beginTransmission(address());

                _i2cPort->write(devReg, regLength);

                if (_i2cPort->endTransmission(stop()) != 0)
                    return ksfTkErrFail; // error with the end transmission
            }

            bFirstInter = false;
        }

[gigapod]

Yeah - missing this pointer check is a mistake - easily fixed.

Regarding the detail:
Breaking a transaction between two method calls - which do different things based off of inputs is a little concerning - I wonder if this could cause some weird side-effects or unexpected behavior in the future.

Options here would be one of the following:

1. Pauls approach - change method behavior based on input params.
2. Change the underlying implementation - breaking up the method into two sub-methods - one for writing the register, one for reading the data. The current read routine would just call these, but a user could call them individually.
3. Just add a delay parameter to the read (and probably) write methods on these core interface methods. 11

I lean to option 3, since the method interface is explicit - use this delay (ms) between address write and data read. Nothing implied and no possibility that the transaction is left hanging (write address and never do the read).

The big question - is there anything else that one would want to do between address write and data read? and if so, do we care?

-K

[PaulZC]

Thanks Kirk,

1. We should add the if (devReg != nullptr && regLength > 0) check regardless. It prevents possible badness and corruption, but also makes it possible to set a zero regLength which is what I need for the coprocessor.

2. would be nice - if time permits?

3. I'm leaning away from this. The delay between write and read is quite specific to this one chip. I can't think of any other chip that needs the same feature. And the delay is easy to add externally - so long as we get 1. with or without 2...

It's your call. I'd be happy with any / all of the above.

Thanks!

[gigapod]

If this special device had an SPI interface ,would the same type of timing delay needs be needed?

If I break the readRegister() command into readRegSendAddr() and readRegGetData() methods, I would probably do this at the bus Interface level ...

[gigapod]

Okay - I did the following:

• Created the branch - feature/read-reg-delay
• I went for option 3 - I just can't get my head around giving the caller an option to leave the bus in a bad state (send reg, don't read data). Also the time out param is transparent since it's optional and the method signature/param is explicit in what it does.
• I added this to all readRegister() calls (in the toolkit).
• And I fixed several parameter checking guards on in the current implementation.

@PaulZC - give this a try and see if it works. It compiles, but I haven't actually tested the implementation.

-Kirk

[PaulZC]

@gigapod - thanks! The feature/read-reg-delay branch is working great. Please merge and release. Cheers!

[gigapod]

Fixed, merged and release (v1.0.5) published.