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

ESP_SPIcontrol._wait_for_ready is eating lots of time polling the READY/BUSY pin #108

Open
siddacious opened this issue Sep 29, 2020 · 12 comments

Comments

@siddacious
Copy link
Contributor

Context:
#106
https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/adafruit_esp32spi/adafruit_esp32spi.py#L176

I haven't verified this exhaustively, but as far as I can tell, practically every method in ESP_SPIcontrol will at some point call _wait_for_ready somewhere in it's call stack.

I've got a simple sensor logger that's calling IO using an airlift, and round trip for a successful post is on average about 8 seconds. Only about 2-3 seconds of that is actually opening sockets, sending, receiving, parsing, etc, the rest of the time we're waiting for the ESP32.

This means that until I added the hack in the PR above, I couldn't have a UI. The hack was admittedly a hack so I closed it, but this is a problem that should be solved one way or another

@tannewt suggested starting on the async/concurrency project which would be nice, but I don't see that being done any time soon and it would be nice to have this addressed in some capacity before then

@tannewt
Copy link
Member

tannewt commented Sep 30, 2020

When this is an important issue, we should do it with asyncio because that is how it's done in CPython. Doing it another way will just make it harder to move to asyncio after and be inconsistent in the meantime. I don't actually think we're that far from asyncio support, it just needs a reason to work on it. @warriorofwire has gotten it pretty far and may have ideas about this library.

@siddacious
Copy link
Contributor Author

What do you mean by "when this is an important issue"? It seems important now. Perhaps you meant "while"?

@kvc0
Copy link

kvc0 commented Sep 30, 2020

Yeah looking at that PR I'm inclined to congratulate you on an industrious workaround that worked for your project. That this library has a _wait_for_ready method means that it can pretty easily support both async and synchronous patterns.

I need to revitalize my circuitpython development environment on my new machine and finish async support. Or maybe you're interested? :-)

@siddacious
Copy link
Contributor Author

It would be interesting to see how the async code you're working on is coming together, however relying on me to get it done would be a good way to not see it be finished this year as I'm already waist deep in several projects.

@tannewt
Copy link
Member

tannewt commented Oct 1, 2020

What do you mean by "when this is an important issue"? It seems important now. Perhaps you meant "while"?

Sorry, I should have said, "When this is our top priority", or left it off entirely. I'd like to see it done with asyncio. I don't have the time to make that happen. Hopefully someone else can help us get there sooner.

@siddacious
Copy link
Contributor Author

Where is it in terms of priority? I'm asking because before too long there's going to be another 4k+ users of this library and it would be nice to provide them a better experience if possible

@tannewt
Copy link
Member

tannewt commented Oct 6, 2020

@siddacious Not very high. I'm more concerned with using less memory during requests than allowing for concurrency.

@siddacious
Copy link
Contributor Author

@tannewt holy cow I had no idea:
image

@tannewt
Copy link
Member

tannewt commented Oct 9, 2020

Yuuuuup! I've made a number of changes in https://github.com/tannewt/Adafruit_CircuitPython_ESP32SPI/tree/split that would be great to merge into main.

@siddacious
Copy link
Contributor Author

@tannewt 👀

@siddacious
Copy link
Contributor Author

@tannewt is the above split branch still wanting to be merged? I saw a bit of talk around NINA/ airlift stuff so I wanted to make sure it wouldn't be obsolete by the time it was mergeable

@tannewt
Copy link
Member

tannewt commented Nov 3, 2020

@siddacious I think it'll be easier to slim down the changes in split to just the wifi and socket APIs that match the new S2 API and then merge them into the Airlift API. The ESP32SPI library has corner cases that will be hard to test like digitalio support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants