Closed Bug 943749 Opened 6 years ago Closed 6 years ago

B2G SMS: Support to Read Class 2 SMS received and stored in SIM

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.3 Sprint 6 - 12/6

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(1 file, 3 obsolete files)

1. According to |4. SMS Data Coding Scheme| in 3GPP 23.040 (
http://www.etsi.org/deliver/etsi_ts/123000_123099/123038/10.00.00_60/ts_123038v100000p.pdf)
we know that when a Class 2 SMS comes, the UE has to be stored in SIM when SIM storage of SMS is available. Otherwise, the UE has to report an error accordingly.

2. According to current implementation of the RIL interface in AOSP, UNSOLICITED_RESPONSE_NEW_SMS_ON_SIM is use to indicate that a new Class 2 SMS is received and stored in the specified index of the EF_SMS in SIM.
3. After testing in the real network with Unagi, the class 2 SMS is notified from UNSOLICITED_RESPONSE_NEW_SMS_ON_SIM instead of UNSOLICITED_RESPONSE_NEW_SMS for normal incoming SMS.

Fire this bug to support to 
1. Copy the incoming class 2 SMS in SIM into the MobileMessagingDatabase when it comes by adopting UNSOLICITED_RESPONSE_NEW_SMS_ON_SIM.
2. Behave as normal incoming SMS to allow user to read the copied one from the Message Inbox.
This patch provides the implementation of copying Class 2 SMS received in SIM to the Message box to allow user to read it as normal incoming SMS.
The patch has been verified with NowSMS in the live network and the incoming SMS can be stored/displayed correctly in the message box.

Hi Gene,

I'd like to apply this patch to support Class 2 SMS for B2G.
BTW, this is not related to any 1.3 committed features.
Hence, you can take your time to review it and give me your suggestion when available. :)

Thanks & regards,
Bevis Tseng
Attachment #8339807 - Flags: review?(gene.lian)
Summary: B2G SMS: Support Class 2 SMS received and stored in SIM → B2G SMS: Support to Read Class 2 SMS received and stored in SIM
Comment on attachment 8339807 [details] [diff] [review]
Patch_Part_1: Support Class 2 SMS received and stored in SIM. r=gene

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

We don't have binary SMS in database.
Attachment #8339807 - Flags: review?(gene.lian) → review-
Sorry, my review queue is really full. I'd like to leave this one to Vicamo. Thanks!
Hi Vicamo,

As we discussed, I'd like to update my desciption in comment#1 to prevent confusing of storing binary SMS into Database:

Fire this bug to support to 
1. Copy and Decode the incoming class 2 SMS in SIM to text message with same flow of normal incoming SMS into the MobileMessagingDatabase when it comes by adopting UNSOLICITED_RESPONSE_NEW_SMS_ON_SIM.
2. Behave as normal incoming SMS to allow user to read the copied one from the Message Inbox.

Thanks & regards,
Bevis Tseng
Attachment #8339807 - Flags: review?(vyang)
Comment on attachment 8339807 [details] [diff] [review]
Patch_Part_1: Support Class 2 SMS received and stored in SIM. r=gene

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

r=me, thank you.

::: dom/system/gonk/ril_worker.js
@@ +6210,5 @@
> +    function onerror(errorMsg) {
> +      if (DEBUG) {
> +        debug("Failed to Read NEW SMS on SIM #" + recordNumber + ", errorMsg: " + errorMsg);
> +      }
> +    }.bind(this));

nit: |this| pointer is not referenced throughout the callback function.

@@ +12366,5 @@
> +    }
> +
> +    ICCIOHelper.loadLinearFixedEF({fileId: ICC_EF_SMS,
> +                                   recordNumber: recordNumber,
> +                                   callback: callback.bind(this),

ditto
Attachment #8339807 - Flags: review?(vyang) → review+
Update patch to
1. update correct reviewer to vyang.
2. remove unnecessary .bind(this).
Attachment #8339807 - Attachment is obsolete: true
There are some conflicts. Could you please fix it?

-      this.workerMessenger.send("ackSMS", {
-        result: (success ? RIL.PDU_FCS_OK
-                         : RIL.PDU_FCS_MEMORY_CAPACITY_EXCEEDED)
-      });
+      // Note: Ack has been done by modem for NEW_SMS_ON_SIM
+      if (message.simStatus === undefined) {
+        this.workerMessenger.send("ackSMS", {
+          result: (success ? RIL.PDU_FCS_OK
+                           : RIL.PDU_FCS_MEMORY_CAPACITY_EXCEEDED)
+        });
+      }
Keywords: checkin-needed
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Oops. 
Sorry, I should re-base it and upload the patch again after finishing the try server.
Update re-based patch and the corresponding try server result as followed:
https://tbpl.mozilla.org/?tree=Try&rev=a20850344e62
the corresponding try server result as followed:
https://tbpl.mozilla.org/?tree=Try&rev=a20850344e62
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae261f3c5429
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Somehow there is a rebase error. Bug 935401 got landed before this and it moved some functions, including |ICCRecordHelper.readSMS|, to |SimRecordHelper|.  This patch is never working because it's still looking for |ICCRecordHelper.readSMS| even now.  Will file a follow-up instead.
Link related bugs up.
Blocks: 949318
You need to log in before you can comment on or make changes to this bug.