Fattoresaimon / I2CEncoderV2.1

GNU General Public License v3.0
109 stars 24 forks source link

writeEEPROM requires some delay #3

Closed hanyazou closed 5 years ago

hanyazou commented 5 years ago

It seems that some delay() is needed after writeEEPROM() issued. Without this delay(), following read/write I2C register requests will fail and Arduino board and the I2C Encoder lose their sync.

for (int k = 0; k < 16; k++) {
  for (int j = 0; j < 16; j++) {
    Encoder.writeEEPROM(k*16+j, (k*16+j));
    delay(3);  // THIS IS NEEDED
  }
}

Is there any method to detect if the I2C Encoder is ready to receive next I2C request?

Fattoresaimon commented 5 years ago

Hello, Yes, you need to wait few ms when you write each byte on the EEPROM. The PIC16F18344 datasheet says 4 - 5ms. As far as i know from the other commands, there is no need to wait between commands. Unfortunately there is no way to know when the I2C Encoder is ready to receive commands.

hanyazou commented 5 years ago

I do not know about I2C in detail and I have never used PIC. But it is better to enable clock stretching, I think. It seems to be disabled SEN (Start Condition Enable/Stretch Enable bit) in I2C1_Initialize() in Firmware/I2CEncoderV2.X/mcc_generated_files/i2c1.c. (I don't know how to 'generate'd this...) If clock stretching will be enabled properly, following I2C request might wait for completion of EEPROM access.

Fattoresaimon commented 5 years ago

The clock stretching can be a solution, but can be a problem... For example on the Raspberry Pi the clock stretching is buggy. Then in order to make the product more compatible i have decided to disable it If you can grab a PIC programmer you can customize the FW. You only need to install the MPLAB X and the XC8 compiler. The peripheral driver are generated with the MCC tool inside of the MPLAB X

mrkale commented 5 years ago

In general, clock stretching is driven by some slave, not by an MCU. Alas, the EEPROM chips belong to those slaves that do not provide clock stretching. So that we have to rely on the time delay by datasheets.

hanyazou commented 5 years ago

I've installed MPLAB X and XC8 on to my Mac and enabled the SEN bit. I just changed SEN bit to 1 (enabled) in SSP1CON2 register. It worked well for Arduino Leonardo. The slave holds I2C clock for about 3ms to delay next I2C request from the master. I could issue EEPROM write requests from Arduino master repeatedly without any delay().

I will try this with Raspberry Pi also when I have a time. If RaPi have any problem with this modification, how about that the clock stretching shall be controlled (enabled / disabled) from master by sending some configuration command so that Arduino master can issue EEPROM write request w/o delay()?

diff --git a/Firmware/I2CEncoderV2.X/mcc_generated_files/i2c1.c b/Firmware/I2CEncoderV2.X/mcc_generated_files/i2c1.c
index c1bf399..3b19913 100644
--- a/Firmware/I2CEncoderV2.X/mcc_generated_files/i2c1.c
+++ b/Firmware/I2CEncoderV2.X/mcc_generated_files/i2c1.c
@@ -79,7 +79,7 @@ void I2C1_Initialize(void) {
     // SSPEN enabled; WCOL no_collision; CKP disabled; SSPM 7 Bit Polling; SSPOV no_overflow;
     SSP1CON1 = 0x26;
     // ACKEN disabled; GCEN disabled; PEN disabled; ACKDT acknowledge; RSEN disabled; RCEN disabled; ACKSTAT received; SEN disabled;
-    SSP1CON2 = 0x00;
+    SSP1CON2 = 0x01; // SEN enabled
     // ACKTIM ackseq; SBCDE disabled; BOEN disabled; SCIE disabled; PCIE disabled; DHEN disabled; SDAHT 100ns; AHEN disabled;
     SSP1CON3 = 0x00;
     // SSP1MSK 127;

screen shot 2019-01-04 at 18 51 27

Fattoresaimon commented 5 years ago

Hello, very good, thank you for the experiment! Yes, I think is possible to add a configuration bit for enable/disable the clock stretch. But unfortunately the GCONF register is already full. Change the registers map it will make incompatible the FW to the people that already have the boards. Since it's not a big bug I'm not intended to add now. I will consider it to the next version!

hanyazou commented 5 years ago

I tired my modification with My Raspberry Pi and raspbian. That did not allow the clock stretching even if it was only 5us. This means that I must disable the SEN bit for the RasPi. The micro seconds order of interrupt latency can not handle by the firmware.

screen shot 2019-01-04 at 23 31 45

the GCONF register is already full.

You can add new register for that. You have enough address space, don't you? If existing programs do not write new register and leave them as initial values, the encoder behaves as is was. So there is no problem of compatibilities.

  /** Internal i2C address definition **/
  typedef enum {
      REG_GCONF = 0x00,
      REG_GP1CONF = 0x01,
          :
      REG_FADEGP = 0x21,
+     REG_GCONF2 = 0x22,
      REG_EEPROMS = 0x80,
};

Since it's not a big bug I'm not intended to add now.

Yeah. It's your choice.

Fattoresaimon commented 5 years ago

Hello, Thank you again for the experiment!

You can add new register for that. You have enough address space, don't you? If existing programs do not write new register and leave them as initial values, the encoder behaves as is was. So there is no problem of compatibilities.

Yes, it's not a problem of space! i just don't want to do some mess with same boards but different FW version. When i will run a new production i can change the marking on the boards from V2.0 to V2.1 and include this feature in the FW.

hanyazou commented 5 years ago

I understand. I recommend you to add ID register and revision registers, from which I can read some fixed values.