1132363 - [STK] IDLE_SCREEN_AVAILABLE should be removed once the event happened.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sean Lee [:seanlee][:weilonge]]

Based on bug 1119640 comment 12, this bug is for discussing the implementation of IDLE_SCREEN_AVAILABLE in icc_events.js .

Quoted from ETSI TS 102 223
 - 7.5.6 Idle screen available event
 - 7.5.6.1 Procedure
"As a result of sending this command to the SIM, the ME shall remove the idle screen available event from its current event list. This is in order for the ME to report this event only once after the event has been requested bythe SIM."

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Hi Jinghua,

Thanks for your information, and I agree with you to implement the current design to this:
"IDLE_SCREEN_AVAILABLE should be removed once the event happened."


About the function "register" in icc_events.js, do you think it's the correct design to remove the event handler if there is no IDLE_SCREEN_AVAILABLE event in the event list?
https://github.com/mozilla-b2g/gaia/blob/v2.1/apps/system/js/icc_events.js#L187

Thank you.

Reference:
[1] ETSI TS 102 223 - 7.5.6 Idle screen available event - 7.5.6.1 Procedure

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Per bug 1119640 comment 13, I think this is caused by another issue (bug 1126691) and we've fixed in master branch.

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Per bug 1119640 comment 13, I think this is caused by another issue (bug 1126691) and we've fixed in master branch. The patch is in applying v2.1 process.

Comment 4

[jinghua.xing]

(In reply to Sean Lee [:seanlee] from comment #1)
> About the function "register" in icc_events.js, do you think it's the
> correct design to remove the event handler if there is no
> IDLE_SCREEN_AVAILABLE event in the event list?

I think it is correct to remove the event handler if there is no IDLE_SCREEN_AVAILABLE event in the event list. Because in the protocol, it says "Any subsequent SET UP EVENT LIST command replaces the current list of events supplied in the previous SET UP EVENT LIST command. The SET UP EVENT LIST command can also be used to remove the entire list of events current in the ME"

But I think for all kind of event, the handler should be removed if there is no this event in the event list. Not only the three kind of event below
STK_EVENT_TYPE_LOCATION_STATUS
STK_EVENT_TYPE_IDLE_SCREEN_AVAILABLE
STK_EVENT_TYPE_BROWSER_TERMINATION

What do you think about this?

Reference:
[1] ETSI TS 101 267 - 6.4.16 SET UP EVENT LIST

Comment 5

[jinghua.xing]

Besides, I think we should not use the 'lockscreen-appopened' event as the IDLE_SCREEN_AVAILABLE event as what said in the protocol: "When the ME next enters a state where it would accept rather than reject a DISPLAY TEXT command of normal priority, the ME shall inform the SIM that this has occurred, by using the ENVELOPE (EVENT DOWNLOAD - idle screen available) command as defined below."

Reference:
[1] ETSI TS 101 267 - 11.6 Idle screen available event - 11.6.1 Procedure

What do you think about this? Thank you.

Comment 6

[Sean Lee [:seanlee][:weilonge]]

(In reply to jinghua.xing from comment #4)
> I think it is correct to remove the event handler if there is no
> IDLE_SCREEN_AVAILABLE event in the event list. Because in the protocol, it
> says "Any subsequent SET UP EVENT LIST command replaces the current list of
> events supplied in the previous SET UP EVENT LIST command. The SET UP EVENT
> LIST command can also be used to remove the entire list of events current in
> the ME"
> 
> But I think for all kind of event, the handler should be removed if there is
> no this event in the event list. Not only the three kind of event below
> STK_EVENT_TYPE_LOCATION_STATUS
> STK_EVENT_TYPE_IDLE_SCREEN_AVAILABLE
> STK_EVENT_TYPE_BROWSER_TERMINATION
> 
> What do you think about this?

Agree with you.
This implementation is ready at master branch, and it is the process to land to v2.1 branch.
If you want to take a look the patch, you can see them at bug 1126691 comment 32 or this link:
https://github.com/mozilla-b2g/gaia/commit/feb5c58c93d440791e81b5f0c4ee834434f381da

> 
> Reference:
> [1] ETSI TS 101 267 - 6.4.16 SET UP EVENT LIST

About the idle screen behavior, I will add the line "navigator.removeIdleObserver(idleObserverObject);" back to follow "IDLE_SCREEN_AVAILABLE should be removed once the event happened.".

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Per your comment 5, I think this is the same issue that we ever discussed at bug 1128793 .
Is it acceptable if we listen 'homescreenopened' rather than 'lockscreen-appopened' to fix it?

Thank you.

Comment 8

[jinghua.xing]

(In reply to Sean Lee [:seanlee] from comment #7)
> Per your comment 5, I think this is the same issue that we ever discussed at
> bug 1128793 .
> Is it acceptable if we listen 'homescreenopened' rather than
> 'lockscreen-appopened' to fix it?
> 
> Thank you.

I have tried to listen 'homescreenopened' rather than 'lockscreen-appopened', the USAT case 27.22.7.6.1 IDLE SCREEN AVAILABLE EVENT SEQ 1.1 passed and I think it can fix the issue.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

I will fix the two parts in this issue after the patch in bug 1126691 landed to v2.1:
1. listen 'homescreenopened' instead of 'lockscreen-appopened'
2. IDLE_SCREEN_AVAILABLE should be removed once the event happened.

Thank you.

Comment 11

[jinghua.xing]

(In reply to Sean Lee [:seanlee] from comment #6)
> About the idle screen behavior, I will add the line
> "navigator.removeIdleObserver(idleObserverObject);" back to follow
> "IDLE_SCREEN_AVAILABLE should be removed once the event happened.".

The line "navigator.removeIdleObserver(idleObserverObject);" is used to remove the listener of STK_EVENT_TYPE_USER_ACTIVITY, it also need to be removed once the event happened and the code is necessary. 

But I think for STK_EVENT_TYPE_IDLE_SCREEN_AVAILABLE, we should add some code like 
>window.removeEventListener('lockscreen-appopened',
> register_icc_event_idlescreen);
>});" 

What do you think about this? Thank you.

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Attachment: Trail Patch for master

Hi Jinghua,

This is a trail patch for you to verify it.
BTW, this modification is based on master branch for your reference.

You can see full icc_events.js in this link:
https://github.com/weilonge/gaia/blob/seanlee/STK/master/Bug1132363/apps/system/js/icc_events.js

Thank you.

Comment 14

[jinghua.xing]

Sean:

I have tried the patch you provided. It is OK with the IDLE_SCREEN_AVAILABLE, but for the STK_EVENT_TYPE_USER_ACTIVITY event, it also need to remove the handler once the event happened as the protocol describes.

Comment 15

[Sean Lee [:seanlee][:weilonge]]

Attachment: Trail Patch for master

Hi Jinghua,

I made some changes based on your comment 14 to remove User Activity's handler once it is triggered.

Thank you.

Comment 16

[jinghua.xing]

Sean：

On the newest version of v2.1，there is a new bug for User Activity case: USAT case 27.22.7.5.1 USER ACTIVITY EVENT SEQ 1.1

There is always an error when we power on the device and run the case above:

File:  simat_envelope.c
Line:  1428
ASSERT(NULL != psig)

What do you think about this? Thank you.

Comment 17

[Sean Lee [:seanlee][:weilonge]]

Hi Jinghua,

Does this error only happen on the newest one?

The previous one:
https://github.com/weilonge/gaia/commit/44f4352ddb51a1e9c6a134b1653cb811adbbb8c0

The latest one:
https://github.com/weilonge/gaia/commit/4937da1e9053167009dbf06fe76631e0ee40f61f

After comparing these two, there is only 3-line code added to the latest one:
+ if (this.stkUserActivity) {
+ navigator.removeIdleObserver(this.stkUserActivity);
+ }

I think it's not possible to execute the above code at power on.

Comment 18

[jinghua.xing]

No，this bug is based on the v2.1 branch and the build id is 20150212135311

Comment 19

[Sean Lee [:seanlee][:weilonge]]

Hi Jinghua,

Due to v2.1 approval process, I am waiting for landing the patch at bug 1126691 to v2.1 .
Because this patch depends bug 1126691's one, I will land the code as soon once the approval process is done.

If you need the patch to verify the result, you can use the following patch and apply to v2.1 even they are for master branch. 

1st https://github.com/mozilla-b2g/gaia/commit/feb5c58c93d440791e81b5f0c4ee834434f381da
2nd https://github.com/weilonge/gaia/commit/4937da1e9053167009dbf06fe76631e0ee40f61f

Thank you.

Comment 20

[jinghua.xing]

Sean：

Can you also help us to uplift this patch and the patch at bug1126691 to v2.1s branch?

Thank you.

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Hi Jinghua,

I will uplift these patches to v2.1s certainly.
Before uplifting, I should go the review process for this patch.

Comment 22

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/STK/master/Bug1132363 > mozilla-b2g:master

Comment 23

[Sean Lee [:seanlee][:weilonge]]

Attachment: PR for master

Hi Fernando,

Could you help to review my PR?
Thank you.

Comment 24

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8565350 [details] [review]
PR for master

LGTM r+

Thank you !

Comment 25

[Sean Lee [:seanlee][:weilonge]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/763b18ce17f5dbd3b1f5c52102ed348267f726a1
      try-server: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=d383aa1755c2

Comment 26

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8565350 [details] [review]
PR for master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
SETUP event of STK feature did not follow these spec:
[1] ETSI TS 102 223 - 7.5.6 Idle screen available event - 7.5.6.1 Procedure
[2] ETSI TS 101 267 - 11.6 Idle screen available event - 11.6.1 Procedure

[Testing completed]:
1. Partner tested PASS
2. Unit test PASS

[Risk to taking this patch] (and alternatives if risky):
The modification is only for STK, and the risk is very minor.

[String changes made]:
None

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/44ea3cbd9a576640b75fbb9618e9236c6ef13619
v2.1: https://github.com/mozilla-b2g/gaia/commit/1eed1e82a0b1166adb3fd78916e9d9c6e54f53a9

Comment 28

[jinghua.xing]

Sean：

I also have one question about the STK_CMD_SET_UP_EVENT_LIST command. I see you have removed the code to send terminal response back to the sim when receive the command in icc_worker.js in the patch: Bug 1088611 - Fix SET UP EVENT for mt-call, connected and disconnected.

Why you remove the code to send the TR and where is the code to send it back to the icc now? 

And the protocol says "When the ME has successfully accepted or removed the list of events, it shall send TERMINAL RESPONSE (OK) to the SIM. "  So I think it is necessary to send a TR back. Thank you.

Reference:
[1] ETSI TS 101 267 - 6.4.16 SET UP EVENT LIST

Comment 29

[Sean Lee [:seanlee][:weilonge]]

Hi Jinghua,

I think the cause of removing TR for SETUP EVENT is by different modem design.
The TR for SETUP EVENT is handled by MODEM, so we removed it.

IMHO, we really need a table to summarize which TerminalResponse/EventEnvelope will be sent by MODEM or Gaia.

For example, the event of CONNECTED, DISCONNECTED, MT_CALL will be handled by MODEM, so we removed the code to send event envelope.
https://github.com/weilonge/gaia/blob/v2.0m/apps/system/js/icc_events.js#L63

Comment 30

[jinghua.xing]

Sean：

But for sprd, the TR for SETUP EVENT cannot be sent by modem, it should be sent by gaia. So can you add the code to send TR in icc_worker.js back on v2.1s branch?

Thank you.

Comment 31

[Sean Lee [:seanlee][:weilonge]]

Hi Jinghua,

No problem.
I've fired a new bug 1137026 to do the change for v2.1s.
Let's discuss this issue at bug 1137026.

Thank you.