Closed Bug 1003645 Opened 6 years ago Closed 5 years ago

[Tarako][FOTA] Need a setting for update from /sdcard

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

RESOLVED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: lianxiang.zhou, Assigned: shiwei.zhang)

Details

(Whiteboard: [tarako_only][partner-blocker] [POVB])

Attachments

(3 files, 1 obsolete file)

We support update from sdcard only. We need a command file for recovery after we copied the update.zip file to sdcard. If we want to update without adb tool, we need a application to set the command and reboot device. The application like "Settings->Device information->More Information->Reset Phone". The difference is only recovery command. We need set the command looks like "--update_package=/sdcard/update.zip".
Summary: [Tarako] Need a setting for update from /sdcard → [Tarako][FOTA] Need a setting for update from /sdcard
Flags: needinfo?(james.zhang)
We need sdcard OTA menu for QA and user.
blocking-b2g: --- → 1.3T?
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
Flags: needinfo?(james.zhang)
Flags: needinfo?(ehung)
Loop UX for the menu design.
Flags: needinfo?(ehung) → needinfo?(mtsai)
My understanding is that we already fallback to using the sdcard when there is not enough space in /data to process the update. See https://mxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#176

ni on Naoki that started some testing around this, and Dave in case I said something technically incorrect.
Flags: needinfo?(nhirata.bugzilla)
Flags: needinfo?(dhylands)
For update from sdcard, I think it could be better to have a hard key (key combination) to boot the device into recovery ROM. Then user can select menu recovery ROM to finish the update or provide an auto check and update mechanism when device is booted into recovery ROM.
(In reply to Fabrice Desré [:fabrice] from comment #3)
> My understanding is that we already fallback to using the sdcard when there
> is not enough space in /data to process the update. See
> https://mxr.mozilla.org/mozilla-central/source/b2g/components/
> DirectoryProvider.js#176
> 
> ni on Naoki that started some testing around this, and Dave in case I said
> something technically incorrect.

Fabrice, I verified when gecko update is compiled as FOTA, when available space in /data is not enough, it is possible to download the update.mar into /sdcard and reboot into recovery to finish the update.
(In reply to Fabrice Desré [:fabrice] from comment #3)
> My understanding is that we already fallback to using the sdcard when there
> is not enough space in /data to process the update. See
> https://mxr.mozilla.org/mozilla-central/source/b2g/components/
> DirectoryProvider.js#176
> 
> ni on Naoki that started some testing around this, and Dave in case I said
> something technically incorrect.

Yep - that sounds correct.

Looking at the code:
https://mxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#147
it checks the sdcard first, and then checks LOCAL_DIR (/data/local).
Flags: needinfo?(dhylands)
(In reply to Kai-Zhen Li from comment #4)
> For update from sdcard, I think it could be better to have a hard key (key
> combination) to boot the device into recovery ROM. Then user can select menu
> recovery ROM to finish the update or provide an auto check and update
> mechanism when device is booted into recovery ROM.

With our other phones, the updater reboots into the recovery ROM without the user having to do anything special. I believe that this is done by librecovery.so
Loop UX Omega for menu design.
Flags: needinfo?(mtsai)
Flags: needinfo?(kkuo)
I think this is a new device-specific feature, put it into backlog.
blocking-b2g: 1.3T? → backlog
Flags: needinfo?(styang)
dhylands, a gecko/gaia flash will not flash the librecovery.so, as that's part of the /system/lib.  You would need to do a full flash like you would do on the Tarako, Flame, Nexus-4, and Geeksphones.  This would also mean that if a boot.img is flashed, the boot.img is reliable... and those images for Tarako and Flame aren't distributable.

So for other devices such as the hamachi, to get the proper librecovery.so people would have to get it from somewhere (ie compile for themselves or get it from someone else that has compiled).

I suppose I could file a releng bug to pull the librecovery.so for the b2g portion and edit the flash script to flash the librecovery.so file as well.  Would that make sense?
Flags: needinfo?(dhylands)
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #10)
> dhylands, a gecko/gaia flash will not flash the librecovery.so, as that's
> part of the /system/lib.  You would need to do a full flash like you would
> do on the Tarako, Flame, Nexus-4, and Geeksphones.  This would also mean
> that if a boot.img is flashed, the boot.img is reliable... and those images
> for Tarako and Flame aren't distributable.
> 
> So for other devices such as the hamachi, to get the proper librecovery.so
> people would have to get it from somewhere (ie compile for themselves or get
> it from someone else that has compiled).
> 
> I suppose I could file a releng bug to pull the librecovery.so for the b2g
> portion and edit the flash script to flash the librecovery.so file as well. 
> Would that make sense?

I think so. FOTA updates don't work properly without the correct librecovery.so file, and if the vendor isn't providing the correct one (this is the case with the hamachi) then we should be flashing it.
Flags: needinfo?(dhylands)
For OTA, looks like if there's space on the sdcard and not the data, then it will try to use the sdcard.  If there's space on the data and not the sdcard then it will use the data partition.

If there's no space on either : bug 1004749

For FOTA: bug 1001818 when there's no space for sdcard
Flags: needinfo?(nhirata.bugzilla)
(In reply to Steven Yang [:styang] from comment #9)
> I think this is a new device-specific feature, put it into backlog.

How can our customer update from sdcard? ADB is disable on user build.
Our customer don't have FOTA server and they need sdcard OTA only, the same as android.
blocking-b2g: backlog → 1.3T?
(In reply to James Zhang from comment #14)
> (In reply to Steven Yang [:styang] from comment #9)
> > I think this is a new device-specific feature, put it into backlog.
> 
> How can our customer update from sdcard? ADB is disable on user build.
> Our customer don't have FOTA server and they need sdcard OTA only, the same
> as android.

The requirement is from Indian open market.
James, you mean that you want people to get a sdcard with the update preloaded and that we need to pick up this update?
James, can your team take this and make this work in your own branch?
ni? Steven as well
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(james.zhang)
(In reply to Joe Cheng [:jcheng] from comment #17)
> James, can your team take this and make this work in your own branch?
> ni? Steven as well

I think [tarako_only] on v1.3t is better solution.
I have talked it with Steven.
Flags: needinfo?(james.zhang)
Whiteboard: [tarako_only]
(In reply to Fabrice Desré [:fabrice] from comment #16)
> James, you mean that you want people to get a sdcard with the update
> preloaded and that we need to pick up this update?

Yes, our customer use this method on android platform, put update.zip to sdcard and enter sdcard OTA menu to upgrade system.
Whiteboard: [tarako_only] → [tarako_only][partner-blocker]
hi James, this is very partner specific, please have your team to take this bug. Thanks
Flags: needinfo?(james.zhang)
Loop Arvin.
Assignee: nobody → arvin.zhang
Flags: needinfo?(james.zhang)
blocking-b2g: 1.3T? → 1.3T+
shiwei, librecovery patch please.
Assignee: arvin.zhang → shiwei.zhang
Flags: needinfo?(kli)
Whiteboard: [tarako_only][partner-blocker] → [tarako_only][partner-blocker] [POVB]
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I thought I would point that this is where we reboot into recovery to apply update.zip with the existing FOTA code:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js?from=nsUpdateService.js#3162
Dave, Thanks! 
Now as I know partner'd like to support update the phone via sdcard but not really OTA. After user save update.zip in sdcard and there is an option in setting allow user to finish the update.
They plan to share the same mechanism as reset phone which reboot the device into recovery and finish the update.
Flags: needinfo?(kli)
This patch allows Settings->Device infomation->More Infomation->Reset Phone do OTA if there is update.zip on SD card, or just reset the phone.
Comment on attachment 8422166 [details] [diff] [review]
librecovery_patch

>diff --git a/librecovery.c b/librecovery.c
>index 4949a1e..5933597 100644
>--- a/librecovery.c
>+++ b/librecovery.c
>@@ -143,7 +143,19 @@ int
> factoryReset()
> {
>   // In AOSP's recovery image, "--wipe_data" is the synonym for factory reset
>-  return execRecoveryCommand((char *) kWipeData, kWipeDataLength);
>+  // return execRecoveryCommand((char *) kWipeData, kWipeDataLength);
I think you should remove this line and add a proper comment to describe this mechanism.

>+  char updatePath[] = "/sdcard/update.zip" ;
>+  int updatePathLength = (int)sizeof(updatePath);
>+  struct stat updateStat;
>+  if (stat(updatePath, &updateStat) == -1) {
>+    ALOGD("could not stat update path \"%s\": %s",
>+         updatePath, strerror(errno));
>+    ALOGD("Just Do Factory Reset");
>+    return execRecoveryCommand((char *) kWipeData, kWipeDataLength);
>+  }else{
>+    ALOGD("Do Install Fota Update");
>+    return installFotaUpdate(updatePath, updatePathLength);
>+  }
> }
> // Some devices use a different mount point for external storage in recovery


James, Could confirm this mechanism is what you expected?
Attachment #8422166 - Flags: feedback?(james.zhang)
(In reply to Kai-Zhen Li from comment #26)
> Comment on attachment 8422166 [details] [diff] [review]
> librecovery_patch
> 
> >diff --git a/librecovery.c b/librecovery.c
> >index 4949a1e..5933597 100644
> >--- a/librecovery.c
> >+++ b/librecovery.c
> >@@ -143,7 +143,19 @@ int
> > factoryReset()
> > {
> >   // In AOSP's recovery image, "--wipe_data" is the synonym for factory reset
> >-  return execRecoveryCommand((char *) kWipeData, kWipeDataLength);
> >+  // return execRecoveryCommand((char *) kWipeData, kWipeDataLength);
> I think you should remove this line and add a proper comment to describe
> this mechanism.

Thank you for your advice and in this patch, I have added some comments and removed the lines. please check again.
Attachment #8422166 - Flags: feedback?(james.zhang) → feedback+
Attachment #8422197 - Flags: review?(kli)
Comment on attachment 8422197 [details] [diff] [review]
librecovery_patch_modify

>diff --git a/librecovery.c b/librecovery.c
>index 4949a1e..37c39d1 100644
>--- a/librecovery.c
>+++ b/librecovery.c
>@@ -142,8 +142,20 @@ execRecoveryCommand(char *command, size_t commandLength)
> int
> factoryReset()
> {
>-  // In AOSP's recovery image, "--wipe_data" is the synonym for factory reset
>-  return execRecoveryCommand((char *) kWipeData, kWipeDataLength);
>+  //If there is update.zip on SD card do OTA, or just reset the phone.
nit: add a space before 'If'. I think the usage 'update' could be better than 'OTA".

>+  char updatePath[] = "/sdcard/update.zip" ;
>+  int updatePathLength = (int)sizeof(updatePath);
>+  struct stat updateStat;
>+  if (stat(updatePath, &updateStat) == -1) {
>+    ALOGD("could not stat update path \"%s\": %s",
>+         updatePath, strerror(errno));
>+    ALOGD("Just Do Factory Reset");
>+    // In AOSP's recovery image, "--wipe_data" is the synonym for factory reset
>+    return execRecoveryCommand((char *) kWipeData, kWipeDataLength);
>+  }else{
nit: add space before and after 'else'

>+    ALOGD("Do Install Fota Update");
>+    return installFotaUpdate(updatePath, updatePathLength);
>+  }
> }
> 
> // Some devices use a different mount point for external storage in recovery


Looks good. There are nits to be fixed.

But I am not the owner so I can't give a review+.
Attachment #8422197 - Flags: review?(kli) → feedback+
Attached file pull request from github (obsolete) —
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:
Attachment #8422365 - Flags: approval-gaia-v1.3?
Attachment #8422365 - Flags: approval-gaia-v1.3? → review?(renfeng.mei)
Awesome!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
So, it looks like the PR was merged without review. That is *not* awesome. Also please squash your commits and set the commit message to be:
Bug 1003645 - [Tarako][FOTA] Need a setting for update from /sdcard r=...
Status: RESOLVED → REOPENED
Flags: needinfo?(renfeng.mei)
Resolution: FIXED → ---
greate
Flags: needinfo?(renfeng.mei)
It was merged into master, which is not what we should to do.
arvin, Please revert the change on  master branch
and only merge the patch into v1.3t branch
Flags: needinfo?(arvin.zhang)
Comment on attachment 8422197 [details] [diff] [review]
librecovery_patch_modify

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

::: librecovery.c
@@ +143,5 @@
>  factoryReset()
>  {
> +  //If there is update.zip on SD card do OTA, or just reset the phone.
> +  char updatePath[] = "/sdcard/update.zip" ;
> +  int updatePathLength = (int)sizeof(updatePath);

use strlen or sizeof(updatePath)-1
Attachment #8422197 - Flags: review+
Dear all,

  I had reverted the change on master branch and should say sorry to everyone for my stupid mistake. I will modify the patch according to all sound advices from kaizhen & James & Fabrice and then ask for review again.
Flags: needinfo?(arvin.zhang)
Attachment #8422365 - Attachment is obsolete: true
Attachment #8422365 - Flags: review?(renfeng.mei)
Attachment #8422942 - Flags: review?(james.zhang)
Attachment #8422942 - Flags: review?(james.zhang) → review+
commit 1c9220d1c3e596071cb57595989e0ba24bd2298c
Author: arvin zhang <arvin.zhang@spreadtrum.com>
Date:   Thu May 15 13:53:00 2014 +0800

    Bug 1003645 - [Tarako][FOTA] Need a setting for update from /sdcard r=James.zhang
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Flags: needinfo?(styang)
You need to log in before you can comment on or make changes to this bug.