fix bug where Dino IP always read 0 from i2c. Add hardware mutex to I2c

and to I2c::Switch

Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>

fpga/include/villas/fpga/ips/dino.hpp

@@ -52,13 +52,15 @@ public:
     return getMasterPort(masterPort);
   }
 
-  IoextPorts getIoextDir();
-  IoextPorts getIoextOut();
+  IoextPorts getIoextDirectionRegister();
+  IoextPorts getIoextOutputRegister();
 
 protected:
   std::shared_ptr<I2c> i2cdev;
   uint8_t i2c_channel;
 
+  IoextPorts getIoextDir();
+  IoextPorts getIoextOut();
   void setIoextDir(IoextPorts ports);
   void setIoextOut(IoextPorts ports);
 };

fpga/include/villas/fpga/ips/i2c.hpp

@@ -44,10 +44,16 @@ public:
   class Switch {
   public:
     Switch(I2c *i2c, uint8_t address)
-        : i2c(i2c), address(address), channel(0), readOnce(false){};
+        : i2c(i2c), address(address), channel(0), readOnce(false),
+          switchLock(){};
     Switch(const Switch &other) = delete;
     Switch &operator=(const Switch &other) = delete;
     void setChannel(uint8_t channel);
+    void setAndLockChannel(uint8_t channel) {
+      switchLock.lock();
+      setChannel(channel);
+    }
+    void unlockChannel() { switchLock.unlock(); }
     uint8_t getChannel();
     void setAddress(uint8_t address) { this->address = address; }
     uint8_t getAddress() { return address; }
@@ -57,6 +63,7 @@ public:
     uint8_t address;
     uint8_t channel;
     bool readOnce;
+    std::mutex switchLock;
   };
   Switch &getSwitch(uint8_t address = I2C_SWTICH_ADDR) {
     if (switchInstance == nullptr) {
@@ -84,6 +91,7 @@ private:
   std::list<MemoryBlockName> getMemoryBlocks() const {
     return {registerMemory};
   }
+  // assumes hwLock is locked
   void waitForBusNotBusy();
   void driverWriteBlocking(u8 *dataPtr, size_t size);
   void driverReadBlocking(u8 *dataPtr, size_t max_read);

fpga/lib/ips/dino.cpp

@@ -37,24 +37,44 @@ Dino::IoextPorts Dino::getIoextDir() {
   if (i2cdev == nullptr) {
     throw RuntimeError("I2C device not set");
   }
-  std::vector<u8> data = {0};
+  std::vector<u8> data;
   i2cdev->readRegister(I2C_IOEXT_ADDR, I2C_IOEXT_REG_DIR, data, 1);
   IoextPorts ports;
   ports.raw = data[0];
   return ports;
 }
 
+Dino::IoextPorts Dino::getIoextDirectionRegister() {
+  if (i2cdev == nullptr) {
+    throw RuntimeError("I2C device not set");
+  }
+  i2cdev->getSwitch().setAndLockChannel(i2c_channel);
+  auto ret = getIoextDir();
+  i2cdev->getSwitch().unlockChannel();
+  return ret;
+}
+
 Dino::IoextPorts Dino::getIoextOut() {
   if (i2cdev == nullptr) {
     throw RuntimeError("I2C device not set");
   }
-  std::vector<u8> data = {0};
+  std::vector<u8> data;
   i2cdev->readRegister(I2C_IOEXT_ADDR, I2C_IOEXT_REG_OUT, data, 1);
   IoextPorts ports;
   ports.raw = data[0];
   return ports;
 }
 
+Dino::IoextPorts Dino::getIoextOutputRegister() {
+  if (i2cdev == nullptr) {
+    throw RuntimeError("I2C device not set");
+  }
+  i2cdev->getSwitch().setAndLockChannel(i2c_channel);
+  auto ret = getIoextOut();
+  i2cdev->getSwitch().unlockChannel();
+  return ret;
+}
+
 DinoAdc::DinoAdc() : Dino() {}
 
 DinoAdc::~DinoAdc() {}
@@ -63,7 +83,7 @@ void DinoAdc::configureHardware() {
   if (i2cdev == nullptr) {
     throw RuntimeError("I2C device not set");
   }
-  i2cdev->getSwitch().setChannel(i2c_channel);
+  i2cdev->getSwitch().setAndLockChannel(i2c_channel);
   IoextPorts ioext = {.raw = 0};
   ioext.fields.sat_detect = true;
   setIoextDir(ioext);
@@ -75,6 +95,7 @@ void DinoAdc::configureHardware() {
   setIoextOut(ioext);
   ioext.fields.n_we = false;
   setIoextOut(ioext);
+  i2cdev->getSwitch().unlockChannel();
 }
 
 DinoDac::DinoDac() : Dino() {}
@@ -85,11 +106,12 @@ void DinoDac::configureHardware() {
   if (i2cdev == nullptr) {
     throw RuntimeError("I2C device not set");
   }
-  i2cdev->getSwitch().setChannel(i2c_channel);
+  i2cdev->getSwitch().setAndLockChannel(i2c_channel);
   IoextPorts ioext = {.raw = 0};
   setIoextDir(ioext);
   ioext.fields.status_led = true;
   setIoextOut(ioext);
+  i2cdev->getSwitch().unlockChannel();
   setGain(GAIN_1);
 }
 
@@ -97,21 +119,24 @@ void DinoDac::setGain(Gain gain) {
   if (i2cdev == nullptr) {
     throw RuntimeError("I2C device not set");
   }
-  i2cdev->getSwitch().setChannel(i2c_channel);
+  i2cdev->getSwitch().setAndLockChannel(i2c_channel);
   IoextPorts ioext = getIoextOut();
   ioext.fields.gain_lsb = gain & 0x1;
   ioext.fields.gain_msb = gain & 0x2;
   setIoextOut(ioext);
+  i2cdev->getSwitch().unlockChannel();
 }
 
 Dino::Gain DinoDac::getGain() {
   if (i2cdev == nullptr) {
     throw RuntimeError("I2C device not set");
   }
-  i2cdev->getSwitch().setChannel(i2c_channel);
+  i2cdev->getSwitch().setAndLockChannel(i2c_channel);
   IoextPorts ioext = getIoextOut();
-  return static_cast<Gain>((ioext.fields.gain_msb << 1) |
-                           ioext.fields.gain_lsb);
+  auto ret =
+      static_cast<Gain>((ioext.fields.gain_msb << 1) | ioext.fields.gain_lsb);
+  i2cdev->getSwitch().unlockChannel();
+  return ret;
 }
 
 static char n_adc[] = "DINO ADC";

fpga/lib/ips/i2c.cpp

@@ -42,6 +42,7 @@ bool I2c::init() {
     return true;
   }
   xConfig.BaseAddress = getBaseAddr(registerMemory);
+  hwLock.lock();
   ret = XIic_CfgInitialize(&xIic, &xConfig, xConfig.BaseAddress);
   if (ret != XST_SUCCESS) {
     throw RuntimeError("Failed to initialize I2C");
@@ -53,12 +54,13 @@ bool I2c::init() {
 
   irqs[i2cInterrupt].irqController->enableInterrupt(irqs[i2cInterrupt],
                                                     polling);
-
+  hwLock.unlock();
   initDone = true;
   return true;
 }
 
 bool I2c::reset() {
+  // we cannot lock here because this may be called in a destructor
   XIic_Reset(&xIic);
   irqs[i2cInterrupt].irqController->disableInterrupt(irqs[i2cInterrupt]);
   initDone = false;
@@ -141,6 +143,7 @@ bool I2c::write(u8 address, std::vector<u8> &data) {
     throw RuntimeError("I2C not initialized");
   }
 
+  hwLock.lock();
   ret =
       XIic_SetAddress(&xIic, XII_ADDR_TO_SEND_TYPE, static_cast<int>(address));
   if (ret != XST_SUCCESS) {
@@ -158,6 +161,7 @@ bool I2c::write(u8 address, std::vector<u8> &data) {
   if (ret != XST_SUCCESS) {
     throw RuntimeError("Failed to stop I2C");
   }
+  hwLock.unlock();
   return true;
 }
 
@@ -171,6 +175,7 @@ bool I2c::read(u8 address, std::vector<u8> &data, size_t max_read) {
   data.resize(data.size() + max_read);
   u8 *dataPtr = data.data() + data.size() - max_read;
 
+  hwLock.lock();
   ret =
       XIic_SetAddress(&xIic, XII_ADDR_TO_SEND_TYPE, static_cast<int>(address));
   if (ret != XST_SUCCESS) {
@@ -188,41 +193,19 @@ bool I2c::read(u8 address, std::vector<u8> &data, size_t max_read) {
   if (ret != XST_SUCCESS) {
     throw RuntimeError("Failed to stop I2C");
   }
+  hwLock.unlock();
 
   return XST_SUCCESS;
 }
 
 bool I2c::readRegister(u8 address, u8 reg, std::vector<u8> &data,
                        size_t max_read) {
-  int ret;
 
-  if (!initDone) {
-    throw RuntimeError("I2C not initialized");
-  }
-
-  data.resize(data.size() + max_read);
-  u8 *dataPtr = data.data() + data.size() - max_read;
-
-  ret =
-      XIic_SetAddress(&xIic, XII_ADDR_TO_SEND_TYPE, static_cast<int>(address));
-  if (ret != XST_SUCCESS) {
-    throw RuntimeError("Failed to set I2C address");
-  }
-
-  ret = XIic_Start(&xIic);
-  if (ret != XST_SUCCESS) {
-    throw RuntimeError("Failed to start I2C");
-  }
-
-  driverWriteBlocking(&reg, sizeof(reg));
-  driverReadBlocking(dataPtr, max_read);
-
-  ret = XIic_Stop(&xIic);
-  if (ret != XST_SUCCESS) {
-    throw RuntimeError("Failed to stop I2C");
-  }
-
-  return XST_SUCCESS;
+  std::vector<u8> regData = {reg};
+  bool ret;
+  ret = write(address, regData);
+  ret &= read(address, data, max_read);
+  return ret;
 }
 
 static const uint8_t CHANNEL_MAP[] = I2C_SWITCH_CHANNEL_MAP;