Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACK polling #9

Closed
valkuc opened this issue Sep 11, 2016 · 35 comments
Closed

ACK polling #9

valkuc opened this issue Sep 11, 2016 · 35 comments

Comments

@valkuc
Copy link
Contributor

valkuc commented Sep 11, 2016

Hello,
I need to read and write to AT24C32 i2c eeprom. To ensure data was written by chip I need to use "acknowledge polling".
From datasheet:

ACKNOWLEDGE POLLING: Once the internally-timed write cycle has started and the
EEPROM inputs are disabled, acknowledge polling can be initiated. This involves sending
a start condition followed by the device address word. The read/write bit is
representative of the operation desired. Only if the internal write cycle has completed
will the EEPROM respond with a zero, allowing the read or write sequence to continue.

How can I do ACK polling with you library?

@valkuc
Copy link
Contributor Author

valkuc commented Sep 11, 2016

Using "classic" i2c_master lib this implemented next way:

do {
    i2c_master_start();
    i2c_master_writeByte((uint8)((AT24C_ADDR << 1) | 1));
} while (!i2c_master_checkAck());
i2c_master_stop();

@pasko-zh
Copy link
Owner

pasko-zh commented Sep 12, 2016

In i2c there is a mechanism called clock stretching, which is used by slaves to signal to the master to wait, i.e. they are pulling SCL low. This is fully supported with brzo i2c.

Now, it seems that this device is implementing this somehow with SDA. From the description in the datasheet I understood ACK polling this way:

After the 8th SCL cycle has finished, the 9th cycle begins, and on the raising edge of SCL the master samples SDA. If SDA is high then it means NACK by the slave. This is the standard i2c operation. Now, this guy seems to keep SDA high for twr msecs (i.e. it is not yet ACKnowledging), before it will pull SDA low, to signal an ACK. i.e., it "stretches" SDA. The datasheet doesn't say what the slave is doing during twr msec with SCL. I guess, it won't pull it down (i.e. not stretching it). If the device is not pulling SCL low, then brzo i2c will read SDA being high and interprete it as NACK.

So, it looks to me that this is very very special way of "clock" (i.e. SDA) stretching.... and I even would go that far to say that it is not really according to the i2c standard. What you would need to do is to wait twr msec after writes with a delay(twr), where twr is 10 msec according to the datasheet if vcc > 2.5V.

I also had a look in the microchip 24AA256/24LC256/24FC256 datasheet under section 7.0, there is a diagram explaining it better (for my understanding at least ;-) If the device could work with a STOP sequence after an NACK was returned, then you can do it with brzo i2c, otherwise not.

@valkuc
Copy link
Contributor Author

valkuc commented Sep 12, 2016

Looks like Microchip EEPROM have same logic for "ACK polling" as Atmel. So in your opinion I just need to set clock stretching to 10000 before issuing write command to EEPROM?

UPD: It's possible to implement this "ACK polling" in assembly in your library?

@pasko-zh
Copy link
Owner

  1. I have some doubts that during ACK polling, SCL is pulled low. It is not clear, at least to me, from the datasheets. But since they did not mention anything on SCL stretching, I think it won't work with clock stretching. Since I don't have the EEPROM I cannot try myself.
    Could you give this a try, please?
  2. If I understood the datasheet correctly, then this ACK polling is not mandatory: "Once the Stop condition for a Write command has been issued from the master, the device initiates the internally timed write cycle. ACK polling can be initiated immediately." Therefore you could maybe do something like: Write one or more byte in one brzo transaction, after that use delay(10) to allow the internal write to happen and then go on with other brzo transactions.
    Could you give this a try, too, please?
  3. I wrote the library to be as close to the i2c specs as possible. Therefore I would have to implement an additional function, something like brzo_i2c_ACK_polling(max_timeout).

@valkuc
Copy link
Contributor Author

valkuc commented Sep 12, 2016

  1. Sure I will try it today evening (after my main work)
  2. Yes, it's actually not mandatory. It was specially introduced to minimize delays between consequent eeprom write operations. So actually I can just use delay(10ms), but if there is a possibility to use approach specially designed for this purpose - why not use it.
  3. Well, it would be nice to have such functionality if clock stretching will not work. I2C EEPROMs sometimes can be quite useful compared to SPI flash and it would be nice if you library can work with them.

P.S.
In my project there is DS3231 module with AT24C32 EEPROM onboard. Currently I use "i2c_master" library to interface both DS3231 and EEPROM chips, but sometimes read/write operations get failed for unknown for me reason (probably timing issues and occured interrupts). So I decided to rework my code using your I2C driver.

@valkuc
Copy link
Contributor Author

valkuc commented Sep 12, 2016

Here is my investigation.
I was able to do some emulation of ASK polling by doing next:

uint8 data[1];
do
{
    brzo_i2c_start_transaction(AT24C_ADDR, 100);
    brzo_i2c_read(data, 1, false);
}
while(brzo_i2c_end_transaction() != 0);

but this more looks like workaround than correct approach. Here is decoded signal from Logic Analizer, writing at address 0 array of 10 bytes all value 3:

Time [s], Analyzer Name, Decoded Protocol Result
0.000005000000000,I2C,Setup Write to ['160'] + ACK
0.000097500000000,I2C,'0' + ACK
0.000189500000000,I2C,'0' + ACK
0.000385500000000,I2C,'3' + ACK
0.000477500000000,I2C,'3' + ACK
0.000569500000000,I2C,'3' + ACK
0.000661500000000,I2C,'3' + ACK
0.000754000000000,I2C,'3' + ACK
0.000846000000000,I2C,'3' + ACK
0.000938000000000,I2C,'3' + ACK
0.001030000000000,I2C,'3' + ACK
0.001122000000000,I2C,'3' + ACK
0.001214000000000,I2C,'3' + ACK
0.001330500000000,I2C,Setup Read to ['161'] + NAK
0.001446500000000,I2C,Setup Read to ['161'] + NAK
0.001562500000000,I2C,Setup Read to ['161'] + NAK
0.001678500000000,I2C,Setup Read to ['161'] + Missing ACK/NAK
0.001794500000000,I2C,Setup Read to ['161'] + Missing ACK/NAK
0.001910500000000,I2C,Setup Read to ['161'] + NAK
0.002026500000000,I2C,Setup Read to ['161'] + NAK
0.002142000000000,I2C,Setup Read to ['161'] + NAK
0.002258000000000,I2C,Setup Read to ['161'] + NAK
0.002374000000000,I2C,Setup Read to ['161'] + NAK
0.002490000000000,I2C,Setup Read to ['161'] + ACK
0.002582000000000,I2C,'255' + NAK

And here is what actually should be:

Time [s], Analyzer Name, Decoded Protocol Result
0.000016000000000,I2C,Setup Write to ['160'] + ACK
0.000242000000000,I2C,'0' + ACK
0.000451500000000,I2C,'0' + ACK
0.000661500000000,I2C,'1' + ACK
0.000870000000000,I2C,'1' + ACK
0.001078000000000,I2C,'1' + ACK
0.001286000000000,I2C,'1' + ACK
0.001494500000000,I2C,'1' + ACK
0.001702500000000,I2C,'1' + ACK
0.001911000000000,I2C,'1' + ACK
0.002119000000000,I2C,'1' + ACK
0.002327500000000,I2C,'1' + ACK
0.002535500000000,I2C,'1' + ACK
0.002796000000000,I2C,Setup Read to ['161'] + NAK
0.003025000000000,I2C,Setup Read to ['161'] + NAK
0.003254000000000,I2C,Setup Read to ['161'] + NAK
0.003483000000000,I2C,Setup Read to ['161'] + NAK
0.003712000000000,I2C,Setup Read to ['161'] + NAK
0.003941000000000,I2C,Setup Read to ['161'] + ACK

While investigated that, I found one thing that makes your library hard to use for EEPROM write operations. Pay attention on my Logic Analizer output, notice two "0" after "Setup write" and then ten "1". Zeroes is the target EEPROM address, ten "1" - is data. The tricky part here is that if I will use two separate calls to brzo_i2c_write I will have additional "Setup write" before address and data - and this is error. Address and Data must be sent one after another, so imagine that I have next function prototype to write data to EEPROM:
bool at24c_write(uint16_t addr, uint8_t* data, uint8_t len)
I can't write body like that because in this case I will have extra "Setup write" between address and data transmission:

uint8_t data_addr[2];
data_addr[0] = (uint8_t)(((unsigned)addr) >> 8);
data_addr[1] = (uint8_t)(((unsigned)addr) & 0xff);
brzo_i2c_write(data_addr, 2, true);
brzo_i2c_write(data, len, false);

To overrun this I need to malloc temporary array with size of data + 2, copy first two bytes of address into it, then copy data and after that send it in one brzo_i2c_write call. I guess this looks like redundant memory allocation:

uint8 *d = os_malloc(len + 2);
d[0] = (uint8)(((unsigned)addr) >> 8);
d[1] = (uint8)(((unsigned)addr) & 0xff);
os_memcpy(d+2, data, len);
brzo_i2c_write(d, len+2, false);
os_free(d);

So actually it would be very nice to have ability to control I2C at a bit lower level than you currently provide. Compared to "i2c_master", your library provide ideal SCL clock and timings and with you lib I get rid of my problem that I mentioned in previous post. But in current implementation it's not very comfortable to use it. By the way, there is an issue #1 - using Arduino "Wire" interface EEPROM write operation can be done like this:

Wire.beginTransmission(m_deviceid);
Wire.write((int)(add >> 8));   // left-part of pointer address
Wire.write((int)(add & 0xFF)); // and the right
Wire.write(data);
Wire.endTransmission();

@valkuc
Copy link
Contributor Author

valkuc commented Sep 12, 2016

Here is some ideas:

  1. Move device address transmission to separate function (maybe in start_transaction?) or introduce bool argument to write function to control send or not to send device address.
  2. Allow read function to be called without arguments to just transmit device address and direction.

@pasko-zh
Copy link
Owner

pasko-zh commented Sep 13, 2016

A big thank you for your investigations 👏

Since my "other work" keeps me rather busy these days, my answer is a bit short:

  • I thought of a kind of ACK emulation as well, but with some loops of brzo_i2c_write(..., ..., true) for the ACK polling instead, i.e. omitting the STOP while polling (cf. figure 7-1 in the microchip datasheet), but... I didn't suggest it, because I felt that's an unnecessary overhead/complication and doesn't really (properly) solve the ACK polling...
  • I have to find some time for 1) and 2) and the other part in your answer.

btw: Since you mentioned "i2c_master", are you using the native SDK only or are you on the arduino toolchain?

EDIT: Will take a look at ACK Polling this weekend.

@valkuc
Copy link
Contributor Author

valkuc commented Sep 13, 2016

Yes, I'm working with native non-os SDK (https://github.com/CHERTS/esp8266-devkit). Your library does not work out-of-box with it, but with minor modifications (like changing boolean to bool and replacing pin related arguments and code from brzo_i2c_setup to native GPIO manipulations with PIN_FUNC_SELECT and GPIO_REG_WRITE) - all ok.

About ACK polling, it looks like really good thing because in my examples using ACK polling I have achieved only 1.4ms EEPROM post-write delay instead of hard-coded 10ms. So it was required to wait less more than in 5 times.

@mroavi
Copy link

mroavi commented Sep 14, 2016

I'm also trying to get the brzo_i2c library to work with a native non-os SDK (https://github.com/pfalcon/esp-open-sdk) . The compiler is giving me an error that I don't know how to fix:

modules/brzo_i2c.c:67:2: error: can't find a register in class 'RL_REGS' while reloading 'asm' asm volatile ( ^ modules/brzo_i2c.c:67:2: error: 'asm' operand has impossible constraints make: *** [build/modules/brzo_i2c.o] Error 1

The line it points to is the asm volatile ( inside the brzo_i2c_write function.

valkuc: Did you also run into this problem? Any idea of how to fix it?

@valkuc
Copy link
Contributor Author

valkuc commented Sep 14, 2016

No, I did not have this issue. Probably you miss necessary includes.

@mroavi
Copy link

mroavi commented Sep 14, 2016

Perhaps it could also be related to the fact that I had to remove the ICACHE_RAM_ATTR specifiers since the compiler was complaining as well. This is what it outputs:

modules/brzo_i2c.c:49:22: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'brzo_i2c_write'
 void ICACHE_RAM_ATTR brzo_i2c_write(uint8_t *data, uint8_t no_of_bytes, bool repeated_start) {
                      ^
modules/brzo_i2c.c:350:22: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'brzo_i2c_read'

Did you have issues with these specifiers as well?

@valkuc
Copy link
Contributor Author

valkuc commented Sep 14, 2016

Switch to gnu99 standard to solve issue with "asm volatile" (i.e. -std=gnu99 in CFLAGS). Or gnu89 if you currently on c89.
UPD: nevermind, yes there is no ICACHE_RAM_ATTR definition in native SDK.

@mroavi
Copy link

mroavi commented Sep 14, 2016

Didn't help. I get the same error using either -std=gnu99 or -std=gnu89 (with -std=gnu99 I get a bunch of new errors). By the way, I'm using the newest SDK from espressif: ESP8266_NONOS_SDK_V2.0.0_16_07_19.

The CFLAG variable in my Makefile looks like this:

# compiler flags using during compilation of source files
CFLAGS      =   -g          \
                -Wpointer-arith     \
                -Wundef         \
                -Wl,-EL         \
                -fno-inline-functions   \
                -nostdlib       \
                -mlongcalls \
                -mtext-section-literals \
                -ffunction-sections \
                -fdata-sections \
                -fno-builtin-printf\
                -DICACHE_FLASH \
                -DBUID_TIME=\"$(DATETIME)\" \
                -std=gnu89

@pasko-zh
Copy link
Owner

@valkuc : Great that you managed to compile it with the native SDK! I had a longer discussion with Pete Scargill about compiling my lib with the native SDK. As it seems, he was not successful.
Would you mind making your bzro-i2c version for the native SDK available? Or even make pull request to this repo, something like /nativeSDK for the sources?

@valkuc
Copy link
Contributor Author

valkuc commented Sep 14, 2016

Sure, I can share my version of bzro-i2c library for non-os SDK. One problem is that I have removed all Arduino related code from it. So I need some time to adopt it to work both on Arduino and clean SDK. Anyway I will upload it today evening/night.

@mroavi
Copy link

mroavi commented Sep 14, 2016

I also managed to compile it for the non-os SDK!

Adding the -O2 option to the CFLAGS did the trick:

# compiler flags using during compilation of source files
CFLAGS      =   -g -O2          \
                -Wpointer-arith     \
                -Wundef         \
                -Wl,-EL         \
                -fno-inline-functions   \
                -nostdlib       \
                -mlongcalls \
                -mtext-section-literals \
                -ffunction-sections \
                -fdata-sections \
                -fno-builtin-printf\
                -DICACHE_FLASH \
                -DBUID_TIME=\"$(DATETIME)\" \
                -std=gnu89

Also works with -g -O1 but not with -g -O0.

@pasko-zh
Copy link
Owner

pasko-zh commented Sep 14, 2016

@valkuc : Cool! Although being (much) less elegant, I could live with separate sources as well, i.e. one for native SDK, the other for Arduino...

@valkuc
Copy link
Contributor Author

valkuc commented Sep 14, 2016

Pull request created. Code base almost same, ASM code untouched, just added conditional defines for ARDUINO

@valkuc
Copy link
Contributor Author

valkuc commented Sep 14, 2016

As @mroavi detected, code must be compiled with -g -O1 or -g -O2 (I always use second one). Maybe @pasko-zh can look why asm code throws errors with -O0 option set. Also, as I previously noted, in order to compile asm volatile statements, gcc standard should be any gnu, not c.

@pasko-zh
Copy link
Owner

@valkuc I've included your pull request into release 1.1.0. I just did two minor changes to your pull-req, if it is a problem let me know.

@pasko-zh
Copy link
Owner

The ACK polling support, I will have to shift for next weeks. Did a couple of local changes in order to have then a bzro_i2c_ACK_polling(uint8_t control_byte, uint32_t ACK_polling_time_out_usec)

@valkuc
Copy link
Contributor Author

valkuc commented Sep 18, 2016

Thanks! Just added my vision about round() usage here #10 (comment)

@pasko-zh
Copy link
Owner

@valkuc : About success on ACK polling, i.e. when the master receives an ACK aft some times of polling: Does the EEPROM expect a STOP and then (of the next command) a START ... or is it without the STOP? If I have a look at the microchip datasheet, it says (page 10) ...If the cycle is complete, then the device will return the ACK and the master can then proceed with the next Read or Write command. => This is as Figure 7-1, i.e. without the STOP.
(btw: the ATMEL datasheet is -- at least for me -- much less clearer about ACK polling)
This is different than your code snippet provided here, which does send the STOP after the master has received an ACK.

So, which behaviour should I implement?

@valkuc
Copy link
Contributor Author

valkuc commented Sep 24, 2016

I think we don't need to send STOP... we just poll, poll, poll slave device in loop until receive response, then exit loop.

@pasko-zh
Copy link
Owner

OK

@valkuc
Copy link
Contributor Author

valkuc commented Sep 24, 2016

From http://www.microchip.com/forums/m536035.aspx

● Initial condition: a Write cycle is in progress.

● Step 1: the bus master issues a Start condition followed by a device select code (the
first byte of the new instruction with write bit low).

● Step 2: if the device is busy with the internal Write cycle, no ACK will be returned and
the bus master goes back to Step 1. If the device has terminated the internal Write
cycle, it responds with an ACK, indicating that the device is ready to receive the second
part of the instruction (the first byte of this instruction having been sent during Step 1).

@pasko-zh
Copy link
Owner

pasko-zh commented Oct 1, 2016

@valkuc

I've just uploaded a first version of ACK polling, it's in this branch.

Since I dont' have the EEPROM I cannot really test it. When I do the test with another i2c slave, I correctly get an ACK polling time out. Also, the scope pictures looks OK, i.e. the loop should work.

For the Arduino Tool Chain, a very simple test sketch looks like this

#include "brzo_i2c\brzo_i2c.h";
uint8_t SDA_PIN = 5;
uint8_t SCL_PIN = 4;
uint8_t SLAVE_ADDRESS = 0x52; 
// 7 Bit Address is 52h aka Control Byte 1010 010 0b, with A2 = 0, A1 = 1, A0 = 0 and W = 0 => 164d, A4h
uint8_t dummy = 10;
uint8_t error = 0;

void setup() {
    delay(1000);
    Serial.begin(115200);
    brzo_i2c_setup(SDA_PIN, SCL_PIN, 20000);
    delay(1000);
}

void loop() {
    Serial.println("Waiting 5 Seconds...");
    delay(5000);
    brzo_i2c_start_transaction(SLAVE_ADDRESS, 400);
        // Write only the slave address, with the EEPROM you would write some bytes of course
        brzo_i2c_write(&dummy, 0, false);
        // With 400 KHz, a 50 usec timeout gives two iterations, 
        //   otherwise here it is with 10 msec, i.e. 10000 usec
        brzo_i2c_ACK_polling(10000);
    error = brzo_i2c_end_transaction();
    Serial.println(error);
}

brzo_i2c_ACK_polling(uint16_t ACK_polling_time_out_usec); :

  • Re-Uses the slave Address given in brzo_i2c_start_transaction(.)
  • The ACK polling timeout can be max 65 msec (2^16) and is given in usec
  • it implements it the following way: Send START, check for ACK as long as time out is not reached. If time out is reached, send STOP. If ACK was detected before timeout, then it will make sure that the postcondition SDA = 1 and SCL = 1 is met -- but this does not send a STOP. It's just the proper postcondition, such that the next i2c command can begin with START again.

Could you please check it? If you have a scope, scope picture will help me doing the debugging 😄

Could you please test it with your EEPROM?

@valkuc
Copy link
Contributor Author

valkuc commented Oct 2, 2016

Thanks!
Sure, I will check it on real EEPROM and let you know.

@valkuc
Copy link
Contributor Author

valkuc commented Oct 4, 2016

I have tested new method. It's "almost" working, only one strange thing happens.
So, I'm using next method to wait until EEPROM finish it's internal write cycle:

static void ICACHE_FLASH_ATTR at24c_write_wait()
{
    do
    {
        os_delay_us(100);
        brzo_i2c_start_transaction(AT24C_ADDR, 100);

        uint32 s = system_get_time();
        brzo_i2c_ACK_polling(10000);
        os_printf("t = %d\n", system_get_time() - s);
    }
    while(brzo_i2c_end_transaction() != 0);
}

Writing 40 bytes to EEPROM (40 is just to write across multiple pages: write 32 bytes to first page, then 8 to second, they will be split by my routines into two write sequences) gives next signals in logic analizer:

Time [s]     Analyzer Name   Decoded Protocol Result
0.000005000000000   I2C Setup Write to ['160'] + ACK
0.000097000000000   I2C 0' + ACK
0.000189000000000   I2C 0' + ACK
0.000281000000000   I2C 5' + ACK
0.000373000000000   I2C 5' + ACK
0.000465000000000   I2C 5' + ACK
0.000557000000000   I2C 5' + ACK
0.000649000000000   I2C 5' + ACK
0.000741500000000   I2C 5' + ACK
0.000833500000000   I2C 5' + ACK
0.000925500000000   I2C 5' + ACK
0.001017500000000   I2C 5' + ACK
0.001109500000000   I2C 5' + ACK
0.001201500000000   I2C 5' + ACK
0.001293500000000   I2C 5' + ACK
0.001385500000000   I2C 5' + ACK
0.001477500000000   I2C 5' + ACK
0.001570000000000   I2C 5' + ACK
0.001662000000000   I2C 5' + ACK
0.001754000000000   I2C 5' + ACK
0.001846000000000   I2C 5' + ACK
0.001938000000000   I2C 5' + ACK
0.002030000000000   I2C 5' + ACK
0.002122000000000   I2C 5' + ACK
0.002214000000000   I2C 5' + ACK
0.002306000000000   I2C 5' + ACK
0.002398500000000   I2C 5' + ACK
0.002490500000000   I2C 5' + ACK
0.002582500000000   I2C 5' + ACK
0.002674500000000   I2C 5' + ACK
0.002766500000000   I2C 5' + ACK
0.002858500000000   I2C 5' + ACK
0.002950500000000   I2C 5' + ACK
0.003042500000000   I2C 5' + ACK
0.003134500000000   I2C 5' + ACK
0.003358500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.003458000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.003555500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.003653000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.003750500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.003847500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.003945000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.004042500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.004140000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.004237000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.004334500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.004432000000000   I2C 160' + ACK
0.004680000000000   I2C Setup Write to ['160'] + ACK
0.004823000000000   I2C Setup Write to ['160'] + ACK
0.004915000000000   I2C 0' + ACK
0.005007000000000   I2C  ' + ACK
0.005099000000000   I2C 5' + ACK
0.005191000000000   I2C 5' + ACK
0.005283000000000   I2C 5' + ACK
0.005375500000000   I2C 5' + ACK
0.005467500000000   I2C 5' + ACK
0.005559500000000   I2C 5' + ACK
0.005651500000000   I2C 5' + ACK
0.005743500000000   I2C 5' + ACK
0.005964500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006062000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006159000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006256500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006354000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006451500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006548500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006646000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006743500000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006841000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.006938000000000   I2C Setup Write to ['160'] + Missing ACK/NAK
0.007035500000000   I2C 160' + ACK

So, looking in communication data it looks like all super and as it should be.
But the strange thing appearing in terminal (console):

t = 1195
t = 106
t = 1176
t = 105

For some reason in terminal I see multiple occurrences of t.

As I noted before, I'm writing 40 bytes, so actually I'm doing 2 write cycles - 32 and 8 bytes each.
For each write operation brzo_i2c_ACK_polling(10000); called twice and take next microseconds to execute: 1195, 106 and 1176, 105. Each first call to method returns error code 2 (If the ACK polling time out was exceeded, we will have a NACK, too), but as you see from logic analizer there was no NACK, and my timeout is 10000us.

UPD:
In my opinion, brzo_i2c_ack_polling looks better than brzo_i2c_ACK_polling, but suit yourself ;)

@pasko-zh
Copy link
Owner

pasko-zh commented Oct 4, 2016

Thanks for the tests. I guess I already know... I forgot something ;-)
When we had an ACK but not a timeout, we jump here. And in the following code onwards from here, everything is done, except: Setting the error code to 0, since it is still 2 from the last NACK iteration. So adding "MOVI.N %[r_error], 0;" schould do it.
Because since it was still 2, the second iteration of your loop takes place. Then the error is set 0 here, we have an ACK immediately after sending the address byte and hence we do not iterate over the ACK polling loop and we just exit, and thus the short delay of 105/106 in your observations...

=> I will add this and you can test more :-)

@pasko-zh
Copy link
Owner

pasko-zh commented Oct 4, 2016

I've just added the (most probably) missing statement here in the ack polling branch.
Could you please try again? Now you should get only one iteration, i.e. something around 1195.

btw: Your waiting loop is dangerous, it will iterate over and over in case of ACK polling timeouts (i.e. error return code 34), but I think you did it for test purposes only. Otherwise change while(brzo_i2c_end_transaction() != 0) to !=34, or don't even loop ;-) just calling brzo_i2c_ACK_polling(10000); once should do the job.

@valkuc
Copy link
Contributor Author

valkuc commented Oct 4, 2016

Tested again. Now all works as expected! Guess it's time to merge this branch into master.

Otherwise change while(brzo_i2c_end_transaction() != 0) to !=34 , or don't even loop ;-) just calling brzo_i2c_ACK_polling(10000); once should do the job.

Sure, I have added loop only to find that issue, now I will remove it.

@valkuc
Copy link
Contributor Author

valkuc commented Oct 4, 2016

Now, about what I described in comment #9 (comment) about os_memcpy and extra buffer. Would you prefer that I create a new problem and move the discussion there?

@pasko-zh
Copy link
Owner

pasko-zh commented Oct 5, 2016

  1. I will merge the branch ASAP, thanks again for testing!
  2. Yes please, open a new Issue concerning buffer.

@valkuc valkuc closed this as completed Oct 5, 2016
pasko-zh added a commit that referenced this issue Oct 5, 2016
ACK polling support added #9  and removed floating point round #12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants