refactor: more work on error handling and logging, as well as general cleanup

AMS_Master_Code/Core/Inc/config_ADBMS6830.h

@@ -2,15 +2,16 @@
 #ifndef __CONFIG_ADBMS6830_H
 #define __CONFIG_ADBMS6830_H
 #include "main.h"
-#define N_BMS 2
 
-[[maybe_unused]] static void mcuAdbmsCSLow() {
+#define N_BMS 2
+#define ADBMS_SPI_TIMEOUT 100 // Timeout in ms
+
+[[maybe_unused]] static inline void mcuAdbmsCSLow() {
     HAL_GPIO_WritePin(AMS_CS_GPIO_Port, AMS_CS_Pin, GPIO_PIN_RESET);
 }
 
-[[maybe_unused]] static void mcuAdbmsCSHigh() {
+[[maybe_unused]] static inline void mcuAdbmsCSHigh() {
     HAL_GPIO_WritePin(AMS_CS_GPIO_Port, AMS_CS_Pin, GPIO_PIN_SET);
 }
 
 #endif //__CONFIG_ADBMS6830_H
- 

AMS_Master_Code/Core/Inc/swo_log.h

@@ -52,10 +52,11 @@ static inline uint32_t __swo_putc(uint32_t c, unsigned int channel) {
   return (c);
 }
 
-#define DEBUG_CHANNEL_ENABLED(channel) ({ \
+#define DEBUG_CHANNEL_ENABLED(channel)                                         \
+  ({                                                                           \
     unsigned int ch = (channel);                                               \
     (ch < 32) ? __ITM_channel_enabled(ch) : false;                             \
-})
+  })
 
 [[gnu::nonnull(2), gnu::null_terminated_string_arg(2)]]
 static inline void __swo_print(unsigned int channel, const char *str) {

AMS_Master_Code/Core/Lib/ADBMS6830B_Driver/Core/Inc/ADBMS_Abstraction.h

@@ -9,15 +9,16 @@
 #define INC_ADBMS_ABSTRACTION_H_
 
 #include "ADBMS_CMD_MAKROS.h"
-#include "ADBMS_LL_Driver.h"
 #include "ADBMS_Driver.h"
+#include "ADBMS_LL_Driver.h"
 #include "main.h"
 
-//see table 103 in datasheet (page 71)
-#define DEFAULT_UV 417	//VUV * 16 * 150 uV + 1.5 V     Default Setting 2.5V
-#define DEFAULT_OV 1125 //VOV * 16 * 150 uV + 1.5 V		  Default Setting 4.2V
 
-#define mV_from_ADBMS6830(x) (((((int16_t) (x))) * 0.150) + 1500)
+// see table 103 in datasheet (page 71)
+#define DEFAULT_UV 417  // VUV * 16 * 150 uV + 1.5 V     Default Setting 2.5V
+#define DEFAULT_OV 1125 // VOV * 16 * 150 uV + 1.5 V		  Default Setting 4.2V
+
+#define mV_from_ADBMS6830(x) (((((int16_t)(x))) * 0.150) + 1500)
 
 HAL_StatusTypeDef amsReset();
 

AMS_Master_Code/Core/Lib/ADBMS6830B_Driver/Core/Inc/ADBMS_Driver.h

@@ -1,7 +1,7 @@
 #ifndef ADBMS_DRIVER_H
 #define ADBMS_DRIVER_H
 
-#include "stm32h7xx_hal.h"
+#include "config_ADBMS6830.h"
 #include <stdint.h>
 
 #define ERROR_TIME_THRESH 150 // ms
@@ -13,8 +13,8 @@ typedef enum : uint16_t {
     ADBMS_OVERVOLT,
     ADBMS_UNDERVOLT,
     ADBMS_OPENWIRE,
-    ADBMS_INTERNAL_BMS_TIMEOUT,
+    ADBMS_INTERNAL_BMS_TIMEOUT,       // BMS went to sleep too many times
-    ADBMS_INTERNAL_BMS_CHECKSUM_FAIL,
+    ADBMS_INTERNAL_BMS_CHECKSUM_FAIL, // BMS CRC failed too many times
     ADBMS_INTERNAL_BMS_OVERTEMP,
     ADBMS_INTERNAL_BMS_FAULT,
     NUM_ERROR_KINDS
@@ -76,6 +76,7 @@ typedef struct {
 
 extern uint32_t error_sources; // Bitfield of error sources
 extern SlaveErrorData error_data[NUM_ERROR_KINDS];
+extern Cell_Module modules[N_BMS];
 
 [[gnu::nonnull]] ADBMS_DetailedStatus AMS_Init(SPI_HandleTypeDef* hspi);
 

AMS_Master_Code/Core/Lib/ADBMS6830B_Driver/Core/Src/ADBMS_Abstraction.c

@@ -14,11 +14,17 @@
 
 extern uint8_t numberofCells;
 
+static const char* const HAL_Statuses[] = {"HAL_OK", "HAL_ERROR", "HAL_BUSY", "HAL_TIMEOUT"};
+
 #define CHECK_RETURN(x)                                                                                                \
     do {                                                                                                               \
         HAL_StatusTypeDef status = x;                                                                                  \
-    if (status != 0)                                                           \
+        if (status != 0) {                                                                                             \
+            debug_log(LOG_LEVEL_ERROR, "@%s:%s:%d: %s failed with status %d (%s)", __FILE_NAME__, __func__, __LINE__,  \
+                      #x, status,                                                                                      \
+                      (status < (sizeof(HAL_Statuses) / sizeof(HAL_Statuses[0]))) ? HAL_Statuses[status] : "Unknown"); \
             return status;                                                                                             \
+        }                                                                                                              \
     } while (0)
 
 HAL_StatusTypeDef amsReset() {
@@ -26,7 +32,21 @@ HAL_StatusTypeDef amsReset() {
     readCMD(SRST, CMD_EMPTY_BUFFER, 0);
 
     uint8_t sidbuffer[CMD_BUFFER_SIZE(SID_GROUP_SIZE)] = {};
-  CHECK_RETURN(readCMD(RDSID, sidbuffer, SID_GROUP_SIZE));
+    if (readCMD(RDSID, sidbuffer, SID_GROUP_SIZE) != HAL_OK) {
+        bool nonzero = false;
+        for (size_t i = 0; i < N_BMS; i++) {
+            if (sidbuffer[BUFFER_BMS_OFFSET(i, SID_GROUP_SIZE)] != 0) {
+                nonzero = true;
+            }
+        }
+        if (nonzero) {
+            debug_log(LOG_LEVEL_ERROR,
+                      "CRC failure when reading BMS IDs, but some IDs are nonzero. --- Intermittent connection?");
+        } else {
+            debug_log(LOG_LEVEL_ERROR, "CRC failure when reading BMS IDs, all IDs are zero. --- No connection?");
+        }
+        return HAL_ERROR;
+    }
 
     debug_log(LOG_LEVEL_INFO, "BMS reset complete");
 
@@ -69,7 +89,7 @@ HAL_StatusTypeDef amsWakeUp() {
 }
 
 HAL_StatusTypeDef amsCellMeasurement(Cell_Module (*module)[N_BMS]) {
-  #warning check conversion counter to ensure that continous conversion has not been stopped
+    #warning check conversion counter to ensure that continuous conversion has not been stopped
     #warning check for OW conditions: ADSV | ADSV_OW_0 / ADSV_OW_1
     return amsReadCellVoltages(module);
 }
@@ -317,11 +337,13 @@ HAL_StatusTypeDef amsReadCellVoltages(Cell_Module (*module)[N_BMS]) {
     return HAL_OK;
 }
 
-//Each selected BMS must have a corresponding address, and the data array for that BMS must be at least datalens[i] bytes long
+// Each selected BMS must have a corresponding address, and the data array for that BMS must be at least datalens[i]
-HAL_StatusTypeDef amsSendI2C(const uint8_t addresses[static N_BMS], uint8_t * data[static N_BMS], const uint8_t datalens[static N_BMS], uint32_t bms_mask) {
+// bytes long
+HAL_StatusTypeDef amsSendI2C(const uint8_t addresses[static N_BMS], uint8_t* data[static N_BMS],
+                             const uint8_t datalens[static N_BMS], uint32_t bms_mask) {
     uint8_t buffer[CMD_BUFFER_SIZE(COMM_GROUP_SIZE)] = {};
 
-  //COMM register works in 3 bytes max per go, interleaved with control information
+    // COMM register works in 3 bytes max per go, interleaved with control information
 
     uint8_t max_datalen = 0;
     for (size_t i = 0; i < N_BMS; i++) {
@@ -341,57 +363,60 @@ HAL_StatusTypeDef amsSendI2C(const uint8_t addresses[static N_BMS], uint8_t * da
         }
     }
 
-  size_t packet_count = ((max_datalen + 1) / 3) + ((max_datalen + 1) % 3 != 0); //number of 3 byte packets needed to send all data
+    size_t packet_count =
+        ((max_datalen + 1) / 3) + ((max_datalen + 1) % 3 != 0); // number of 3 byte packets needed to send all data
 
   for (size_t i = 0; i < (packet_count * 3); i++) { //i - 1 is the number of data bytes sent so far (1 byte for the address)
         for (size_t j = 0; j < N_BMS; j++) {
             size_t offset = BUFFER_BMS_OFFSET(j, COMM_GROUP_SIZE);
             if (!(bms_mask & (1 << j)))
-          continue; //skip BMS that are not selected
+                continue; // skip BMS that are not selected
 
             if (i == 0) {
                 buffer[offset + 0] = I2C_SEND_START;
-        buffer[offset + 1] = addresses[j] << 1; //shift the address left by 1 to make space for the R/W bit
+                buffer[offset + 1] = addresses[j] << 1; // shift the address left by 1 to make space for the R/W bit
                 continue;
             }
 
-      //add data to the buffer
+            // add data to the buffer
 
-      //case 1: the last group of 3 bytes contained the last data byte, so we need to send a stop
+            // case 1: the last group of 3 bytes contained the last data byte, so we need to send a stop
             if (datalens[j] - (int)i == -1) {
                 buffer[offset + ((i % 3) * 2)] = I2C_SEND_STOP;
                 continue;
             }
 
-      //case 2: we are more than one byte past the end, so we don't need to send any data
+            // case 2: we are more than one byte past the end, so we don't need to send any data
             if (datalens[j] - (int)i < -1) {
                 buffer[offset + ((i % 3) * 2)] = I2C_SEND_NOTRANSFER;
                 continue;
             }
 
-      //case 3: we are still sending data
+            // case 3: we are still sending data
             buffer[offset + ((i % 3) * 2)] = I2C_SEND;
             buffer[offset + ((i % 3) * 2) + 1] = data[j][i - 1];
         }
 
-    //send the data
+        // send the data
         if (i % 3 == 0 && i != 0) {
             CHECK_RETURN(writeCMD(WRCOMM, buffer, COMM_GROUP_SIZE));
 
-      __pollCMD(STCOMM, 72); //wait 72 cycles for the I2C transfer to complete (datasheet page 43)
+            __pollCMD(STCOMM, 72); // wait 72 cycles for the I2C transfer to complete (datasheet page 43)
-                                               //TODO: not sure if this is correct for a daisy chain
+                                   // TODO: not sure if this is correct for a daisy chain
         }
     }
 
-  //send the last packet
+    // send the last packet
     CHECK_RETURN(writeCMD(WRCOMM, buffer, COMM_GROUP_SIZE));
     __pollCMD(STCOMM, 72);
 
     return HAL_OK;
 }
 
-//Each selected BMS must have a corresponding address, and the data array for that BMS must be at least datalens[i] bytes long
+// Each selected BMS must have a corresponding address, and the data array for that BMS must be at least datalens[i]
-HAL_StatusTypeDef amsReadI2C(const uint8_t addresses[static N_BMS], uint8_t * data[static N_BMS], const uint8_t datalens[static N_BMS], uint32_t bms_mask) {
+// bytes long
+HAL_StatusTypeDef amsReadI2C(const uint8_t addresses[static N_BMS], uint8_t* data[static N_BMS],
+                             const uint8_t datalens[static N_BMS], uint32_t bms_mask) {
     uint8_t buffer[CMD_BUFFER_SIZE(COMM_GROUP_SIZE)] = {};
 
     uint8_t max_datalen = 0;

AMS_Master_Code/Core/Lib/ADBMS6830B_Driver/Core/Src/ADBMS_HighLevel.c

@@ -54,7 +54,7 @@ ADBMS_DetailedStatus AMS_Init(SPI_HandleTypeDef* hspi) {
         int first_match = -1;                                                                                          \
         for (size_t __any_intern_i = 0; __any_intern_i < N_BMS; __any_intern_i++) {                                    \
             Cell_Module module = modules[__any_intern_i];                                                              \
-            if (x) {                                                                                                   \
+            if ((x)) {                                                                                                 \
                 first_match = __any_intern_i;                                                                          \
                 break;                                                                                                 \
             }                                                                                                          \
@@ -68,7 +68,7 @@ ADBMS_DetailedStatus AMS_Idle_Loop() {
         // set_error_source(ERROR_SOURCE_INTERNAL);         //so we can't tell if we timed out
     }
 
-    packetChecksumFails += amsAuxAndStatusMeasurement(&modules);
+    packetChecksumFails += (amsAuxAndStatusMeasurement(&modules) == HAL_ERROR);
 
     static int match = 0;
     if ((match = any(module.status.SLEEP))) {
@@ -94,8 +94,8 @@ ADBMS_DetailedStatus AMS_Idle_Loop() {
         return (ADBMS_DetailedStatus){ADBMS_INTERNAL_BMS_FAULT, match};
     }
 
-    packetChecksumFails += amsCellMeasurement(&modules);
+    packetChecksumFails += (amsCellMeasurement(&modules) == HAL_ERROR);
-    packetChecksumFails += amsCheckUnderOverVoltage(&modules);
+    packetChecksumFails += (amsCheckUnderOverVoltage(&modules) == HAL_ERROR);
 
     if (packetChecksumFails > MAX_PACKET_CHECKSUM_FAILS) {
         return (ADBMS_DetailedStatus){ADBMS_INTERNAL_BMS_CHECKSUM_FAIL, -1};

AMS_Master_Code/Core/Lib/ADBMS6830B_Driver/Core/Src/ADBMS_LL_Driver.c

@@ -15,8 +15,6 @@
 
 #define INITIAL_COMMAND_PEC        0x0010
 #define INITIAL_DATA_PEC           0x0010 
-#define ADBMS_SPI_TIMEOUT          100 // Timeout in ms
-#warning ask about the timeout value
 
 SPI_HandleTypeDef* adbmsspi;
 

AMS_Master_Code/STM32-for-VSCode.config.yaml

@@ -10,7 +10,7 @@ target: AMS Master Nucleo
 # Can be C or C++
 language: C
 
-optimization: Og
+optimization: O2
 
 # MCU settings
 targetMCU: stm32h7x
@@ -43,6 +43,7 @@ cFlags:
   - -ffunction-sections
   - -std=gnu23
   - -flto
+#  - -fanalyzer
 
 assemblyFlags: 
   - -Wall