Semaphore cleanup (#165)

* add missing semaphores to i2c commands Co-authored-by: pwittich <[email protected]> * add explicit casts to clean up code * clean up some type mismatches * semaphore extra command option * Update ZynqMonTask.c * i2c CLI commands need semaphore give/take commands Co-authored-by: Peace Kotamnives <[email protected]>
apollo-lhc · Nov 9, 2022 · 9e857db · 9e857db
1 parent 71a2ecf
commit 9e857db
Show file tree

Hide file tree

Showing 14 changed files with 165 additions and 81 deletions.
diff --git a/common/utils.c b/common/utils.c
@@ -266,7 +266,7 @@ void errbuffer_init(uint8_t minblk, uint8_t maxblk)
   ebuf->n_continue = 0;
 }
 
-void errbuffer_reset()
+void errbuffer_reset(void)
 {
   uint32_t addr = ebuf->minaddr;
   uint32_t maddr = ebuf->maxaddr;
@@ -501,7 +501,7 @@ void float_to_ints(float val, int *tens, int *fraction)
   return;
 }
 
-bool checkStale(int oldTime, int newTime)
+bool checkStale(unsigned oldTime, unsigned newTime)
 {
   return ((oldTime < newTime) && (newTime - oldTime) > 60);
 }
diff --git a/common/utils.h b/common/utils.h
@@ -11,6 +11,7 @@
 #include <stdint.h>
 #include <stddef.h>
 #include <stdbool.h>
+#include <sys/_types.h>
 
 #include "math.h"
 #include "stdlib.h"
@@ -118,7 +119,7 @@ void stopwatch_reset(void);
 uint32_t stopwatch_getticks(void);
 
 // freertos tick compare including rollover
-bool checkStale(int oldTime, int newTime);
+bool checkStale(unsigned oldTime, unsigned newTime);
 
 void float_to_ints(float val, int *tens, int *fraction);
 // this will suffer from the double evaluation bug

diff --git a/projects/cm_mcu/CommandLineTask.c b/projects/cm_mcu/CommandLineTask.c
@@ -336,7 +336,7 @@ static struct command_t commands[] = {
     {"psmon", psmon_ctl, "Displays a table showing the state of power supplies.\r\n", 1},
     {"psreg", psmon_reg, "<which> <reg>. which: LGA80D (10*dev+page), reg: reg address in hex\r\n", 2},
     {"restart_mcu", restart_mcu, "Restart the microcontroller\r\n", 0},
-    {"semaphore", sem_ctl, "args: <i2cdev 1-6> <take|release>\r\nTake or release a semaphore\r\n", 2},
+    {"semaphore", sem_ctl, "args: (none)|<i2cdev 1-6> <take|release>\r\nTake or release a semaphore\r\n", -1},
     {"snapshot", snapshot,
      "args:# (0|1)\r\nDump snapshot register. #: which of 5 LGA80D (10*dev+page). 0|1 decide "
      "if to reset snapshot.\r\n",

diff --git a/projects/cm_mcu/I2CCommunication.c b/projects/cm_mcu/I2CCommunication.c
@@ -169,7 +169,7 @@ int apollo_i2c_ctl_reg_w(uint8_t device, uint8_t address, uint8_t nbytes_addr, u
   return r;
 }
 
-int apollo_i2c_ctl_w(uint8_t device, uint8_t address, uint8_t nbytes, int value)
+int apollo_i2c_ctl_w(uint8_t device, uint8_t address, uint8_t nbytes, unsigned int value)
 {
   tSMBus *p_sMaster = pSMBus[device];
   tSMBusStatus *p_eStatus = eStatus[device];

diff --git a/projects/cm_mcu/I2CCommunication.h b/projects/cm_mcu/I2CCommunication.h
@@ -24,7 +24,7 @@ int apollo_i2c_ctl_r(uint8_t device, uint8_t address, uint8_t nbytes, uint8_t da
 int apollo_i2c_ctl_reg_r(uint8_t device, uint8_t address, uint8_t nbytes_addr,
                          uint16_t packed_reg_address, uint8_t nbytes,
                          uint32_t *packed_data);
-int apollo_i2c_ctl_w(uint8_t device, uint8_t address, uint8_t nbytes, int value);
+int apollo_i2c_ctl_w(uint8_t device, uint8_t address, uint8_t nbytes, unsigned int value);
 int apollo_i2c_ctl_reg_w(uint8_t device, uint8_t address, uint8_t nbytes_addr,
                          uint16_t packed_reg_address, uint8_t nbytes,
                          uint32_t packed_data);

diff --git a/projects/cm_mcu/InitTask.c b/projects/cm_mcu/InitTask.c
@@ -40,8 +40,8 @@ void InitTask(void *parameters)
     vTaskDelay(pdMS_TO_TICKS(1000));
   }
 #endif                  // REV1
-  init_registers_ff();  // initalize I/O expander for fireflies -- with FF monitoring via I2C in other threads, it grabs semaphore inside
-  init_registers_clk(); // initalize I/O expander for clocks
+  init_registers_ff();  // initialize I/O expander for fireflies -- with FF monitoring via I2C in other threads, it grabs semaphore inside
+  init_registers_clk(); // initialize I/O expander for clocks
   readFFpresent();
   log_info(LOG_SERVICE, "Clock I/O expander initialized\r\n");
 #ifdef REV2
@@ -51,7 +51,14 @@ void InitTask(void *parameters)
   for (int i = 0; i < 5; ++i) {
     init_load_clk(i); // load each clock config from EEPROM
   }
-  xSemaphoreGive(i2c2_sem);
+  // check if we have the semaphore
+  if (xSemaphoreGetMutexHolder(i2c2_sem) == xTaskGetCurrentTaskHandle()) {
+    xSemaphoreGive(i2c2_sem);
+  }
+  else {
+    log_info(LOG_SERVICE, "tried to release semaphore I don't own\r\n");
+  }
+  //xSemaphoreGive(i2c2_sem);
   log_info(LOG_SERVICE, "Clocks configured\r\n");
   getFFpart(); // the order of where to check FF part matters -- it won't be able to read anything if check sooner
 #endif         // REV2

diff --git a/projects/cm_mcu/LocalTasks.c b/projects/cm_mcu/LocalTasks.c
@@ -1354,6 +1354,7 @@ void init_registers_ff()
 
 #define EEPROM_MAX_PER_PAGE 126
 
+// You must claim the semaphore at a higher level than this
 static int load_clk_registers(int reg_count, uint16_t reg_page, uint16_t i2c_addrs)
 {
   int8_t HighByte = -1; // keep track when reg0 is changed
@@ -1372,7 +1373,6 @@ static int load_clk_registers(int reg_count, uint16_t reg_page, uint16_t i2c_add
                                           packed_reg0_address, 3, &triplet); // read triplet from eeprom
       if (status_r != 0) {
         log_error(LOG_SERVICE, "read failed: %s\r\n", SMBUS_get_error(status_r));
-        xSemaphoreGive(i2c2_sem);
         return status_r;
       }
       // organize the three bytes
@@ -1385,7 +1385,6 @@ static int load_clk_registers(int reg_count, uint16_t reg_page, uint16_t i2c_add
         status_w = apollo_i2c_ctl_reg_w(CLOCK_I2C_DEV, i2c_addrs, 1, 0x01, 1, reg0); // write a page change to a clock chip
         if (status_w != 0) {
           log_error(LOG_SERVICE, "write failed: %s\r\n", SMBUS_get_error(status_w));
-          xSemaphoreGive(i2c2_sem);
           return status_w; // fail writing and exit
         }
         HighByte = reg0; // update the current high byte or page
@@ -1394,7 +1393,6 @@ static int load_clk_registers(int reg_count, uint16_t reg_page, uint16_t i2c_add
       status_w = apollo_i2c_ctl_reg_w(CLOCK_I2C_DEV, i2c_addrs, 1, reg1, 1, data); // write data to a clock chip
       if (status_w != 0) {
         log_error(LOG_SERVICE, "write status is %d \r\n", status_w);
-        xSemaphoreGive(i2c2_sem);
         return status_w; // fail writing and exit
       }
     }

diff --git a/projects/cm_mcu/MonitorI2CTask.c b/projects/cm_mcu/MonitorI2CTask.c
@@ -93,6 +93,9 @@ void MonitorI2CTask(void *parameters)
     for (int ps = 0; ps < args->n_devices; ++ps) {
       log_debug(LOG_MONI2C, "%s: device %d\r\n", args->name, ps);
 
+      if (ps == args->n_devices - 1 && getPowerControlState() != POWER_ON) { // avoid continues to infinite loops due to multi-threading when pwr is not on
+        break;
+      }
       if (!IsCLK) {                           // Fireflies need to be checked if the links are connected or not
         if (args->i2c_dev == I2C_DEVICE_F1) { // FPGA #1
 #ifdef REV1
@@ -138,7 +141,12 @@ void MonitorI2CTask(void *parameters)
         }
       }
       // if the power state is unknown, don't do anything
-
+      else {
+        log_info(LOG_MONI2C, "%s: power state %d unknown\r\n", args->name,
+                 getPowerControlState());
+        vTaskDelay(10);
+        continue;
+      }
       // select the appropriate output for the mux
       uint8_t data;
       data = 0x1U << args->devices[ps].mux_bit;
@@ -182,7 +190,14 @@ void MonitorI2CTask(void *parameters)
 
     } // loop over devices
 
-    xSemaphoreGive(args->xSem);
+    if (xSemaphoreGetMutexHolder(args->xSem) == xTaskGetCurrentTaskHandle()) {
+      xSemaphoreGive(args->xSem);
+    }
+    else {
+      log_info(LOG_MONI2C, "tried to release semaphore I don't own\r\n");
+    }
+
+    //xSemaphoreGive(args->xSem);
 
     // monitor stack usage for this task
     CHECK_TASK_STACK_USAGE(args->stack_size);

diff --git a/projects/cm_mcu/ZynqMonTask.c b/projects/cm_mcu/ZynqMonTask.c
@@ -270,8 +270,8 @@ void zm_set_adcmon(struct zynqmon_data_t data[], int start)
 {
   // update the data for ZMON
   for (int i = 0; i < ADC_CHANNEL_COUNT; i++) {
-    data[i].sensor = i + start;      // sensor id
-    data[i].data.f = getADCvalue(i); // sensor value and type
+    data[i].sensor = i + start;              // sensor id
+    data[i].data.f = (__fp16)getADCvalue(i); // sensor value and type
   }
 }
 
@@ -295,14 +295,14 @@ void zm_set_psmon_legacy(struct zynqmon_data_t data[], int start)
             j * (dcdc_args.n_commands * dcdc_args.n_pages) + l * dcdc_args.n_commands + k;
 
         if (stale) {
-          data[index].data.f = __builtin_nanf("");
+          data[index].data.f = (__fp16)__builtin_nanf("");
         }
         else {
-          data[index].data.f = dcdc_args.pm_values[index];
+          data[index].data.f = (__fp16)dcdc_args.pm_values[index];
           if (data[index].data.f < -900.f)
-            data[index].data.f = __builtin_nanf("");
+            data[index].data.f = (__fp16)__builtin_nanf("");
         }
-        int reg = offsets[j * 2 + l] + k;
+        uint8_t reg = offsets[j * 2 + l] + k;
         data[index].sensor = reg;
       }
     }
@@ -329,12 +329,12 @@ void zm_set_psmon(struct zynqmon_data_t data[], int start)
             j * (dcdc_args.n_commands * dcdc_args.n_pages) + l * dcdc_args.n_commands + k;
 
         if (stale) {
-          data[ll].data.f = __builtin_nanf("");
+          data[ll].data.f = (__fp16)__builtin_nanf("");
         }
         else {
-          data[ll].data.f = dcdc_args.pm_values[index];
+          data[ll].data.f = (__fp16)dcdc_args.pm_values[index];
           if (data[l].data.f < -900.f)
-            data[ll].data.f = __builtin_nanf("");
+            data[ll].data.f = (__fp16)__builtin_nanf("");
         }
         data[ll].sensor = ll + start;
         ++ll;
@@ -354,12 +354,12 @@ void zm_set_fpga(struct zynqmon_data_t data[], int start)
   for (int j = 0; j < fpga_args.n_commands * fpga_args.n_devices; ++j) {
 
     if (stale) {
-      data[j].data.f = __builtin_nanf("");
+      data[j].data.f = (__fp16)__builtin_nanf("");
     }
     else {
-      data[j].data.f = fpga_args.pm_values[j];
+      data[j].data.f = (__fp16)fpga_args.pm_values[j];
       if (data[j].data.f < -900.f)
-        data[j].data.f = __builtin_nanf("");
+        data[j].data.f = (__fp16)__builtin_nanf("");
     }
     data[j].sensor = j + start;
   }

diff --git a/projects/cm_mcu/cm_mcu.c b/projects/cm_mcu/cm_mcu.c
@@ -250,6 +250,7 @@ int main(void)
   for (enum log_facility_t i = 0; i < NUM_LOG_FACILITIES; ++i) {
     log_set_level(LOG_INFO, i);
   }
+  log_set_level(LOG_ERROR, LOG_MON); // for now
 
   // Initialize all semaphores
   initSemaphores();

diff --git a/projects/cm_mcu/commands/I2CCommands.c b/projects/cm_mcu/commands/I2CCommands.c
@@ -6,9 +6,57 @@
  */
 
 #include "I2CCommands.h"
+#include "commands/parameters.h"
 #include "common/smbus_helper.h"
+#include "Semaphore.h"
 #include "projdefs.h"
 
+SemaphoreHandle_t getSemaphore(int number)
+{
+  SemaphoreHandle_t s;
+  switch (number) {
+    case 1:
+      s = i2c1_sem;
+      break;
+    case 2:
+      s = i2c2_sem;
+      break;
+    case 3:
+      s = i2c3_sem;
+      break;
+    case 4:
+      s = i2c4_sem;
+      break;
+    case 5:
+      s = i2c5_sem;
+      break;
+    case 6:
+      s = i2c6_sem;
+      break;
+    default:
+      s = 0;
+      break;
+  }
+  return s;
+}
+#define MAX_TRIES 10
+static int acquireI2CSemaphore(SemaphoreHandle_t s)
+{
+  int retval = pdTRUE;
+  if (s == NULL) {
+    return pdFAIL;
+  }
+  int tries = 0;
+  while (xSemaphoreTake(s, (TickType_t)10) == pdFALSE) {
+    ++tries;
+    if (tries > MAX_TRIES) {
+      retval = pdFAIL;
+      break;
+    }
+  }
+  return retval;
+}
+
 static bool isValidDevice(int device)
 {
 #ifdef REV1
@@ -21,7 +69,7 @@ static bool isValidDevice(int device)
 
 BaseType_t i2c_ctl_r(int argc, char **argv, char *m)
 {
-  BaseType_t device = strtol(argv[1], NULL, 16);
+  BaseType_t device = strtol(argv[1], NULL, 10);
   BaseType_t address = strtol(argv[2], NULL, 16);
   BaseType_t nbytes = strtol(argv[3], NULL, 10);
   uint8_t data[I2C_CTL_MAX_BYTES] = {0, 0, 0, 0};
@@ -33,6 +81,7 @@ BaseType_t i2c_ctl_r(int argc, char **argv, char *m)
     snprintf(m, SCRATCH_SIZE, "%s: invalid device %ld\r\n", argv[0], device);
     return pdFALSE;
   }
+
   int status = apollo_i2c_ctl_r(device, address, nbytes, data);
   if (status == 0) {
     snprintf(m, SCRATCH_SIZE, "%s: dev %ld, addr 0x%02lx: val=0x%02x %02x %02x %02x\r\n", argv[0],
@@ -41,15 +90,17 @@ BaseType_t i2c_ctl_r(int argc, char **argv, char *m)
   else {
     snprintf(m, SCRATCH_SIZE, "%s: failure %d (%s)\r\n", argv[0], status, SMBUS_get_error(status));
   }
+
   return pdFALSE;
 }
 
 BaseType_t i2c_ctl_reg_r(int argc, char **argv, char *m)
 {
-  UBaseType_t device, address, packed_reg_address;
+  UBaseType_t address, packed_reg_address;
+  BaseType_t device;
   uint32_t packed_data = 0U;
   BaseType_t nbytes_addr, nbytes;
-  device = strtol(argv[1], NULL, 16); // i2c device
+  device = strtol(argv[1], NULL, 10); // i2c device
   if (!isValidDevice((int)device)) {
     snprintf(m, SCRATCH_SIZE, "%s: invalid device %lu\r\n", argv[0], device);
     return pdFALSE;
@@ -76,15 +127,16 @@ BaseType_t i2c_ctl_reg_r(int argc, char **argv, char *m)
     snprintf(m + copied, SCRATCH_SIZE - copied, "%s: failure %d (%s)\r\n", argv[0], status,
              SMBUS_get_error(status));
   }
+
   return pdFALSE;
 }
 
 BaseType_t i2c_ctl_reg_w(int argc, char **argv, char *m)
 {
   // first byte is the register, others are the data
-  UBaseType_t device, address, packed_reg_address, packed_data;
-  BaseType_t nbytes_addr, nbytes;
-  device = strtoul(argv[1], NULL, 16); // i2c device
+  UBaseType_t address, packed_reg_address, packed_data;
+  BaseType_t device, nbytes_addr, nbytes;
+  device = strtol(argv[1], NULL, 10); // i2c device
   if (!isValidDevice(device)) {
     snprintf(m, SCRATCH_SIZE, "%s: invalid device %lu\r\n", argv[0], device);
     return pdFALSE;
@@ -113,9 +165,10 @@ BaseType_t i2c_ctl_reg_w(int argc, char **argv, char *m)
 
 BaseType_t i2c_ctl_w(int argc, char **argv, char *m)
 {
-  UBaseType_t device, address, value;
+  UBaseType_t address, value;
   UBaseType_t nbytes;
-  device = strtoul(argv[1], NULL, 16);
+  BaseType_t device;
+  device = strtol(argv[1], NULL, 10);
   if (!isValidDevice(device)) {
     snprintf(m, SCRATCH_SIZE, "%s: invalid device %lu\r\n", argv[0], device);
     return pdFALSE;
@@ -143,11 +196,20 @@ BaseType_t i2c_ctl_w(int argc, char **argv, char *m)
 BaseType_t i2c_scan(int argc, char **argv, char *m)
 {
   // takes one argument
-  int device = strtol(argv[1], NULL, 16); // i2c device
+  int device = strtol(argv[1], NULL, 10); // i2c device
   if (!isValidDevice(device)) {
     snprintf(m, SCRATCH_SIZE, "%s: invalid device %d\r\n", argv[0], device);
     return pdFALSE;
   }
+  SemaphoreHandle_t s = getSemaphore(device);
+  if (s == NULL) {
+    snprintf(m, SCRATCH_SIZE, "%s: could not get semaphore\r\n", argv[0]);
+    return pdFALSE;
+  }
+  if (acquireI2CSemaphore(s) == pdFAIL) {
+    snprintf(m, SCRATCH_SIZE, "%s: could not get semaphore in time\r\n", argv[0]);
+    return pdFALSE;
+  }
 
   int copied = 0;
   copied += snprintf(m, SCRATCH_SIZE - copied, "i2c bus scan, device %d\r\n", device);
@@ -169,5 +231,6 @@ BaseType_t i2c_scan(int argc, char **argv, char *m)
   copied += snprintf(m + copied, SCRATCH_SIZE - copied, "\r\n");
   configASSERT(copied < SCRATCH_SIZE);
 
+  xSemaphoreGive(s);
   return pdFALSE;
 }