Closed valkuc closed 7 years ago
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();
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.
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?
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?brzo_i2c_ACK_polling(max_timeout)
. 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.
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 https://github.com/pasko-zh/brzo_i2c/issues/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();
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.
A big thank you for your investigations :clap:
Since my "other work" keeps me rather busy these days, my answer is a bit short:
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...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.
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.
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?
No, I did not have this issue. Probably you miss necessary includes.
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?
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.
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
@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?
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.
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
.
@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...
Pull request created. Code base almost same, ASM code untouched, just added conditional defines for ARDUINO
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
.
@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.
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)
Thanks! Just added my vision about round() usage here https://github.com/pasko-zh/brzo_i2c/pull/10#issuecomment-247860231
@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?
I think we don't need to send STOP... we just poll, poll, poll slave device in loop until receive response, then exit loop.
OK
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).
@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);
:
brzo_i2c_start_transaction(.)
Could you please check it? If you have a scope, scope picture will help me doing the debugging :smile:
Could you please test it with your EEPROM?
Thanks! Sure, I will check it on real EEPROM and let you know.
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 ;)
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 :-)
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.
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.
Now, about what I described in comment https://github.com/pasko-zh/brzo_i2c/issues/9#issuecomment-246518954 about os_memcpy
and extra buffer. Would you prefer that I create a new problem and move the discussion there?
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:
How can I do ACK polling with you library?