Add I2C slave support #3287
Add I2C slave support #3287
Conversation
Renaming, adding static keyword
Renaming, adding static keyword
|
see also #3237 |
There was a problem hiding this comment.
At this point the PR contains too much unused commented out code, and very few comments to explain the logic. I am not confident that maintainers of the repo will be able to support this code effectively unless these issues are addressed.
The other problem i see is that the code size (including IRAM) and DRAM usage will increase for all users of Wire library, even if they are not using slave mode. The PR should come with some measurements which show how much the code/data size have increased. If this increase is significant, measures should be taken to ensure that new features are only linked into the binary if slave mode is actually used in the application.
| //pinMode(12, OUTPUT); | ||
| //pinMode(14, OUTPUT); | ||
| //pinMode(13, OUTPUT); | ||
| //digitalWrite(2, HIGH); |
There was a problem hiding this comment.
Instead of commenting unused lines, please consider removing them
| ets_timer_setfn(&timer, onTimer, NULL); | ||
|
|
||
| // create event task | ||
| ets_task(eventTask, EVENTTASK_QUEUE_PRIO, eventTaskQueue, EVENTTASK_QUEUE_SIZE); |
There was a problem hiding this comment.
When using I2C in master mode, it should not be necessary to create this event task and the timer. Same applies to twi_txBuffer and twi_rxBuffer — these should be linked in if only master mode is used.
| // transmit master read ready signal, with or without ack | ||
| if (ack) { | ||
| //TWCR = _BV(TWEN) | _BV(TWIE) | _BV(TWINT) | _BV(TWEA); |
There was a problem hiding this comment.
Is this commented out line needed?
| } | ||
|
|
||
| #if 1 |
| void ICACHE_RAM_ATTR twi_stop(void) | ||
| { | ||
| // send stop condition |
There was a problem hiding this comment.
The reset of the file is indented with spaces, please make indentation consistent.
| while (false) { //TWCR & _BV(TWSTO)){ | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This loop seems to be a no-op, can it be removed?
| void ICACHE_RAM_ATTR onTimer(void *timer_arg) | ||
| { | ||
| //digitalWrite(13, HIGH); |
There was a problem hiding this comment.
likewise — remove commented out lines of code.
| //switch(TW_STATUS){ | ||
| switch(status) { | ||
| #if 0 |
There was a problem hiding this comment.
Can this block be removed instead of disabling it?
| #define TWIP_READ_ACK 12 | ||
| #define TWIP_RWAIT_ACK 13 | ||
| #define TWIP_WRITE 14 | ||
| #define TWIP_BUS_ERR 15 |
There was a problem hiding this comment.
It would be great to describe what each of these states means, and what are the possible transitions. Looking at the code, it seems to me that some of the states here are not actually being used. This does not help understand how the proposed code works.
|
CC @bjoham |
|
Closing in favor of #5226 . |
This is from the i2c_slave branch of bjoham/esp8266_Arduino, rebased against master.