916863 - [NFC] NFC support in emulator

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Emulator, defect)

Description

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

Having NFC support in the emulator will certainly be helpful for automated testing.

Building full NFC support is probably too much work. The goal here is to build a virtual device that is compatible with the Broadcom user-space driver and can be used by nfcd without generating errors. From there, we can add features as needed.

Comment 1

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

Attachment: Qemu patches

Hi Vicamo,

Here is a snapshot of the NFC device for the emulator. It's far from being complete, but can start our NFC daemon. Any feedback is welcome. Please ignore all the debugging printfs. :)

The device is based on the NCI standard for NFCC access. If you need some information about NCI, just ping me. And you can get the standard at [1]. There are also some proprietary commands in the device that are used by Broadcom's user-space driver. Reverse-engineering how they work probably took most of the time.

I'll finish the device's state machines and add support for more NCI commands. I also want to add NFC support to the Goldfish console, so we can manipulate the device during testing.

Best regards and a happy moon festival :)
Thomas

[1] http://www.nfc-forum.org/specs/spec_list/#nci

Comment 2

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

Attachment: Kernel patches

Just the kernel patches. There is nothing special here. The driver forwards NCI messages between device and user-space driver.

Comment 3

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

https://android.googlesource.com/kernel/msm.git/+/android-msm-mako-3.4-jb-mr2/drivers/nfc/bcm2079x-i2c.c

Comment 4

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

Awesome job!  Emulator rocks!

Two things in concerned.  First, we're running out of IRQs on emulator-x86.  Anything new that takes a IRQ will become a trouble in bug 753928 (but it's not really a serious task anytime soon), and I guess this might be one of the reasons you don't have a line in `external/qemu/hw/pc.c`.  There are three possible solutions:

  1. Have some dirty hack to reuse IRQs originally assigned to other devices.
     Probably the most easy one.

  2. A merge request[1] was posted to Android review but doesn't seem to be
     merged any time soon.

  3. All goldfish_tty devices share the same IRQ since bug 870422.  If we find
     a way to reuse goldfish_tty driver, then the lack of IRQ problem exists
     no more.  So, we may have a NCI_UART driver instead, just like kernel has
     NCI_SPI now, and define a new tty line discipline type, say, N_NFC_NCI,
     just like bluetooth HCI_UART[2] has N_HCI.  This is an arch/platform
     neutral way but will probably take a lot of extra work in both kernel
     and user space.

Second, it's a pity that the official bcm2079x driver[3] depends on I2C.  Emulator has no I2C, or we can simply include that GPL-licensed driver in emulator kernel.  But then we have the IRQ problem again, so I'd better just forget it.

I'm not good enough to review kernel patches.  Still learning from you.  The architecture here looks good to me.  I guess no a single line of code has to be changed in user space.

BTW, you need a PR for https://github.com/mozilla-b2g/platform_prebuilts_qemu-kernel , too.

[1]: https://android-review.googlesource.com/#/c/31092/
[2]: http://lxr.free-electrons.com/source/drivers/bluetooth/hci_ldisc.c
[3]: https://android.googlesource.com/kernel/msm.git/+/android-msm-mako-3.4-jb-mr2/drivers/nfc/bcm2079x-i2c.c

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #1)
> Here is a snapshot of the NFC device for the emulator. It's far from being
> complete, but can start our NFC daemon. Any feedback is welcome. Please
> ignore all the debugging printfs. :)
> 
> The device is based on the NCI standard for NFCC access. If you need some
> information about NCI, just ping me. And you can get the standard at [1].
> There are also some proprietary commands in the device that are used by
> Broadcom's user-space driver. Reverse-engineering how they work probably
> took most of the time.

I have to say I do worry about such strong link to a certain real-world device.  I wonder why can't we do as GPS emulation does -- forget kernel driver and let nfcd communicate to emulator host via qemud/qemu-pipe.  Is there anything tie to kernel /proc, /sys, audio, network or the like that we'll certainly need to have fully functional, test-able NFC support in the emulator?  Even there is, we can probably work around them with the same technique, qemud/qemu-pipe.  But considering to remove Android specific stuff from QEmu as possible, the way you have now fits this target most.

> I'll finish the device's state machines and add support for more NCI
> commands. I also want to add NFC support to the Goldfish console, so we can
> manipulate the device during testing.
> 
> Best regards and a happy moon festival :)
> Thomas

Thank you :)

Comment 5

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

Hi Vicamo

>   1. Have some dirty hack to reuse IRQs originally assigned to other devices.
>      Probably the most easy one.

This is known as IRQ sharing. My kernel driver already supports this. It's just a matter of changing qemu's goldfish bus to assign the same IRQ line to multiple devices.

>   2. A merge request[1] was posted to Android review but doesn't seem to be
>      merged any time soon.

Can we just pick up that patch and merge it into our code base?

>   3. All goldfish_tty devices share the same IRQ since bug 870422.  If we
> find
>      a way to reuse goldfish_tty driver, then the lack of IRQ problem exists
>      no more.  So, we may have a NCI_UART driver instead, just like kernel
> has
>      NCI_SPI now, and define a new tty line discipline type, say, N_NFC_NCI,
>      just like bluetooth HCI_UART[2] has N_HCI.  This is an arch/platform
>      neutral way but will probably take a lot of extra work in both kernel
>      and user space.
> 
> Second, it's a pity that the official bcm2079x driver[3] depends on I2C. 
> Emulator has no I2C, or we can simply include that GPL-licensed driver in
> emulator kernel.  But then we have the IRQ problem again, so I'd better just
> forget it.

I didn't know that this code was available. I just had a brief look at it, but all this is already solved in my kernel driver. It's really just store-and-forward in either direction.

> [3]:
> https://android.googlesource.com/kernel/msm.git/+/android-msm-mako-3.4-jb-
> mr2/drivers/nfc/bcm2079x-i2c.c

> > The device is based on the NCI standard for NFCC access. If you need some
> > information about NCI, just ping me. And you can get the standard at [1].
> > There are also some proprietary commands in the device that are used by
> > Broadcom's user-space driver. Reverse-engineering how they work probably
> > took most of the time.
> 
> I have to say I do worry about such strong link to a certain real-world
> device.  I wonder why can't we do as GPS emulation does -- forget kernel
> driver and let nfcd communicate to emulator host via qemud/qemu-pipe.  Is
> there anything tie to kernel /proc, /sys, audio, network or the like that
> we'll certainly need to have fully functional, test-able NFC support in the
> emulator?  Even there is, we can probably work around them with the same
> technique, qemud/qemu-pipe.  But considering to remove Android specific
> stuff from QEmu as possible, the way you have now fits this target most.

The idea is to have full-stack testing. There is a generic user-space driver for NCI-compatible devices [1], provided by Broadcom. The driver contains some device-specific code known as 'hal'. Vendors only rewrite hal to get their device supported. If we'd go for a generic device, we'd have to write our own hal implementation, or at least fork the one provided by Broadcom. We can do that, but it's probably more work for now and a lot more likely to break. For testing the emulated device, I need a stable and working implementation at the other side, and the Broadcom driver is just that.

I tried to indicate clearly, which parts of the device's code are Broadcom-specific. If you have strong concerns, we could try to fork the Broadcom hal and convert it to 'pure NCI', once the code stabilizes and works.

Best regards
Thomas

[1] https://www.codeaurora.org/cgit/external/gigabyte/platform/external/libnfc-nci/

Comment 6

[arno]

The NCI specs define a transport mapping for UART (section 11.1) that the Broadcom driver also supports (set TRANSPORT_DRIVER to /dev/ttyS in /etc/libnfc-brcm.conf). Doing this would avoid using the Broadcom-specific /dev/bcm2079x and it would also not require any Goldfish extensions.

Arno

Comment 7

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

Hi

(In reply to arno from comment #6)
> The NCI specs define a transport mapping for UART (section 11.1) that the
> Broadcom driver also supports (set TRANSPORT_DRIVER to /dev/ttyS in
> /etc/libnfc-brcm.conf). Doing this would avoid using the Broadcom-specific
> /dev/bcm2079x and it would also not require any Goldfish extensions.
> 
> Arno

The first point will not work. Whatever is on the other end connection will be specific to Broadcom, because we use the Broadcom Hal. They use some extra information to distinguish NCI from other protocols, and we'll have to handle this.

The second point is more interesting. Moving the code out of the kernel and qemu, and into a separate user-space process (within the guest, right?) might give some benefits in terms of maintainability. I don't think we'd need a UART here. The Broadcom driver uses standard open/read/write/poll and that's all we'd probably need. A fifo would certainly do the job. If there are any ioctl's in the Broadcom driver, I haven't seen them yet. How would we communicate with this user-space program?*

Best regards
Thomas

* Mo-o-o-o-orse ;)

Comment 8

[arno]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> The first point will not work. Whatever is on the other end connection will
> be specific to Broadcom, because we use the Broadcom Hal. They use some
> extra information to distinguish NCI from other protocols, and we'll have to
> handle this.

Interesting. Would you mind to elaborate a little? Did Broadcom make proprietary extensions or did they not even comply with the standard NCI messages? But either way, you are right that the implementation would not be Broadcom agnostic.

> The second point is more interesting. Moving the code out of the kernel and
> qemu, and into a separate user-space process (within the guest, right?)
> might give some benefits in terms of maintainability. I don't think we'd
> need a UART here. The Broadcom driver uses standard open/read/write/poll and
> that's all we'd probably need. A fifo would certainly do the job. If there
> are any ioctl's in the Broadcom driver, I haven't seen them yet. How would
> we communicate with this user-space program?*

Actually, I thought you can bridge /dev/ttySx to the host via the qemu command line option -serial. No idea if this would work, but apart from making some changes to /etc/libnfc-brcm.conf you wouldn't need to do anything special in the guest.

Arno

Comment 9

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

(In reply to arno from comment #8)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> > The first point will not work. Whatever is on the other end connection will
> > be specific to Broadcom, because we use the Broadcom Hal. They use some
> > extra information to distinguish NCI from other protocols, and we'll have to
> > handle this.
> 
> Interesting. Would you mind to elaborate a little? Did Broadcom make
> proprietary extensions or did they not even comply with the standard NCI
> messages? But either way, you are right that the implementation would not be
> Broadcom agnostic.

It's all of that, actually. While Broadcom uses NCI, they also add some proprietary extensions in the ways allowed by the NCI spec. And they also use an HCI command during initialization. Outside users, such as nfcd, don't see this, because it all happens between Hal, kernel-driver and hardware. To distinguish between NCI and HCI, all packets between user-space and kernel drivers are prefixed by another byte that tells the packet's content. The kernel driver strips this byte when forwarding packets to the hardware, or adds the correct byte when reading from the hardware.

> > The second point is more interesting. Moving the code out of the kernel and
> > qemu, and into a separate user-space process (within the guest, right?)
> > might give some benefits in terms of maintainability. I don't think we'd
> > need a UART here. The Broadcom driver uses standard open/read/write/poll and
> > that's all we'd probably need. A fifo would certainly do the job. If there
> > are any ioctl's in the Broadcom driver, I haven't seen them yet. How would
> > we communicate with this user-space program?*
> 
> Actually, I thought you can bridge /dev/ttySx to the host via the qemu
> command line option -serial. No idea if this would work, but apart from
> making some changes to /etc/libnfc-brcm.conf you wouldn't need to do
> anything special in the guest.

Having everything in the guest seems beneficial to me, because we can build it as part of our regular build process. And it's self contained; no need for extra test setups. In this case, getting the tests into tpbl should be straight-forward.

Comment 10

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

Hi Thomas, do you have detailed steps to verify your work here?

Comment 11

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

Hi Vicamo,

Sorry for letting you wait for so long. I wanted to get the code into a stable state, and than I got distracted with other things. For testing the current emulation, try to

 - clone the repos,
 - build the kernel with GOLDFISH_NFC=y,
 - copy kernel's zImage to prebuilts/qemu-kernel/arm/kernel-qemu-armv7,
 - clone nfcd from https://github.com/dleeMozilla/nfcd.git to external/nfcd,
 - clone libnfc-nci from https://android.googlesource.com/platform/external/libnfc-nci to external/libnfc-nci, rev is android-4.3_r2.1,
 - apply libnfc-nci patch via git am,
 - apply build patch in build/,
 - run './build.sh',
 - run './run-emulator.sh',
 - in parallel run 'adb shell nfcd' to start the NFC daemon,
 - the emulator should print lots of debugging information on its console,
 - in parallel run 'telnet localhost 5554' to open the emulator console.

You should now be able to control the NFC device. The plan is to allow for the sending of NCI notifications to the user-space driver, and transmit and receive data. Currently there are two NCI notifications supported. The interesting one is 'rf_intf_activated', which means that a remote endpoint (i.e., some other device) is close. There are two built-in devices, 0 and 1. If you enter

 - 'nfc ntf rf_intf_activated 0' on the telnet session

the device should tell the user-space driver that remote endpoint 0 has shown up. Both, driver and remote endpoint, should now exchange llcp SYMM packets every 2 seconds. The emulator will show constant debugging noise on its console.

At this point we'll be able to transmit NCI data packets, once this feature has been added. A marionette test case could write a data packet to the emulator and check if it receives the packet from gecko.

Comment 12

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

Attachment: libnfc-nci patch

Comment 13

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

Attachment: build patch

Comment 14

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)

I've created repos you need:

  https://github.com/mozilla-b2g/nfcd
  https://github.com/mozilla-b2g/platform_external_libnfc-nci

Before you may want to have a PR for b2g-manifest, we should decide where should nfcd live.  Usually it will be in external/, which makes its name platform_external_nfcd for me.  But you & mwu might have your preference.

Comment 15

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

Comment on attachment 810568 [details] [diff] [review]
libnfc-nci patch

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

Is this patch a MUST?  If we can keep it the way it is, then we don't have to fork libnfc-nci right now.

Comment 16

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13)
> Created attachment 810570 [details] [diff] [review]
> build patch

You can have a pull request to https://github.com/mozilla-b2g/platform_build/tree/b2g-4.3_r2.1

Comment 17

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> Comment on attachment 810568 [details] [diff] [review]
> libnfc-nci patch
> 
> Review of attachment 810568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this patch a MUST?  If we can keep it the way it is, then we don't have
> to fork libnfc-nci right now.

This seems to be a bug in the Android.mk. $(TARGET_DEVICE) resolves to 'generic', but libhardware will search for libraries ending with 'goldfish'. If there is a way to work around this problem and not fork libnfc-nci, I'd prefer it.

Comment 18

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> I've created repos you need:
> 
>   https://github.com/mozilla-b2g/nfcd
>   https://github.com/mozilla-b2g/platform_external_libnfc-nci
> 
> Before you may want to have a PR for b2g-manifest, we should decide where
> should nfcd live.  Usually it will be in external/, which makes its name
> platform_external_nfcd for me.  But you & mwu might have your preference.

From https://github.com/mozilla-b2g/b2g-manifest/pull/115, I've renamed 'nfcd' to 'platform_system_nfcd'.

Comment 19

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

Attachment: Build-scripts patches

Comment 20

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> Comment on attachment 810568 [details] [diff] [review]
> libnfc-nci patch
> 
> Review of attachment 810568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this patch a MUST?  If we can keep it the way it is, then we don't have
> to fork libnfc-nci right now.

I've found a way to work around the problem by changing 'ro.product.board' to 'generic' in the build scripts. So we don't have to change libnfc-nci.

Comment 21

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

Attachment: Goldfish patches

Comment 22

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

A quick update on the status of this bug, because it's been a while. I just did some testing and the emulator seems to work at a basic level. I

 - applied the patches in bugs 674741, 933635, 933591, and 866907;
 - started the emulator;
 - opened the Settings app; and
 - turned on NFC.

As a result, I could see nfcd starting up the emulated NFC device and discovering devices. When I telnet'ed into the emulator and sent an RC_INTF_ACTIVATED notification, I could see it being received by Gaia's System app. There was an error message in the log, but that seems to be unrelated to the emulator.

>>> snip >>>

I/nfcd    (   46): external/nfcd/src/MessageHandler.cpp:29 void MessageHandler::notifyTechDiscovered(android::Parcel&, void*)
D/nfcd    (   49): void NfcIpcSocket::writeToOutgoingQueue(uint8_t*, size_t) enter, data=0x2a0480b8, dataLen=20
D/nfcd    (   49): Writing 20 bytes to gecko 
I/Gecko   (   45): Nfc Worker: NFC_NOTIFICATION_TECH_DISCOVERED
D/nfcd    (   49): void NfcService::handleLlcpLinkActivation(NfcEvent*): exit
D/BrcmNciX(   49): 0000020000
D/BrcmNciR(   49): 0000020000
I/nfcd    (   46): external/libnfc-nci/halimpl/bcm2079x/adaptation/userial_linux.c:1256 write 0x10 0x0 0x0 0x2
I/nfcd    (   46): external/libnfc-nci/halimpl/bcm2079x/adaptation/userial_linux.c:1260
I/nfcd    (   46): external/libnfc-nci/halimpl/bcm2079x/adaptation/userial_linux.c:635 read 0x10 0x0 0x0 0x2  0x0
D/BrcmNciX(   49): 0000020000
I/nfcd    (   46): external/libnfc-nci/halimpl/bcm2079x/adaptation/userial_linux.c:1256 write 0x10 0x0 0x0 0x2
I/nfcd    (   46): external/libnfc-nci/halimpl/bcm2079x/adaptation/userial_linux.c:1260
I/GeckoDump(   45): [DEBUG] SYSTEM NFC: Technology Discovered: {"type":"techDiscovered","tech":["P2P"],"ndef":[],"sessionToken":"{01b3003d-2955-45f6-becb-2c6db9d0480f}"}
I/GeckoDump(   45): [DEBUG] SYSTEM NFC: command.tech: P2P
I/GeckoDump(   45): [DEBUG] SYSTEM NFC: Going through NFC Technologies: 0
I/GeckoDump(   45): [DEBUG] SYSTEM NFC: handleNdefDiscovered: undefined
E/GeckoConsole(   45): [JavaScript Error: "ndefMsg is undefined" {file: "app://system.gaiamobile.org/js/nfc_manager.js" line: 329}]
E/GeckoConsole(   45): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "ndefMsg is undefined" {file: "app://system.gaiamobile.org/js/nfc_manager.js" line: 329}]' when calling method: [nsIDOMSystemMessageCallback::handleMessage]" {file: "jar:file:///system/b2g/omni.ja!/components/SystemMessageManager.js" line: 97}]

Comment 23

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

> RC_INTF_ACTIVATED

RF_INTF_ACTIVATED

Comment 24

[arno]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #22)
> I/GeckoDump(   45): [DEBUG] SYSTEM NFC: Technology Discovered:
> {"type":"techDiscovered","tech":["P2P"],"ndef":[],"sessionToken":"{01b3003d-

It seems that nfcd already sends a tech discovered with P2P as the technology list. When you send an RF activation notification, is this already an indication of a P2P peer to be in the vicinity?

Comment 25

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

(In reply to arno from comment #24)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #22)
> > I/GeckoDump(   45): [DEBUG] SYSTEM NFC: Technology Discovered:
> > {"type":"techDiscovered","tech":["P2P"],"ndef":[],"sessionToken":"{01b3003d-
> 
> It seems that nfcd already sends a tech discovered with P2P as the
> technology list. When you send an RF activation notification, is this
> already an indication of a P2P peer to be in the vicinity?

I think so. RF_INTF_ACTIVATED_NTF means that the hardware noticed another device.

In the error message, ndefMsg is undefined. But RF_INTF_ACTIVATED_NFC is not supposed to transmit NDEF data. Maybe one of the other components (nfcd, gecko) needs to clear this field. I don't know what happens on the Nexus 4, though.

BTW, what exactly is the criteria for P2P here? I have an NFC tag with hard-coded properties in the emulator. It uses the NFC-DEP protocol, which implies LLCP on top of it. I always assumed this means P2P. Is there a good description available somewhere?

Comment 26

[arno]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25)
> In the error message, ndefMsg is undefined. But RF_INTF_ACTIVATED_NFC is not
> supposed to transmit NDEF data. Maybe one of the other components (nfcd,
> gecko) needs to clear this field. I don't know what happens on the Nexus 4,
> though.

We changed the protocol to include an optional NDEF message in the tech discovered notification. That is why it is encoded as an array (0-length means there was no NDEF). For P2P this array will be empty since at this point no NDEF has been sent. So, this is OK.

Dimi may know the answer to your other question.

Comment 27

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

But the problem is that ndefMsg is undefined. It looks like the array was never created at all.

Comment 28

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

I just updated gecko and nfcd, but the problem persists. I'll probably have to go through the code.

Comment 29

[Garner Lee]

Just to check in case it matters: which NFC tag are you using to get the NDEF message (I handed several types out: a quick description might help)? Or if trying P2P, which device?

Comment 30

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

(In reply to Garner Lee from comment #29)
> Just to check in case it matters: which NFC tag are you using to get the
> NDEF message (I handed several types out: a quick description might help)?
> Or if trying P2P, which device?

Neither, because it's all emulated. :) Properties of remote endpoints are hard-coded, and one can select one of them at activation time (i.e., RF_INTF_ACTIVATED_NTF).

Comment 31

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

Comment on attachment 806078 [details]
Qemu patches

This branch is based on b2g-jellybean.  The problem is we don't have emulator-jb tests online yet, and even we do, there is still no way to distinguish jb-only/ics-only test cases.  It's actually an issue when we're going to land bug 935525, basic nfc marionette test cases.

Comment 32

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

Need yet another pull request to b2g-manifest for libnfc-nci and nfcd.

Comment 33

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

Attachment: Github pull request for b2g-manifest

Comment 34

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

Attachment: Github pull request for prebuilts/qemu-kernel

Comment 35

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

Attachment: Pull request for qemu patches

Comment 36

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

Attachment: Github pull request for kernel

Hi Dave,

You've been working on Linux drivers, right? Could you review the NFC kernel driver? Thanks.

Comment 37

[Dave Hylands [:dhylands]]

Comment on attachment 8345842 [details]
Github pull request for kernel

Looking pretty good.

I made a bunch of comments on github, and I'd like to take another look after you've revised things, so I'm giving r-

Comment 38

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

Attachment: Github pull request for kernel

Hi,

Thanks for the review. I fixed all the points you mentioned.

Comment 39

[Dave Hylands [:dhylands]]

Bah - I can't seem to even find my old comments in github.

Comment 40

[Dave Hylands [:dhylands]]

Comment on attachment 8356568 [details]
Github pull request for kernel

A few minor nits.

I think that there are some missing calls at device removal time, and a misplaced printk.

Comment 41

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

Attachment: Github pull request for kernel

r+'ed by dhylands

Comment 42

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

Comment on attachment 806081 [details]
Kernel patches

Github pull request in attachment 8357069 [details].

Comment 43

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

Comment on attachment 806078 [details]
Qemu patches

Github pull request in attachment 8342996 [details].

Comment 44

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

Comment on attachment 8342996 [details]
Pull request for qemu patches

I had two comments in https://github.com/tdz/platform_external_qemu/commit/fd30cb6abedbfb80c52f04db214edd49f5fa67fc#commitcomment-4842693 and https://github.com/tdz/platform_external_qemu/commit/195ab7f70870a444392eefc766e097f1795847d6#commitcomment-4842724 .

We don't really use Android hw properties.  So please either remove "hw.nfc" or add a "android_hw->hw_nfc" check.

Comment 45

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

Comment on attachment 8342996 [details]
Pull request for qemu patches

Hi Vicamo,

I updated the code according to your comments.

Comment 46

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

Comment on attachment 826785 [details]
Build-scripts patches

Need an additional moz specific property ro.moz.nfc.enabled=true to enable NFC. See https://github.com/mozilla-b2g/device-mako/blob/b2g-4.3_r2.1/device.mk#L277

Comment 47

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

Attachment: Github pull request for device config

The goldfish patches including ro.moz.nfc.enabled.

Comment 48

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

Comment on attachment 8358328 [details]
Github pull request for device config

I know there is already an example in ril/Android.mk that uses ADDITIONAL_BUILD_PROPERTIES, but that's not the best way with AOSP JellyBean or newer.  My fault!

For ICS, "full.mk" is also inherited by other physical devices, so emulator product specific bits can't be included in it.  But from JB and on, all physical devices should inherit either "full_base_telephony.mk" or "generic.mk" instead.  All emulator product specific bits can be safely added in "emulator.mk".  And actually that's the best home for them.

"Android.mk" files inside device folders should contain only module specific stuff while "BoardConfig.mk" and "device.mk" can contain product specific bits.  So please move this ro property to "build/target/product/emulator.mk" and use PRODUCT_PROPERTY_OVERRIDES to include it.

By "module specific", I mean variables only takes effect on a certain module(s).  By "product specific", I mean variables should only take effect on a certain product(s).  ADDITIONAL_BUILD_PROPERTIES takes effect on all builds no matter what product is selected, so we'd better avoid using it to prevent unexpected interference in the future.

Comment 49

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

Comment on attachment 8341009 [details]
Github pull request for b2g-manifest

Should we move qemu-kernel into emulator-jb.xml instead?  Don't know why it's in base-jb.xml now.

Comment 50

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

Comment on attachment 8341011 [details]
Github pull request for prebuilts/qemu-kernel

Confirmed these built images work on ICS/JB ARM emulators.  Currently ICS/JB x86 emulators boot but b2g crashes with or without NFC enabled kernel.  @Thomas, could you help confirm this as well?

Comment 51

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #48)
> I know there is already an example in ril/Android.mk that uses
> ADDITIONAL_BUILD_PROPERTIES, but that's not the best way with AOSP JellyBean
> or newer.  My fault!

See bug 959019 if you're interested.

Comment 52

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

Comment on attachment 8342996 [details]
Pull request for qemu patches

Qemu process crashes on receiving console command "nfc ntf rf_intf_activated 0".  See https://github.com/mozilla-b2g/platform_external_qemu/pull/52#issuecomment-32143430 for backtrace.

Comment 53

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

Comment on attachment 8358328 [details]
Github pull request for device config

I removed the additional property.

Comment 54

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

Attachment: Github pull request for build scripts

Includes ro.moz.nfc.enabled

Comment 55

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

Comment on attachment 8358328 [details]
Github pull request for device config

Will merge after manifest getting merged.

Comment 56

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

Comment on attachment 8359122 [details]
Github pull request for build scripts

Need a rebase on bug 959019.

Comment 57

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #52)
> Comment on attachment 8342996 [details]
> Pull request for qemu patches
> 
> Qemu process crashes on receiving console command "nfc ntf rf_intf_activated
> 0".  See
> https://github.com/mozilla-b2g/platform_external_qemu/pull/52#issuecomment-
> 32143430 for backtrace.

A quick check shows that it works for me. I currently update all my repos to the latest refs and will try again.

Comment 58

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

I uploaded my kernel binaries to

  https://github.com/tdz/platform_prebuilts_qemu-kernel/tree/bug-916863

so you can try with these if you like.

Comment 59

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

Hi Vicamo,

I tested with the latest refs of gaia, gecko, nfcd, libnfc-nci and the emulated device, but I cannot reproduce the problem you report. The error message indicates that the device is in an incorrect state, which can happen with older versions of nfcd or libnfc-nci. Could you update to the latest versions and try again? My kernel binaries might help as well.

Comment 60

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

Updated build script pull request for changes in bug 959019.

Comment 61

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #60)
> Updated build script pull request for changes in bug 959019.

s/nfd/nfc/

Comment 62

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

Gecko: de8f18e28cc7e12547a17004236e3bc6ee4955a7 (branch master HEAD)
Gaia: e8801c124e694b79938a36363a8378e489a7aff5 (branch master HEAD)
nfcd: f995ccdb7c023b7edd8064c9d06fbea8f7108c45 (branch master HEAD)
libnfc-nci: a0d589508ae446c8a0d0db9e8efa4f0245b9f0a5 (tag android-4.3_r2.1)
QEmu: https://github.com/tdz/platform_external_qemu/commit/79ba8c4e825ae1862dfd3f19af2cf5a89761fe3e (tdz/bug-916863 HEAD)
kernel source: https://github.com/vicamo/b2g_kernel_goldfish/commit/661373c1c0c4200393c4fb87d7cbe122340401c6 (merge of https://github.com/tdz/kernel_goldfish/commit/267f9de19b3152a8795c3d26690e7e200c0d2a76 , tdz/bug-916863 HEAD)
qemu-kernel: https://github.com/vicamo/b2g_platform_prebuilts_qemu-kernel/commit/11663426672fcc46e2a0f29239afa736b90635ba (merge of https://github.com/vicamo/b2g_platform_prebuilts_qemu-kernel/commit/0cd1eef0a511de1da5b4b8162a63db5bd9a05775 , which contains binary kernel images built from https://github.com/vicamo/b2g_kernel_goldfish/commit/661373c1c0c4200393c4fb87d7cbe122340401c6)

Still fails.

Comment 63

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

With kernel image from Thomas (https://github.com/tdz/platform_prebuilts_qemu-kernel/commit/82369c7b28d51ebba6a429a6d9decf0337114c36 , HEAD of tdz/bug-916863), I still have the same SIGABRT.

Comment 64

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #58)
> I uploaded my kernel binaries to
>   https://github.com/tdz/platform_prebuilts_qemu-kernel/tree/bug-916863
> so you can try with these if you like.

It must be based on https://github.com/mozilla-b2g/platform_prebuilts_qemu-kernel/commit/5282f3cbfdb62f3b576da6588ca2f0ca4f4bade8 and has commit messages like:

  Bug 916863: update 2.6 kernel for ARM & x86

  -from commit <commit url for the merged goldfish kernel pull request>
  -add goldfish nfc platform driver

If you build your kernel from commits not based on current HEAD, you will still have to rebuild them and commit new images again, or they'll probably break existing test cases like bluetooth's (bug 939441, bug 939637).

Comment 65

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

Switch libnfc_nci to d23865cf04954c28b4fd4805d85b7042f71b3ce6 (aosp/master HEAD), still fails.

Comment 66

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

For libnfc-nci I use d23865cf04954c28b4fd4805d85b7042f71b3ce6, which works for me. I'll checkout your refs to tests again.

Comment 67

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #62)
> Gecko: de8f18e28cc7e12547a17004236e3bc6ee4955a7 (branch master HEAD)
> Gaia: e8801c124e694b79938a36363a8378e489a7aff5 (branch master HEAD)
> nfcd: f995ccdb7c023b7edd8064c9d06fbea8f7108c45 (branch master HEAD)
> libnfc-nci: a0d589508ae446c8a0d0db9e8efa4f0245b9f0a5 (tag android-4.3_r2.1)
> QEmu:
> https://github.com/tdz/platform_external_qemu/commit/
> 79ba8c4e825ae1862dfd3f19af2cf5a89761fe3e (tdz/bug-916863 HEAD)
> kernel source:
> https://github.com/vicamo/b2g_kernel_goldfish/commit/
> 661373c1c0c4200393c4fb87d7cbe122340401c6 (merge of
> https://github.com/tdz/kernel_goldfish/commit/
> 267f9de19b3152a8795c3d26690e7e200c0d2a76 , tdz/bug-916863 HEAD)
> qemu-kernel:
> https://github.com/vicamo/b2g_platform_prebuilts_qemu-kernel/commit/
> 11663426672fcc46e2a0f29239afa736b90635ba (merge of
> https://github.com/vicamo/b2g_platform_prebuilts_qemu-kernel/commit/
> 0cd1eef0a511de1da5b4b8162a63db5bd9a05775 , which contains binary kernel
> images built from
> https://github.com/vicamo/b2g_kernel_goldfish/commit/
> 661373c1c0c4200393c4fb87d7cbe122340401c6)
> 
> Still fails.

Did you pull

  device/genericgoldfish: https://github.com/tdz/device_generic_goldfish/commit/0fe9e9d6ff268282a268464e04e9d93890c8e580
  build: https://github.com/tdz/platform_build/commit/41dbae25aa360e820b47937384ea8db13e47b271

?

Comment 68

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

Hi Vicamo,

I can reproduce a crash with the Gecko ref you mention. I currently investigate this.

Comment 69

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #67)
> Did you pull
> 
>   device/genericgoldfish:
> https://github.com/tdz/device_generic_goldfish/commit/
> 0fe9e9d6ff268282a268464e04e9d93890c8e580

I had https://github.com/tdz/device_generic_goldfish/commit/05f18aa1e85ff062d6b53bc7b3e36dcbcfa1617e instead, which is the one listed in https://github.com/mozilla-b2g/device_generic_goldfish/pull/7

>   build:
> https://github.com/tdz/platform_build/commit/
> 41dbae25aa360e820b47937384ea8db13e47b271

Yes, but with a local fix to address comment 61.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #68)
> Hi Vicamo,
> I can reproduce a crash with the Gecko ref you mention. I currently
> investigate this.

Nice!

Comment 70

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #66)
> For libnfc-nci I use d23865cf04954c28b4fd4805d85b7042f71b3ce6, which works
> for me. I'll checkout your refs to tests again.

Please try tag "android-4.3_r2.1" first.  That's the one we should use for Jellybean.  If somehow it doesn't work, then we try cherry-pick/fast-forward.

Comment 71

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

Vicamo,

I can only reproduce the assertion when NFC has been turned off. When you test by hand, did you turn on NFC in the Settings app before running 'nfc ntf rf_intf_activated 0'?

Comment 72

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

Tests are blocked by P2P regression.

Comment 73

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

Vicamo, with the patches from bug 950790 and the updates in bug 935525, the tests work again for me.

Comment 74

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #73)
> Vicamo, with the patches from bug 950790 and the updates in bug 935525, the
> tests work again for me.

I applied those new test script patches in bug 935525, rebase them to Gecko HEAD, but that doesn't really help :(  Still crashes and I've attached some log files there, too.

At least one thing I don't really understand is, should there ever be an assertion?  I think assertions are for internal states, not for commands that come from users.  We should not assume those commands are coming in a certain order, should we?

Comment 75

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

Comment on attachment 8358328 [details]
Github pull request for device config

I added a patch for setting the device file's owner and group to nfc:nfc. Otherwise nfcd cannot access it.

Comment 76

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

Comment on attachment 8342996 [details]
Pull request for qemu patches

One of the reverse-engineered commands was incomplete. This made nfcd fail when running as non-root. It's a problem with an undefined value. I added a patch to fix the problem.

Comment 77

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

Hi Vicamo,

Let me first apologize for this taking so long and adding so much work for you.

Your comment 15 in bug 935525 pointed me into the direction of incorrect file permissions for the device file. I used to run nfcd as root during development, so I never noticed. Fixing this exposed a bug in the emulator where a returned value wasn't defined. It took me a while to figure this out.

I reopened two review requests for the respective pull requests.

(Maybe you need 'ac_add_options --enable-nfc' in default-gecko-config, but I'm not sure about this.)

Comment 78

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

> At least one thing I don't really understand is, should there ever be an
> assertion?  I think assertions are for internal states, not for commands
> that come from users.  We should not assume those commands are coming in a
> certain order, should we?

Actually, this assertion is supposed to check for an internal state. There should probably be a test in the console interface that also checks the state and simply returns an error.

Comment 79

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #77)
> Hi Vicamo,
> 
> Let me first apologize for this taking so long and adding so much work for
> you.

np.

> Your comment 15 in bug 935525 pointed me into the direction of incorrect
> file permissions for the device file. I used to run nfcd as root during
> development, so I never noticed.

Ha!  You know I even use `rvim` instead of `vim`, so I will surely avoid running anything with root permission as possible ;)

> Fixing this exposed a bug in the emulator
> where a returned value wasn't defined. It took me a while to figure this out.
> I reopened two review requests for the respective pull requests.

Thank you!

> (Maybe you need 'ac_add_options --enable-nfc' in default-gecko-config, but
> I'm not sure about this.)

You don't need that.  On B2G it's default enabled and you can explicitly disable it by --disable-nfc.

Comment 80

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

Supposed merge sequence:

1) kernel, qemu, qemu-kernel
   < wait until gmo picks changes in qemu >
2) manifest, device/generic/goldfish
   < wait until gmo picks manifest >
3) build

Comment 81

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #80)
> Supposed merge sequence:
> 
> 1) kernel, qemu, qemu-kernel
>    < wait until gmo picks changes in qemu >
> 2) manifest, device/generic/goldfish
>    < wait until gmo picks manifest >
> 3) build

Great. Do I need to provide prebuilt kernels, or will that happen automatically?

Comment 82

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

https://github.com/mozilla-b2g/kernel_goldfish/commit/661373c1c0c4200393c4fb87d7cbe122340401c6
https://github.com/mozilla-b2g/platform_prebuilts_qemu-kernel/commit/11663426672fcc46e2a0f29239afa736b90635ba
https://github.com/tdz/platform_external_qemu/commit/8f7c9ac889ae2c778197b4a4c0529d60530f480b

Comment 83

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

https://github.com/mozilla-b2g/device_generic_goldfish/commit/09485b73629856b21b2ed6073e327ab0e69a1189
https://github.com/mozilla-b2g/b2g-manifest/commit/a8fcda44818084d8f6f9988dc2613084a907ddac

Comment 84

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

https://github.com/tdz/platform_build/commit/317f25e0a4cb3e8e86e2b76c37a14081372f0307