Added support for NTAG 21x tags #53

SloCompTech · 2017-02-26T17:28:55Z

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

reading pages
writting to pages
authentication with tag
configuring counters

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

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

smortex

Hello, thank you for this PR.

I added a commit (019bfd1) to revert to UNIX line endings (LF, not Windows CR+LF) to make the diff more readable, please check your editor's configuration and the output of git diff to avoid this kind of problems.

There are currently some style inconsistencies in the libfreefare code, but it would be cool to avoid introducing more (that would be a story for another PR). For example, we have spaces between a function name and the opening parenthesis of it's parameters (but not in diagnostic messages) and also spaces after comas and semi column. Please refer the style section of the file HACKING.md at the repository root for details.

I added a few more points inline

smortex · 2017-03-24T12:57:52Z

libfreefare/Makefile.am

@@ -14,7 +14,8 @@ libfreefare_la_SOURCES = felica.c \
 			 mifare_desfire_key.c \
 			 mad.c \
 			 mifare_application.c \
-			 tlv.c
+			 tlv.c \
+			 ntag21x.c


Please keep the file list in alphabetical order.

smortex · 2017-03-24T12:58:38Z

examples/CMakeLists.txt

+        mifare-detect-tag
+        ntag-write
+        ntag-setauth
+        ntag-removeauth


Please keep the list in alphabetical order.

smortex · 2017-03-24T12:58:57Z

examples/Makefile.am

+				 mifare-detect-tag \
+				 ntag-write \
+				 ntag-setauth \
+				 ntag-removeauth


Alphabetical order + indentation

smortex · 2017-03-24T12:59:45Z

examples/Makefile.am

+
+ntag_removeauth_SOURCES = ntag-removeauth.c
+ntag_removeauth_LDADD = $(top_builddir)/libfreefare/libfreefare.la
+


Alphabetical order of targets

smortex · 2017-03-24T13:00:55Z

libfreefare/freefare.c

    } else if (mifare_ultralight_taste (device, target)) {
-	tag = mifare_ultralight_tag_new (device, target);
+	     tag = mifare_ultralight_tag_new (device, target);


Indentation changed unexpectedly

smortex · 2017-03-24T13:01:13Z

libfreefare/freefare.c

@@ -169,6 +171,8 @@ freefare_get_tag_friendly_name (FreefareTag tag)
 	return "Mifare UltraLightC";
    case MIFARE_ULTRALIGHT:
 	return "Mifare UltraLight";
+    case NTAG_21x:
+  return "NTAG21x";


Indentation look inconsistent

smortex · 2017-03-24T13:04:19Z

libfreefare/freefare.h

+int ntag21x_authenticate (FreefareTag tag, const NTAG21xKey key); // Authenticate with tag
+bool is_ntag21x(FreefareTag tag); // Check if tag type is NTAG21x
+bool ntag21x_is_auth_supported (nfc_device *device, nfc_iso14443a_info nai); // Check if tag supports 21x commands
+


See Function prototypes in the style guide for the magic of indentation here :-)

smortex · 2017-03-24T13:05:08Z

libfreefare/freefare_internal.h

@@ -260,7 +282,7 @@ struct mifare_ultralight_tag {
 #define MIFARE_CLASSIC(tag) ((struct mifare_classic_tag *) tag)
 #define MIFARE_DESFIRE(tag) ((struct mifare_desfire_tag *) tag)
 #define MIFARE_ULTRALIGHT(tag) ((struct mifare_ultralight_tag *) tag)
-


Keep the blank line between this group of macros and the comment

smortex · 2017-03-24T13:07:45Z

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

doegox · 2017-03-24T14:33:12Z

@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 · 2017-06-20T11:03:02Z

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

smortex · 2017-06-26T07:49:14Z

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 👍

SloCompTech · 2017-06-26T11:21:15Z

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 · 2017-06-26T11:31:44Z

@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 · 2017-06-26T21:38:01Z

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

smortex · 2017-06-27T06:41:04Z

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 😮.

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!

Most problems where fixed, I have local modification that fix the few remaining ones.

SloCompTech · 2017-06-27T08:09:45Z

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 · 2017-06-27T08:45:11Z

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 · 2017-06-27T08:51:25Z

Changes squashed and merged. Thanks!

smortex previously requested changes Mar 24, 2017

View reviewed changes

smortex added bug enhancement and removed bug labels Apr 3, 2017

SloCompTech added 2 commits June 26, 2017 23:31

Added NTAG21x support

25f4752

Added ntag to supported list

553861a

smortex self-requested a review June 27, 2017 06:42

Merge branch 'master' into master

fbf53f0

Fix spacing problems

c8ab322

smortex approved these changes Jun 27, 2017

View reviewed changes

smortex merged commit b2eca83 into nfc-tools:master Jun 27, 2017

smortex mentioned this pull request Jun 27, 2017

Use astyle(1) to make style consistent #64

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for NTAG 21x tags #53

Added support for NTAG 21x tags #53

SloCompTech commented Feb 26, 2017

smortex left a comment

smortex Mar 24, 2017

smortex Mar 24, 2017

smortex Mar 24, 2017

smortex Mar 24, 2017

smortex Mar 24, 2017

smortex Mar 24, 2017

smortex Mar 24, 2017

smortex Mar 24, 2017

smortex commented Mar 24, 2017

doegox commented Mar 24, 2017

SloCompTech commented Jun 20, 2017

smortex commented Jun 26, 2017

SloCompTech commented Jun 26, 2017

smortex commented Jun 26, 2017

SloCompTech commented Jun 26, 2017

smortex commented Jun 27, 2017

SloCompTech commented Jun 27, 2017

smortex commented Jun 27, 2017

smortex commented Jun 27, 2017


		ntag_removeauth_SOURCES = ntag-removeauth.c
		ntag_removeauth_LDADD = $(top_builddir)/libfreefare/libfreefare.la

Added support for NTAG 21x tags #53

Added support for NTAG 21x tags #53

Conversation

SloCompTech commented Feb 26, 2017

smortex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smortex commented Mar 24, 2017

doegox commented Mar 24, 2017

SloCompTech commented Jun 20, 2017

smortex commented Jun 26, 2017

SloCompTech commented Jun 26, 2017

smortex commented Jun 26, 2017

SloCompTech commented Jun 26, 2017

smortex commented Jun 27, 2017

SloCompTech commented Jun 27, 2017

smortex commented Jun 27, 2017

smortex commented Jun 27, 2017