Closed Bug 916863 Opened 11 years ago Closed 11 years ago

[NFC] NFC support in emulator

Categories

(Firefox OS Graveyard :: Emulator, defect)

Product:
Firefox OS Graveyard
Component:
Emulator
Platform:
x86_64
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(6 files, 8 obsolete files)

Github pull request for b2g-manifest
382 bytes, text/html
mwu
: review+
Details
Github pull request for prebuilts/qemu-kernel
430 bytes, text/html
vicamo
: feedback+
Details
Pull request for qemu patches
409 bytes, text/html
vicamo
: review+
Details
Github pull request for kernel
385 bytes, text/html
tzimmermann
: review+
Details
Github pull request for device config
409 bytes, text/html
vicamo
: review+
Details
Github pull request for build scripts
385 bytes, text/html
vicamo
: review+
Details
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.
Blocks: b2g-emulator
Component: NFC → Emulator
Attached file Qemu patches (obsolete) —
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
Attachment #806078 - Flags: feedback?(vyang)
Attached file Kernel patches (obsolete) —
Just the kernel patches. There is nothing special here. The driver forwards NCI messages between device and user-space driver.
Attachment #806081 - Flags: feedback?(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 :)
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/
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
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 ;)
(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
(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.
Hi Thomas, do you have detailed steps to verify your work here?
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.
Attached patch libnfc-nci patch (obsolete) — — Splinter Review
Attached patch build patch (obsolete) — — Splinter Review
(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 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.
(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
(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.
(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'.
Depends on: 932779
Attached file Build-scripts patches (obsolete) —
Attachment #810568 - Attachment is obsolete: true
Attachment #810570 - Attachment is obsolete: true
(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.
Attachment #806078 - Attachment description: Goldfish patches → Qemu patches
Attachment #806078 - Flags: feedback?(vyang)
Attachment #806081 - Flags: feedback?(vyang)
Attached file Goldfish patches (obsolete) —
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}]
> RC_INTF_ACTIVATED RF_INTF_ACTIVATED
(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?
(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?
(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.
But the problem is that ndefMsg is undefined. It looks like the array was never created at all.
I just updated gecko and nfcd, but the problem persists. I'll probably have to go through the code.
Blocks: 935525
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?
(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 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.
Need yet another pull request to b2g-manifest for libnfc-nci and nfcd.
Attachment #8342996 - Flags: review?(vyang)
Attached file Github pull request for kernel (obsolete) —
Hi Dave, You've been working on Linux drivers, right? Could you review the NFC kernel driver? Thanks.
Attachment #8345842 - Flags: review?(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-
Attachment #8345842 - Flags: review?(dhylands) → review-
Attached file Github pull request for kernel (obsolete) —
Hi, Thanks for the review. I fixed all the points you mentioned.
Attachment #8345842 - Attachment is obsolete: true
Attachment #8356568 - Flags: review?(dhylands)
Bah - I can't seem to even find my old comments in github.
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.
Attachment #8356568 - Flags: review?(dhylands) → review+
r+'ed by dhylands
Attachment #8357069 - Flags: review+
Attachment #8356568 - Attachment is obsolete: true
Comment on attachment 806081 [details] Kernel patches Github pull request in attachment 8357069 [details].
Attachment #806081 - Attachment is obsolete: true
Comment on attachment 806078 [details] Qemu patches Github pull request in attachment 8342996 [details].
Attachment #806078 - Attachment is obsolete: true
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.
Attachment #8342996 - Flags: review?(vyang)
Comment on attachment 8342996 [details] Pull request for qemu patches Hi Vicamo, I updated the code according to your comments.
Attachment #8342996 - Flags: review?(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
The goldfish patches including ro.moz.nfc.enabled.
Attachment #827345 - Attachment is obsolete: true
Attachment #8358328 - Flags: review?(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.
Attachment #8358328 - Flags: review?(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.
Attachment #8341009 - Flags: review?(mwu)
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?
Attachment #8341011 - Flags: review?(tzimmermann)
(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 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.
Attachment #8342996 - Flags: review?(vyang) → review-
Comment on attachment 8358328 [details] Github pull request for device config I removed the additional property.
Attachment #8358328 - Flags: review?(vyang)
Includes ro.moz.nfc.enabled
Attachment #826785 - Attachment is obsolete: true
Attachment #8359122 - Flags: review?(vyang)
Comment on attachment 8358328 [details] Github pull request for device config Will merge after manifest getting merged.
Attachment #8358328 - Flags: review?(vyang) → review+
Comment on attachment 8359122 [details] Github pull request for build scripts Need a rebase on bug 959019.
(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.
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.
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.
Updated build script pull request for changes in bug 959019.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #60) > Updated build script pull request for changes in bug 959019. s/nfd/nfc/
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.
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.
(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).
Switch libnfc_nci to d23865cf04954c28b4fd4805d85b7042f71b3ce6 (aosp/master HEAD), still fails.
For libnfc-nci I use d23865cf04954c28b4fd4805d85b7042f71b3ce6, which works for me. I'll checkout your refs to tests again.
(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 ?
Hi Vicamo, I can reproduce a crash with the Gecko ref you mention. I currently investigate this.
(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!
(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.
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'?
Flags: needinfo?(vyang)
Tests are blocked by P2P regression.
Depends on: 960790
Vicamo, with the patches from bug 950790 and the updates in bug 935525, the tests work again for me.
Attachment #8359122 - Flags: review?(vyang) → review+
Flags: needinfo?(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 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.
Attachment #8358328 - Flags: review+ → review?(vyang)
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.
Attachment #8342996 - Flags: review- → review?(vyang)
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.)
> 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.
(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.
Attachment #8358328 - Flags: review?(vyang) → review+
Attachment #8342996 - Flags: review?(vyang) → review+
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
(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?
Attachment #8341009 - Flags: review?(mwu) → review+
Attachment #8341011 - Flags: review?(tzimmermann) → feedback+
https://github.com/tdz/platform_build/commit/317f25e0a4cb3e8e86e2b76c37a14081372f0307
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: