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

added clock stretch yield, [issue 2162] fixed twi::status #6860

Merged
merged 1 commit into from
Dec 1, 2019
Merged

added clock stretch yield, [issue 2162] fixed twi::status #6860

merged 1 commit into from
Dec 1, 2019

Conversation

Tech-TX
Copy link
Contributor

@Tech-TX Tech-TX commented Nov 30, 2019

Finally figured out what I'd done wrong with the earlier commit. No -a in a local commit!

issue #2162

Added devyte's PolledTimeout in a nested loop replacing the original WAIT_CLOCK_STRETCH(). The outer loop exits if SCL goes high or the loop times out. The inner loop does a yield() every 5ms while we're waiting.
Additionally, two small changes to twi::status that add a clock stretch test before we exit if we find SCL stuck low, and the trailing if (!write_start()) at the end is removed as it leaves SDA low, killing the bus.

@devyte devyte added this to the 2.7.0 milestone Dec 1, 2019
@earlephilhower
Copy link
Collaborator

Failure looked to be a transient Chocolatey failure (Windows package manager). Restarted, it's going OK now...

@devyte devyte merged commit cc6d346 into esp8266:master Dec 1, 2019
@dok-net
Copy link
Contributor

dok-net commented Dec 1, 2019

I propose amending this to, pending #6804 (for corrected timing):

        esp8266::polledTimeout::oneShotFastUs timeout(twi_clockStretchLimit);
        while(!timeout && !SCL_READ())  // loop is stretch duration up to stretch limit
        { 
            optimistic_yield(5000);
        }

Like this, it may fire yield() for the first time earlier, but since yield() runs recurrent scheduled functions (internally resetting the optimistic yield timeout in turn) it's still better and the right way, IMHO.

@dok-net
Copy link
Contributor

dok-net commented Dec 1, 2019

@devyte Code smell, priority inversion? A busy wait - you do this only for hi performance waiting - that every 5ms suddenly doesn't care about timely response and yields to whatever is in the scheduler, makes no sense to me. I propose this is a prime candidate for the esp_yield/esp_delay/esp_schedule pattern, if anything - or it's a "real-time" busy wait, period. Then the yield() has to go.

@Tech-TX Tech-TX deleted the bugfix/issue_2162 branch December 1, 2019 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants