nfc-tools / libfreefare

A convenience API for NFC cards manipulations on top of libnfc.
Other
402 stars 105 forks source link

Added support for NTAG 21x tags #53

Closed SloCompTech closed 7 years ago

SloCompTech commented 7 years ago

Hello, I added support for NTAG21x tags according to NTAG213/215/216 documentation Rev. 3.2 June 2015 It includes:

Before it can be merged some things need to be checked:

  1. I added myself in copyright in files I writen or I updated, so authors need to give permission that they agree with changes
  2. Please check if I correctly updated compiler settings for ntag21x.c and examples
smortex commented 7 years ago

Oh, and no need to worry about the copyright, #49 is about removing them all since they are never updated.

doegox commented 7 years ago

@smortex we've a "style" phony target in libnfc, maybe you could setup sth similar for libfreefare? https://github.com/nfc-tools/libnfc/blob/master/Makefile.am

SloCompTech commented 7 years ago

Fixed some identation, is there easy way to check if all is correct ?

smortex commented 7 years ago

Hello @SloCompTech,

is there easy way to check if all is correct ?

In order to check style "the easy way", I simply look at the Files changed tab of the pull request (on top of this page) which shows the wholes changes of all commits combined.

Since these changes are related to adding a new feature, the page should only contain bunches of new lines of code (new files, etc), and a few logical changes.

Therefore, any space change is suspicious and even if the spacing is wrong according to the style, it's better to keep it as it is in order to make the review easier (e.g. most changes in libfreefare/freefare.c should not be part of this pull request in order to make it easy to spot the actual changes (L74-75)).

On this tab, the in-line review comments are automatically hidden when they are attached to code that has been changed, so the comments I made some time ago and are still relevant are shown there.

Thank you for taking care of this, much appreciated :+1:

SloCompTech commented 7 years ago

Hi, I think it's better that I take original repository and copy and paste changes into original version and then commit so other stuff will be unaffected.

smortex commented 7 years ago

@SloCompTech yes, you can create a new branch in order to have a reference to the current state of the repository, revert your master branch to origin/master and then update your master branch to only include the wanted changes.

Then, when you push your changes (-f will be required in order to force the commit since some pushed commits will have been changed) the pull request will be updated automatically (when you create a pull-request, it is linked to the branch you created, adding / modifying / removing commits in the branch updates the pull-request).

SloCompTech commented 7 years ago

Hi, I used clean current repo and insert changes only and used force push, so I think it's all done.

smortex commented 7 years ago

Hi

I cloned the repository, rebased on top of master, and added a commit to fix some spacing problems, unfortunately I can't push it to your repository :open_mouth:.

Either you did not "Allow edits from maintainers" (checkbox at the bottom of the right column of this page), otherwise GitHub do not want me to rebase your changes and I should revert that. Can you please check that maintainers edits are allowed and tell me?

Thansk!

SloCompTech commented 7 years ago

hi @smortex, I have edit by maintainers enabled, but just in case I added you as colaborator, so please try to push to repo again

smortex commented 7 years ago

Nah, with your authorization I realized that I was pushing to a new non-existent branch in your repo and not in master… I just cherry-picked my commit and cleaned-up my mess!

smortex commented 7 years ago

Changes squashed and merged. Thanks!