-
Notifications
You must be signed in to change notification settings - Fork 46
Remove delays #40
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
Closed
Closed
Remove delays #40
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that when you issue a command to the PN532
_wait_ready
is called before it is ready to respond, which results in an OSError. I tried to check in CP what error was being triggered and it was a NACK. When I removed this line it ends up only taking a few milliseconds to send the ready status. But with this line, it got stuck sleeping for half a second. And if there is no_req
pin setup, the only thing thatself._wakeup
does is sleep for half a second, which is overkill for here.It is also worth noting that this problem didn't present itself on older versions of CP for me. I think some of the newer performance improvements are the reason this is surfacing.
_wait_ready
was never called fast enough after a command to cause the OSError to be thrown like it is now. Sooo that's kind of cool lol.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not return early from
_wakeup
if_req
isn't available? Seems like we do want the ability to sleep and wake the device.🎉 Does the OSError vary depending on whether it's asleep or waking up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a good idea.
It doesn't seem to matter. I'm not 100% sure, but when running one of the simple examples each time you loop through it you always get the same OSError.
I've been looking through the PN532 manual, and while I definitely am not familiar with reading this kind of thing, so I could be wrong, but I think the issue doesn't have to do with waking up. At least not the problem I was seeing.
I think waking up happens in between
I2C address Write
andCOMMAND frame
. The OSError is being thrown when we try to read in betweenCOMMAND frame
andI2C address Read + Acknowledge frame
. So wouldn't calling_wakeup
be unnecessary?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know anything about the sensor itself. Does it power down automatically or can we track when the driver shuts it down?