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

Introduce timeout on DESfire operations #97

Closed
wants to merge 4 commits into from

Conversation

manu0401
Copy link
Contributor

@manu0401 manu0401 commented Dec 7, 2018

There are a few reasons why an NFC operation may timeout, including
the pn533 USB toggle bit bug. In that case it helps to report the
problem to calling process so that it can retry operation, instead
of hanging forever.

For instance, mifare_desfire_format_picc() may make the chip
unresponsive (see commit 91d3ff9),
but if calling process gets ETIMEDOUT, it can select_application,
authenticate and format_picc again, with some success.

There are a few reasons why an NFC operation may timeout, including
the pn533 USB toggle bit bug. In that case it helps to report the
problem to calling process so that it can retry operation, instead
of hanging forever.

For instance, mifare_desfire_format_picc() may make the chip
unresponsive (see commit 91d3ff9),
but if calling process gets ETIMEDOUT, it can select_application,
authenticate and format_picc again, with some success.
Copy link
Contributor

@SloCompTech SloCompTech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only added timemout for reading from chip, it is reasonable.

@smortex
Copy link
Contributor

smortex commented Dec 7, 2018

Yes, this looks reasonable. Can you rebase / squash your changes on top of this project's master so that the PR has only a single commit @manu0401 ? Thanks!

There are a few reasons why an NFC operation may timeout, including
the pn533 USB toggle bit bug. In that case it helps to report the
problem to calling process so that it can retry operation, instead
of hanging forever.

For instance, mifare_desfire_format_picc() may make the chip
unresponsive (see commit 91d3ff9),
but if calling process gets ETIMEDOUT, it can select_application,
authenticate and format_picc again, with some success.
@manu0401
Copy link
Contributor Author

manu0401 commented Dec 8, 2018

@smortex: I rebased and made a new pullup, but I am not completely sure this is what you were looking for. If it is not, please be more precise (git commands!)

@smortex
Copy link
Contributor

smortex commented Dec 10, 2018

@manu0401 git is a pain :-)

Assuming your git repo has two remotes, origin and manu0401 and you are in the master branch of manu0401:

git fetch --all          # Gather all commits in all remotes locally
git rebase origin/master # Move all commits on top of the the master branch of origin
# You may have to fix conflicts here
vim libfreefare/mifare_desfire.c
git add libfreefare/mifare_desfire.c
git rebase --continue
# Repeat to fix each conflict

You will end with two commits "above" the master branch of this repository. Now combine them into a single commit:

git rebase -i origin/master

This will open your editor listing all commits between origin/master and manu0401/master. On the second line, replace "pick" with "fixup", save and exit.

Now that the commit have been rewritten, push your changes (-f is required since you changed the history):

git push -f manu0401 master

I let you try this as an exercise 😃 If you don't make it, no worry, I'll merge outside of GitHub

@manu0402
Copy link

@smortex: it is not a pain, it is a nasty mess.

I could not get it done, the thing would always complain about conflicts. I restarted it from scratch and made pull request #98

@smortex
Copy link
Contributor

smortex commented Dec 11, 2018

Close in favor of #98.

@smortex smortex closed this Dec 11, 2018
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

Successfully merging this pull request may close these issues.

None yet

4 participants