860907 - NFC Daemon for B2G

Status: RESOLVED DUPLICATE of bug 906579

(Firefox OS Graveyard :: NFC, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Review bug for NFC daemon that will handle B2G communications with libnxp

Comment 1

[Garner Lee]

Attachment: NFC Daemon for B2G - v01 - nfcd_android_01.patch

This patch is android standard code for nfcd.

Comment 2

[Garner Lee]

Attachment: NFC Daemon for B2G - v01 - nfcd_01.patch

Feature code for NFCD.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

So I'm not sure what these patches are? What does standard code versus feature code mean?

Comment 4

[Garner Lee]

The 2 files are the original NFCD patch, split into two patches.

The entirety of the android directory is in the "NFC Daemon for B2G - v01 - nfcd_android_01.patch" since it was mentioned earlier that the original android NFC code should be reviewed separately from the rest of the NFCD code feature. 

The "NFC Daemon for B2G - v01 - nfcd_01.patch", is the rest of the patch for hte NFCD code feature.

The only difference is, I used "git show", so they both ended up with the commit message header of the same bug. One caveat: I updated the commit message in between uploads, so the commit message is different. If that messes things up, I can re-upload.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I still don't understand what this is.

How much of the nfc code from android did you actually have to /edit/ or /create/? That's what I'm interested in. Was it really 300kb worth of patch that you guys wrote?

If there's a ton of android code in here that didn't get edited, what repositories did it come from? Is there any way we can submodule those repositories instead of maintaining the files in our own repositories?

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Also, if you could throw the URL for the repo you're maintaining on github in this comment thread, that'd be great. While we're looking at this on bugzilla, all moves will still be on github after review.

Comment 7

[Garner Lee]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #5)
> I still don't understand what this is.
> 
> How much of the nfc code from android did you actually have to /edit/ or
> /create/? That's what I'm interested in. Was it really 300kb worth of patch
> that you guys wrote?
> 
> If there's a ton of android code in here that didn't get edited, what
> repositories did it come from? Is there any way we can submodule those
> repositories instead of maintaining the files in our own repositories?

The android directory is un-modified android code, but based on a subset of android code of the original android 4.1.3 r1(?) code. You make a good point. I'll talk to Arno about the exact origin. We can certainly investigate using a Makefile to gather all the files from elsewhere (possibly in the usual FFOS source build tree).

We are maintaining the main FFOS/B2G fork at github.com/svic, with nfcd here:
https://github.com/svic/b2g-nfcd

I will ask if there was a remaining reason we made it private, as all of it is now, and has been, designated fully open source. I will update the comments.

Comment 8

[Garner Lee]

nfcd repo now public, regardless of history, as the patch is already public for some time.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

First things first: All of the files you edited or included are gonna need license headers. Have you guys had any discussions with legal on your side about licensing needs for contributions? I'm guessing the Java->C files will have to follow apache, not sure what your plans are for nfcd?

Comment 10

[Garner Lee]

We will update on the license issue as soon as possible.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 736597 [details] [diff] [review]
NFC Daemon for B2G - v01 - nfcd_01.patch

Review of attachment 736597 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, beginning on trying to review this. But it's still a gigantic patch that's hard to discern what's what in, and I do need to be able to understand it as a whole to be able to r+ it. Here is the patch set I'd like to see:

- One patch of Android.mk and documentation. Probably won't change much
- Probably one patch per directory, since that seems the easiest way to divide this up
-- Within each directory tell me which files you changed, and which files you brought over wholesale from android. I'd generally like to keep the straight up copies to a minimum.
-- More comments in the files you wrote would be good, there's a lot of things happening in the JNI emulation that are kind of opaque for devs not familiar with JNI.

I've added a few comments below too. Also, licensing would be good to get updated on just so it's out of the way.

::: README.md
@@ +2,5 @@
> +
> +Source struture:
> +
> +  android	JNI source files taken from Android 4.03 (https://android.googlesource.com/platform/packages/apps/Nfc.git and https://android.googlesource.com/platform/frameworks/base.git). The sources are patched to allow SE (Secure Element) handling from
> +  the SEEK for Android project (http://code.google.com/p/seek-for-android/)

If these source files are directly from an external repo, why not have them as external repos on our B2G manifest?

@@ +12,5 @@
> +
> +  src		Main implementation of nfcd. Socket code partially based on rilproxy. Nfc reading/writing based on Java code from Android (https://android.googlesource.com/platform/frameworks/base.git)
> +
> +  external/gc
> +		Boehm GC used for leak detection (https://github.com/ivmai/bdwgc)

This was removed

@@ +15,5 @@
> +  external/gc
> +		Boehm GC used for leak detection (https://github.com/ivmai/bdwgc)
> +
> +  external/jansson-2.3.1
> +		JSON parser and builder, with MIT license (http://www.digip.org/jansson)

This was removed

@@ +18,5 @@
> +  external/jansson-2.3.1
> +		JSON parser and builder, with MIT license (http://www.digip.org/jansson)
> +
> +  external/libatomic	
> +		Library for atomic memory operations used by Boehm GC (https://github.com/ivmai/libatomic_ops)

This was removed

::: jni/gen-jni-default.py
@@ +1,1 @@
> +#!/usr/bin/env python

Is this run by hand? Should it be added to the android.mk?

::: jni/jni-orig.h
@@ +1,1 @@
> +/*

So you have a jni.h already. Why is this here?

::: jni/jni_default.cpp
@@ +1,2 @@
> +
> +// File generated from jni.h

This should probably be generated on build?

::: jni/refcount_tmpl.cpp
@@ +1,2 @@
> +
> +#include <iostream>

I don't see this file being included in the Android.mk?

@@ +10,5 @@
> +class RefCountBase {
> +private:
> +    int retainCount;
> +
> +public:    

Nit: All sorts of whitespace cleanup here and below

Comment 12

[Garner Lee]

Will update. We're looking at removing the android directory, but this also some SE support.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Garner Lee from comment #7)

> We are maintaining the main FFOS/B2G fork at github.com/svic, with nfcd here:
> https://github.com/svic/b2g-nfcd

Hi,

I cloned this repo along with the dependencies it lists in its README. But the code doesn't compile because it cannot find the JNI headers (but which are part of the repo itself, right?). Do I need anything else here?

Comment 14

[psiddh]

Attachment: NFC Daemon for B2G - v02 - nfcd_02.patch

Update the nfcd daemon reference implementation as per https://bugzilla.mozilla.org/show_bug.cgi?id=897312

Note that this implementation is based of JSON format. Should the format be changed to binary, changes would be made appropriately.

Comment 15

[psiddh]

Attachment: NFC Daemon for B2G - v02 - nfcd_android_02.patch

Android source files of nfcd reference implementation. (Modified partially to support the protocol)

Comment 16

[Daniel Veditz [:dveditz]]

Not sure if we need a separate security review for this; flagging for Paul to at least glance at.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

removing depedency per https://bugzilla.mozilla.org/show_bug.cgi?id=906579#c2

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Development moved to bug 906579 (please just mark as dupe and close out next time :) ).