Fix lockups when accessing SD from foreground and background task simultaneously #2446
Conversation
Note how the inhibit value is decremented in both the regular case and the exceptional case. Closes: adafruit#2417
|
I'll fix the failing build soon, I don't think it'll change the substance of the PR. |
dhalbert
left a comment
dhalbert
left a comment
There was a problem hiding this comment.
I wonder if this is going to cause significant possibly fatal hiccups in background tasks. It certainly might cause hiccups in display refresh, and we also expect to service USB at least every 1 msec.
|
We sure can't guarantee that the I/O operation will take any bounded length of time, so I agree there's a chance of missing some ticks. But isn't that already the case, when the display background text needs to read from SD? The main difference I can think of is, the total quantity of I/O in one background task is probably bounded, but a user could probably read larger amounts of data (say, 32kB if they work at it?) in the foreground, and that blocks background by much longer than reading a kB or two of bitmap or audio data. |
|
On my pygamer with this sd card https://adafru.it/1294, reading 1 512-byte block takes about 7ms and scales linearly from there. It's no trouble at all to make a sd read take the majority of a second. |
Yes, but that is interruptible, I think, so that USB background tasks are not blocked during that time. Display refresh prevents itself from being called recursively. Blocking all background tasks is problematic. Blocking background tasks that do SD operations is what we want to do. I'm not sure if it works to just fail out of a display background refresh if it needs to read from SD. If that works, great. But if not, then we can have a semaphore/lock on SD use, and have each background task just return immediately if it knows it might need to use the SD. For the current set of background tasks, that's just display refresh, I think. |
|
Result of in-the-weeds discussion: make this specific to display and audio and it may be mergeable. |
|
Can we close this for now? Should we have an issue for it instead? It could link to this branch. |
|
will get back to this at some point, but not right now. when I do, it'll need to be pretty completely re-worked, so I'll open a fresh PR. |
3 participants
No description provided.