Closed Bug 975268 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

(b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: allstars.chh, Assigned: dgarnerlee)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Garner, I wonder if you or someone form DT can own this bugs.
Flags: needinfo?(dgarnerlee)
Assignee: nobody → dgarnerlee
Flags: needinfo?(dgarnerlee)
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.
^ 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.
Depends on: 963556
Attachment #8383931 - Attachment is obsolete: true
Attachment #8389391 - Flags: review?(alive)
The pull req is for "Solution #2".
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'
Attachment #8389391 - Flags: review?(alive) → review-
Summary: [System] NFC: reset the screen timout when a NFC tag/device is detected. → [System] NFC: reset the screen timeout when a NFC tag/device is detected.
Attachment #8389391 - Attachment description: Reset the screen idle timout when a NFC tag/device is detected → Reset the screen idle timeout when a NFC tag/device is detected
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.
Flags: needinfo?(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?
Flags: needinfo?(alive) → needinfo?(jhuang)
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.
Flags: needinfo?(jhuang)
(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...)
Flags: needinfo?(jhuang)
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?
Flags: needinfo?(jhuang)
(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.
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 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.
Attachment #8389391 - Flags: review- → review?(alive)
Same as previous pull request to bubble-tea, only rebased on top of master.
Attachment #8389391 - Attachment is obsolete: true
Blocks: b2g-NFC-2.0
No longer blocks: b2g-NFC-2.0
Target Milestone: --- → 1.4 S5 (11apr)
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
Attachment #8394328 - Flags: review?(alive)
Attachment #8394328 - Flags: review?(alive) → review+
Keywords: checkin-needed
Blocks: 993107
Master: https://github.com/mozilla-b2g/gaia/commit/d1861d1dfa9d9e6bc1c5055c1697c514555d123d
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 1.4 S5 (11apr) → 1.5 S1 (9may)
You need to log in before you can comment on or make changes to this bug.