New I2c slave driver (IDFGH-2501)

[loboris]

This PR proposes a different, in my opinion better and more convinient, way of handling ESP32 I2C slave device.

In slave mode i2c device with 128-4096 memory is emulated, via the slave buffer.
All master's read/write requests for data are handled by the driver's interrupt routine. The user only needs to provide the initial buffer content and, optionally, to handle the master's request from the slave task.

Master can read from or write to the ESP32 i2c device, providing the memory (buffer) address to read from or write to.

For buffer sizes 128-256 bytes, 8-bit addressing is used, for buffer sizes >256 bytes, 16-bit addressing is used.

---

Typical master write sequence is as follows:

For slave buffer size <= 256 8-bit addressing is used

_____________________________________________________________________________________
| start | slave_addr + wr_bit + ack | buff_addr + ack | write n bytes + ack  | stop |
--------|---------------------------|-----------------|----------------------|------|

For slave buffer size > 256 16-bit addressing is used

_____________________________________________________________________________________________________________
| start | slave_addr + wr_bit + ack | buff_addr_hi + ack | buff_addr_lo + ack | write n bytes + ack  | stop |
--------|---------------------------|--------------------|--------------------|----------------------|------|

Typical master read sequence is as follows:

For slave buffer size <= 256 8-bit addressing is used

_________________________________________________________________________________________________________________________________________________
| start | slave_addr + wr_bit + ack | buff_addr + ack | start | slave_addr + rd_bit + ack | read n-1 bytes + ack | read n-th byte + nack | stop |
--------|---------------------------|-----------------|-------|---------------------------|----------------------|-----------------------|------|

For slave buffer size > 256 16-bit addressing is used

_________________________________________________________________________________________________________________________________________________________________________
| start | slave_addr + wr_bit + ack | buff_addr_hi + ack | buff_addr_lo + ack | start | slave_addr + rd_bit + ack | read n-1 bytes + ack | read n-th byte + nack | stop |
--------|---------------------------|--------------------|--------------------|-------|---------------------------|---------  -----------|-----------------------|------|

---

Optional read only area at the end of the slave buffer can be set. Master can only read from that area, writing to it by the master will be ignored.

The API provides functions to read and write data from/to the slave buffer at any time: i2c_slave_read_buffer() and i2c_slave_write_buffer.

I2C slave task can be created which receives notifications from the i2c driver about the slave events: address set, data sent to master, data received from master.
Slave buffer address, number of bytes sent or received and overflow status are available to the slave task to take some action on slave events.

It is up to the user to organize the slave buffer in a way most appropriate for the application.
Writting to some addresses can, for example, be treated by the slave task as commands with optional arguments and some action taken.
Read only area can contain the slave ID, revision number, sensor data etc.

---

I2C example and documentation are updated for the new driver.

---

The driver was extensively tested with ESP32 i2c master (on the same and different board), STM32F4 hardware and software I2C driver and Atmel xmega chip's i2c driver with clock frequencies of 100000 and 400000.

[antonio-fiol]

I tried to use this new driver in my project (because I found that it must be much better than the default one), by patching the current master.
Unfortunately ...

Guru Meditation Error: Core  0 panic'ed (LoadProhibited). Exception was unhandled.
Core 0 register dump:
PC      : 0x400eefac  PS      : 0x00060b30  A0      : 0x800d3c92  A1      : 0x3ffc7dc0  
0x400eefac: i2c_master_cmd_begin at /media/esp/esp-idf/components/driver/i2c.c:1209 (discriminator 4)

A2      : 0x00000001  A3      : 0x3ffb4264  A4      : 0x00000020  A5      : 0x00000008  
A6      : 0x00000000  A7      : 0x00000001  A8      : 0x3ffb4d70  A9      : 0x3ffc7db0  
A10     : 0x00000000  A11     : 0x00000000  A12     : 0x00000064  A13     : 0x3ffc7aa0  
A14     : 0x3ffc7aa0  A15     : 0x00000001  SAR     : 0x00000019  EXCCAUSE: 0x0000001c  
EXCVADDR: 0x00000004  LBEG    : 0x4000c2e0  LEND    : 0x4000c2f6  LCOUNT  : 0xffffffff  

Backtrace: 0x400eefac:0x3ffc7dc0 0x400d3c8f:0x3ffc7df0
0x400eefac: i2c_master_cmd_begin at /media/esp/esp-idf/components/driver/i2c.c:1209 (discriminator 4)

0x400d3c8f: readRegister(unsigned char, unsigned char) at /media/esp/vias/main/Adafruit_ADS1015.cpp:75

Weirdly enough, it seems that the problem comes up when using the MASTER code, which @loboris did not change.

EDIT: Completely forget that. It was me being dumb. The new driver is working like a charm.

[tuupola]

I also found this more convenient way to handle i2c slaves than the current driver. Any change this is going to be merged?

[antonio-fiol]

@loboris Is it possible that your sample design for the slave might ignore some notifications?
It always had been my idea to do exactly as you described: "Writting to some addresses can, for example, be treated by the slave task as commands with optional arguments and some action taken."
I based my code on the same pattern from the sample, where xTaskNotifyWait is used to receive the notification from the task, then processing happens (including my custom code) but it would seem that before I ended up my processing the slave_state already changed value.
The only explanation I found so far is the fact that all the printing takes time and another I2C transaction from the master might come and overwrite the state.
Wondering if a better pattern would be queuing copies of the slave_state (advantage: quick operation, disadvantage: memory usage).
Maybe a better pattern is keeping the slave task processing time under 20 "bit-time" (shortest possible I2C message, right?) that is, under 50 microseconds... which feels like an eternity unless you're printing or doing other slow stuff.

[koobest]

Hi, @loboris
Thank you helping to improve our I2C driver, it's really a good implementation. But now I have some ideals. Since we are work in slave mode, so, how about to define our own protocol (of course only for our slave mode)? the users should to follow it (maybe not a protocol, but user can easily implement this framework)

write timing : |start| + |saddr+w+ack| + |cmd+ack| + |data+ack| + |stop|

read timing  : |start| + |saddr+w+ack| + |cmd+ack| + |stop|  (some_delay)  |start| + |saddr+r+ack| + |(n-1)data + ack| + |last 1 byte + nack|+ |stop|

I think the important field is cmd, which can be one or more bytes, it tells what the commucation purpose is, read or write, and the reg(Maybe ESP32 does not have this concept) can also be in this field. we only need to inform the user how many bytes were received (we can use a queue to notify the user), and other implementations depend on the users.
Do you have any ideals?
thanks !!

[antonio-fiol]

This new driver actually does something like that, except for the CMD part. You do not need a CMD to know if it is read or write, because that's already stated as one additional bit in the address byte. The new driver defines the concept of I2C registers (which does not exist in the underlying hardware, if my understanding is correct), and even proposes the ability to have some R/W registers and some read-only registers, probably a good idea as well. Once that concept is implemented, the new driver implements the usual protocol, the usual way that I2C devices with registers are accessed. With this new driver, for example, I have successfully used i2cget / i2cset from a raspberry pi, with the usual syntax, both for single-byte and for word sizes. I guess it would be possible for bigger sizes but I did not need that.

…

On Tue, Dec 18, 2018 at 4:36 AM kooho ***@***.***> wrote: Thank you helping to improve our I2C driver, it's really a good implementation. But now I have some ideals. Since we are work in slave mode, so, how about to define our own protocol (of course only for our slave mode)? the users should to follow it (maybe not a protocol, but user can easily implement this framework) write timing : |start| + |saddr+w+ack| + |cmd+ack| + |data+ack| + |stop| read timing : |start| + |saddr+w+ack| + |cmd+ack| + |stop| (some_delay) |start| + |saddr+r+ack| + |(n-1)data + ack| + |last 1 byte + nack|+ |stop| I think the important field is cmd, which can be one or more bytes, it tells what the commucation purpose is, read or write, and the reg(Maybe ESP32 does not have this concept) can also be in this field. we only need to inform the user how many bytes were received (we can use a queue to notify the user), and other implementations depend on the users. Do you have any ideals? thanks !! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2096 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWOtcQn-i7iLM7g0HB_8CrZUfzHhp-y-ks5u6GK-gaJpZM4U1LTf> .

[koobest]

Yes, the address byte have the bit to distinguish between reading and writing, but this bit only

available for I2C hardware. So I think it is necessary to keep the CMD field.

[koobest]

Hi, @ loboris
Your PR is reviewing, we have made some changes and it will soon merged into master. thanks for your contribution.

[loboris]

Yes, the address byte have the bit to distinguish between reading and writing, but this bit only

available for I2C hardware. So I think it is necessary to keep the CMD field.

I don't think implementing the CMD field or any protocol specific field is a good idea.
All protocol related things can be easily handled by the i2c slave task, so the user can implement his own protocol.
In a proposed way the driver can emulate any existing I2C device, and limiting the user to use any specific protocol or timings would only make its usability worse.

[koobest]

Hi, @loboris
When the slave receives some bytes, the driver layer will inform the application layer of this event and data length. the same with your implementation :-).

[tuupola]

Any news about this?

[CLAassistant]

All committers have signed the CLA.

[klaus-liebler]

Hopefully, this gets done soon. I am waiting eagerly...

[ftjuh]

Can you please, please, please advance this issue?

It's such a shame that a wonderful device like the ESP32 is not able to do sth. as simple as implementing a proper I2C slave.

It seems there is a conceptually sound and well tested solution by @loboris that only needs to be merged, and it's only a contributor's Agreement that is missing? Please, whoever is responsible or capable, please do sth. about this issue after all these years, I'd hate to have to switch to another platform because of this situation.

[klaus-liebler]

I have just committed C++ implementation for a ESP32 I2C slave. It emulates a 256byte memory with callbacks before read transactions and after write transactions. I did a complete rewrite based on the recently published hal libraries. It is not really well tested, but basics are working. Feel free to use whatever you need.
https://github.com/klaus-liebler/sensactIO/blob/main/sensactIO_firmware/main/i2c_mem.hh

Regards, Klaus

[0xjakob]

@loboris @klaus-liebler @ftjuh @tuupola We'll take a look at this PR during the next weeks. Thanks for this submission!
@loboris Could you please also sign the license/CLA, please? Otherwise we can't use the code in IDF since the licensing is unclear.

Best,
Jakob

[Alvin1Zhang]

@loboris Thanks for your contribution, could you please also sign the license/CLA? Thanks.

[0xjakob]

Sorry, as long as the licensing situation is not clear for us we won't be able to accept the code. In general, we highly welcome all contributions as long as the CLA has been signed. We are closing this for now since there hasn't been any activity related to the CLA signing recently.

[marciogranzotto]

@loboris could you take a look on this again please?