975268 - [System] NFC: reset the screen timeout when a NFC tag/device is detected.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

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

We need to reset the timeout value for screen timeout when a NFC tag/device is discovered. 

See 
https://bugzilla.mozilla.org/show_bug.cgi?id=941765#c8, and 
https://bugzilla.mozilla.org/show_bug.cgi?id=941765#c10

Comment 1

[Ken Chang[:kenkai]]

Garner, I wonder if you or someone form DT can own this bugs.

Comment 2

[Garner Lee]

Some comments on previous comments:

Use wakelock:
https://bugzilla.mozilla.org/show_bug.cgi?id=941765#c10

This bug says to reset timeout:

Solution 1:
I've implemented the wakelock version (suspend screen timer), and simutaneously have been testing with an NFC enabled keyboard (for android). One thing of note is since the NFC keyboard would not be removed for the entire duration, a wakelock in these edge use cases probably would drain the battery fairly quickly by leaving the screen on.

Solution 2: 
Resetting the timer seems fairly simple, on techDiscovered or lost, call ScreenManager.turnScreenOn() to reset the timer to acknowledge user actions. If the user leaves it over a tag or device, it will eventually still turn off the screen.

2 is working fine so far, and has a simple implementation.

Comment 3

[Garner Lee]

^ Solution 3: Just tossing this here: Reach into the same low level event handlers for all hardware (screen touch, keyboard events, etc), and reset timers there.

Comment 4

[Garner Lee]

Attachment: Bug 975268: Reset the screen timout when a NFC tag/device is detected. ScreenManager version.

Comment 5

[Garner Lee]

Attachment: Reset the screen idle timeout when a NFC tag/device is detected

Comment 6

[Garner Lee]

The pull req is for "Solution #2".

Comment 7

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

Comment on attachment 8389391 [details] [review]
Reset the screen idle timeout when a NFC tag/device is detected

1. Do not touch method of other module directly.
2. Reset timeout !=== Turn on the screen.

Please send an event to screen manager for it to do reset the idle timer,
but do not 'turn on screen'

Comment 8

[Garner Lee]

Modified to send 'wake' window event. The direct idle timeout function call has some UX associated with it in the normal case that's missing (a timeout with a dimming warning, among other things).

Is comment 7 intended to migrate callers of ScreenManager to using events, or indicate that a idle call (_reconfigScreenTimeOut, _setIdleTimeout) needs to be added in some manner as a new handleEvent case? The later case will require a more investigation.

Comment 9

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

(In reply to Garner Lee from comment #8)
> Modified to send 'wake' window event. The direct idle timeout function call
> has some UX associated with it in the normal case that's missing (a timeout
> with a dimming warning, among other things).

You shouldn't send wake event. The event means the power button is pressed when screen is off.

> 
> Is comment 7 intended to migrate callers of ScreenManager to using events,
> or indicate that a idle call (_reconfigScreenTimeOut, _setIdleTimeout) needs
> to be added in some manner as a new handleEvent case? The later case will
> require a more investigation.

You should have a new event and let screen manager to change the timer accordingly.

And I still don't understand, are we going to 'enable NFC' when screen is off?
From the 1st and 2nd patch it seems so...do I misunderstand something? Isn't UX just want screen timeout to be reset or expanded?

Comment 10

[Juwei Huang[:juwei]]

We are **NOT** enable NFC when screen is off, what I mean in the Bug 941765 is that we suspend screen time-out when NFC tag/device is *already* paired.
As I said clearly in https://bugzilla.mozilla.org/show_bug.cgi?id=941765#c10,
 "once two devices paired/a device is reading tag, the screen should not turn off"

I did not mean to wake up screen to enable NFC, and we should not enable NFC while screen is off due to privacy issue.

Comment 11

[Ken Chang[:kenkai]]

(In reply to Juwei Huang from comment #10)
> We are **NOT** enable NFC when screen is off, what I mean in the Bug 941765
> is that we suspend screen time-out when NFC tag/device is *already* paired.
> As I said clearly in https://bugzilla.mozilla.org/show_bug.cgi?id=941765#c10,
>  "once two devices paired/a device is reading tag, the screen should not
> turn off"
Hi Juwei,
  It seems that we need the NFC to be in enabled or standby mode for payments feature and some special features. Please check bug 973480, 979868, and 979888. We need to think of those cases. To have different options?("tap to pay" option or...)

Comment 12

[Juwei Huang[:juwei]]

I see, thank you Ken!
For payment & pass-through-gate features, they indeed have to activate while screen is off. We probably need to design these features case by case.
I list down all the possible scenarios to check if NFC should enable when screen off:
1) NFC tag - Payment: enable
2) NFC tag - Pass-through-gate: enable
3) NFC tag - Wifi access: disable
4) NFC connects BT device : disable
5) NFC share media contents : disable

Scenario 1& 2 should let users activate NFC tag while screen is off. For scenario 3&4&5, we shall not let users to transfer files or connect other devices while screen is off.

Anything I missed?

Comment 13

[Ken Chang[:kenkai]]

(In reply to Juwei Huang from comment #12)
Agree with your opinions. But bug 973480 is related to case 3,4,and 5 and asking to enable NFC when screen is off...:-(. It seem be a kind of customization. I am not sure if we need to consider about these cases or we have alternative way for this bug.

Comment 14

[Garner Lee]

Can someone add me to the Bug 973480 reference above so I can get some more context? (no permission).

The current implementation I just uploaded uses a new handleEvent case 'reset-screen-idle-timeout', which is just the simplified version of turnScreenOn. That works. There is nothing in this code that directly deals with the actual power configuration of NFC (that honor goes to dispatchHardwareChangeEvt, controlled by screenchange and lock/unlock handleEvent swtich cases).

The power disucssion UX/User stories might be related, but appears to be a bit of a tangent to this particular bug: it's essentially a simple keep-alive for the screen (power usage) based on physical user actions.

Comment 15

[Garner Lee]

Comment on attachment 8389391 [details] [review]
Reset the screen idle timeout when a NFC tag/device is detected

New handleEvent for reset-screen-idle-timeout now added to screen manager.

Comment 16

[Garner Lee]

Attachment: Bug 975268: [System][NFC] Reset the screen idle timeout when a NFC tag/device is detected.

Same as previous pull request to bubble-tea, only rebased on top of master.

Comment 17

[Garner Lee]

Comment on attachment 8394328 [details] [review]
Bug 975268: [System][NFC] Reset the screen idle timeout when a NFC tag/device is detected.

Separate github comment:
In apps/system/js/screen_manager.js:

>> @@ -211,6 +212,10 @@ var ScreenManager = {
>>          this.turnScreenOn();
>>          break;
>>  
>> +      case 'reset-screen-idle-timeout':
>> +        this.resetScreenIdleTimeout();

> I am sure that you just need to call this._reconfigScreenTimeout().

Once it gets dim, the "in transition" dimmed screen timer needs to be reset as well, along with restoring the screen brightness. I have currently removed, it, but it no longer "undims" if the user sees the screen dim, and does an action, and the timer still fires to blank the screen. This could be a UX intent, so I currently have the code as you indicated. Otherwise, it does work as intended (_reconfigScreenTimeout is called).

Other code issues fixed in the unit test cases, and is passing green: 

https://travis-ci.org/mozilla-b2g/gaia/builds/22149612

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/d1861d1dfa9d9e6bc1c5055c1697c514555d123d