-
Notifications
You must be signed in to change notification settings - Fork 106
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
Replaced DESFIRE_TRANSCEIVE by a function (TODO n°3) #105
Conversation
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.
Thank you for taking care of this! I added some inline comments. Can you have a look?
- Added missing "free(read_buffer);" - Uppercase macro names
You have not authorized maintainers to push changes to your branch. Can you configure with Thanks! I also feel that when compiling |
I added you as collaborator - hope this solves the problem :)
Done, fixed similar issues with other parts of the lib as well.
"./configure --enable-debug" works just fine for me, no problems whatsoever |
Travis CI fails for missing nfc-utils header file. I did not touch that. Any idea what possibly can cause that problem? |
Is there anything left for me to do in order to get this pull request passing all checks and get eventually merged? Sorry for asking possibly dumb questions, the git process is still new to me. Thanks! |
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 re-triggerd the build for Travis-CI and it have passed.
The code looks right, but I do not own anymore NFC readers, so can't run the regression test suite with real hardware to be 100% confident we do not introduce regressions.
@nfc-tools/core, @nfc-tools/contributors can somebody give it a try?
test$ ./run-test.sh don't have time to dig into this but if someone has a quick fix let me know and i'll try again... |
@AdamLaurie if I recall correctly, this is the typical failure when cutter is not installed when the autotools where run. Double-check that cutter is installed, and re-run Thanks! |
I'll give testing this a shot tonight. |
I've confirmed this change passes all tests with a MIFARE DESFire EV1. I've also noticed that some tests fail with a MIFARE DESFire EV2 (filed #120), but I don't think that is a regression or related to this issue. I know @smortex previously approved this change, but it looks like my conflict fix removed his approval. |
Ah, indentation looks weird (maybe we should add a test for this in Travis-CI?). I think you should be able to approve other people Pull Requests and merge them, but maybe if you contributed some code you cannot anymore? Would you mind checking |
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.
So, we still rely on a macro but it's a HUGE step forward IMHO! Thanks!
Next try without mixing up my branches.
Replaced DESFIRE_TRANSCEIVE by a function, updating all commands it used.