Fixes connect() bug, crash bug, I/O error bug, and more. #23
Conversation
This file contains hidden or 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
added 6 commits
August 14, 2023 11:51
The device address was incorrectly hard-coded as 0 when the channel was re-initialized after recovery of a USB_TYPE_ERROR.
The HAL_HCD_HC_Halt() function doesn't actually halt a channel correctly, so this fix replaces those calls with code that does. This is necessary both when freeing the bulk endpoints when a USB thumb drive is disconnected, but also when performing error recovery on a channel that is blocked in a NAK state. The HAL_HCD_HC_Init() function doesn't return the channel to an active state itself, so there's also code added to do that. Finally, the endpoint object must be returned to USB_TYPE_IDLE at the end of such an error recovery.
Reverting previous incorrect spelling mistake fix. The comment should really be about unstall (not uninstall) as in clearing the halted feature of the USB EPs when receiving a stall.
An older fix (commit 72d0aa6) introduced the possiblity of dereferencing an uninitialized host pointer by moving the getHostInst() call from USBHostMSD()'s constructor to connect(). This is now solved by adding guards against an uninitialized host pointer in checkResult(), SCSITransfer() and getMaxLun(). The practical implication of the bug was that mounting before connecting led to a crash, but now just leads to an error message from the mount call.
Adds a 50 us delay after the request to disable a channel, to give it time to actually disable.
Setting state to USB_TYPE_IDLE allows the library to recover from an error after the channels have been cleared. But this might lead to some errors going unreported, and should be improved in the future - but at least the library passes all our tests from higher levels in the stack at this point, which it didn't before. Our tests don't show any signs of data loss when the error recovery kicks in with the current solution. Fixing this last problem will probably take quite some effort (time), which isn't possible at the moment.
|
Memory usage change @ d8287c3
Click for full report table
Click for full report CSV |
|
The Spell Check failure is because a comment correctly contains the word "unstall" - as in clearing the halted feature of the USB EPs when receiving a stall. It's not supposed to be "uninstall" even though that would satisfy Spell Check. |
per1234
requested changes
Aug 14, 2023
There was a problem hiding this comment.
The Spell Check failure is because a comment correctly contains the word "unstall"
Please add the word that is producing the false spell check positive to the ignore-words-list field of the .codespellrc configuration file:
Arduino_USBHostMbed5/.codespellrc
Line 5 in d8287c3
See commit 3d1019e for more information.
|
Memory usage change @ 69326c0
Click for full report table
Click for full report CSV |
Set state to USB_TYPE_IDLE to allow recover from the error, but return the error. The previous solution didn't correctly report all errors.
|
Memory usage change @ c333aa4
Click for full report table
Click for full report CSV |
added 2 commits
August 20, 2023 14:02
MAX_NYET_RETRY is an incorrect name, because URB_NOTREADY can have several different causes that are unrelated to each other. Therefore, MAX_NOTREADY_RETRY is a better name. This commit also changes a comment regarding the case where the max retries limit has been reached, to better describe what happens.
Increases the MAX_NOTREADY_RETRY from 5 to 50000. The previous value (5) was not nearly enough to handle the slow writes to some USB thumb drives. While it worked with some models, it failed all the time with others. 50000 might be a bit on the high side, but it adds margin since we can't possibly test all the models out there.
|
Memory usage change @ 8c3c35a
Click for full report table
Click for full report CSV |
|
Memory usage change @ a4ac6d1
Click for full report table
Click for full report CSV |
alrvid
changed the title
added 3 commits
August 20, 2023 17:42
This commit improves the source code comments in several locations, adding explanations for things that might easily be misunderstood, or take a long time to understand.
errror => error
recover => recovery
|
Memory usage change @ 3a90c05
Click for full report table
Click for full report CSV |
|
Lovely work, thanks a lot! |
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.
Uh oh!
There was an error while loading. Please reload this page.