ataradov / dgw

USB HID Data Gateway
50 stars 10 forks source link

i2c_write_byte infinite loop #5

Open pedro-javierf opened 3 years ago

pedro-javierf commented 3 years ago

I'm using the function _i2c_writebyte but it may happen that it never exits the while(1) loop:

//-----------------------------------------------------------------------------
bool i2c_write_byte(uint8_t byte)
{
  I2C_SERCOM->I2CM.DATA.reg = byte;

  while (1)
  {
    int flags = I2C_SERCOM->I2CM.INTFLAG.reg;

    if (flags & SERCOM_I2CM_INTFLAG_MB)
      break;

    if (flags & (SERCOM_I2CM_INTFLAG_SB | SERCOM_I2CM_INTFLAG_ERROR))
      return false;
  }

  if (I2C_SERCOM->I2CM.STATUS.reg & SERCOM_I2CM_STATUS_RXNACK)
  {
    I2C_SERCOM->I2CM.CTRLB.reg |= SERCOM_I2CM_CTRLB_CMD(3);
    return false;
  }

  return true;
}

That's because neither of the two conditions are met. What scenario does each condition represent? This is my test code, i reckon I'm doing the initialization correctly:


  i2c_init(100000); //100KHz
  i2c_start(0x20);
  if(!i2c_write_byte(255)) //0xFF == 11111111
  {
    //not okay
  }
  i2c_stop();
ataradov commented 3 years ago

This usually happens when pull-up resistors are missing.

MB is set when a byte is transmitted, so this condition breaks the loop. SB here is not important, I don't know if it can even be set. The ERROR is going to be set in case of bus errors, so it exits with false.

pedro-javierf commented 3 years ago

This usually happens when pull-up resistors are missing.

MB is set when a byte is transmitted, so this condition breaks the loop. SB here is not important, I don't know if it can even be set. The ERROR is going to be set in case of bus errors, so it exits with false.

Where should the pull-up resistor go, the SDA cable?

EDIT: Additionally, I've seen the library at i2c_init() sets the SDA and SCL pins as input, so that may be causing the issue. I believe the i2c_pins function can be used to change this (though in the main.c of your projects it's only used once with parameters that are passed from USB). What is the value argument? This may be the key for the i2c_write_byte() function to not loop forever

void i2c_pins(int mask, int value)
{
  if (mask & I2C_PINS_SDA)
  {
    HAL_GPIO_SDA_out();
    HAL_GPIO_SDA_write(value & I2C_PINS_SDA);
  }
  else
  {
    HAL_GPIO_SDA_in();
    HAL_GPIO_SDA_clr();
  }

  if (mask & I2C_PINS_SCL)
  {
    HAL_GPIO_SCL_out();
    HAL_GPIO_SCL_write(value & I2C_PINS_SCL);
  }
  else
  {
    HAL_GPIO_SCL_in();
    HAL_GPIO_SCL_clr();
  }

  HAL_GPIO_SDA_pmuxdis();
  HAL_GPIO_SCL_pmuxdis();
}

EDIT 2: I've also seen that i2c_init uses GCLK0, which in my case is 48MHz for the samd21. Could that be a problem? I know the i2c speed cannot be as high as 48MHz but I'm unsure if this clock is directly used to drive the SCL signal or it only drives the internal SERCOM.

ataradov commented 3 years ago

Both SCL and SDA lines must be pulled up. Look at any I2C schematic, or I2C specification and you will see those resistors.

As soon as PMUX is configured, input or output setting does not matter, they are controlled by the peripherals.

Don't overthink it, the code is correct. I use this project all the time as a USB to I2C gateway. It works. The code is not at faults, your setup is.

i2c_pins() function is not used at all in normal operation. It is designed so that host can manually override the values on the pins. This function is used for my bootloaders, where I2C also act as a bootloader enter request on reset.

pedro-javierf commented 3 years ago

Just checked, I have pull up resistors on both SDA & SCL, so that can be discarded.

There has to be something on the software setup that is causing this problem. I tried a simple Arduino sketch that uses i2c and it works fine with my board and the i2c peripherals I have on the bus. The address setup is being sent:

imagen

Edit: found someone with a problem on the exact same spot https://www.avrfreaks.net/forum/i2c-communication-samd21g18a

ataradov commented 3 years ago

Can you capture logic analyzer tace fo my code?

ryan4volts commented 3 years ago

I thought arduino still bitbangs i2c. So it would be driven by the main micro. By comparison, the i2c hardware only works as an open collector so it would strongly depend on the pull up resistor. Perhaps scope the lines instead, maybe they're not hitting the thresholds of you analyser?

pedro-javierf commented 3 years ago

Can you capture logic analyzer tace fo my code?

The image above is a trace running your code. The address being sent by my code (0x20, which is the addr of the peripheral I'm trying to connect to) can be seen, but after that, nothing else

@ryan4volts could you elaborate? I'm afraid I don't understand what you are saying. I think the arduino framework uses the SERCOM peripherals available on the micro, but I haven't checked.

ataradov commented 3 years ago

This is very strange because it shows a normal complete transaction with NAK for the address and a normal STOP condition.

The reason for no followup is because device 0x20 did not ACK the request. Make sure that you've got 7-bit vs 8-bit address thing correct.

But I see no reason for this to be stuck in the loop. And the fact that STOP was issued, indicates that it exited the loop.

pedro-javierf commented 3 years ago

This is very strange because it shows a normal complete transaction with NAK for the address and a normal STOP condition.

The reason for no followup is because device 0x20 did not ACK the request. Make sure that you've got 7-bit vs 8-bit address thing correct.

But I see no reason for this to be stuck in the loop. And the fact that STOP was issued, indicates that it exited the loop.

What is that 7-bit vs 8-bit thing you mention?

On another note, I checked that the loop is indeed not exited. Just by adding a simple uart_putc('.'); inside, it prints it forever on an uart terminal

ataradov commented 3 years ago

There are two ways to specify I2C address - full 8-bit (with lower bit always 0, which will be filled with R/W bit), and just the top 7-bits. This code expects full 8-bit address.

What is your I2C device? Just for a test you can try (0x20 >> 1) as the address.

I don't know what generated that STOP condition then. Something strange is going on, this is not a typical behaviour.

pedro-javierf commented 3 years ago

There are two ways to specify I2C address - full 8-bit (with lower bit always 0, which will be filled with R/W bit), and just the top 7-bits. This code expects full 8-bit address.

What is your I2C device? Just for a test you can try (0x20 >> 1) as the address.

I don't know what generated that STOP condition then. Something strange is going on, this is not a typical behaviour.

PCA9534: https://www.ti.com/lit/ds/symlink/pca9534.pdf?ts=1627983578850&ref_url=https%253A%252F%252Fwww.google.com%252F

ataradov commented 3 years ago

Correction. Assuming 0x20 is the 7-bit address, then the 8-bit address would be (0x20 << 1)

ataradov commented 3 years ago

Yes, for this device with this code the correct address is 0x40.

pedro-javierf commented 3 years ago

EDIT: Solved. It was indeed looping indefinitely because it did not find the slave on the bus. Thanks for the help @ataradov !

By the way, isn't an infinite loop like this quite a dangerous behaviour for a micro? It of course depends on what you are building, but for critical systems, I think it could be better to return an error if the slave on the bus doesn't respond after a number of attempts, rather than locking the cpu in an endless loop.

pedro-javierf commented 3 years ago

I think the issue can be closed now

ataradov commented 3 years ago

It supposed to return. I don't know where this stop condition comes from. You need to look closer at the bus with an oscilloscope to figure out what is going on.

pedro-javierf commented 3 years ago

It supposed to return. I don't know where this stop condition comes from. You need to look closer at the bus with an oscilloscope to figure out what is going on.

while (1)
  {
    int flags = I2C_SERCOM->I2CM.INTFLAG.reg;

    if (flags & SERCOM_I2CM_INTFLAG_MB) //stop cond. 1 
      break;

    if (flags & (SERCOM_I2CM_INTFLAG_SB | SERCOM_I2CM_INTFLAG_ERROR)) //stop cond. 2
      return false;
  }

Those are the two stop conditions in the code, and when trying to write a byte to an i2c SERCOM interface that doesn't have a valid slave on the bus, none will trigger. I will take a look at the datasheets and errata and see if I can find anything

ataradov commented 3 years ago

After NAK is received, MB would trigger normally, since master owns the bus at this point. But in your case a false stop condition is generated, which causes the loop.

This behavior is specific to your setup. I have never seen this code lock.

You may have pull-up resistors that are not strong enough or excessive bus capacitance or some other problem.

ataradov commented 3 years ago

Also, make sure that you are not integrating this code 8nto some system where interrupt is configured for the same sercom. It is possible that interrupt handler is called and it clears the flag and generates the stop.

pedro-javierf commented 3 years ago

Currently this sercom does not have any interrupt. I'm using a version of this library though: https://github.com/Vampir8/PCA9534 That I have rewritten to not use the arduino framework.

pinMode looks like this:

...
// Write the configuration of the individual pins as inputs or outputs
  i2c_start(_i2caddr);
  i2c_write_byte(PCA9534_CONF_REGISTER); 
  i2c_write_byte(_port);
  i2c_stop();

and digitalWrite like this:

...
// Write the status of the pins on the output register
  i2c_start(_i2caddr);
  i2c_write_byte(PCA9534_OP_REGISTER);
  i2c_write_byte(_port);
  i2c_stop();  
ataradov commented 3 years ago

You need to check the result of i2c_start() and if it return false, don't proceed with writes. The stop is performed by that function ("I2C_SERCOM->I2CM.CTRLB.reg |= SERCOM_I2CM_CTRLB_CMD(3);" part). After it returns false, trying to write new bytes will lock, since the device is no longer on the bus.