Rework I2C interface #13
Conversation
Remove I2C device address requirement for SPI busses Add error and warning codes Also refactor some things for consistency
Add gitattributes and gitignore files. Rename LICENSE to LICENSE.md for language recognition. Update library description.
Update top header text to SPDX licensing. Refactor base class. Refactor base i2c class. Create Device settings class. Add helper utility functions for i2c. Refactor arduino i2c class. Rename sfe_i2c_arduino.cpp to include the last _.
Add error for null dev settings. Implement read and write functions. Remove old commented out code.
Fix how passing in the device settings should work.
Changed () to {} in constructors.
| *.mo | ||
|
|
||
| #Mr Developer | ||
| .mr.developer.cfg |
There was a problem hiding this comment.
Thoughts on changing the .gitignore to include only relevant things for this repo? I'm not really a fan of continually copying "the one master .gitignore" for all projects. What the heck even is .mr.develop.cfg?
| author=SparkFun Electronics | ||
| maintainer=SparkFun Electronics | ||
| sentence=A utility library that other SparkFun libraries can take advantage of. | ||
| sentence=A generic communications utility library that other SparkFun libraries can take advantage of. |
There was a problem hiding this comment.
The reason it's called "Toolkit" is because Kirk was thinking about expanding it to do more than just communication at some point.
| #pragma once | ||
|
|
||
| #include "sfe_i2c_arduino.h" | ||
| // #include "sfe_spi_arduino.h" |
There was a problem hiding this comment.
Shouldn't this just #include "sfe_bus.h"?
|
|
||
| // To repeatedly use this toolkit, you may need to wrap this class in a namespace. | ||
| // namespace sfe_XXX { | ||
| /// @brief An abstract Bus address class for enabling multiple types of addresses. |
There was a problem hiding this comment.
Suggest changing to something like "An abstract class for specifying parameters required to communicate with a particular device on a bus"
| // To repeatedly use this toolkit, you may need to wrap this class in a namespace. | ||
| // namespace sfe_XXX { | ||
| /// @brief An abstract Bus address class for enabling multiple types of addresses. | ||
| class SFEBusDevSettings{}; // Nothing to see here... |
There was a problem hiding this comment.
Nit: thoughts on Params instead of Settings?
| while (bytesToSend > 0) | ||
| { | ||
| // Limit sendLength to the size of the I2C buffer to send in chunks. | ||
| uint8_t sendLength = (bytesToSend > _i2cBufferSize) ? _i2cBufferSize : bytesToSend; |
There was a problem hiding this comment.
There may be an off-by-2 error here, because both pAddr->devAddr and regAddr have to be stored in the Wire.h buffer. Same with the other read/write functions.
| if (numRead < readLength) | ||
| return SFE_BUS_W_UNDER_READ; | ||
|
|
||
| if (_i2cBus->available()) | ||
| { | ||
| for (uint8_t i = 0; i < readLength; i++) | ||
| data[readOffset + i] = _i2cBus->read(); | ||
| } | ||
| else | ||
| return SFE_BUS_E_NO_RESPONSE; |
There was a problem hiding this comment.
I think this order should be flipped. First loop through numRead then check if (numRead < readLength). That way the user could still get partial data at least.
| _i2cBus->write(regAddr); | ||
|
|
||
| // Repeat start condition, return if there's an error. | ||
| int8_t result = _mapError(_i2cBus->endTransmission(false)); |
There was a problem hiding this comment.
Don't think this should restart, it should send a stop bit.
| // Start transmission. | ||
| _i2cBus->beginTransmission(pAddr->devAddr); | ||
|
|
||
| // Repeat start condition, return if there's an error. | ||
| int8_t result = _mapError(_i2cBus->endTransmission(false)); | ||
| if (SFE_BUS_OK != result) | ||
| return result; |
There was a problem hiding this comment.
I don't think this is necessary, should just call _i2cBus->requestFrom()
| if (!error) | ||
| return SFE_BUS_OK; | ||
| else if (error == 1) | ||
| return SFE_BUS_E_DATA_TOO_LONG; | ||
| else if ((error == 2) || (error == 3)) | ||
| return SFE_BUS_E_NO_RESPONSE; | ||
| else if (error == 5) | ||
| return SFE_BUS_E_TIMEOUT; | ||
| else | ||
| return SFE_BUS_E_UNKNOWN; |
There was a problem hiding this comment.
Might be nice to add a comment with a link here: https://www.arduino.cc/reference/en/language/functions/communication/wire/endtransmission/
After the previous discussion based on Dryw's PR, I rewrote the interface with the discussed changes. SPI is unchanged in this PR. Tested with the OPT4048 library in the examples folder and it works.