Fix for spurious interrupts during I2C communications #1665
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This version no longer needs an interrupt for each byte transferred. It only needs interrupts for START, STOP, FIFO empty/Full or error conditions. This dramatically reduces the interrupt overhead. I think the prior version was causing an interrupt overload condition where the ISR was not able to process every interrupt as they happened.
|
Tested it in my application on a TTGO v2.1 board with i2c SSD1306 OLED. Log level info: i don't see any errors or warnings.
|
|
@cyberman54 It looks like you are using an old file. The error message in this file is changed. You are using rc3. To test this change copy this file over top of the one in rc3. on my computer (win7 pro)
Chuck. |
|
@stickbreaker I thought i did copy the file to the right place. Looks like i'm doing wrong with my platformIO installation. I am not using Arduino IDE, but arduino-espressif32 framework on platformIO. I copied the file to this path:
|
|
@stickbreaker i managed to replace the file (just renamed the old one before, but that doesn't work since it is still compiled). Now it works, the speed is back and no more errors! Looks like this change in the i2c code was a heavy lifting, thank you for your efforts! |
|
always happy to merge such fixes :D |
stickbreaker
added a commit
to stickbreaker/arduino-esp32
that referenced
this pull request
Aug 2, 2018
pr espressif#1665 introduce a problem with ReSTART, when solving this problem I found an interaction between the TxFifo refill, RxFifo empty and CMD[] fill. during certain sequences a dataqueue command would be skipped, this skipping resulted in a mismatch between the contents of the TxFifo and the i2c command sequence. The problem manifested as an ACK error. In addition to this required bug fix I propose: * `Wire.begin()` be changed from a `void` to a `bool` this will allow the reset functionality of `Wire.begin()` to be reported. Currently `Wire.begin()` attempts to reset the i2c Peripheral, but cannot report success/failure. * `Wire.busy()` be added. this `bool` function returns the hardware status of the bus. This status can be use in multi-master environments for application level interleaving of commands, also in single master environment, it can be used to detect a 'hung' bus. With the functional change to `Wire.begin()` this allows app level recover of a hung bus. * `Wire.lastError()` value updated for all errors, previously when interleaving `Wire.endTransmission(false)` and `Wire.readTransmission(false)`, the 128 byte `Wire.write()` buffer was exhausted without generating and error(very exotic). I discovered this error when I created a sequence of directed reads to a EEPROM. Each directed read used 2 bytes of the 128 byte `write()` buffer, so after 64 consecutive ReSTART writes with ReSTART reads, `Wire()` had no room to record the directed address bytes. It generated just a NAK check without setting the EEPROMs internal register address. The succeeding ReSTART read succeeded at incorrect address. * Changes to the HAL layer: ** added `i2cGetStatus()` which returns the i2c peripheral status word, used to detect bus_busy currently ** added `i2cDebug()` programmatic control of debug buffer output ** changed `i2cAddQueue()` to allow data_only queue element this will allow a i2c transaction to use multiple data pointers. ** removed direct access to DumpInts(), DumpI2c() from app, use i2cDebug() to set trigger points *
stickbreaker
added a commit
to stickbreaker/arduino-esp32
that referenced
this pull request
Aug 2, 2018
pr espressif#1665 introduce a problem with ReSTART, when solving this problem I found an interaction between the TxFifo refill, RxFifo empty and CMD[] fill. during certain sequences a dataqueue command would be skipped, this skipping resulted in a mismatch between the contents of the TxFifo and the i2c command sequence. The problem manifested as an ACK error. In addition to this required bug fix I propose: * `Wire.begin()` be changed from a `void` to a `bool` this will allow the reset functionality of `Wire.begin()` to be reported. Currently `Wire.begin()` attempts to reset the i2c Peripheral, but cannot report success/failure. * `Wire.busy()` be added. this `bool` function returns the hardware status of the bus. This status can be use in multi-master environments for application level interleaving of commands, also in single master environment, it can be used to detect a 'hung' bus. With the functional change to `Wire.begin()` this allows app level recover of a hung bus. * `Wire.lastError()` value updated for all errors, previously when interleaving `Wire.endTransmission(false)` and `Wire.readTransmission(false)`, the 128 byte `Wire.write()` buffer was exhausted without generating and error(very exotic). I discovered this error when I created a sequence of directed reads to a EEPROM. Each directed read used 2 bytes of the 128 byte `write()` buffer, so after 64 consecutive ReSTART writes with ReSTART reads, `Wire()` had no room to record the directed address bytes. It generated just a NAK check without setting the EEPROMs internal register address. The succeeding ReSTART read succeeded at incorrect address. * Changes to the HAL layer: ** added `i2cGetStatus()` which returns the i2c peripheral status word, used to detect bus_busy currently ** added `i2cDebug()` programmatic control of debug buffer output ** changed `i2cAddQueue()` to allow data_only queue element this will allow a i2c transaction to use multiple data pointers. ** removed direct access to DumpInts(), DumpI2c() from app, use i2cDebug() to set trigger points *
me-no-dev
pushed a commit
that referenced
this pull request
Aug 14, 2018
* ReSTART fix, Sequencing fix pr #1665 introduce a problem with ReSTART, when solving this problem I found an interaction between the TxFifo refill, RxFifo empty and CMD[] fill. during certain sequences a dataqueue command would be skipped, this skipping resulted in a mismatch between the contents of the TxFifo and the i2c command sequence. The problem manifested as an ACK error. In addition to this required bug fix I propose: * `Wire.begin()` be changed from a `void` to a `bool` this will allow the reset functionality of `Wire.begin()` to be reported. Currently `Wire.begin()` attempts to reset the i2c Peripheral, but cannot report success/failure. * `Wire.busy()` be added. this `bool` function returns the hardware status of the bus. This status can be use in multi-master environments for application level interleaving of commands, also in single master environment, it can be used to detect a 'hung' bus. With the functional change to `Wire.begin()` this allows app level recover of a hung bus. * `Wire.lastError()` value updated for all errors, previously when interleaving `Wire.endTransmission(false)` and `Wire.readTransmission(false)`, the 128 byte `Wire.write()` buffer was exhausted without generating and error(very exotic). I discovered this error when I created a sequence of directed reads to a EEPROM. Each directed read used 2 bytes of the 128 byte `write()` buffer, so after 64 consecutive ReSTART writes with ReSTART reads, `Wire()` had no room to record the directed address bytes. It generated just a NAK check without setting the EEPROMs internal register address. The succeeding ReSTART read succeeded at incorrect address. * Changes to the HAL layer: ** added `i2cGetStatus()` which returns the i2c peripheral status word, used to detect bus_busy currently ** added `i2cDebug()` programmatic control of debug buffer output ** changed `i2cAddQueue()` to allow data_only queue element this will allow a i2c transaction to use multiple data pointers. ** removed direct access to DumpInts(), DumpI2c() from app, use i2cDebug() to set trigger points * * Update esp32-hal-i2c.c * Update Wire.cpp * ReSTART, Sequencing pr #1665 introduce a problem with ReSTART, when solving this problem I found an interaction between the TxFifo refill, RxFifo empty and CMD[] fill. during certain sequences a dataqueue command would be skipped, this skipping resulted in a mismatch between the contents of the TxFifo and the i2c command sequence. The problem manifested as an ACK error. In addition to this required bug fix I propose: * `Wire.begin()` be changed from a `void` to a `bool` this will allow the reset functionality of `Wire.begin()` to be reported. Currently `Wire.begin()` attempts to reset the i2c Peripheral, but cannot report success/failure. * `Wire.busy()` be added. this `bool` function returns the hardware status of the bus. This status can be use in multi-master environments for application level interleaving of commands, also in single master environment, it can be used to detect a 'hung' bus. With the functional change to `Wire.begin()` this allows app level recover of a hung bus. * `Wire.lastError()` value updated for all errors, previously when interleaving `Wire.endTransmission(false)` and `Wire.readTransmission(false)`, the 128 byte `Wire.write()` buffer was exhausted without generating and error(very exotic). I discovered this error when I created a sequence of directed reads to a EEPROM. Each directed read used 2 bytes of the 128 byte `write()` buffer, so after 64 consecutive ReSTART writes with ReSTART reads, `Wire()` had no room to record the directed address bytes. It generated just a NAK check without setting the EEPROMs internal register address. The succeeding ReSTART read succeeded at incorrect address. * Changes to the HAL layer: ** added `i2cGetStatus()` which returns the i2c peripheral status word, used to detect bus_busy currently ** added `i2cDebug()` programmatic control of debug buffer output ** changed `i2cAddQueue()` to allow data_only queue element this will allow a i2c transaction to use multiple data pointers. ** removed direct access to DumpInts(), DumpI2c() from app, use i2cDebug() to set trigger points * * Forgot DebugFlags Return @andriyadi found this, total brain fade on my part.
Curclamas
pushed a commit
to Curclamas/arduino-esp32
that referenced
this pull request
Aug 21, 2018
This version no longer needs an interrupt for each byte transferred. It only needs interrupts for START, STOP, FIFO empty/Full or error conditions. This dramatically reduces the interrupt overhead. I think the prior version was causing an interrupt overload condition where the ISR was not able to process every interrupt as they happened.
Curclamas
pushed a commit
to Curclamas/arduino-esp32
that referenced
this pull request
Aug 21, 2018
* ReSTART fix, Sequencing fix pr espressif#1665 introduce a problem with ReSTART, when solving this problem I found an interaction between the TxFifo refill, RxFifo empty and CMD[] fill. during certain sequences a dataqueue command would be skipped, this skipping resulted in a mismatch between the contents of the TxFifo and the i2c command sequence. The problem manifested as an ACK error. In addition to this required bug fix I propose: * `Wire.begin()` be changed from a `void` to a `bool` this will allow the reset functionality of `Wire.begin()` to be reported. Currently `Wire.begin()` attempts to reset the i2c Peripheral, but cannot report success/failure. * `Wire.busy()` be added. this `bool` function returns the hardware status of the bus. This status can be use in multi-master environments for application level interleaving of commands, also in single master environment, it can be used to detect a 'hung' bus. With the functional change to `Wire.begin()` this allows app level recover of a hung bus. * `Wire.lastError()` value updated for all errors, previously when interleaving `Wire.endTransmission(false)` and `Wire.readTransmission(false)`, the 128 byte `Wire.write()` buffer was exhausted without generating and error(very exotic). I discovered this error when I created a sequence of directed reads to a EEPROM. Each directed read used 2 bytes of the 128 byte `write()` buffer, so after 64 consecutive ReSTART writes with ReSTART reads, `Wire()` had no room to record the directed address bytes. It generated just a NAK check without setting the EEPROMs internal register address. The succeeding ReSTART read succeeded at incorrect address. * Changes to the HAL layer: ** added `i2cGetStatus()` which returns the i2c peripheral status word, used to detect bus_busy currently ** added `i2cDebug()` programmatic control of debug buffer output ** changed `i2cAddQueue()` to allow data_only queue element this will allow a i2c transaction to use multiple data pointers. ** removed direct access to DumpInts(), DumpI2c() from app, use i2cDebug() to set trigger points * * Update esp32-hal-i2c.c * Update Wire.cpp * ReSTART, Sequencing pr espressif#1665 introduce a problem with ReSTART, when solving this problem I found an interaction between the TxFifo refill, RxFifo empty and CMD[] fill. during certain sequences a dataqueue command would be skipped, this skipping resulted in a mismatch between the contents of the TxFifo and the i2c command sequence. The problem manifested as an ACK error. In addition to this required bug fix I propose: * `Wire.begin()` be changed from a `void` to a `bool` this will allow the reset functionality of `Wire.begin()` to be reported. Currently `Wire.begin()` attempts to reset the i2c Peripheral, but cannot report success/failure. * `Wire.busy()` be added. this `bool` function returns the hardware status of the bus. This status can be use in multi-master environments for application level interleaving of commands, also in single master environment, it can be used to detect a 'hung' bus. With the functional change to `Wire.begin()` this allows app level recover of a hung bus. * `Wire.lastError()` value updated for all errors, previously when interleaving `Wire.endTransmission(false)` and `Wire.readTransmission(false)`, the 128 byte `Wire.write()` buffer was exhausted without generating and error(very exotic). I discovered this error when I created a sequence of directed reads to a EEPROM. Each directed read used 2 bytes of the 128 byte `write()` buffer, so after 64 consecutive ReSTART writes with ReSTART reads, `Wire()` had no room to record the directed address bytes. It generated just a NAK check without setting the EEPROMs internal register address. The succeeding ReSTART read succeeded at incorrect address. * Changes to the HAL layer: ** added `i2cGetStatus()` which returns the i2c peripheral status word, used to detect bus_busy currently ** added `i2cDebug()` programmatic control of debug buffer output ** changed `i2cAddQueue()` to allow data_only queue element this will allow a i2c transaction to use multiple data pointers. ** removed direct access to DumpInts(), DumpI2c() from app, use i2cDebug() to set trigger points * * Forgot DebugFlags Return @andriyadi found this, total brain fade on my part.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This version no longer needs an interrupt for each byte transferred. It only needs interrupts for START, STOP, FIFO empty/Full or error conditions. This dramatically reduces the interrupt overhead. I think the prior version was causing an interrupt overload condition where the ISR was not able to process every interrupt as they happened.