Changes due to review comments

- Split bms_status (system status) to real_bms_status (bms status)
- Put #ifdef DEBUG_LOG around all log calls.
- Do not update bms_status (system status) directly, fire event instead from automatic precharge control
- Add Battery BMS status to webserver for MEB battery type only

Software/Software.ino

@@ -306,30 +306,9 @@ void core_loop(void* task_time_us) {
     if (check_pause_2s.elapsed()) {
       emulator_pause_state_transmit_can_battery();
     }
-    static bms_status_enum previous_state = FAULT;
+#ifdef DEBUG_LOG
-    if (previous_state != datalayer.battery.status.bms_status) {
+    logging.log_bms_status(datalayer.battery.status.real_bms_status, 1);
-      switch (datalayer.battery.status.bms_status) {
+#endif
-        case ACTIVE:
-          logging.printf("BMS state changed to: OK\n");
-          break;
-        case UPDATING:
-          logging.printf("BMS state changed to: UPDATING\n");
-          break;
-        case FAULT:
-          logging.printf("BMS state changed to: FAULT\n");
-          break;
-        case INACTIVE:
-          logging.printf("BMS state changed to: INACTIVE\n");
-          break;
-        case STANDBY:
-          logging.printf("BMS state changed to: STANDBY\n");
-          break;
-        default:
-          logging.printf("BMS state changed to: ??\n");
-          break;
-      }
-      previous_state = datalayer.battery.status.bms_status;
-    }
 
     vTaskDelayUntil(&xLastWakeTime, xFrequency);
   }

Software/src/battery/MEB-BATTERY.cpp

@@ -643,7 +643,9 @@ void update_values_battery() {  //This function maps all the values fetched via
 void handle_incoming_can_frame_battery(CAN_frame rx_frame) {
   last_can_msg_timestamp = millis();
   if (first_can_msg == 0) {
+#ifdef DEBUG_LOG
     logging.printf("MEB: First CAN msg received\n");
+#endif
     first_can_msg = last_can_msg_timestamp;
   }
   switch (rx_frame.ID) {
@@ -1013,31 +1015,39 @@ void handle_incoming_can_frame_battery(CAN_frame rx_frame) {
         case 3:  // EXTERN CHARGING
         case 4:  // AC_CHARGING
         case 6:  // DC_CHARGING
+#ifdef DEBUG_LOG
           if (!datalayer.system.status.battery_allows_contactor_closing)
             logging.printf("MEB Contactors closed\n");
-          if (datalayer.battery.status.bms_status != FAULT)
+#endif
-            datalayer.battery.status.bms_status = ACTIVE;
+          if (datalayer.battery.status.real_bms_status != BMS_FAULT)
+            datalayer.battery.status.real_bms_status = BMS_ACTIVE;
           datalayer.system.status.battery_allows_contactor_closing = true;
           break;
         case 5:  // Error
+#ifdef DEBUG_LOG
           if (datalayer.system.status.battery_allows_contactor_closing)
             logging.printf("MEB Contactors opened\n");
-          datalayer.battery.status.bms_status = FAULT;
+#endif
+          datalayer.battery.status.real_bms_status = BMS_FAULT;
           datalayer.system.status.battery_allows_contactor_closing = false;
           break;
         case 7:  // Init
+#ifdef DEBUG_LOG
           if (datalayer.system.status.battery_allows_contactor_closing)
             logging.printf("MEB Contactors opened\n");
-          if (datalayer.battery.status.bms_status != FAULT)
+#endif
-            datalayer.battery.status.bms_status = INACTIVE;
+          if (datalayer.battery.status.real_bms_status != BMS_FAULT)
+            datalayer.battery.status.real_bms_status = BMS_STANDBY;
           datalayer.system.status.battery_allows_contactor_closing = false;
           break;
         case 2:  // BALANCING
         default:
+#ifdef DEBUG_LOG
           if (datalayer.system.status.battery_allows_contactor_closing)
             logging.printf("MEB Contactors opened\n");
-          if (datalayer.battery.status.bms_status != FAULT)
+#endif
-            datalayer.battery.status.bms_status = STANDBY;
+          if (datalayer.battery.status.real_bms_status != BMS_FAULT)
+            datalayer.battery.status.real_bms_status = BMS_STANDBY;
           datalayer.system.status.battery_allows_contactor_closing = false;
       }
       BMS_HVIL_status = (rx_frame.data.u8[2] & 0x18) >> 3;
@@ -1511,14 +1521,16 @@ void handle_incoming_can_frame_battery(CAN_frame rx_frame) {
       handle_obd_frame(rx_frame);
       break;
     default:
+#ifdef DEBUG_LOG
       logging.printf("Unknown CAN frame received:\n");
+#endif
       dump_can_frame(rx_frame, MSG_RX);
       break;
   }
   datalayer.battery.status.CAN_battery_still_alive = CAN_STILL_ALIVE;
   if (can_msg_received == 0xFFFF && nof_cells_determined) {
-    if (datalayer.battery.status.bms_status == INACTIVE)
+    if (datalayer.battery.status.real_bms_status == BMS_DISCONNECTED)
-      datalayer.battery.status.bms_status = STANDBY;
+      datalayer.battery.status.real_bms_status = BMS_STANDBY;
   }
 }
 
@@ -1526,12 +1538,16 @@ void transmit_can_battery() {
   unsigned long currentMillis = millis();
   // Send 10ms CAN Message
   if (datalayer.system.settings.equipment_stop_active || currentMillis > last_can_msg_timestamp + 500) {
+#ifdef DEBUG_LOG
     if (first_can_msg)
       logging.printf("MEB: No CAN msg received for 500ms\n");
+#endif
     can_msg_received = RX_DEFAULT;
     first_can_msg = 0;
-    if (datalayer.battery.status.bms_status != FAULT)
+    if (datalayer.battery.status.real_bms_status != BMS_FAULT){
-      datalayer.battery.status.bms_status = INACTIVE;
+      datalayer.battery.status.real_bms_status = BMS_DISCONNECTED;
+      datalayer.system.status.battery_allows_contactor_closing = false;
+    }
   }
 
   if (currentMillis - previousMillis10ms >= INTERVAL_10_MS) {
@@ -1601,15 +1617,17 @@ void transmit_can_battery() {
 
     //HV request and DC/DC control lies in 0x503
 
-    if (datalayer.battery.status.bms_status != FAULT && /*first_can_msg > 0 && currentMillis > first_can_msg + 2000*/
+    if (datalayer.battery.status.real_bms_status != BMS_FAULT && /*first_can_msg > 0 && currentMillis > first_can_msg + 2000*/
-        (datalayer.battery.status.bms_status == STANDBY || datalayer.battery.status.bms_status == ACTIVE) &&
+        (datalayer.battery.status.real_bms_status == BMS_STANDBY || datalayer.battery.status.real_bms_status == BMS_ACTIVE) &&
         (labs(((int32_t)datalayer.battery.status.voltage_dV) -
               ((int32_t)datalayer_extended.meb.BMS_voltage_intermediate_dV)) < 200)) {
+#ifdef DEBUG_LOG
       if (MEB_503.data.u8[3] == BMS_TARGET_HV_OFF) {
         logging.printf("MEB Requesting HV\n");
       }
+#endif
       MEB_503.data.u8[1] =
-          0x30 | (datalayer.battery.status.bms_status == ACTIVE ? 0x00 : 0x80);  // Disable precharing if ACTIVE
+          0x30 | (datalayer.battery.status.real_bms_status == BMS_ACTIVE ? 0x00 : 0x80);  // Disable precharing if ACTIVE
       MEB_503.data.u8[3] = BMS_TARGET_HV_ON;  //TODO, should we try AC_2 or DC charging?
       MEB_503.data.u8[5] = 0x82;              // Bordnetz Active
       MEB_503.data.u8[6] = 0xE0;              // Request emergency shutdown HV system == 0, false

Software/src/communication/can/obd.cpp

@@ -23,6 +23,7 @@ void show_dtc(uint8_t byte0, uint8_t byte1) {
 }
 
 void handle_obd_frame(CAN_frame& rx_frame) {
+#ifdef DEBUG_LOG
   if (rx_frame.data.u8[1] == 0x7F) {
     const char* error_str = "?";
     switch (rx_frame.data.u8[3]) {  // See https://automotive.wiki/index.php/ISO_14229
@@ -105,6 +106,7 @@ void handle_obd_frame(CAN_frame& rx_frame) {
     }
   }
   dump_can_frame(rx_frame, MSG_RX);
+#endif
 }
 
 void transmit_obd_can_frame(unsigned int address, int interface) {

Software/src/communication/precharge_control/precharge_control.cpp

@@ -27,9 +27,12 @@ static int32_t prev_external_voltage = 20000;
 
 void init_precharge_control() {
   // Setup PWM Channel Frequency and Resolution
+#ifdef DEBUG_LOG
   logging.printf("Precharge control initialised\n");
+#endif
   pinMode(PRECHARGE_PIN, OUTPUT);
   digitalWrite(PRECHARGE_PIN, LOW);
+  digitalWrite(POSITIVE_CONTACTOR_PIN, LOW);
 }
 
 // Main functions
@@ -44,8 +47,8 @@ void handle_precharge_control() {
   switch (prechargeStatus) {
     case PRECHARGE_IDLE:
 
-      if (datalayer.battery.status.bms_status == STANDBY && datalayer.system.status.inverter_allows_contactor_closing &&
+      if (datalayer.battery.status.bms_status != FAULT && datalayer.battery.status.real_bms_status == BMS_STANDBY &&
-          !datalayer.system.settings.equipment_stop_active) {
+          datalayer.system.status.inverter_allows_contactor_closing && !datalayer.system.settings.equipment_stop_active) {
         prechargeStatus = START_PRECHARGE;
       }
       break;
@@ -56,7 +59,9 @@ void handle_precharge_control() {
       ledcWriteTone(PRECHARGE_PIN, freq);  // Set frequency and set dutycycle to 50%
       prechargeStartTime = currentTime;
       prechargeStatus = PRECHARGE;
+#ifdef DEBUG_LOG
       logging.printf("Precharge: Starting sequence\n");
+#endif
       digitalWrite(POSITIVE_CONTACTOR_PIN, HIGH);
 
       break;
@@ -82,39 +87,50 @@ void handle_precharge_control() {
           freq = Precharge_max_PWM_Freq;
         if (freq < Precharge_min_PWM_Freq)
           freq = Precharge_min_PWM_Freq;
+#ifdef DEBUG_LOG
         logging.printf("Precharge: Target: %d V  Extern: %d V  Frequency: %u\n", target_voltage / 10,
                        external_voltage / 10, freq);
+#endif
         ledcWriteTone(PRECHARGE_PIN, freq);
       }
 
-      if ((datalayer.battery.status.bms_status != STANDBY && datalayer.battery.status.bms_status != ACTIVE) ||
+      if ((datalayer.battery.status.real_bms_status != BMS_STANDBY && datalayer.battery.status.real_bms_status != BMS_ACTIVE) ||
-          datalayer.system.settings.equipment_stop_active) {
+          datalayer.battery.status.bms_status != ACTIVE || datalayer.system.settings.equipment_stop_active) {
         pinMode(PRECHARGE_PIN, OUTPUT);
         digitalWrite(PRECHARGE_PIN, LOW);
         digitalWrite(POSITIVE_CONTACTOR_PIN, LOW);
         prechargeStatus = PRECHARGE_IDLE;
+#ifdef DEBUG_LOG
         logging.printf("Precharge: Disabling Precharge bms not standby/active or equipment stop\n");
+#endif
       } else if (currentTime - prechargeStartTime >= MAX_PRECHARGE_TIME_MS) {
         pinMode(PRECHARGE_PIN, OUTPUT);
         digitalWrite(PRECHARGE_PIN, LOW);
         digitalWrite(POSITIVE_CONTACTOR_PIN, LOW);
         prechargeStatus = PRECHARGE_OFF;
-        datalayer.battery.status.bms_status = FAULT;
+#ifdef DEBUG_LOG
         logging.printf("Precharge: Disabled (timeout reached) -> PRECHARGE_OFF\n");
+#endif
+        set_event(EVENT_AUTOMATIC_PRECHARGE_FAILURE, 0);
+
         // Add event
       } else if (datalayer.system.status.battery_allows_contactor_closing) {
         pinMode(PRECHARGE_PIN, OUTPUT);
         digitalWrite(PRECHARGE_PIN, LOW);
         digitalWrite(POSITIVE_CONTACTOR_PIN, LOW);
         prechargeStatus = COMPLETED;
+#ifdef DEBUG_LOG
         logging.printf("Precharge: Disabled (contacts closed) -> COMPLETED\n");
+#endif
       }
       break;
 
     case COMPLETED:
       if (datalayer.system.settings.equipment_stop_active || datalayer.battery.status.bms_status != ACTIVE) {
         prechargeStatus = PRECHARGE_IDLE;
+#ifdef DEBUG_LOG
         logging.printf("Precharge: equipment stop activated -> IDLE\n");
+#endif
       }
       break;
 
@@ -125,7 +141,9 @@ void handle_precharge_control() {
         prechargeStatus = PRECHARGE_IDLE;
         pinMode(PRECHARGE_PIN, OUTPUT);
         digitalWrite(PRECHARGE_PIN, LOW);
+#ifdef DEBUG_LOG
         logging.printf("Precharge: equipment stop activated -> IDLE\n");
+#endif
       }
       break;
 

Software/src/datalayer/datalayer.h

@@ -92,8 +92,11 @@ typedef struct {
   uint8_t CAN_battery_still_alive = CAN_STILL_ALIVE;
 
   /** Other */
-  /** The current BMS status */
+  /** The current system status, which for now still has the name bms_status */
   bms_status_enum bms_status = ACTIVE;
+
+  /** The current battery status, which for now has the name real_bms_status */
+  real_bms_status_enum real_bms_status = BMS_DISCONNECTED;
 } DATALAYER_BATTERY_STATUS_TYPE;
 
 typedef struct {

Software/src/devboard/safety/safety.cpp

@@ -343,12 +343,16 @@ void emulator_pause_state_transmit_can_battery() {
   allowed_to_send_CAN = (!emulator_pause_CAN_send_ON || emulator_pause_status == NORMAL);
 
   if (previous_allowed_to_send_CAN && !allowed_to_send_CAN) {
+#ifdef DEBUG_LOG
     logging.printf("Safety: Pausing CAN sending");
+#endif
     //completely force stop the CAN communication
     ESP32Can.CANStop();
   } else if (!previous_allowed_to_send_CAN && allowed_to_send_CAN) {
     //resume CAN communication
+#ifdef DEBUG_LOG
     logging.printf("Safety: Resuming CAN sending");
+#endif
     ESP32Can.CANInit();
   }
 }

Software/src/devboard/utils/events.cpp

@@ -168,6 +168,7 @@ void init_events(void) {
   events.entries[EVENT_SOH_LOW].level = EVENT_LEVEL_ERROR;
   events.entries[EVENT_HVIL_FAILURE].level = EVENT_LEVEL_ERROR;
   events.entries[EVENT_PRECHARGE_FAILURE].level = EVENT_LEVEL_INFO;
+  events.entries[EVENT_AUTOMATIC_PRECHARGE_FAILURE].level = EVENT_LEVEL_ERROR;
   events.entries[EVENT_INTERNAL_OPEN_FAULT].level = EVENT_LEVEL_ERROR;
   events.entries[EVENT_INVERTER_OPEN_CONTACTOR].level = EVENT_LEVEL_INFO;
   events.entries[EVENT_INTERFACE_MISSING].level = EVENT_LEVEL_INFO;
@@ -340,6 +341,8 @@ const char* get_event_message_string(EVENTS_ENUM_TYPE event) {
              "Battery will be disabled!";
     case EVENT_PRECHARGE_FAILURE:
       return "Battery failed to precharge. Check that capacitor is seated on high voltage output.";
+    case EVENT_AUTOMATIC_PRECHARGE_FAILURE:
+      return "Automatic precharge failed to reach target voltae.";
     case EVENT_INTERNAL_OPEN_FAULT:
       return "High voltage cable removed while battery running. Opening contactors!";
     case EVENT_INVERTER_OPEN_CONTACTOR:
@@ -501,15 +504,9 @@ static void update_bms_status(void) {
       break;
     case EVENT_LEVEL_UPDATE:
       datalayer.battery.status.bms_status = UPDATING;
-#ifdef DOUBLE_BATTERY
-      datalayer.battery2.status.bms_status = UPDATING;
-#endif
       break;
     case EVENT_LEVEL_ERROR:
       datalayer.battery.status.bms_status = FAULT;
-#ifdef DOUBLE_BATTERY
-      datalayer.battery2.status.bms_status = FAULT;
-#endif
       break;
     default:
       break;

Software/src/devboard/utils/events.h

@@ -112,6 +112,7 @@
   XX(EVENT_MQTT_CONNECT)                \
   XX(EVENT_MQTT_DISCONNECT)             \
   XX(EVENT_EQUIPMENT_STOP)              \
+  XX(EVENT_AUTOMATIC_PRECHARGE_FAILURE) \
   XX(EVENT_NOF_EVENTS)
 
 typedef enum { EVENTS_ENUM_TYPE(GENERATE_ENUM) } EVENTS_ENUM_TYPE;

Software/src/devboard/utils/logging.cpp

@@ -134,3 +134,31 @@ void Logging::printf(const char* fmt, ...) {
   previous_message_was_newline = message_buffer[size - 1] == '\n';
 #endif  // DEBUG_LOG
 }
+
+void Logging::log_bms_status(real_bms_status_enum bms_status, int battery_id){
+  static real_bms_status_enum previous_state = BMS_FAULT;
+  const char *id = "";
+  if (battery_id == 2){
+    id = "2";
+  }
+  if (previous_state != bms_status) {
+    switch (bms_status) {
+      case BMS_ACTIVE:
+        logging.printf("Battery%s BMS state changed to: OK\n", id);
+        break;
+      case BMS_DISCONNECTED:
+        logging.printf("Battery%s BMS state changed to: DISCONNECTED\n");
+        break;
+      case BMS_FAULT:
+        logging.printf("Battery%s BMS state changed to: FAULT\n");
+        break;
+      case BMS_STANDBY:
+        logging.printf("Battery%s BMS state changed to: STANDBY\n");
+        break;
+      default:
+        logging.printf("Battery%s BMS state changed to: ??\n");
+        break;
+    }
+    previous_state = bms_status;
+  }
+}

Software/src/devboard/utils/logging.h

@@ -3,6 +3,7 @@
 
 #include <inttypes.h>
 #include "Print.h"
+#include "types.h"
 
 class Logging : public Print {
   void add_timestamp(size_t size);
@@ -11,6 +12,7 @@ class Logging : public Print {
   virtual size_t write(const uint8_t* buffer, size_t size);
   virtual size_t write(uint8_t) { return 0; }
   void printf(const char* fmt, ...);
+  void log_bms_status(real_bms_status_enum bms_status, int battery_id);
   Logging() {}
 };
 

Software/src/devboard/utils/types.h

@@ -4,6 +4,7 @@
 #include <string>
 
 enum bms_status_enum { STANDBY = 0, INACTIVE = 1, DARKSTART = 2, ACTIVE = 3, FAULT = 4, UPDATING = 5 };
+enum real_bms_status_enum { BMS_DISCONNECTED = 0, BMS_STANDBY = 1, BMS_ACTIVE = 2, BMS_FAULT = 3 };
 enum battery_chemistry_enum { NCA, NMC, LFP };
 enum led_color { GREEN, YELLOW, RED, BLUE };