959109 - B2G NFC: Disable NFC when screen is off.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Some NFC hardware functionalities (like discovery) should be disabled when screen is off.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

When screen is off, Gaia will send a ContentEvent 

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/nfc_manager.js#L158

Comment 2

[Garner Lee]

Attachment: Check screen and lock state at the same time.

Comment 3

[Garner Lee]

My mistake. 952212 was not the right dependency. The bug depends Gaia change in Bug 950269.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Hi Garner
I thought this should be done in Gecko part,

if you think the Gaia part is not correct (acceptNfcEvents()), can you kindly explain or maybe file another bug?

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8360120 [details] [review]
Check screen and lock state at the same time.

See github

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8360120 [details] [review]
Check screen and lock state at the same time.

It's already done in acceptNFCEvents(), please explain why this change is made.

Comment 7

[Garner Lee]

It relates to 959104, where "vibrate forever" is techDiscovered or techLost being fired continuously (and seems to have some effect on ShrinkUI in p2p mode).

In handleEvent, acceptNFCEvents() was used by the screenchange case, which might send a conflicting value (NFC_HW_STATE_ENABLE_DISCOVERY, NFC_HW_STATE_DISABLE_DISCOVERY) with screen locked. It is by independent, but closely related to screenchange. Change that case to only check screen on/off fixed the vibrate forever problem. Other potential behavior issues in gecko/nfcd might be hidden however. If you see other issues, or intended to fix something else in this bug, please let me know.

I'll check Alive's review comment, and investigate some more.

Comment 8

[Garner Lee]

Alive's change request applied and still working. There's still some ocassional *extra* discovery event from the hardware (nfcd debounce  didn't catch it?), but that seems to be a different bug.

If OK, check-in needed.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

As discussed with Garner, we don't have to ask Alive for comments.

Comment 10

[Dimi Lee [:dimi]]

Attachment: [WIP] Enable/disable discovery when screen on/off

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Comment on attachment 8363600 [details] [diff] [review]
[WIP] Enable/disable discovery when screen on/off

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

Add r=me

::: b2g/chrome/content/shell.js
@@ +747,5 @@
>          KeyboardHelper.handleEvent(detail);
>          break;
> +      case 'nfc-hardware-state-change':
> +        Services.obs.notifyObservers(null, 'nfc-hardware-state-change',
> +          JSON.stringify({ nfcHardwareState: detail.nfcHardwareState}));

extra space before nfcHarwardState.
or either also put a space at the end.

::: dom/system/gonk/Nfc.js
@@ +639,5 @@
> +      case NFC.TOPIC_HARDWARE_STATE:
> +        let state = JSON.parse(data);
> +        if (state) {
> +          let level = this.hardwareStateToPowerlevel(state.nfcHardwareState);
> +          this.setConfig({powerLevel:level});

put a space after :
powerLevel: level

Comment 12

[Dimi Lee [:dimi]]

Try server result :
https://tbpl.mozilla.org/?tree=Try&rev=d8198d43baf8

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

address nits first and add r=me

Comment 14

[Dimi Lee [:dimi]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #13)
> address nits first and add r=me

Sorry, i forget to attach updated patch...

Comment 15

[Dimi Lee [:dimi]]

Attachment: bug 959109 fix

Comment 16

[Dimi Lee [:dimi]]

Delay checkin-needed until Bug 962530 is landed

Comment 17

[Ken Chang[:kenkai]]

Since Bug 962539 has been landed, should we land this now?

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

https://hg.mozilla.org/integration/b2g-inbound/rev/2cd547d09fcf

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

Attachment: bug 959109 fix. v2

update the subject of the patch.

Add r=allstars.chh

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2cd547d09fcf

Comment 21

[Tomoaki Konno]

Is NFC payment disabled when screen is off?

Comment 22

[Dimi Lee [:dimi]]

(In reply to Tomoaki Konno from comment #21)
> Is NFC payment disabled when screen is off?

NFC payment should not be off in this case.
I will pay attention to this use case when implement payment related function

Comment 23

[Sandip Kamat]

Ming, please see comment 21.

Comment 24

[Ken Chang[:kenkai]]

This bug is conflicting to bug 973480, bug 979868, and bug 979888. We should add a standby mode for NFC when NFC is on but screen is off. Filing bug 984207 for tracking.

Comment 25

[Ken Chang[:kenkai]]

(In reply to Tomoaki Konno from comment #21)
> Is NFC payment disabled when screen is off?
In current design, the answer is yes. But we should add a suspend/standby mode for NFC. Please check bug 984207.

Comment 26

[Sandip Kamat]

Please refer to: https://bugzilla.mozilla.org/show_bug.cgi?id=973480#c8

Also, some of these bugs look like duplicates. Can these be appropriately duplicated?

Comment 27

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

https://bugzilla.mozilla.org/show_bug.cgi?id=973480#c11
It depends on how partner wants to track it. I'm fine either way.

Comment 28

[Ken Chang[:kenkai]]

(In reply to Sandip Kamat from comment #26)
> Please refer to: https://bugzilla.mozilla.org/show_bug.cgi?id=973480#c8
> 
> Also, some of these bugs look like duplicates. Can these be appropriately
> duplicated?
I don't think so.
Bug 984207 is proposing a standby/low power mode for NFC. But it doesn't define when device should enter this mode, which depends on the user story and what partners want.

Comment 29

[Alison Shiue]

https://moztrap.mozilla.org/manage/cases/?filter-tag=2786&pagenumber=1&pagesize=50&sortfield=created_on&sortdirection=desc

Comment 30

[Alison Shiue]

Verify on
Gaia      f3b5d74dd3428c89cab06db734c62f3c9dbb8c4d
Gecko     https://hg.mozilla.org/mozilla-central/rev/e86a0d92d174
BuildID   20140525160203
Version   32.0a1