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

RESOLVED FIXED in 2.6 S3 - 12/18

Status

Firefox OS
NFC
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dimi, Assigned: tnguyen)

Tracking

(Blocks: 1 bug)

unspecified
2.6 S3 - 12/18
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 6 obsolete attachments)

52 bytes, text/x-github-pull-request
gerard
: review+
Details | Review | Splinter Review
208.47 KB, text/x-log
Details
60 bytes, text/x-github-pull-request
gerard
: review+
Details | Review | Splinter Review
66 bytes, text/x-github-pull-request
gerard
: review+
allstars
: review+
Details | Review | Splinter Review
60 bytes, text/x-github-pull-request
allstars
: review+
gerard
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
Enable NFC function
(Reporter)

Comment 1

3 years ago
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
Blocks: 860906
(Reporter)

Comment 2

3 years ago
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.
Flags: needinfo?(mwu)

Comment 3

3 years ago
NFC currently works. Are you proposing to build it ourselves? gerard-majax can help you with figuring that out if that's the goal.
Flags: needinfo?(mwu)
(Reporter)

Comment 4

3 years ago
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.
Flags: needinfo?(lissyx+mozillians)
Well, I'm not sure what you need from me. Looks like you have everything.
Flags: needinfo?(lissyx+mozillians)

Comment 6

3 years ago
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
(Reporter)

Comment 7

3 years ago
Hi Michael,
As explained in Comment 4, i think we should build our own libnfc-nci,
Do you have any suggestion ? Thanks !
Flags: needinfo?(mwu)

Comment 8

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

Comment 9

3 years ago
The necessary commits are all listed here. Follow the instructions and push that to a new branch that the manifests can reference.
Flags: needinfo?(mwu)
(Reporter)

Updated

3 years ago
Depends on: 1144011
(Reporter)

Updated

3 years ago
Depends on: 1144561
(Reporter)

Comment 10

3 years ago
Created attachment 8589434 [details] [review]
pull request to platform_external_libnfc-nci
Attachment #8589434 - Flags: review?(mwu)
Attachment #8589434 - Flags: review?(allstars.chh)
(Reporter)

Comment 11

3 years ago
Created attachment 8589515 [details] [review]
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
Attachment #8589515 - Flags: review?(mwu)
Attachment #8589515 - Flags: review?(allstars.chh)

Updated

3 years ago
Attachment #8589434 - Flags: review?(mwu) → review?(lissyx+mozillians)

Updated

3 years ago
Attachment #8589515 - Flags: review?(mwu) → review?(lissyx+mozillians)
Attachment #8589515 - Flags: review?(allstars.chh) → review+
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
Attachment #8589434 - Flags: review?(allstars.chh) → review-
(Reporter)

Comment 13

3 years ago
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
Flags: needinfo?(alin.jerpelea)
(Reporter)

Updated

3 years ago
Attachment #8589434 - Flags: review?(lissyx+mozillians)
(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.
Flags: needinfo?(alin.jerpelea)
Flags: needinfo?(dlee)
Attachment #8589515 - Flags: review?(lissyx+mozillians) → review+
(Reporter)

Comment 15

3 years ago
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.
Flags: needinfo?(dlee)
Summary: [Lightsaber] Support NFC → Sony Z3C - nfcd should build with Z3 compatiable libnfc-nci library

Comment 16

3 years ago
(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.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1144561
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.
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.
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.
Flags: needinfo?(alin.jerpelea)

Comment 21

3 years ago
hi 
all you have to do is remove prebuilts and use the code from aosp 
don not mix blobs with code!
Flags: needinfo?(alin.jerpelea)
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 :)
Flags: needinfo?(vchang)
Flags: needinfo?(allstars.chh)
We found those patches cause some problems in Aries-L, still need some time to confirm the root-cause.
Flags: needinfo?(allstars.chh)
Flags: needinfo?(changyihsin)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1201427
(Assignee)

Updated

2 years ago
Assignee: dlee → tnguyen
(Assignee)

Updated

2 years ago
Attachment #8589434 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8589515 - Attachment is obsolete: true
(Assignee)

Comment 25

2 years ago
Created attachment 8676121 [details] [review]
Pull request device-shinano-common v1

Remove extracting proprietary Sony lib and config
Attachment #8676121 - Flags: review?(mwu)
Attachment #8676121 - Flags: review?(allstars.chh)
(Assignee)

Comment 26

2 years ago
Created attachment 8676127 [details] [review]
Pull request platform_external_libnfc-nci v1
(Assignee)

Updated

2 years ago
Attachment #8676127 - Flags: review?(mwu)
Attachment #8676127 - Flags: review?(allstars.chh)
(Assignee)

Comment 27

2 years ago
Created attachment 8676130 [details] [review]
Pull request b2g-manifest v1
Attachment #8676130 - Flags: review?(mwu)
Attachment #8676130 - Flags: review?(allstars.chh)
(Assignee)

Updated

2 years ago
Blocks: 1216473
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.
Attachment #8676127 - Flags: review?(allstars.chh)

Updated

2 years ago
Attachment #8676121 - Flags: review?(mwu) → review?(lissyx+mozillians)

Updated

2 years ago
Attachment #8676127 - Flags: review?(mwu) → review?(lissyx+mozillians)

Updated

2 years ago
Attachment #8676130 - Flags: review?(mwu) → review?(lissyx+mozillians)
I'll wait on comment 28 answered
(Assignee)

Comment 30

2 years ago
(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?
(Assignee)

Comment 31

2 years ago
(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
Flags: needinfo?(alin.jerpelea)
(Assignee)

Comment 32

2 years ago
Created attachment 8676744 [details]
log.log

Could not enable NFC log
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.
Flags: needinfo?(alin.jerpelea) → needinfo?(tnguyen)
Namely, those patches are for AOSP-based kernel, which is not our case.
Comment on attachment 8676127 [details] [review]
Pull request platform_external_libnfc-nci v1

r- for the comments on github.
Attachment #8676127 - Flags: review-
Comment on attachment 8676121 [details] [review]
Pull request device-shinano-common v1

please describe where these files came from?
Attachment #8676121 - Flags: review?(allstars.chh)
(Assignee)

Comment 37

2 years ago
(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
Flags: needinfo?(tnguyen)
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.
(Assignee)

Comment 39

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8589515 - Attachment is obsolete: false
(Assignee)

Comment 40

2 years ago
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
Attachment #8676121 - Attachment is obsolete: true
Attachment #8676121 - Flags: review?(lissyx+mozillians)
(Assignee)

Updated

2 years ago
Attachment #8589515 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8676127 - Attachment is obsolete: true
Attachment #8676127 - Flags: review?(lissyx+mozillians)
(Assignee)

Comment 41

2 years ago
Created attachment 8677818 [details]
Pull request platform_external_libnfc-nci v2
Attachment #8677818 - Flags: review?(lissyx+mozillians)
Attachment #8677818 - Flags: review?(allstars.chh)
(Assignee)

Comment 42

2 years ago
(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
(Assignee)

Comment 44

2 years ago
Created attachment 8677819 [details] [review]
Pull request kernel v1

Changes:
- Update pn547 driver in kernel follow comment 6
Attachment #8677819 - Flags: review?(lissyx+mozillians)
Attachment #8677819 - Flags: review?(allstars.chh)
(Assignee)

Comment 45

2 years ago
Created attachment 8677820 [details] [review]
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
Attachment #8677820 - Flags: review?(lissyx+mozillians)
Attachment #8677820 - Flags: review?(allstars.chh)
(Assignee)

Comment 46

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

Change MIMI type of attachment
Attachment #8677818 - Attachment mime type: text/plain → text/x-github-pull-request
(Assignee)

Updated

2 years ago
Attachment #8677818 - Attachment is obsolete: true
Attachment #8677818 - Flags: review?(lissyx+mozillians)
Attachment #8677818 - Flags: review?(allstars.chh)
(Assignee)

Comment 47

2 years ago
Created attachment 8677832 [details] [review]
Pull request platform_external_libnfc-nci v2

The previous attachment could not be detected as PR format. Re-attach
Attachment #8677832 - Flags: review?(lissyx+mozillians)
Attachment #8677832 - Flags: review?(allstars.chh)
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?
Flags: needinfo?(tnguyen)
(Reporter)

Comment 49

2 years ago
(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 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.
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
Flags: needinfo?(dlee)
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.
Attachment #8676130 - Flags: review?(lissyx+mozillians)
Attachment #8677819 - Flags: review?(lissyx+mozillians)
Attachment #8677820 - Flags: review?(lissyx+mozillians)
Attachment #8677832 - Flags: review?(lissyx+mozillians)
(Assignee)

Comment 53

2 years ago
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 <jizhou.liao@nxp.com> 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
Flags: needinfo?(tnguyen)
(Assignee)

Updated

2 years ago
Attachment #8676130 - Flags: review?(allstars.chh)
(Assignee)

Updated

2 years ago
Attachment #8677819 - Flags: review?(allstars.chh)
(Assignee)

Updated

2 years ago
Attachment #8677820 - Flags: review?(allstars.chh)
(Assignee)

Updated

2 years ago
Attachment #8677832 - Flags: review?(allstars.chh)
(Assignee)

Comment 54

2 years ago
I will add review again after fully test the patch
(Reporter)

Comment 55

2 years ago
checked with thomas, we will do more tests locally and also see if QA could help us verify the patch before asking for review.
Flags: needinfo?(dlee)

Comment 56

2 years ago
(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.
(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?
Flags: needinfo?(dlee)
Flags: needinfo?(ashiue)
(Reporter)

Comment 58

2 years ago
(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 ?
Flags: needinfo?(dlee) → needinfo?(lissyx+mozillians)
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?
Flags: needinfo?(lissyx+mozillians)
(Assignee)

Comment 60

2 years ago
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
(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

2 years ago
Here are the test cases in moztrap for NFC: https://goo.gl/HQnR5t
Flags: needinfo?(ashiue)
(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.
Flags: needinfo?(ashiue)

Comment 64

2 years ago
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.
Flags: needinfo?(ashiue)

Comment 65

2 years ago
[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
(Assignee)

Updated

2 years ago
Blocks: 1223324
(Assignee)

Comment 66

2 years ago
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
Flags: needinfo?(lissyx+mozillians)
(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 ?
Flags: needinfo?(tnguyen)
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(ashiue)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1223324
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1216473
(Assignee)

Comment 70

2 years ago
Created attachment 8689896 [details] [review]
Device shinano PR v3
Attachment #8677820 - Attachment is obsolete: true
(Assignee)

Comment 71

2 years ago
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.
Flags: needinfo?(tnguyen)
Sure, but what about those skipped tests ?

Comment 73

2 years ago
> 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.
Flags: needinfo?(ashiue)
(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!
Flags: needinfo?(tnguyen)
Flags: needinfo?(ashiue)

Comment 75

2 years ago
> 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.
Flags: needinfo?(ashiue)
(Assignee)

Updated

2 years ago
Flags: needinfo?(tnguyen)
Attachment #8676130 - Flags: review?(lissyx+mozillians)
(Assignee)

Updated

2 years ago
Attachment #8677819 - Flags: review?(lissyx+mozillians)
Attachment #8677819 - Flags: review?(allstars.chh)
(Assignee)

Updated

2 years ago
Attachment #8677819 - Flags: review?(allstars.chh)
(Assignee)

Updated

2 years ago
Attachment #8677832 - Flags: review?(lissyx+mozillians)
Attachment #8677832 - Flags: review?(allstars.chh)
(Assignee)

Updated

2 years ago
Attachment #8689896 - Flags: review?(lissyx+mozillians)
Attachment #8689896 - Flags: review?(allstars.chh)
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?
Flags: needinfo?(nhirata.bugzilla)
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.
Flags: needinfo?(nhirata.bugzilla)
Flags: needinfo?(lissyx+mozillians)
It's for KK. I do share the same concerns.
Flags: needinfo?(lissyx+mozillians)
Attachment #8676130 - Flags: review?(lissyx+mozillians) → review+
Attachment #8677819 - Flags: review?(lissyx+mozillians) → review+
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? ...
Attachment #8677832 - Flags: review?(lissyx+mozillians) → review+
Comment on attachment 8689896 [details] [review]
Device shinano PR v3

see comment on github
Attachment #8689896 - Flags: review?(lissyx+mozillians)
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.
Attachment #8689896 - Flags: review?(allstars.chh)
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.
Attachment #8677832 - Flags: review?(allstars.chh)
(Assignee)

Updated

2 years ago
Attachment #8677832 - Flags: review?(allstars.chh)
(Assignee)

Comment 83

2 years ago
Create separate bug 1228473 to track the Hr0 and Hr1 issue
(Assignee)

Comment 84

2 years ago
(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.
(Assignee)

Comment 85

2 years ago
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
Attachment #8689896 - Flags: review?(lissyx+mozillians)
Attachment #8689896 - Flags: review?(allstars.chh)
Attachment #8689896 - Flags: review?(lissyx+mozillians) → review+
(Assignee)

Comment 86

2 years ago
Change config.xml to test github branch and run try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a9e7ae6212d
Attachment #8677832 - Flags: review?(allstars.chh) → review+
Attachment #8689896 - Flags: review?(allstars.chh) → review+
I ask that we land this after Orlando work week, please.
(Assignee)

Comment 88

2 years ago
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
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(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.
Flags: needinfo?(nhirata.bugzilla)
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.
Flags: needinfo?(nhirata.bugzilla)
You need to log in before you can comment on or make changes to this bug.