Add I2C and SPI interfaces #11
Conversation
Remove I2C device address requirement for SPI busses Add error and warning codes Also refactor some things for consistency
SFE-Brudnerd
left a comment
SFE-Brudnerd
left a comment
There was a problem hiding this comment.
Overall it's a good start, I think there's just a few different things we should be doing differently, however I like the overall structure as it currently stands.
| #define SFE_BUS_E_NO_RESPONSE -4 | ||
| #define SFE_BUS_E_DATA_TOO_LONG -5 | ||
| #define SFE_BUS_W_UNKNOWN 1 | ||
| #define SFE_BUS_W_UNDER_READ 1 |
There was a problem hiding this comment.
Should this be a 2 or are we ok with multiple warnings with the same code?
There was a problem hiding this comment.
Whoops, typo! Should be 2. IMO every return code should have a unique value so a user can properly debug.
| */ | ||
| class SfeI2C : public SfeBus | ||
| /// @brief An abstract interface for an I2C communication bus | ||
| class SFE_I2C : public SFE_Bus |
There was a problem hiding this comment.
I think this could be a templated class that is used to create the derived classes.
See Kirk's OLED library wrt the different types of OLEDs.
There was a problem hiding this comment.
I'm not opposed to using a template structure instead of inherited interfaces. Let's discuss this more tomorrow.
| virtual int8_t writeRegisters(uint8_t regAddr, const uint8_t *data, uint8_t numBytes); | ||
|
|
||
| /// @brief Reads a number of bytes starting at the given register address. | ||
| /// @param regAddr The first register address to read from. | ||
| /// @param data Data buffer to read from registers. | ||
| /// @param numBytes Number of bytes to read. | ||
| /// @return 0 for success, negative for failure, positive for warning. | ||
| virtual int8_t readRegisters(uint8_t regAddr, uint8_t *data, uint8_t numBytes); |
There was a problem hiding this comment.
These should not be virtual since they're the end implementations.
There was a problem hiding this comment.
That's probably a copy/paste bug, good catch!
| /// @brief An I2C communication bus implementation for Arduino | ||
| class SFE_I2C_Arduino : public SFE_I2C | ||
| { | ||
| public: |
There was a problem hiding this comment.
Missing constructor that defines the _i2cPort pointer.
There was a problem hiding this comment.
please rename to sfe_i2c_arduino.cpp.
There was a problem hiding this comment.
Oops, thought I'd fixed that, guess not!
| #define SFE_BUS_W_UNDER_READ 1 | ||
|
|
||
| /// @brief An abstract interface for a communication bus | ||
| class SFE_Bus |
There was a problem hiding this comment.
Class name does not follow code style conventions. I would rename to: SFEBaseBus.
There was a problem hiding this comment.
Good catch, my mistake! Do we want to use the word "Base" in the name? I know some people have strong opinions about that.
There was a problem hiding this comment.
It would be following how I've seen Kirk do things in the past. Base is usually used for purely virtual classes anyway.
| */ | ||
| class SfeI2C : public SfeBus | ||
| /// @brief An abstract interface for an I2C communication bus | ||
| class SFE_I2C : public SFE_Bus |
There was a problem hiding this comment.
Class name does not follow code style conventions. I would rename to: SFEBaseI2C.
| #include <Wire.h> | ||
|
|
||
| /// @brief An I2C communication bus implementation for Arduino | ||
| class SFE_I2C_Arduino : public SFE_I2C |
There was a problem hiding this comment.
Class name does not follow code style conventions. I would rename to: SFEArduinoI2C.
| */ | ||
| class SfeSPI : public SfeBus | ||
| /// @brief An abstract interface for an SPI communication bus | ||
| class SFE_SPI : public SFE_Bus |
There was a problem hiding this comment.
Class name does not follow code style conventions. I would rename to: SFEBaseSPI.
| #include <SPI.h> | ||
|
|
||
| /// @brief An SPI communication bus implementation for Arduino | ||
| class SFE_SPI_Arduino : public SFE_SPI |
There was a problem hiding this comment.
Class name does not follow code style conventions. I would rename to: SFEArduinoSPI.
No description provided.