1054934 - [NFC] Move emulation into separate library

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: NFC, defect)

Description

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

We need to support NFC emulation on multiple emulators, JB, KK, and others. If we move the NFC code into a separate library, we can save a lot of porting effort later on.

Comment 1

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

Attachment: libnfcemu

Dimi,

I moved all NFC commits out of the emulator and applied them to a clean repository. Then I changed the license to Apache 2, added a public interface and a lot of cleanup patches to make the code build again. The result is a shared library that is expected to behave like the built-in emulation. All patches keep their original author.

There's still a lot to cleanup, but it's a good step towards a more flexible module that we can use from within different emulators.

Could you review the code and run the test cases? Thanks.


Sid,

You made a commit to the emulator's NFC code. This library should replace the original code. As I changed the license to Apache 2, I'd like you to review this change and give your r+ if you're OK with it. Thanks.

Comment 2

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

Attachment: Github pull request for emulator-jb

Comment 3

[psiddh]

Hi Thomas,

I assume that different emulator implementations would include 'nfcemu.h' in their respective implementation and call into the exported functions now.

Thanks

Comment 4

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

Thanks Sid,

Yes, there's a small API in the files under include/nfcemu/, which is used by the emulators. The rest of the code is internal to the library.

Comment 5

[Dimi Lee [:dimi]]

Comment on attachment 8474597 [details]
libnfcemu

Hi Thomas,
review- because one test case fail because miss following code in nfcemu_init.c

assert(recv_dta);
cb.recv_dta = recv_dta;

And
in nfc-debug.h
#  define  NFC_D(...)   fprintf(stderr, __VA_ARGS__)
There is no line feed for debug message.

Comment 6

[Dimi Lee [:dimi]]

Comment on attachment 8474599 [details]
Github pull request for emulator-jb

Looks good!
See github comments for nits

Comment 7

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

Comment on attachment 8474599 [details]
Github pull request for emulator-jb

Updated Github pull request

  - removed obsolete ndef.{c,h}, nfc-tag.{c,h}

Comment 8

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

Comment on attachment 8474597 [details]
libnfcemu

Updated Github tree:

  - fixed initialization if recv_dta callback
  - print '\n' after debug messages

Comment 9

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

Attachment: Github pull request for manifest

Comment 10

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> Created attachment 8475935 [details]
> Github pull request for manifest

ping

Comment 11

[Michael Wu [:mwu]]

Nothing for the KK emulator yet?

As usual, you'll need to request a mirror for this new repo. If you already filed a bug for that, make it block this.

Comment 12

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

(In reply to Michael Wu [:mwu] from comment #11)
> Nothing for the KK emulator yet?

There's an initial port to KK in bug 964697, but nothing official; so I didn't bother.

> As usual, you'll need to request a mirror for this new repo. If you already
> filed a bug for that, make it block this.

I moved this to bug 1059118.

Comment 13

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

Comment on attachment 8474599 [details]
Github pull request for emulator-jb

Vicamo,

Do you want to have a look at the build-system changes?

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #0)
> We need to support NFC emulation on multiple emulators, JB, KK, and others.
> If we move the NFC code into a separate library, we can save a lot of
> porting effort later on.

Actually I don't quite understand why will this save porting effort in any way. Could you elaborate on this a little bit? Since there is still no NFC in emulator-kk, I don't know why we have porting problems between different emulator's'.

Besides, I just had a merge from master to b2g-jellybean and to b2g-kitkat again yesterday. A trivial merge. If you have concerns about such weekly, even monthly I admit, pulls, maybe we should have those pull requests merged to master/jellybean/kitkat at the same time.

Anyway, if you really want to create an standalone library for NFC, the that has to be a static library. This is the only requirement so far.

Comment 15

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

Hi Vicamo,

Porting is the most obvious example, but there are other reasons as well.

> Actually I don't quite understand why will this save porting effort in any
> way. Could you elaborate on this a little bit? Since there is still no NFC
> in emulator-kk, I don't know why we have porting problems between different
> emulator's'.

This works for the simple case of running tests locally that we currently have.

If we want to use more emulators and in automated testing, we'll end up writing multiple patches for different versions and porting bug fixes between them. Code bases diverge over time, so it's not always a simple copy-paste job.

I currently work on a larger refactoring of Bluetooth, where we have this situation with two branches, and I effectively have to write all patches twice. There's the official Bluetooth code under dom/bluetooth/ and the new one under dom/bluetooth2/. Changes need go into both. This is a massive waste of time.

Second point is licensing. The code has been relicensed to Apache 2, while the emulator uses GPL. Using a library makes it obvious which files are covered by which license.

Finally, if we have a separate library, interested outside parties might pick up the code, or even make a contribution. This certainly won't happen if the code is 'locked up' in the emulator repository.

> Anyway, if you really want to create an standalone library for NFC, the that
> has to be a static library. This is the only requirement so far.

I'll see what I can do.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15)
> Hi Vicamo,
> 
> Porting is the most obvious example, but there are other reasons as well.
> 
> > Actually I don't quite understand why will this save porting effort in any
> > way. Could you elaborate on this a little bit? Since there is still no NFC
> > in emulator-kk, I don't know why we have porting problems between different
> > emulator's'.
> 
> This works for the simple case of running tests locally that we currently
> have.
> 
> If we want to use more emulators and in automated testing, we'll end up
> writing multiple patches for different versions and porting bug fixes
> between them. Code bases diverge over time, so it's not always a simple
> copy-paste job.

Currently we have following branch scheme for external/qemu and hardware/ril:

  o-------o b2g/b2g-kitkat (merge b2g/b2g-jellybean)
         /
  o-----o b2g/b2g-jellybean (merge b2g/master)
       /
  o---o b2g/master

So every pull request landed in b2g/master branch (for NFC, b2g-jellybean) will be "merged" into b2g-jellybean and b2g-kitkat. One may go check current b2g-kitkat HEAD [1] and find out __every__ NFC pull request has been included. No one, but I myself, has ever prepared a second pull request or worried about build break, feature code not available in another branch or so.

It's true that with a separated repository, you may only have to take care one branch. But that implies you have taken care of version differences inline. Maybe by __ANDROID_API__ or something else. You don't do this now because there are barely changes between JB and KK in regard to NFC emulation. As I said, usually I got a trivial merge. No conflict at all.

So back to the original problem, you don't even have to copy-paste in external/qemu.

It's also true there is a HUGE re-org in AOSP idea133 branch, and that will be in our way when we start to support Android L Gonk.

> I currently work on a larger refactoring of Bluetooth, where we have this
> situation with two branches, and I effectively have to write all patches
> twice. There's the official Bluetooth code under dom/bluetooth/ and the new
> one under dom/bluetooth2/. Changes need go into both. This is a massive
> waste of time.

True, but there is no two external/qemu folders?

> Second point is licensing. The code has been relicensed to Apache 2, while
> the emulator uses GPL. Using a library makes it obvious which files are
> covered by which license.

I personally don't feel anything wrong with GPL here, but you have your own preference. Relicense is a strong reason for me.

> Finally, if we have a separate library, interested outside parties might
> pick up the code, or even make a contribution. This certainly won't happen
> if the code is 'locked up' in the emulator repository.

How about locked up in the 'nfcemu' repository? Now both qemu and nfcemu are repositories under https://github.com/mozilla-b2g , why do you say "locked up in qemu repository" but not "locked up in nfcemu repository"? That statement does not provide any reason at all.

> > Anyway, if you really want to create an standalone library for NFC, the that
> > has to be a static library. This is the only requirement so far.
>
> I'll see what I can do.

Thank you. This is for packaging emulator binaries. See https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L325

[1]: https://github.com/mozilla-b2g/platform_external_qemu/tree/b2g-kitkat

Comment 17

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

Hi Vicamo,

Thanks for the long writeup.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)

>   o-------o b2g/b2g-kitkat (merge b2g/b2g-jellybean)
>          /
>   o-----o b2g/b2g-jellybean (merge b2g/master)
>        /
>   o---o b2g/master

Just out of interest; did you consider to use only one single emulator branch for all of our versions? For example, running the ICS B2G on a KK emulator binary. If yes or no, why or why not?


> It's true that with a separated repository, you may only have to take care
> one branch. But that implies you have taken care of version differences
> inline. Maybe by __ANDROID_API__ or something else.

I don't really worry about Android versions, because NFC emulation is plain C code with no dependencies besides libc.


> > I currently work on a larger refactoring of Bluetooth, where we have this
> > situation with two branches, and I effectively have to write all patches
> > twice. There's the official Bluetooth code under dom/bluetooth/ and the new
> > one under dom/bluetooth2/. Changes need go into both. This is a massive
> > waste of time.
> 
> True, but there is no two external/qemu folders?

The problem is the same: two different versions with subtle differences; thus not allowing simple merging of patches. I know that it's currently not the case, but I wouldn't expect it to stay this way.


> > Second point is licensing. The code has been relicensed to Apache 2, while
> > the emulator uses GPL. Using a library makes it obvious which files are
> > covered by which license.
> 
> I personally don't feel anything wrong with GPL here, but you have your own
> preference. Relicense is a strong reason for me.

I like the GPL, but I asked licensing guys and Mozilla's preferred license for system code is MPL or Apache 2. And that might also be a factor for potential contributors outside of Mozilla.


> > Finally, if we have a separate library, interested outside parties might
> > pick up the code, or even make a contribution. This certainly won't happen
> > if the code is 'locked up' in the emulator repository.
> 
> How about locked up in the 'nfcemu' repository? Now both qemu and nfcemu are
> repositories under https://github.com/mozilla-b2g , why do you say "locked
> up in qemu repository" but not "locked up in nfcemu repository"? That
> statement does not provide any reason at all.

Then let me clarify this: The library doesn't depend on the emulator in any way. No code, no #include_s, nothing. Any program could incorporate it and use it. This isn't the case if the code is located in the emulator's repository. My hope is that the NFC library could be useful to others as well. And that's also one of the reasons for the license change.


> > > Anyway, if you really want to create an standalone library for NFC, the that
> > > has to be a static library. This is the only requirement so far.
> >
> > I'll see what I can do.
> 
> Thank you. This is for packaging emulator binaries. See
> https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L325
> 
> [1]: https://github.com/mozilla-b2g/platform_external_qemu/tree/b2g-kitkat

Comment 18

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

Comment on attachment 8474599 [details]
Github pull request for emulator-jb

Updated Github pull request:

  - link libnfcemu statically
  - removed CONFIG_NFCEMU
  - cleaned up FLAGS variables

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8474599 [details]
Github pull request for emulator-jb

Just a nit: I find there are two |#if 0 ...| statements. Why do you need them? If they're already dead lines, please remove them. Or, some comments will be appreciated.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17)
> Hi Vicamo,
> 
> Thanks for the long writeup.
> 
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> 
> >   o-------o b2g/b2g-kitkat (merge b2g/b2g-jellybean)
> >          /
> >   o-----o b2g/b2g-jellybean (merge b2g/master)
> >        /
> >   o---o b2g/master
> 
> Just out of interest; did you consider to use only one single emulator
> branch for all of our versions? For example, running the ICS B2G on a KK
> emulator binary. If yes or no, why or why not?

Emulator builds on darwin depend on extra variables in build/core/combo/Host_darwin-x86.mk, and build/ changes from release to release. You may simply checkout b2g/b2g-kitkat branch within a B2G ICS emulator repo and have a try. The emulator will not be built at all. Some AOSP build system changes may also affect qemu, e.g. the removal of LOCAL_MODULE_TAGS. In addition, AVD dynamic skin and OpenGL tie qemu and sdk/ together. They have to match in both sides.

However, from AOSP SDK releases, we know there is only one copy of emulator binaries, and AOSP SDK is capable of running all revisions of platform images. So, this is probably something we should have a try.

> > It's true that with a separated repository, you may only have to take care
> > one branch. But that implies you have taken care of version differences
> > inline. Maybe by __ANDROID_API__ or something else.
> 
> I don't really worry about Android versions, because NFC emulation is plain
> C code with no dependencies besides libc.
> 
> 
> > > I currently work on a larger refactoring of Bluetooth, where we have this
> > > situation with two branches, and I effectively have to write all patches
> > > twice. There's the official Bluetooth code under dom/bluetooth/ and the new
> > > one under dom/bluetooth2/. Changes need go into both. This is a massive
> > > waste of time.
> > 
> > True, but there is no two external/qemu folders?
> 
> The problem is the same: two different versions with subtle differences;
> thus not allowing simple merging of patches. I know that it's currently not
> the case, but I wouldn't expect it to stay this way.

If NFC emulation only depends on libc, why wouldn't you expect it to stay this way? On the other hand, if NFC emulation is meant to vary between two releases, can it stay with one single branch after being moved into a standalone repository? I don't understand.

> > > Second point is licensing. The code has been relicensed to Apache 2, while
> > > the emulator uses GPL. Using a library makes it obvious which files are
> > > covered by which license.
> > 
> > I personally don't feel anything wrong with GPL here, but you have your own
> > preference. Relicense is a strong reason for me.
> 
> I like the GPL, but I asked licensing guys and Mozilla's preferred license
> for system code is MPL or Apache 2. And that might also be a factor for
> potential contributors outside of Mozilla.

GPL asks people to contribute back. People prefer Apache 2 because they don't have to contribute back. Then we move to Apache 2 to expect people to contribute back? Well, maybe.

> > > Finally, if we have a separate library, interested outside parties might
> > > pick up the code, or even make a contribution. This certainly won't happen
> > > if the code is 'locked up' in the emulator repository.
> > 
> > How about locked up in the 'nfcemu' repository? Now both qemu and nfcemu are
> > repositories under https://github.com/mozilla-b2g , why do you say "locked
> > up in qemu repository" but not "locked up in nfcemu repository"? That
> > statement does not provide any reason at all.
> 
> Then let me clarify this: The library doesn't depend on the emulator in any
> way. No code, no #include_s, nothing. Any program could incorporate it and
> use it. This isn't the case if the code is located in the emulator's
> repository. My hope is that the NFC library could be useful to others as
> well. And that's also one of the reasons for the license change.

License & reuse are good reasons. Anyway, these things are irrelevant. QEmu links various external libraries, and NFC can certainly be one of them as long as you think it appropriate.

> > > > Anyway, if you really want to create an standalone library for NFC, the that
> > > > has to be a static library. This is the only requirement so far.
> > >
> > > I'll see what I can do.
> > 
> > Thank you. This is for packaging emulator binaries. See
> > https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L325
> > 
> > [1]: https://github.com/mozilla-b2g/platform_external_qemu/tree/b2g-kitkat

Comment 21

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

Comment on attachment 8474599 [details]
Github pull request for emulator-jb

Updated pull request

  - removed out-commented code

Comment 22

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

Attachment: Github pull request for libnfcemu

This pull request adds static-linking support to libnfcemu. It's the final piece for landing the code.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

b2g-jellybean: https://github.com/mozilla-b2g/platform_external_qemu/commit/73bee3507d4593a96162ebdac8054a9b3ffe4fa8

master: https://github.com/mozilla-b2g/libnfcemu/commit/9bc8d2f3054f75c618d820cc24659edcf218b366

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://tbpl.mozilla.org/php/getParsedLog.php?id=47699885&tree=B2g-Inbound

b2g-jellybean: https://github.com/mozilla-b2g/platform_external_qemu/commit/d259117b4976decbe2f76eeed85218bf0109190f

master: https://github.com/mozilla-b2g/libnfcemu/commit/c7ccf6eff27f99e39a9eca94cde48aaece5e47db

Comment 25

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

emulator-jb requires static linking against libnfcemu. The pull request for  libnfcemu adds this. I can only reproduce the issue if the pull request for emulator-jb has landed, but the one for libnfcemu has not.

Ryan, is there a way to see the exact commits that were used for building? Could it happen that the emulator patch was applied, but the libnfcemu patch was not? Would it help to first land the library patch?

Comment 26

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

Oh, found something. libnfcemu in

  https://github.com/mozilla-b2g/libnfcemu/commit/682e556367e0049bb3ae127cec6e6c459abca1b0

still points to the revision without the pull request.

Comment 27

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

The patch for libnfcemu lands in bug 1065255.

Comment 28

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

Comment on attachment 8485661 [details]
Github pull request for libnfcemu

Landed in bug 1065255.

Comment 29

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

Attachment: Github pull request for emulator-jb (v2)

Changes since v1:

  - created new pull request

Comment 30

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

I'd like to land the patches in attachment 8508636 [details] again. Since the last attempt, the requirements for this issue have landed in bug 1065255 and bug 1065400.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

b2g-jellybean: https://github.com/mozilla-b2g/platform_external_qemu/commit/2786ef57fec62bd125083fc974c141c4b23728e9

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Still broken :(
b2g-jellybean: https://github.com/mozilla-b2g/platform_external_qemu/commit/c5f8d282efe4a4e8b1e31a37300944e338e60e4f

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=673062&repo=b2g-inbound

08:45:32 INFO - out/host/linux-x86/obj/EXECUTABLES/qemu-android-x86_intermediates/android/console.o: In function `do_nfc_tag':
08:45:32 INFO - /builds/slave/b2g_b2g-in_emu-jb_dep-00000000/build/external/qemu/android/console.c:3496: undefined reference to `nfc_cmd_tag'
08:45:32 INFO - out/host/linux-x86/obj/EXECUTABLES/qemu-android-x86_intermediates/android/console.o: In function `do_nfc_llcp':
08:45:32 INFO - /builds/slave/b2g_b2g-in_emu-jb_dep-00000000/build/external/qemu/android/console.c:3480: undefined reference to `nfc_cmd_llcp'
08:45:32 INFO - out/host/linux-x86/obj/EXECUTABLES/qemu-android-x86_intermediates/android/console.o: In function `do_nfc_snep':
08:45:32 INFO - /builds/slave/b2g_b2g-in_emu-jb_dep-00000000/build/external/qemu/android/console.c:3448: undefined reference to `nfc_cmd_snep'
08:45:32 INFO - out/host/linux-x86/obj/EXECUTABLES/qemu-android-x86_intermediates/android/console.o: In function `do_nfc_nci':
08:45:32 INFO - /builds/slave/b2g_b2g-in_emu-jb_dep-00000000/build/external/qemu/android/console.c:3464: undefined reference to `nfc_cmd_nci'

Comment 33

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

Sorry :(