Closed Bug 830425 Opened 11 years ago Closed 11 years ago

Phone takes too long to wake up for incoming phone calls

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: jaws, Assigned: airpingu)

References

Details

(Whiteboard: [triage:1/21])

Attachments

(1 file, 12 obsolete files)

4.40 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
I've missed many phone calls in the past few days due to this bug.

I just tried calling my Unagi device from a separate phone next to me. Through the headset I heard about three rings before the Unagi screen turned on and started to make any noise. The Unagi device was only able to ring about once (for about three seconds) before the call went to my voicemail.

This has been very frustrating for me as I then have to call the person back and explain to them that my phone was not able to answer the call quick enough even though I would have been readily able to do so.

To reproduce, it may help to have Wifi enabled and have the screen off. This is with build 20130110070201.
Etienne could you do some tests ? Is it a device problem or is it a carrier issue?
Assignee: nobody → etienne
Flags: needinfo?(etienne)
Keywords: qawanted
could this be tested in target market with target market SIM?
I know we had a platform issue where the phone was going back to sleep before gaia got the system message for the new call, and then waking up again.

We had a work around but it might have been removed.

I'm sure somebody on the platform will know, cc'ing.
Assignee: etienne → nobody
Component: Gaia::Dialer → General
Flags: needinfo?(etienne)
Someone from the RIL team should look at this.
Assignee: nobody → vyang
blocking-b2g: tef? → tef+
This issue was previously filed in bug 804707, and closed with a conclusion to fix by chip vendor's RIL.

I think we should fix this in Mozilla's RIL as well.
Blocks: 812059
I confirmed this still happens pretty consistently with me with latest 1/14 build as well. I am using AT&T and the same sim in my Android phone, I am able to hear all 4 rings before it goes to voicemail. With Unagi, I maybe hear the last ring and then goes to voicemail.

FWIW, the phone is locked when I call. The lock screen comes on. Then on the last ring the answer slider comes on with the name/number of the person calling. If I do not slide the answer up fast enough, it goes to voicemail and I have no choice but to call back.

Anything I can do to help with debugging this issue then please let me know what to provide.

dkl
tracking-b2g18: ? → ---
Whiteboard: [triage:1/16]
As an added note, it will pick up faster (two-three rings) if the Phone app has been launched prior. So it seems to me that the app is just not being loaded quickly enough to display the call answer screen.

dkl
QAWanted : 
Based on comment 7, try to reproduce:
a) w/o dialer up 
b) w/ dialer up

My guess is that it has something to do with sleep state as well.  My phone was asleep and then my friend had called it and I couldn't answer at all.
Not able to reproduce using the latest nightly build. I tried a variety of different configurations including the ones Naoki suggested in Comment 8. My SIM card is AT&T.

Can people seeing this bug run adb shell cat /proc/version and confirm which kernel version you are running on your device as well as clarify whether you are running on beta or nightly?

Also, what other apps are running when you are seeing this happen?
Keywords: qawanted
(In reply to Marcia Knous [:marcia] from comment #9)
> Not able to reproduce using the latest nightly build. I tried a variety of
> different configurations including the ones Naoki suggested in Comment 8. My
> SIM card is AT&T.

To be clear, you are able to hear all 4 (or at least 3) rings on the unagi phone before it goes to voicemail?

Do you have a link to the nightly where this works for you so I can try it myself?

> Can people seeing this bug run adb shell cat /proc/version and confirm which
> kernel version you are running on your device as well as clarify whether you
> are running on beta or nightly?

beta 20130117

Linux version 3.0.8-perf (songsy@ubuntu-cdr) (gcc version 4.4.3 (GCC) ) #1 PREEMPT Wed Dec 5 04:47:49 PST 2012
 
> Also, what other apps are running when you are seeing this happen?

None. Basically a fresh boot and leave it locked with screen off.

dkl
I can reproduce this by calling my CityFone SIM (B2G) from my Fido SIM (Android):  between 2 and 3 rings on the Android phone, the B2G screen lights up and around 3 rings I get the ability to answer on my unagi.
Action item: 
1. To extend the wake up timeout value. 
2. To ask the partner to double confirm if CPU could be used with maximum usage when a call is coming.
Whiteboard: [triage:1/16] → [triage:1/21]
Please see bug 804707, comment #31 and bug 804707, comment #35. Implementing solution B (a type of vendor-independant solution) might probably help if the root cause is indeed due to the lack of wake lock. I'll try to work out a patch in a couple of days.
(In reply to Gene Lian [:gene] [:clian] from comment #13)
> Please see bug 804707, comment #31 and bug 804707, comment #35. Implementing
> solution B (a type of vendor-independant solution) might probably help if
> the root cause is indeed due to the lack of wake lock. I'll try to work out
> a patch in a couple of days.

Okay, I've assigned it to you now, Gene.

Is there anyone who could work on this before Gene gets a chance?
Assignee: vyang → clian
Flags: needinfo?(jcheng)
(In reply to Andrew Overholt [:overholt] from comment #11)
> I can reproduce this by calling my CityFone SIM (B2G) from my Fido SIM
> (Android):  between 2 and 3 rings on the Android phone, the B2G screen
> lights up and around 3 rings I get the ability to answer on my unagi.

Just to add, seems to get slightly worse the longer you use it and the more apps you gave opened/closed. Hardware/resources not keeping up?

dkl
(In reply to Andrew Overholt [:overholt] from comment #14)
> (In reply to Gene Lian [:gene] [:clian] from comment #13)
> > Please see bug 804707, comment #31 and bug 804707, comment #35. Implementing
> > solution B (a type of vendor-independant solution) might probably help if
> > the root cause is indeed due to the lack of wake lock. I'll try to work out
> > a patch in a couple of days.
> 
> Okay, I've assigned it to you now, Gene.
> 
> Is there anyone who could work on this before Gene gets a chance?

Sorry for confusing. I mean I'm currently putting my 100% efforts on this issue (doesn't mean I'd start with it after a couple of days). Hope I can provide a patch today. Well, tomorrow at most. Please stay tuned. ;)
The extension of wake lock life cycle seems working. However, I need more time to test everything to make sure the wake lock acquiring/releasing between RIL and System Message is consistent and bug-free. Should be able to upload fine-tuned patches for formal reviews tomorrow.

Also, cc'ing more people related to this issue.
Hi Fabrice,

It's me again. I hope to provide a .getNumPagesToOpen() to let the caller have chance to advancedly know how many pages are going to be opened before calling .sendMessage(). or .broadcastMessage().

In this way, the caller is able to know how many "SystemMessageManager:HandleMessageDone" it should receive, so that it can know the exact time point to release the wake lock when all the pages are opened for sure.

Also, I think the child should return "SystemMessageManager:HandleMessageDone" when there is no handler registered for this message (not just at the end of ._dispatchMessage()), because it's also a kind of "done" for handling the message.

Others are alignment things for clean-up if you don't mind. Could you please take this review? Thanks!
Attachment #704813 - Attachment is obsolete: true
Attachment #705285 - Flags: review?(fabrice)
Hi Vicamo,

Since we need to acquire/release wake locks from ril_worker.js, we need to utilize ctypes technique to expose .acquire_wake_lock() and .release_wake_lock() from libhardware_legacy. You can find the detailed implementations for the native codes at [1]. Would you mind taking this review? Thanks!

[1]/home/gene/B2G_OTORO/hardware/libhardware_legacy/power/power.c
Attachment #704814 - Attachment is obsolete: true
Attachment #705287 - Flags: review?(vyang)
Hi Vicamo,

This is following patch 2.

The ril_worker is in charge of acquiring/releasing the wake lock when a call-ring starts/ends. To know when the system message(s) is done handled by page(s), the RadioInterfaceLayer is responsible for counting how many "SystemMessageManager:HandleMessageDone" have been received. When all of them are collected, it will ask ril_worker to release the wake lock through rilMessageType: "releaseCallRingWakeLock".

I think each call-ring should have its own counts and timer. It's really error-prone if we use general counts and timer, because different calls will wrongly cancel others' counts and timer, thus producing bad effects.

Thanks for the review again!
Attachment #704816 - Attachment is obsolete: true
Attachment #705290 - Flags: review?(vyang)
After testing these patches on Unagi, we can successfully improve the call-ring waiting time from 3~5 "doo-doo" sounds to only *one* "doo-doo" sound.

Btw, we need to test this issue by unplugging the USB line and turn-off the screen, which is the worse case of call-ring waiting time.
(In reply to Gene Lian [:gene] [:clian] from comment #24)
> After testing these patches on Unagi, we can successfully improve the
> call-ring waiting time from 3~5 "doo-doo" sounds to only *one* "doo-doo"
> sound.
> 
> Btw, we need to test this issue by unplugging the USB line and turn-off the
> screen, which is the worse case of call-ring waiting time.

After more testings, the following shows the experimental results with my patches:

  - mozilla-central: 4 doo-doos -> 1 doo-doo
  - mozilla-b2g18:   7 doo-doos -> 4 doo-doos

We can indeed have significant improvements with my patches. However, why these 2 branches are different? Eventually, I found out another Bug 812368 (by philikon) also improves some doo-doo, which wasn't checked in into b2g18, though.

With the patches both at bug 812368 and here, we can have 1 doo-doo sound left in the long run. Highly suggesting we should check in bug 812368, too!
Blocks: 812368
No longer blocks: 812368
Depends on: 812368
(In reply to Gene Lian [:gene] [:clian] from comment #25)
> With the patches both at bug 812368 and here, we can have 1 doo-doo sound
> left in the long run. Highly suggesting we should check in bug 812368, too!

Not only the reason for improvements. We have to uplift the patch at bug 812368 for b2g18. My part-3 patch here has conflicts with that. Marking dependency to that.
Attachment #705287 - Flags: review?(vyang) → review+
Attached patch Alternative wakelock (obsolete) — Splinter Review
Hey Gene, I missed your progress on this bug. Great job. I had a WIP patch for acuqiring a wakelock in RadioInterfaceLayer that seems a bit simpler. What do you think?
Comment on attachment 705290 [details] [diff] [review]
Part 3, acquire and release wake locks in ril_worker.js and RadioInterfaceLayer.js

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1227,5 @@
>          break;
>      }
>    },
>  
> +  _timersForCallRingWakeLocks: {},

Please set it to null here and initialize to {} in constructor. We have to enforce this for coming MultiSIM support.

@@ +1231,5 @@
> +  _timersForCallRingWakeLocks: {},
> +  _cancelTimerForCallRingWakeLock: function _cancelTimerForCallRingWakeLock(sysMsgId) {
> +    let timer = this._timersForCallRingWakeLocks[sysMsgId];
> +    if (timer) {
> +      timer.timer.cancel();

timer.timer sounds strange for me :(

@@ +1236,5 @@
> +      delete this._timersForCallRingWakeLocks[sysMsgId];
> +    }
> +
> +    if (Object.keys(this._timersForCallRingWakeLocks).length == 0) {
> +      this.worker.postMessage({rilMessageType: "releaseCallRingWakeLock"});

So we'll acquire wake lock for every call ring in ril_worker but only release it once? That's probably wrong.
Attachment #705290 - Flags: review?(vyang)
Comment on attachment 705285 [details] [diff] [review]
Part 1, provide .getNumPagesToOpen() in system message

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

::: dom/messages/SystemMessageInternal.js
@@ +175,5 @@
> +  getNumPagesToOpen: function getNumPagesToOpen(aType, aPageURI, aManifestURI) {
> +    let pagesToOpen = {};
> +
> +    this._pages.forEach(function(aPage) {
> +      let isURIsValid = pageURI && manifestURI;

pageURI and manifestURI are not yet defined...
Don't you mean aPageURI and aManifestURI ? If so, move isURIsValid outside of the forEach, and rename it to areURIsValid

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +37,5 @@
>     */
>    void registerPage(in DOMString type, in nsIURI pageURI, in nsIURI manifestURI);
> +
> +  /*
> +   * Get the number of pages to be oprned to handle this message type.

nit: s/oprned/opened
Attachment #705285 - Flags: review?(fabrice) → review-
(In reply to Philipp von Weitershausen [:philikon] from comment #27)
> Created attachment 705421 [details] [diff] [review]
> Alternative wakelock
> 
> Hey Gene, I missed your progress on this bug. Great job. I had a WIP patch
> for acuqiring a wakelock in RadioInterfaceLayer that seems a bit simpler.
> What do you think?

Hi Philikon, thanks you for providing this patch. However, I'd have some reasons to defend my solution:

1. If you still remember, we used to have a conclusion at bug 804707, comment #35. The lower place where we grab the wake lock, the less gaps we would have between the wake locks crossing modules. In other words, acquiring/releasing wake locks in ril_worker sounds more ideal than RadioInterfaceLayer, because it's closer to connect to the wake locks in ril_d.

2. Currently, we're using .broadcaseMessage("telephony-new-call") to start up the Dialer app. The Dialer app will turn on the screen, thus taking over the wake lock life cycle, and then we release the wake lock when the call ends. Your solution indeed works for this scenario. However, supposing there is another app also registers this system message? When the call ends, we might release the wake lock too earlier to handle the system messages for other apps. Although the Dialer app must already extend the wake lock life cycle, it's a bit weird to me that different content processes need to depend on the wake lock life cycles from others. Therefore, my solution is to release all the wake locks *only when* all the system messages are properly broadcasted and handled by their apps.

3. It's safer to set an extra timer to release the wake lock to avoid draining battery. I mean what happen if the Dialer app dies for any unknown reasons? We would have no way to release the wake lock. It's not hard to add a timer in your solution, though.

I could be wrong. Please let me know if it doesn't sound reasonable to you. :)
Attachment #705285 - Attachment is obsolete: true
Attachment #705699 - Flags: review?(fabrice)
(In reply to Gene Lian [:gene] [:clian] from comment #30)
> (In reply to Philipp von Weitershausen [:philikon] from comment #27)
> > Created attachment 705421 [details] [diff] [review]
> > Alternative wakelock
> > 
> > Hey Gene, I missed your progress on this bug. Great job. I had a WIP patch
> > for acuqiring a wakelock in RadioInterfaceLayer that seems a bit simpler.
> > What do you think?
> 
> Hi Philikon, thanks you for providing this patch. However, I'd have some
> reasons to defend my solution:

I think we should avoid using libhardware to acquire wake locks directly if possible.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #28)
> > +    if (Object.keys(this._timersForCallRingWakeLocks).length == 0) {
> > +      this.worker.postMessage({rilMessageType: "releaseCallRingWakeLock"});
> 
> So we'll acquire wake lock for every call ring in ril_worker but only
> release it once? That's probably wrong.

I think this is fine. You might try (2 times):

  root@android:/sys/power # echo "vicamo" > wake_lock
  root@android:/sys/power # echo "vicamo" > wake_lock

and then check: 

  root@android:/sys/power # cat wake_lock

You can only see one "vicamo" in the wake_lock. Therefore, the linux kernel only registers *once* in the /sys/power/wake_lock no matter how many incoming calls we have. Afterwards, if you call: 

  root@android:/sys/power # echo "vicamo" > wake_unlock

then "vicamo" will disappear in the /sys/power/wake_lock.
Hi Vicamo and Philikon,

After discussing with Kanru, he highly suggested we shouldn't manage wake locks beyond powerManagerService (comment #32).

Eventually, I provide another RadioInterfaceLayer-only solution, combining Philikon's approach and the advantages of system message mechanism (because of item #2 at comment #30).

After testing, it's still working perfectly by acquiring/releasing wake locks from the RadioInterfaceLayer. ;)
Attachment #705290 - Attachment is obsolete: true
Attachment #705723 - Flags: review?(vyang)
Attachment #705723 - Flags: review?(philipp)
Fix minor typo. Please see comment #34. Thanks!
Attachment #705723 - Attachment is obsolete: true
Attachment #705723 - Flags: review?(vyang)
Attachment #705723 - Flags: review?(philipp)
Attachment #705726 - Flags: review?(vyang)
Attachment #705726 - Flags: review?(philipp)
Attachment #705699 - Flags: review?(fabrice) → review+
(In reply to Gene Lian [:gene] from comment #30)
> 1. If you still remember, we used to have a conclusion at bug 804707,
> comment #35. The lower place where we grab the wake lock, the less gaps we
> would have between the wake locks crossing modules. In other words,
> acquiring/releasing wake locks in ril_worker sounds more ideal than
> RadioInterfaceLayer, because it's closer to connect to the wake locks in
> ril_d.

It seems like comment 34 has made this point obsolete.

> 2. Currently, we're using .broadcaseMessage("telephony-new-call") to start
> up the Dialer app. The Dialer app will turn on the screen, thus taking over
> the wake lock life cycle, and then we release the wake lock when the call
> ends. Your solution indeed works for this scenario. However, supposing there
> is another app also registers this system message? When the call ends, we
> might release the wake lock too earlier to handle the system messages for
> other apps.

That's assuming the call ends so quickly and we release the wakelock so early again that we don't even get to launch the other apps. That's kind of an edge case we don't really need to worry about, I think. In fact, I would go even further than that and say that any app interested in the 'telephony-new-call' system message besides the Dialer should be of secondary concern to us.

> 3. It's safer to set an extra timer to release the wake lock to avoid
> draining battery. I mean what happen if the Dialer app dies for any unknown
> reasons? We would have no way to release the wake lock. It's not hard to add
> a timer in your solution, though.

Yup. I like the timer idea.
Comment on attachment 705726 [details] [diff] [review]
Part 3, acquire and release wake locks in RadioInterfaceLayer.js, V2.1

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +360,5 @@
> +      if (!sysMsgId) {
> +        return;
> +      }
> +      let cpuWakeLock = this._callRingCpuWakeLocks[sysMsgId];
> +      if (cpuWakeLock && --cpuWakeLock.numPages <= 0) {

No inline increments or decrements, please.

That said, this whole "SystemMessageManager:HandleMessageDone" business seems rather excessive. Why not keep a wakelock for ~5 seconds and then release it no matter what. We should really have launched the dialer by then which then takes over in terms of wakelock management. In other words, keep the timer, get rid of "SystemMessageManager:HandleMessageDone".

That way we also don't have to compute a UUID for every system message, and we can just use 1 wakelock and 1 timer. If there's still a timer going on, you can extend its delay by doing

  timer.delay += 5000;

Otherwise you can reuse the timer object by calling `timer.init()` again. By reusing the `timer` and `wakelock` objects, we save expensive XPCOM and cycle collection operations that would happen on every incoming call.
Attachment #705726 - Flags: review?(philipp)
Things are simpler now. ;)
Attachment #705726 - Attachment is obsolete: true
Attachment #705726 - Flags: review?(vyang)
Attachment #705766 - Flags: review?(philipp)
(In reply to Philipp von Weitershausen [:philikon] from comment #37)
> Otherwise you can reuse the timer object by calling `timer.init()` again. By
> reusing the `timer` and `wakelock` objects, we save expensive XPCOM and
> cycle collection operations that would happen on every incoming call.

Currently, the wake-lock mechanism cannot allow us to lock it again through the same instance. It starts locking exactly when the instance is created. Therefore, we cannot reuse the wake-lock instance. Please see [1] for more details. Thanks!

[1] /dom/power/nsIDOMWakeLock.idl
Philikon,

I'm afraid I need to leave office now and probably have no chance to check this until tomorrow morning. If we're rushing to check this in, please go ahead to do so after getting your review+ for this patch. *We only need to land part-3 patch*.

Note: before landing this, we need to land bug 812368 first. Before landing that bug, we need to land bug bug 818623 first.
Both bug 812368 and bug 818623 are landed. ;)
Attachment #705287 - Attachment is obsolete: true
Attachment #705421 - Attachment is obsolete: true
Attachment #705699 - Attachment is obsolete: true
Attachment #705766 - Attachment description: Part 3, acquire and release wake locks in RadioInterfaceLayer.js, V3 → acquire and release wake locks in RadioInterfaceLayer.js, V3
Comment on attachment 705766 [details] [diff] [review]
acquire and release wake locks in RadioInterfaceLayer.js, V3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1243,5 @@
> +        Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +    }
> +    this._callRingWakeLockTimer.
> +      initWithCallback(this._cancelCallRingWakeLockTimer.bind(this),
> +                       5000, Ci.nsITimer.TYPE_ONE_SHOT);

Please const this value at the top of the file, e.g.

  const CALL_WAKELOCK_TIMEOUT = 5000;

Also please align operators like so:

  this._callRingWakeLockTimer
      .initWithCallback(...);

Other than that I have no objections. Great work! r=me

(I was worried for a moment that re-initing a timer might cause unforeseen behaviour, but it turns out, the nsITimer implementation expects this case: https://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp#263)
Attachment #705766 - Flags: review?(philipp) → review+
Addressing comment #42. Thanks for the review!

r=philikon
Attachment #705766 - Attachment is obsolete: true
Attachment #706072 - Flags: review+
Correct the commit message (remove "part 3"). Sorry for the noise.
Attachment #706072 - Attachment is obsolete: true
Attachment #706078 - Flags: review+
Target Milestone: --- → B2G C4 (2jan on)
Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.
Verified fixed on:

Unagi build ID: 20130214070203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e
Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49

4 incoming rings heard before called was missed and call went to voicemial.
Unagi had wifi on and screen in hibernated mode.

Also with headset plugged in all 4 incoming rings were heard at the same time as from the Unagi itself.
Status: RESOLVED → VERIFIED
Kernel was Dec 5.

This was suppose to be part of Comment #48.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: