Closed Bug 1106949 Opened 6 years ago Closed 3 years ago

B2G RIL: [STK] Expose SIM refresh type to Gaia.

Categories

(Firefox OS Graveyard :: RIL, defect, P2)

defect

Tracking

(b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 affected, b2g-v2.2 affected)

RESOLVED WONTFIX
Tracking Status
b2g-v2.0 --- affected
b2g-v2.0M --- affected
b2g-v2.1 --- affected
b2g-v2.2 --- affected

People

(Reporter: sku, Assigned: sku)

References

Details

Attachments

(4 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1106449 +++

Based on 3GPP TS 11.14, 6.4.22 SET UP IDLE MODE TEXT.
Gaia needs to know which refresh type of SIM refresh, then determine which behavior to act.

However, Gecko did not export refresh type attribute to Gaia (see [1]) currently.
We need to make this part up.


[1] - http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#
Assignee: nobody → sku
Blocks: b2g-stk
blocking-b2g: --- → 2.0M?
Attachment #8531859 - Flags: review?(echen)
Attachment #8531860 - Flags: review?(htsai)
Attachment #8531860 - Flags: review?(echen)
Attachment #8531862 - Flags: review?(echen)
Attachment #8531914 - Flags: review?(echen)
More description on why we need expose refresh type to Gaia.

3GPP TS 11.14
6.4.22 SET UP IDLE MODE TEXT
...
The text shall be removed from the ME's memory and display if either: 
- the ME is powered off or;
- the SIM is removed or electrically reset or;
- a REFRESH command occurs with "initialisation" or "reset".
Comment on attachment 8531914 [details] [diff] [review]
Bug 1106949 Part4: test patch - B2G RIL: [STK] Expose SIM refresh type to Gaia.

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

also need some revision on http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/tests/test_ril_system_messenger.js

cancel review request for this part first.
Attachment #8531914 - Flags: review?(echen)
Attachment #8532202 - Flags: review?(echen)
Comment on attachment 8531860 [details] [diff] [review]
Bug 1106949 Part2: webIDL patch - B2G RIL: [STK] Expose SIM refresh type to Gaia.

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

::: dom/webidl/MozStkCommandEvent.webidl
@@ +606,5 @@
> +{
> +  /**
> +   * Indicate the STK refresh type.
> +   * It shall be one of following:
> +   * - nsIDOMMozIccManager.STK_REFRESH_NAA_INIT_AND_FULL_FILE_CHANGE

We already move Icc Manager to WEBIDL, please help to revise the comments here to 
MozIccManager.STK_REFRESH_*.
Thank you.
Attachment #8531860 - Flags: review?(echen) → review+
(In reply to shawn ku [:sku] from comment #6)
> More description on why we need expose refresh type to Gaia.
> 
> 3GPP TS 11.14
> 6.4.22 SET UP IDLE MODE TEXT
> ...
> The text shall be removed from the ME's memory and display if either: 
> - the ME is powered off or;
> - the SIM is removed or electrically reset or;
> - a REFRESH command occurs with "initialisation" or "reset".
Shawn, I think this can be done by ril_worker.js inside processRefresh function without having to expose the refresh type all the way up to Gaia. Couldn't ril_worker not handle REFRESH-FCN case by itself?
(In reply to Anshul from comment #10)
> (In reply to shawn ku [:sku] from comment #6)
> > More description on why we need expose refresh type to Gaia.
> > 
> > 3GPP TS 11.14
> > 6.4.22 SET UP IDLE MODE TEXT
> > ...
> > The text shall be removed from the ME's memory and display if either: 
> > - the ME is powered off or;
> > - the SIM is removed or electrically reset or;
> > - a REFRESH command occurs with "initialisation" or "reset".
> Shawn, I think this can be done by ril_worker.js inside processRefresh
> function without having to expose the refresh type all the way up to Gaia.
> Couldn't ril_worker not handle REFRESH-FCN case by itself?

Hi Anshul:
Gecko handled REFRESH-FCN already, however, gaia needs this type to determine if SET UP IDLE MODE TEXT needs to be dismissed or not (see comment 6).

IDLE MODE Text is displayed by gaia, and there is no way for gecko to closed the UI.

Please let me know if anything wrong above.
Thanks!!
Shawn
Shawn, seems like Gaia currently always removes the idleTextNotifications when it receives the Stk cmd refresh. As part of new notification API (bug 782211) support was added to remove the previous notification if an empty notification for the same SIM is sent, which is being used in bug 873397. Can we modify icc_worker.js code to look for an empty text to know it has to remove the idle mode text or else don't clear the text.
Hi Shawn,
2.0M+, thanks!
blocking-b2g: 2.0M? → 2.0M+
Flags: needinfo?(sku)
(In reply to Anshul from comment #12)
> Shawn, seems like Gaia currently always removes the idleTextNotifications
> when it receives the Stk cmd refresh. As part of new notification API (bug
> 782211) support was added to remove the previous notification if an empty
> notification for the same SIM is sent, which is being used in bug 873397.
> Can we modify icc_worker.js code to look for an empty text to know it has to
> remove the idle mode text or else don't clear the text.

Hi Anshul:
 Do you mean ril_worker.js to send up an empty SET UP IDLE MODE TEXT if gaia needs to remove the text,
and don't send REFRESH command up?

It's too implicit for code reading.

Besides, gaia will trigger TR (Terminal Response) with OK to gecko which has been removed (Bug 1091504) due to AOSP in-comparability. 

Even we can do the things based on your information (bug 873397).
However, it might be better to follow the 3GPP spec. to complete SET UP IDLE MODE TEXT feature per my understanding.

Please let me know if any further concerns you may have.
Thanks!!
Shawn
Flags: needinfo?(anshulj)
IMHO, removing idle text by sending an empty text again seems more look like a workaround solution to me. And another concern is I don't know if this "empty text" will remove other notification that we aren't expecting to remove. So I prefer to provide clearer information to gaia and let gaia handle this.
(In reply to Josh Cheng [:josh] from comment #13)
> Hi Shawn,
> 2.0M+, thanks!

Hi Josh:
 Let get things done on bug 1106449, since partner still have concerns on 1106949.
There might need some discussion back and forth. Might not be able to catch up the schedule.

De-nominate this 2.0m+ here.
Flags: needinfo?(sku)
blocking-b2g: 2.0M+ → ---
See Also: → 1106449
(In reply to Edgar Chen [:edgar][:echen] from comment #15)
> IMHO, removing idle text by sending an empty text again seems more look like
> a workaround solution to me. And another concern is I don't know if this
> "empty text" will remove other notification that we aren't expecting to
> remove. So I prefer to provide clearer information to gaia and let gaia
> handle this.

Edgar and Shawn, what I was proposing is what is done by AOSP code. I now understand that you were aiming for a clear API and I am fine with your approach. My concern was if exposing refresh type too low level for Gaia to know about.
Flags: needinfo?(anshulj)
(In reply to Anshul from comment #17)
> (In reply to Edgar Chen [:edgar][:echen] from comment #15)
> > IMHO, removing idle text by sending an empty text again seems more look like
> > a workaround solution to me. And another concern is I don't know if this
> > "empty text" will remove other notification that we aren't expecting to
> > remove. So I prefer to provide clearer information to gaia and let gaia
> > handle this.
> 
> Edgar and Shawn, what I was proposing is what is done by AOSP code. I now
> understand that you were aiming for a clear API and I am fine with your
> approach. My concern was if exposing refresh type too low level for Gaia to
> know about.

But from [1], it more looks like AOSP exposes refresh type to APP and let APP handle clearing notification, no?
Please let me know if I misunderstand anything, thank you.

[1] http://androidxref.com/5.0.0_r2/xref/packages/apps/Stk/src/com/android/stk/StkAppService.java#350
(In reply to Edgar Chen [:edgar][:echen] from comment #18)
> (In reply to Anshul from comment #17)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #15)
> > > IMHO, removing idle text by sending an empty text again seems more look like
> > > a workaround solution to me. And another concern is I don't know if this
> > > "empty text" will remove other notification that we aren't expecting to
> > > remove. So I prefer to provide clearer information to gaia and let gaia
> > > handle this.
> > 
> > Edgar and Shawn, what I was proposing is what is done by AOSP code. I now
> > understand that you were aiming for a clear API and I am fine with your
> > approach. My concern was if exposing refresh type too low level for Gaia to
> > know about.
> 
> But from [1], it more looks like AOSP exposes refresh type to APP and let
> APP handle clearing notification, no?
> Please let me know if I misunderstand anything, thank you.
> 
> [1]
> http://androidxref.com/5.0.0_r2/xref/packages/apps/Stk/src/com/android/stk/
> StkAppService.java#350

Hi all,
Actually my first thought is the same as Anshul's, i.e. exposing refresh type seems too low level, but I can't think of a better solution without exposing refresh type. :( 

Another concern is as we know work of STK_REFRESH is almost done on modem and gaia shouldn't send terminal response, we should have not exposed even STK_REFRESH command to gaia IMO. Therefore, with the proposal of having more information along with STK_REFRESH command, I am concerned we seem introduce an API more confusing, because in some case, STK_REFRESH would matter to gaia, but in others it wouldn't. Android's solution [1] releases Anshul's and my concern 1. However, my concern 2 remains with both [1] or Shawn's proposal here. It looks to me that [1] tries to hide something but not cleanly enough. 

Then my 3rd concern: 
Don't we need to handle UNSOLICITED_SIM_REFRESH in addition to STK_REFRESH? Anyone has an idea about the relation b/w STK_REFRESH and UNSOLICITED_SIM_REFRESH, e.g. are they exclusive or are they totally duplicate? Per my understanding of the spec, idle mode text should be removed within coverage of both of these commands, right? 

So, considering all above, it makes more sense to me that the solution will NOT expose STK_REFRESH command to gaia at all, but the solution will use a new way to notify gaia something needs to be done, e.g. icc.onrefresh event. How do you think about this? 

[1] http://androidxref.com/5.0.0_r2/xref/frameworks/opt/telephony/src/java/com/android/internal/telephony/cat/CommandParamsFactory.java#522
[2] http://androidxref.com/5.0.0_r2/xref/packages/apps/Stk/src/com/android/stk/StkAppService.java#349
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #19)
> So, considering all above, it makes more sense to me that the solution will
> NOT expose STK_REFRESH command to gaia at all, but the solution will use a
> new way to notify gaia something needs to be done, e.g. icc.onrefresh event.
> How do you think about this? 

Total agreed with you, notifying gaia with a different event, e.g. icc.onrefresh, is much better than exposing STK_REFRESH command directly.
Comment on attachment 8531859 [details] [diff] [review]
Bug 1106949 Part1: ICC patch - B2G RIL: [STK] Expose SIM refresh type to Gaia.

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

It seems we need to have more discussion about this, so I would like to cancel the review request first? Thank you.
Attachment #8531859 - Flags: review?(echen)
Attachment #8531860 - Flags: review?(htsai)
Attachment #8531862 - Flags: review?(echen)
Attachment #8532202 - Flags: review?(echen)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #19)
> (In reply to Edgar Chen [:edgar][:echen] from comment #18)
> > (In reply to Anshul from comment #17)
> > > (In reply to Edgar Chen [:edgar][:echen] from comment #15)
> > > > IMHO, removing idle text by sending an empty text again seems more look like
> > > > a workaround solution to me. And another concern is I don't know if this
> > > > "empty text" will remove other notification that we aren't expecting to
> > > > remove. So I prefer to provide clearer information to gaia and let gaia
> > > > handle this.
> > > 
> > > Edgar and Shawn, what I was proposing is what is done by AOSP code. I now
> > > understand that you were aiming for a clear API and I am fine with your
> > > approach. My concern was if exposing refresh type too low level for Gaia to
> > > know about.
> > 
> > But from [1], it more looks like AOSP exposes refresh type to APP and let
> > APP handle clearing notification, no?
> > Please let me know if I misunderstand anything, thank you.
> > 
> > [1]
> > http://androidxref.com/5.0.0_r2/xref/packages/apps/Stk/src/com/android/stk/
> > StkAppService.java#350
> 
> Hi all,
> Actually my first thought is the same as Anshul's, i.e. exposing refresh
> type seems too low level, but I can't think of a better solution without
> exposing refresh type. :( 
> 
> Another concern is as we know work of STK_REFRESH is almost done on modem
> and gaia shouldn't send terminal response, we should have not exposed even
> STK_REFRESH command to gaia IMO. Therefore, with the proposal of having more
> information along with STK_REFRESH command, I am concerned we seem introduce
> an API more confusing, because in some case, STK_REFRESH would matter to
> gaia, but in others it wouldn't. Android's solution [1] releases Anshul's
> and my concern 1. However, my concern 2 remains with both [1] or Shawn's
> proposal here. It looks to me that [1] tries to hide something but not
> cleanly enough. 
> 
> Then my 3rd concern: 
> Don't we need to handle UNSOLICITED_SIM_REFRESH in addition to STK_REFRESH?
> Anyone has an idea about the relation b/w STK_REFRESH and
> UNSOLICITED_SIM_REFRESH, e.g. are they exclusive or are they totally
> duplicate? Per my understanding of the spec, idle mode text should be
> removed within coverage of both of these commands, right? 
> 
> So, considering all above, it makes more sense to me that the solution will
> NOT expose STK_REFRESH command to gaia at all, but the solution will use a
> new way to notify gaia something needs to be done, e.g. icc.onrefresh event.
> How do you think about this? 
> 
> [1]
> http://androidxref.com/5.0.0_r2/xref/frameworks/opt/telephony/src/java/com/
> android/internal/telephony/cat/CommandParamsFactory.java#522
> [2]
> http://androidxref.com/5.0.0_r2/xref/packages/apps/Stk/src/com/android/stk/
> StkAppService.java#349

Then my 3rd concern: 
Don't we need to handle UNSOLICITED_SIM_REFRESH in addition to STK_REFRESH? Anyone has an idea about the relation b/w STK_REFRESH and UNSOLICITED_SIM_REFRESH, e.g. are they exclusive or are they totally duplicate? 

[Shawn] Those two are totally duplicated.

Per my understanding of the spec, idle mode text should be removed within coverage of both of these commands, right? 

[Shawn] Yes. Idle mode text should be removed by following condition.
(BTW, the way AOSP handling refresh seems pretty hacky to me.)


3GPP TS 11.14, 6.4.22 SET UP IDLE MODE TEXT
...
The text shall be removed from the ME's memory and display if either: 
- the ME is powered off or; 
- the SIM is removed or electrically reset or; 
- a REFRESH command occurs with "initialisation" or "reset".
Hi HsinYi,
Could you help to check comment 22 from Shawn? DO we still want to fix this issue?
Thanks!
Flags: needinfo?(htsai)
(In reply to Josh Cheng [:josh] from comment #23)
> Hi HsinYi,
> Could you help to check comment 22 from Shawn? DO we still want to fix this
> issue?
> Thanks!

Yes, this bug is still valid.
And thanks to Shawn's comment 22, I think the proposal in comment 20 looks good to go.
Flags: needinfo?(htsai)
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.