Added temp sensor #31
Conversation
bvngee
left a comment
bvngee
left a comment
There was a problem hiding this comment.
@meghanabaddipudireddy Thanks for this! Sorry for the delay in reviewing it. I left 6 or so comments, which are all relatively simple changes. Let me know when you can get a chance to work on these (hoping to test this out on an old fs-3 bms/acc board as soon as break is over!)
| @@ -1,16 +1,19 @@ | |||
| #include "LTC6811.h" | |||
| #include "LTC6810.h" | |||
There was a problem hiding this comment.
@meghanabaddipudireddy Instead of renaming/replacing the LTC6811 class with the LTC6810 class, could you keep them both but as separate files? This should be fairly straightforward since you still have the LTC6811 version in the main branch. Reasoning being that we will very likely want to test this TMP1075 code on an old spare fs-3 BMS board and acc board, which uses the LTC6811, but we'll use the LTC6810 for fs-4's bms, so we should be ready to use both at any time.
| uint16_t *getGpioPin(GpioSelection pin); | ||
| void buildCOMMBytes(uint8_t icom, uint8_t fcom, uint8_t data, uint8_t *commBytes); | ||
| float readTemperatureTMP1075(); | ||
| bool verifyI2CStatus(uint8_t *rxData); |
There was a problem hiding this comment.
These functions should be static if they're gonna be in the LTC6811/LTC6810 class, since they don't act on any of the data of the instance of LTC6811/LTC6810.
| m_chips.push_back(LTC6810(bus, i)); | ||
| } | ||
| for (int i = 0; i < BMS_BANK_COUNT; i++) { | ||
| // m_chips[i].getConfig().gpio5 = LTC6811::GPIOOutputState::kLow; |
There was a problem hiding this comment.
See my comment below; the change to GPIOOutputState::kHigh gpio configs should be done here (in the constructor of BMSThread, where the LTC6811's are created) rather than in the default constructor of the LTC6811
| .gpio2 = GPIOOutputState::kPassive, | ||
| .gpio4 = GPIOOutputState::kHigh, | ||
| .gpio3 = GPIOOutputState::kHigh, | ||
| .gpio2 = GPIOOutputState::kHigh, |
There was a problem hiding this comment.
I think GPIOOutputState::kPassive is probably the correct default for the LTC6811 class, and these changes to GPIOOutputState::kHigh should be moved to where we construct the class, in BMSThread's constructor. See my comment above in BMSThread
|
|
||
| LTC6811::LTC6811(LTC681xBus &bus, uint8_t id) : m_bus(bus), m_id(id) { | ||
| #define TMP1075_I2C_ADDR 0x48 | ||
| #define TMP1075_TEMP_REG 0x00 |
There was a problem hiding this comment.
These #defines should be removed, since they are only true for some configurations of the TMP1075 (eg. 0x48 is the I2C address for a sensor where all three adress pins A0/A1/A2 are pulled low, but we will have multiple configurations). We will need to use many different TMP1075s with different I2C addresses.
Instead, they should be made parameters to LTC6810::readTemperatureTMP1075().
| commBytes[1] = ((data & 0x0F) << 4) | fcom; | ||
| } | ||
|
|
||
| float LTC6810::readTemperatureTMP1075() { |
There was a problem hiding this comment.
Eventually we should probably try to decouple the TMP1075 I2C register logic from the LTC6811 I2C COMM group logic, in case we want to use the I2C for something else. But its alright for now, until we get it working
Changed the LTC6811 to the LTC6810 and made adjustments.
Also configured LTC6810 to work as I2C
All information on how the code works and what the functions do is in the document labeled BMS Temp Sensing