1136512 - Sony Z3C - nfcd should build with Z3 compatiable libnfc-nci library

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Dimi Lee [:dimi]]

Enable NFC function

Comment 1

[Dimi Lee [:dimi]]

At least following is required:
1.Add libnfc-nci repo to manifest
2.Add nfcd repo to manifest
3.Add nfcd service in init.b2g.rc in gonk-misc
4.Add libnfc-nxp.conf and libnfc-brcm.conf
5.Add firmware library nfc_nci.shinano.so
6.set ro.moz.nfc.enabled=true in device.mk

Comment 2

[Dimi Lee [:dimi]]

Hi Michael,
NFC require an external library called libnfc-nci.
Normally we can fetch it from AOSP. But because sony Z3 use NXP chipset, we will require additionally patches from AOSP to make it work.

According to
http://developer.sonymobile.com/knowledge-base/open-source/open-devices/aosp-build-instructions/how-to-build-aosp-lollipop-for-unlocked-xperia-devices/

We will need following commands
1.git fetch https://android.googlesource.com/platform/external/libnfc-nci refs/changes/42/103142/1 && git cherry-pick FETCH_HEAD
2.git fetch https://android.googlesource.com/platform/external/libnfc-nci refs/changes/23/103123/1 && git cherry-pick FETCH_HEAD
3.git fetch https://android.googlesource.com/platform/external/libnfc-nci refs/changes/51/97051/1 && git cherry-pick FETCH_HEAD

Could you give me some suggestion how could we do this ? Thanks.

Comment 3

[Michael Wu [:mwu]]

NFC currently works. Are you proposing to build it ourselves? gerard-majax can help you with figuring that out if that's the goal.

Comment 4

[Dimi Lee [:dimi]]

Now NFC works because we are using libnfc-nci.so from SONY's image.
But for NFC, we have a nfc daemon process which will include header from external/libnfc-nci

If we use libnfc-nci.so from SONY's image but include the header of libnfc-nci from AOSP, it may produce unpredictable error. We have faced this problem in flame before : bug 1008809

So I suggest we should build it ourselves.

Comment 5

[:gerard-majax]

Well, I'm not sure what you need from me. Looks like you have everything.

Comment 6

[Alin Jerpelea]

for functional NFC you need 

kernel 
https://github.com/sonyxperiadev/kernel/commits/aosp/LNX.LA.3.5.2.2-03010-8x74.0
commit 
4c0b860df94eeea14e7744f9e8e2bf0607547a26
4c0b860df94eeea14e7744f9e8e2bf0607547a26

cd ../../../external/libnfc-nci/
git fetch https://android.googlesource.com/platform/external/libnfc-nci refs/changes/42/103142/1 && git cherry-pick FETCH_HEAD
git fetch https://android.googlesource.com/platform/external/libnfc-nci refs/changes/23/103123/1 && git cherry-pick FETCH_HEAD
git fetch https://android.googlesource.com/platform/external/libnfc-nci refs/changes/51/97051/1 && git cherry-pick FETCH_HEAD
cd ../../hardware/libhardware/
git fetch https://android.googlesource.com/platform/hardware/libhardware refs/changes/21/103221/2 && git cherry-pick FETCH_HEAD

Comment 7

[Dimi Lee [:dimi]]

Hi Michael,
As explained in Comment 4, i think we should build our own libnfc-nci,
Do you have any suggestion ? Thanks !

Comment 8

[Alin Jerpelea]

as i wrote in coment #6 that is all you need to compile a working NFC lib

Comment 9

[Michael Wu [:mwu]]

The necessary commits are all listed here. Follow the instructions and push that to a new branch that the manifests can reference.

Comment 10

[Dimi Lee [:dimi]]

Attachment: pull request to platform_external_libnfc-nci

Comment 11

[Dimi Lee [:dimi]]

Attachment: pull request to device-shinano-common

This pull request update following:
1. Remove libnfc-nci.so from extract-files.sh
2. Add nfc config files to shinano-common because we may need to update the config file in the future

Comment 12

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

Comment on attachment 8589434 [details] [review]
pull request to platform_external_libnfc-nci

We don't plan to maintain libnfc-nci now,
this should be done in somewhere like https://github.com/sonyxperiadev

Comment 13

[Dimi Lee [:dimi]]

Hi
Is there any repository for libnfc-nci that already contains patches you mentioned in comment 6 ?
then we can just update manifest to point to that git repository

Comment 14

[:gerard-majax]

(In reply to Yoshi Huang[:allstars.chh] from comment #12)
> Comment on attachment 8589434 [details] [review]
> pull request to platform_external_libnfc-nci
> 
> We don't plan to maintain libnfc-nci now,
> this should be done in somewhere like https://github.com/sonyxperiadev

There is no such repo.

Comment 15

[Dimi Lee [:dimi]]

Currently NFC works on Z3C.

But right now libnfc-nci.so used in Z3C is from SONY's base image. In B2G NFC architecture, nfcd also need to build with libnfc-nci and currently this library is from AOSP.
So the potential issue is that if the header files of these two libraries is not sync, it may have unpredictable issue. This bug is used to track this issue.

There are two solutions if we find bugs related to unsync libnfc-nci library
1. As yoshi mentioned in Comment 12, we should use repository maintained by sony, the benefit of this solution is that we don't have to maintain libnfc-nci
2. Mozilla side maintain its own libnfc-nci library for z3 and apply patches mentioned in comment 6.

I would prefer solution 1 because maintain libnfc-nci is complex.

Comment 16

[Michael Wu [:mwu]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #15)
> There are two solutions if we find bugs related to unsync libnfc-nci library
> 1. As yoshi mentioned in Comment 12, we should use repository maintained by
> sony, the benefit of this solution is that we don't have to maintain
> libnfc-nci
> 2. Mozilla side maintain its own libnfc-nci library for z3 and apply patches
> mentioned in comment 6.
> 
> I would prefer solution 1 because maintain libnfc-nci is complex.

See comment 14.

Comment 18

[Vincent Chang[:vchang][changyihsin]]

Currently, we use nci library pulled from stock sony z3c device. We double check with our QA and NFC basic functions work fine on spark so far. Not quite sure if we find any NFC issues that blocks our dogfood plan? 

However, there might be some potential issues if we don't build nfcd with correct nci library source code.

The nci library needs some proprietary patches provides by sonymobile.
We can't point it to repository in AOSP directly. In case of Flame, T2M provides the repository maintain in T2M's server. When we have a problem related to nci library, we can contact them and get feedback.

We open this bug to track that if sonymobile can provide their nci library repository point to sonymobile's server. In that way, we can prevent any maintain efforts in the future when we found problem related to nci library. 

We are approaching with sonymobile and see if they can provide positive feedback soon.

Comment 19

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

I've asked question in SonyXperia's forum 
https://talk.sonymobile.com/t5/Lollipop/libnfc-nci-patches-from-Android/m-p/982493#U982493

The question is it looks there are 9 patches from NXP but Sony only applies 3 of them.
Is this intentional or they just forgot this?

We have no clue why or what Sony did in their Z3C for the NFC part.
So there's why we need some response from them.

I don't know if it's some policy reason that Android doesn't merge NXP's patch.
But perhaps Sony and Mozilla should work together on this since this will greatly help users/developers from both parties.

Comment 20

[Vincent Chang[:vchang][changyihsin]]

Hi Alin, could you help to provide some comments for Yoshi's question in comment 19? It would be very appreciated if you could help to drive and provide the libnfc-nci repository used for sony z3c.

Comment 21

[Alin Jerpelea]

hi 
all you have to do is remove prebuilts and use the code from aosp 
don not mix blobs with code!

Comment 22

[:gerard-majax]

https://android-review.googlesource.com/#/c/103142/
https://android-review.googlesource.com/#/c/97051/
https://android-review.googlesource.com/#/c/103123/
https://android-review.googlesource.com/#/c/103221/

Alin pointed me to those patches we will be needing. I think it will answer your previous questions :)

Comment 23

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

We found those patches cause some problems in Aries-L, still need some time to confirm the root-cause.

Comment 25

[Thomas Nguyen (:tnguyen)]

Attachment: Pull request device-shinano-common v1

Remove extracting proprietary Sony lib and config

Comment 26

[Thomas Nguyen (:tnguyen)]

Attachment: Pull request platform_external_libnfc-nci v1

Comment 27

[Thomas Nguyen (:tnguyen)]

Attachment: Pull request b2g-manifest v1

Comment 28

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

Comment on attachment 8676127 [details] [review]
Pull request platform_external_libnfc-nci v1

Please explain why you didn't follow Comment 2?

And explain https://github.com/tungmangtdh3/platform_external_libnfc-nci/commit/12ce4cc769a12e9105b0a2cdc6599b918f1d833a?diff=split,

if this is some commit already exists in AOSP, you should use cherry-pick instead of making your own commit, otherwise if this is your own fix, please explain what this is doing.

Comment 29

[:gerard-majax]

I'll wait on comment 28 answered

Comment 30

[Thomas Nguyen (:tnguyen)]

(In reply to Yoshi Huang[:allstars.chh] from comment #28)
> Comment on attachment 8676127 [details] [review]
> Pull request platform_external_libnfc-nci v1
> 
> Please explain why you didn't follow Comment 2?
> 
> And explain
> https://github.com/tungmangtdh3/platform_external_libnfc-nci/commit/
> 12ce4cc769a12e9105b0a2cdc6599b918f1d833a?diff=split,
> 
> if this is some commit already exists in AOSP, you should use cherry-pick
> instead of making your own commit, otherwise if this is your own fix, please
> explain what this is doing.

Sorry for late reply.
I tried to follow Sony instruction in their website, but no luck:
- It seems we are using a different kernel/drivers (pn544 or pn754, I am not sure) than AOSP kernel.
NFC could not be enabled and be stuck in i2c_open() and in a pn544 function.

I try to keep using HAL lib from sony and just use AOSP lib-nci (adding some fixes from NXP engineer, grab from AOSP master branch), and NFC works (including SE functionality in bug 1201427)

Do you have any ideas or suggestions?

Comment 31

[Thomas Nguyen (:tnguyen)]

(In reply to Alin Jerpelea from comment #6)
> for functional NFC you need 
> 
> kernel 
> https://github.com/sonyxperiadev/kernel/commits/aosp/LNX.LA.3.5.2.2-03010-
> 8x74.0
> commit 
> 4c0b860df94eeea14e7744f9e8e2bf0607547a26
> 4c0b860df94eeea14e7744f9e8e2bf0607547a26
> 
> cd ../../../external/libnfc-nci/
> git fetch https://android.googlesource.com/platform/external/libnfc-nci
> refs/changes/42/103142/1 && git cherry-pick FETCH_HEAD
> git fetch https://android.googlesource.com/platform/external/libnfc-nci
> refs/changes/23/103123/1 && git cherry-pick FETCH_HEAD
> git fetch https://android.googlesource.com/platform/external/libnfc-nci
> refs/changes/51/97051/1 && git cherry-pick FETCH_HEAD
> cd ../../hardware/libhardware/
> git fetch https://android.googlesource.com/platform/hardware/libhardware
> refs/changes/21/103221/2 && git cherry-pick FETCH_HEAD

Hi Anil,
I did follow your instruction and applied the patches. However, NFC could not be enabled with the following error :

10-21 11:31:37.243  1558  1564 D         : phNxpLog_InitializeLogLevel: global =1, Fwdnld =1, extns =1,                 hal =1, tml =1, ncir =1,                 ncix =1
10-21 11:31:37.243  1558  1564 E NxpTml  : _i2c_open() Failed: retval ffffffff
10-21 11:31:37.243  1558  1564 E NxpHal  : phTmlNfc_Init Failed

Kernel we are using
git://github.com/mozilla-b2g/codeaurora_kernel_msm, branch shinano

libnfc_nci we are using git://codeaurora.org/platform/external/libnfc-nci branch kk_3.5 ( I tried to switch to
https://android.googlesource.com/platform/external/libnfc-nci, branch kitkat-mr2-release and have the same problem)

Additional information:
Using HAL proprietary lib nfc_nci_pn547.msm8974.so and self build libnxf-nci (based on AOSP adding API for NXP specific NCI command) NFC basic functions work fine
Do you have any suggestions on that?
Thanks

Comment 32

[Thomas Nguyen (:tnguyen)]

Attachment: log.log

Could not enable NFC log

Comment 33

[:gerard-majax]

Thomas, Alin is working on Camera for Lollipop only at the moment and is traveling a lot, thus he has no time to investigate NFC issues on an unsupported device for them. I fear you will have to track that all by yourself.

Comment 34

[:gerard-majax]

Namely, those patches are for AOSP-based kernel, which is not our case.

Comment 35

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

Comment on attachment 8676127 [details] [review]
Pull request platform_external_libnfc-nci v1

r- for the comments on github.

Comment 36

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

Comment on attachment 8676121 [details] [review]
Pull request device-shinano-common v1

please describe where these files came from?

Comment 37

[Thomas Nguyen (:tnguyen)]

(In reply to Yoshi Huang[:allstars.chh] from comment #36)
> Comment on attachment 8676121 [details] [review]
> Pull request device-shinano-common v1
> 
> please describe where these files came from?

The patch changes:
- Remove extracting proprietary HAL lib from sony z3c in build process
- Remove extracting config file from sony z3c in build process.Instead, the config file should be located in source code so that we can update configuration in future. 
The new config file adding in the patch is exactly the same as config file we extracted from z3c device so far

Comment 38

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

Comment on attachment 8676121 [details] [review]
Pull request device-shinano-common v1

I noticed there's already a r+'ed PR made by dimi 6 months ago,
if your two PR are the same, you should NOT use his code and become yours.
If not, you should explain why you obsolete his PR.

Comment 39

[Thomas Nguyen (:tnguyen)]

(In reply to Yoshi Huang[:allstars.chh] from comment #38)
> Comment on attachment 8676121 [details] [review]
> Pull request device-shinano-common v1
> 
> I noticed there's already a r+'ed PR made by dimi 6 months ago,
> if your two PR are the same, you should NOT use his code and become yours.
> If not, you should explain why you obsolete his PR.

At the beginning, I would like to change some configs, and obsolete the old commit. Later the change is moved to 1216473.
Currently, the patch is exactly the same with Dimi's, so I will use the old patch.
Thanks for your review.

Comment 40

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8676121 [details] [review]
Pull request device-shinano-common v1

Duplicated with the old patch
https://github.com/mozilla-b2g/device-shinano-common/pull/16

Comment 41

[Thomas Nguyen (:tnguyen)]

Attachment: Pull request platform_external_libnfc-nci v2

Comment 42

[Thomas Nguyen (:tnguyen)]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #41)
> Created attachment 8677818 [details]
> Pull request platform_external_libnfc-nci v2

Changes:
- Follow instruction in Comment 6 for platform/external/libnfc-nci
- Rename HAL module correctly

Comment 43

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8677818 [details]
Pull request platform_external_libnfc-nci v2

https://github.com/mozilla-b2g/platform_external_libnfc-nci/pull/6

Comment 44

[Thomas Nguyen (:tnguyen)]

Attachment: Pull request kernel v1

Changes:
- Update pn547 driver in kernel follow comment 6

Comment 45

[Thomas Nguyen (:tnguyen)]

Attachment: Pull request device-shinano-common v2

Changes:

- Remove extracting nfc blobs from device
- Link pn544 to pn547 so that libnfc-nci could use the correct nfc driver

Comment 46

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8677818 [details]
Pull request platform_external_libnfc-nci v2

Change MIMI type of attachment

Comment 47

[Thomas Nguyen (:tnguyen)]

Attachment: Pull request platform_external_libnfc-nci v2

The previous attachment could not be detected as PR format. Re-attach

Comment 48

[:gerard-majax]

Thomas, you are removing the nfc_nci_pn547.msm8974.so blob but there is no hardware module being built at all with your patches applied. Are you sure this is what you want?

Comment 49

[Dimi Lee [:dimi]]

(In reply to Alexandre LISSY :gerard-majax from comment #48)
> Thomas, you are removing the nfc_nci_pn547.msm8974.so blob but there is no
> hardware module being built at all with your patches applied. Are you sure
> this is what you want?
This hardware module will be built inside libnfc-nci library but we don't use it right now.
I guess what missing in this patch is adding libnfc-nci and nfc_nci_pn547.xxx into |PRODUCT_PACKAGES| in device/sony/shinano-common/device.mk

Comment 50

[:gerard-majax]

(In reply to Dimi Lee[:dimi][:dlee] from comment #49)
> (In reply to Alexandre LISSY :gerard-majax from comment #48)
> > Thomas, you are removing the nfc_nci_pn547.msm8974.so blob but there is no
> > hardware module being built at all with your patches applied. Are you sure
> > this is what you want?
> This hardware module will be built inside libnfc-nci library but we don't
> use it right now.
> I guess what missing in this patch is adding libnfc-nci and
> nfc_nci_pn547.xxx into |PRODUCT_PACKAGES| in
> device/sony/shinano-common/device.mk

Yeah, that was my guess also, that just makes me a bit unhappy to review something that feels like was not even tested at all.

Comment 51

[:gerard-majax]

The problem is that none of the targets I have tried are working:
 - nfc_nci.pn54x
 - nfc_nci.msm8974.so
 - nfc_nci.pn54x.msm8974.so
 - nfc_nci.pn547.msm8974.so
 - nfc_nci_pn547.msm8974.so

Comment 52

[:gerard-majax]

I got it to work with nfc_nci.pn54x.default.so. I'm surprised why the target is using default and not msm8974 ... It looks to be too much unstable for me to validate the current patches. Did someone tested that before me? Obviously not. Please make sure you test everything thoroughly and validate with QA, the current libs have been used for six months without any problem so we might be risking regressing a ton of stuff.

Comment 53

[Thomas Nguyen (:tnguyen)]

Sorry for the delayed reply.

(In reply to Dimi Lee[:dimi][:dlee] from comment #49)
> (In reply to Alexandre LISSY :gerard-majax from comment #48)
> > Thomas, you are removing the nfc_nci_pn547.msm8974.so blob but there is no
> > hardware module being built at all with your patches applied. Are you sure
> > this is what you want?
> This hardware module will be built inside libnfc-nci library but we don't
> use it right now.
> I guess what missing in this patch is adding libnfc-nci and
> nfc_nci_pn547.xxx into |PRODUCT_PACKAGES| in
> device/sony/shinano-common/device.mk

(In reply to Alexandre LISSY :gerard-majax from comment #52)
> I got it to work with nfc_nci.pn54x.default.so. I'm surprised why the target
> is using default and not msm8974 ... It looks to be too much unstable for me
> to validate the current patches. Did someone tested that before me?
> Obviously not. Please make sure you test everything thoroughly and validate
> with QA, the current libs have been used for six months without any problem
> so we might be risking regressing a ton of stuff.

Thanks Alexandre and Dimi.
There's HAL implementation lib provided by NXP engineer Jizhou Liao <[e-mail]> and suggested in comment 6. Reference https://android-review.googlesource.com/#/c/103142/  . This lib (module name nfc_nci.pn54x.default.so in external/libnfc-nci/halimpl/pn54x/Android.mk) must be added to device/sony/shinano-common/device.mk.

For Aries, currently we are developing nfcd based on blobs extracted from sony device which cannot be looked into. The APIs and headers may be used incorrectly then we may have issues or some features may not work (for example SE in bug 1201427). Fortunately, Sony and NXP provided HAL implementation source code (pn54x) that can work in this case, what I am trying to do is put the hal into Aries code base and make it works.
The new approach may have issues (for example bug 1216473) which must be solved and validated before pushing commits in this bug. 
Thanks

Comment 54

[Thomas Nguyen (:tnguyen)]

I will add review again after fully test the patch

Comment 55

[Dimi Lee [:dimi]]

checked with thomas, we will do more tests locally and also see if QA could help us verify the patch before asking for review.

Comment 56

[Alison Shiue]

(In reply to Dimi Lee[:dimi][:dlee] from comment #55)
> checked with thomas, we will do more tests locally and also see if QA could
> help us verify the patch before asking for review.

Basic function (P2P sharing, read/write tag and payment) works fine when I tried with the local build Thomas provided.

Comment 57

[:gerard-majax]

(In reply to Alison Shiue from comment #56)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #55)
> > checked with thomas, we will do more tests locally and also see if QA could
> > help us verify the patch before asking for review.
> 
> Basic function (P2P sharing, read/write tag and payment) works fine when I
> tried with the local build Thomas provided.

Sorry, but "basic testing" is not an option here: we are changing the whole code of a library for that feature. Code we have been exercizing for months. We need more than just "basic testing". Basic testing on the blob did not exposed the Mifare Classic issue. Who knows what kind of regression we might be landing?

Comment 58

[Dimi Lee [:dimi]]

(In reply to Alexandre LISSY :gerard-majax from comment #57)
> (In reply to Alison Shiue from comment #56)
> > (In reply to Dimi Lee[:dimi][:dlee] from comment #55)
> > > checked with thomas, we will do more tests locally and also see if QA could
> > > help us verify the patch before asking for review.
> > 
> > Basic function (P2P sharing, read/write tag and payment) works fine when I
> > tried with the local build Thomas provided.
> 
> Sorry, but "basic testing" is not an option here: we are changing the whole
> code of a library for that feature. Code we have been exercizing for months.
> We need more than just "basic testing". Basic testing on the blob did not
> exposed the Mifare Classic issue. Who knows what kind of regression we might
> be landing?

Hi Alison, do you have a complete test cases for NFC then thomas could verify all of them ?

Hi Alexandre, is that ok for you if we have the list and test all of them ? or do you think it is still to risky to land this patch ?

Comment 59

[:gerard-majax]

Sorry but I am not a NFC expert, so I don't think the list will be useful to me. Yes, that will always be risky to land that patch. But we can reduce the risk with extensive QA. Currently, sorry if that sounds rude, but at the third review round I got patches that were not even able to enable NFC: how can I be confident that this will not regress when I get patches to test that obviously nobody even tried to build with before?

Comment 60

[Thomas Nguyen (:tnguyen)]

Sorry for making you confuse about the patches.
I did test the patches locally, but only remove the old libs + config files and "adb push" the new ones ( 3 libs : libnfc-nci, nfcd, nfc_nci.xxx.so, config files: libnfc-brcm.conf and libnfc-nxp.conf) . I did not build a clean image, therefore there's a missing change in device.mk.
Bug I found during my test:
- Mifare Classic may disable RF
The image I gave QA is a clean build (clone a new code base and make a clean build). For me, the new approach using HAL pn54x source would be better (at least we have full source to trace found issue, if we use Sony blobs, sometimes we have no way to fix). Apparently, it's a bit risky but we need more intensive tests.
What do you think about that? Do you have any advice?
Thanks

Comment 61

[:gerard-majax]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #60)
> Sorry for making you confuse about the patches.
> I did test the patches locally, but only remove the old libs + config files
> and "adb push" the new ones ( 3 libs : libnfc-nci, nfcd, nfc_nci.xxx.so,
> config files: libnfc-brcm.conf and libnfc-nxp.conf) . I did not build a
> clean image, therefore there's a missing change in device.mk.
> Bug I found during my test:
> - Mifare Classic may disable RF

Yes. And my point is: that got reported two weeks ago, while we have the device running since mid February. So I don't want we land that too fast and break users.

> The image I gave QA is a clean build (clone a new code base and make a clean
> build). For me, the new approach using HAL pn54x source would be better (at
> least we have full source to trace found issue, if we use Sony blobs,
> sometimes we have no way to fix).

Yes. I know. If you look on bugzilla you can see I have a lot of pending bugs assigned to me to kill more blobs. That is not done because of the same reason of what I am asking you: extensive testing.

> Apparently, it's a bit risky but we need
> more intensive tests.
> What do you think about that? Do you have any advice?

Still the same: I am happy we kill blob. But that should be done with care and not landing a big bunch of regressions. Yes I do have devices, but I am working on a lot of stuff on those in parallel with those.

I don't have NFC stuff though.

Comment 62

[Alison Shiue]

Here are the test cases in moztrap for NFC: https://goo.gl/HQnR5t

Comment 63

[:gerard-majax]

(In reply to Alison Shiue from comment #62)
> Here are the test cases in moztrap for NFC: https://goo.gl/HQnR5t

Great. That is still not answering my question: I want confirmation from QA that extensive testing has been done are we are not regressing NFC functionnality.

Comment 64

[Alison Shiue]

I will arrange a NFC full test for this patch.
You could please refer test cases on comment 62.
Please give any advice if I missed any possible affected part.

Comment 65

[Alison Shiue]

[2015/11/11] Test result of 1st full run NFC test:
　* Moztrap link: https://moztrap.mozilla.org/runtests/run/8358/env/347/
　* Total cases: 88, Pass: 51, Failed: 17, Skipped: 20 
　　- Some Cases are skipped caused by: 
　　　(1) On QA side, there is no nano SIM supporting MIFARE object
　　　(2) AOSP only supports exchange APDU in Lollipop
　　- 1 New issue found (bug 1223324), and it fails 17 test cases.
　* Build information
　　　Build ID               20151103111445
　　　Gaia Revision          519d03420e2d9e9ff2924ea04fba584e47a1756c
　　　Gecko Revision         n/a
　　　Gecko Version          45.0a1
　　　Device Name            aries
　　　Bootloader             s1


[2015/11/16] Test result of 2nd full run NFC test (with patch for bug 1223324):
　* Moztrap link: https://moztrap.mozilla.org/runtests/run/8422/env/347/
　* Total cases: 88, Pass: 68, Failed: 0, Skipped: 20 
　　- Some Cases are skipped caused by:
　　　(1) On QA side, there is no nano SIM supporting MIFARE object
　　　(2) AOSP only supports exchange APDU in Lollipop
　　- No issue found
　* Build information
　　　Build ID               20151112100428
　　　Gaia Revision          f17421e9a23b9dbd88c41347f1b9c442a97f48e0
　　　Gecko Revision         n/a
　　　Gecko Version          45.0a1
　　　Device Name            aries
　　　Bootloader             s1

Comment 66

[Thomas Nguyen (:tnguyen)]

Thanks Alison for the test result and your great finding
In summary, we found 2 issues during full test. Brief description:
-  Bug 1223324 - [NFC]NFC tag can't be correctly identified by phone (unplugged USB device). NFC works if device is plugged to PC though USB port.
Main reason is, there's a config flag in updated driver was not set when building kernel (NFC_PN547_PMC8974_CLK_REQ), therefore NFCC does not work properly with power supply.
Set the flag in Kconfig and the issue was fixed
- Bug 1216473 - [Aries] Mifare Classic is not fully supported in Aries, reading this kind of tag could disable RF occasionally. 
Mifare Classic has not been supported in Aries (even with Sony's blobs). So just disable the Mifare reader extension mode( for reading this kind of tag) in libnfc-nxp.conf. The issue was fixed

The 2 issues were fixed in the new image (2nd) I gave QA. The new libs work correctly and passed 68 in 88 test cases we have(0 failed and skipped 20 due to no testing environment). 
It's a good result so far, I think we could continue this bug (for review and then land the fix). What do you think about it?
I know you are busy with a lot of stuff. Sorry to bother you, your help is really appreciated.
Thanks

Comment 67

[:gerard-majax]

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #66)
> Thanks Alison for the test result and your great finding
> In summary, we found 2 issues during full test. Brief description:
> -  Bug 1223324 - [NFC]NFC tag can't be correctly identified by phone
> (unplugged USB device). NFC works if device is plugged to PC though USB port.
> Main reason is, there's a config flag in updated driver was not set when
> building kernel (NFC_PN547_PMC8974_CLK_REQ), therefore NFCC does not work
> properly with power supply.
> Set the flag in Kconfig and the issue was fixed
> - Bug 1216473 - [Aries] Mifare Classic is not fully supported in Aries,
> reading this kind of tag could disable RF occasionally. 
> Mifare Classic has not been supported in Aries (even with Sony's blobs). So
> just disable the Mifare reader extension mode( for reading this kind of tag)
> in libnfc-nxp.conf. The issue was fixed
> 
> The 2 issues were fixed in the new image (2nd) I gave QA. The new libs work
> correctly and passed 68 in 88 test cases we have(0 failed and skipped 20 due
> to no testing environment). 
> It's a good result so far, I think we could continue this bug (for review
> and then land the fix). What do you think about it?
> I know you are busy with a lot of stuff. Sorry to bother you, your help is
> really appreciated.
> Thanks

It's unclear to me: where is the fix for bug 1223324 ?

Also, you mention bug 1216473 but as far as I know that bug will be fixed by switching the build in the present bug. Does it means we need to fix only bug 1223324 to get everything ready ?

What are those 20 skipped tests ? Why do we have tests we cannot test ?

Comment 70

[Thomas Nguyen (:tnguyen)]

Attachment: Device shinano PR v3

Comment 71

[Thomas Nguyen (:tnguyen)]

We want to land the fix here with no regression bug. The PRs of this issue should the fix of the 2 above issues. 
Marked bug 1216473 and bug 1223324 as duplicated of this issue.
Information: 

> - Bug 1216473 - [Aries] Mifare Classic is not fully supported in Aries,
> reading this kind of tag could disable RF occasionally. 
> Mifare Classic has not been supported in Aries (even with Sony's blobs). So
> just disable the Mifare reader extension mode( for reading this kind of tag)
> in libnfc-nxp.conf. The issue was fixed

-https://github.com/mozilla-b2g/device-shinano-common/pull/34/commits
 commit 85e1e0701bf1e73420062c241104555b47c80ead disable mifare classic extension reader

> -  Bug 1223324 - [NFC]NFC tag can't be correctly identified by phone
> (unplugged USB device). NFC works if device is plugged to PC though USB port.
> Main reason is, there's a config flag in updated driver was not set when
> building kernel (NFC_PN547_PMC8974_CLK_REQ), therefore NFCC does not work
> properly with power supply.
> Set the flag in Kconfig and the issue was fixed
https://github.com/mozilla-b2g/codeaurora_kernel_msm/pull/47/commits
commit a6b01bf03007b34432571dca9f5664a0bd8460c0 enable NFC_PN547_PMC8974_CLK_REQ 

We only have to use the PRs to get everything ready.
Let me know when you are ready to review.

Comment 72

[:gerard-majax]

Sure, but what about those skipped tests ?

Comment 73

[Alison Shiue]

> What are those 20 skipped tests ? Why do we have tests we cannot test?

These skipped tests cases are related to SE API and Mifare.
SE API related test cases can be tested on Nexus L.
Mifare related test cases can be tested on Flame and Nexus devices since QA have micro SIM includes Mifare objects.

That's why we have these test cases.

Comment 74

[:gerard-majax]

(In reply to Alison Shiue from comment #73)
> > What are those 20 skipped tests ? Why do we have tests we cannot test?
> 
> These skipped tests cases are related to SE API and Mifare.
> SE API related test cases can be tested on Nexus L.
> Mifare related test cases can be tested on Flame and Nexus devices since QA
> have micro SIM includes Mifare objects.
> 
> That's why we have these test cases.

Right. Is that just QA missing SIM support for those ? Did we ever validated support for those on the Z3c KK port ?

I'll give a new try to those patches when I have time :). Thomas, can you flag me for review after making sure there is nothing missing? Thanks!

Comment 75

[Alison Shiue]

> Right. Is that just QA missing SIM support for those ? Did we ever validated
> support for those on the Z3c KK port ?

No, since no SIM available for Aries. 
We need DT's support for mifare testing.

Comment 76

[:gerard-majax]

Quick test worked for me, I'll review that more deeply later. Naoki, if we switch to source-build libnfc, do you want that to be included within the FOTA fullimg we will be able to issue to users once storage of the MARs will be addressed (hopefully this week), or do you want to conduct extended testing before?

Comment 77

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

We're going to try using a safe build this week for dogfood builds that are released at Orlando.
There's a new batch going out.

I would prefer to release after Orlando and after there's some testing.
Most people probably won't use NFC currently.  At the same time, my concerns are that it could potentially crash the system.

Are the patches for kk or l or both?  Current Dogfood builds are kk, not l.

Comment 78

[:gerard-majax]

It's for KK. I do share the same concerns.

Comment 79

[:gerard-majax]

Comment on attachment 8677832 [details] [review]
Pull request platform_external_libnfc-nci v2

I guess this is mostly the code we run on the L port? ...

Comment 80

[:gerard-majax]

Comment on attachment 8689896 [details] [review]
Device shinano PR v3

see comment on github

Comment 81

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

Comment on attachment 8689896 [details] [review]
Device shinano PR v3

I don't want Mifare problem to be fixed here as it's a separate problem.

Comment 82

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

Comment on attachment 8677832 [details] [review]
Pull request platform_external_libnfc-nci v2

The commit 'T1T HR0 and HR1 parameters missing in tag dectection' should be a seperate bug.

Comment 83

[Thomas Nguyen (:tnguyen)]

Create separate bug 1228473 to track the Hr0 and Hr1 issue

Comment 84

[Thomas Nguyen (:tnguyen)]

(In reply to Yoshi Huang[:allstars.chh] from comment #81)
> Comment on attachment 8689896 [details] [review]
> Device shinano PR v3
> 
> I don't want Mifare problem to be fixed here as it's a separate problem.

Reopen bug 1216473 for tracking this issue.

Comment 85

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8689896 [details] [review]
Device shinano PR v3

Removed commit related to Mifare Classic.
This should be tracked and fixed in bug 1216473

Comment 86

[Thomas Nguyen (:tnguyen)]

Change config.xml to test github branch and run try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a9e7ae6212d

Comment 87

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I ask that we land this after Orlando work week, please.

Comment 88

[Thomas Nguyen (:tnguyen)]

oh, sure. I just tried to see if the test branch could run on try, to make sure it does not break anything.
Will continue, add checkin-needed keyword and ask to land after work week.
Thanks

Comment 89

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/b2g-manifest/commit/658eee9ecf19546144508f2bd912ac4c62e78e7d
https://github.com/mozilla-b2g/codeaurora_kernel_msm/commit/da1273d39b3b940b3b2a5bc5fdb81c521d74caa3
https://github.com/mozilla-b2g/platform_external_libnfc-nci/commit/2d017f975f4542c6d23b226ec172b9b30383c31c
https://github.com/mozilla-b2g/device-shinano-common/commit/03f7deda712e213aabf79eb657bd902497e3b201

Comment 90

[:gerard-majax]

(In reply to Carsten Book [:Tomcat] from comment #89)
> https://github.com/mozilla-b2g/b2g-manifest/commit/
> 658eee9ecf19546144508f2bd912ac4c62e78e7d
> https://github.com/mozilla-b2g/codeaurora_kernel_msm/commit/
> da1273d39b3b940b3b2a5bc5fdb81c521d74caa3
> https://github.com/mozilla-b2g/platform_external_libnfc-nci/commit/
> 2d017f975f4542c6d23b226ec172b9b30383c31c
> https://github.com/mozilla-b2g/device-shinano-common/commit/
> 03f7deda712e213aabf79eb657bd902497e3b201

I fear we would want to issue a first FOTA fullimg to those devices BEFORE changing that.

Comment 91

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Clearing name.  A following bug in comment 82 is fixed.

There's going to be a FOTA build with the bug in comment 82 broken, we're testing a new build based on today's build that we're going to test tomorrow.  Hopefully we can get that released tomorrow.