Closed Bug 911242 Opened 6 years ago Closed 5 years ago

[LED] De-couple the control of screen backlight and keyboard backlight.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mchen, Assigned: selin)

References

Details

(Whiteboard: [u=devices c=power p=0])

Attachments

(1 file, 6 obsolete files)

Currently keyboard backlight is controlled align with screen backlight. For power saving we can control them individually. 

Ex: keyboard backlight can be controlled by 
  1. Enable it when any input event is coming.
  2. Then start a timer.
  3. timer will be re-start when any input event is coming.
  4. disable it when timer is timeout.
Duplicate of this bug: 901859
Already sent a mail into dev-webapi for discussing the new attribute in MozPowerManager.
Blocks: 915565
Attached patch Part 1: WebIDL (obsolete) — Splinter Review
Hi Dave,

Could you give the feedback of this patch based on bug description?
According to this is related to device API, I ask for your feedback first then super review.

Thanks very much.
Assignee: nobody → mchen
Attachment #812451 - Flags: feedback?(dhylands)
Comment on attachment 812451 [details] [diff] [review]
Part 1: WebIDL

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

Sorry to take so long to reply to this.

I just have some issues with the comment. The attribute itself seems like a reasonable thing to do.

::: dom/webidl/MozPowerManager.webidl
@@ +49,5 @@
>       */
>      attribute boolean screenEnabled;
>  
>      /**
> +     * Is the device's keypad/button's backlight enabled? set this attribute to

nit: I'd drop the 's on button.
nit: Capitalize the S in "Set this attribute"

@@ +50,5 @@
>      attribute boolean screenEnabled;
>  
>      /**
> +     * Is the device's keypad/button's backlight enabled? set this attribute to
> +     * false will trun off keypad/button's backlight.

nit: s/trun/turn/
nit: I'd drop the 's here as well.

@@ +51,5 @@
>  
>      /**
> +     * Is the device's keypad/button's backlight enabled? set this attribute to
> +     * false will trun off keypad/button's backlight.
> +     * The brightness level is the same with scrrenBrightness.

nit: s/scrren/screen/
nit: s/with/as/
Attachment #812451 - Flags: feedback?(dhylands) → feedback+
(In reply to Dave Hylands [:dhylands] from comment #4)
> Comment on attachment 812451 [details] [diff] [review]
> Part 1: WebIDL
> 
Oh, that is my rudeness on such nits when asking for feedback from others.
Thanks for your feedback then starting to implement it.
Whiteboard: [u=devices c=power p=0]
Status: NEW → ASSIGNED
I am not working on this item now so un-assign my self.
Assignee: mchen → nobody
Assignee: nobody → selin
Attached patch Patch attempt 1 (obsolete) — Splinter Review
Taking over the implementation of the WebIDL and posting it for review.
Attachment #812451 - Attachment is obsolete: true
Attachment #8428587 - Flags: review?(mchen)
Comment on attachment 8428587 [details] [diff] [review]
Patch attempt 1

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

I think you missing the implementation on GonkHal.cpp.
Attachment #8428587 - Flags: review?(mchen)
Attached patch Patch attempt 2 (obsolete) — Splinter Review
Adding implementations to GonkHall.cpp
Attachment #8428587 - Attachment is obsolete: true
Attachment #8429147 - Flags: review?(mchen)
Comment on attachment 8429147 [details] [diff] [review]
Patch attempt 2

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

::: hal/Hal.h
@@ +134,5 @@
> +/**
> + * Enable or disable the device's keypad/button backlight.
> + *
> + * Note that it may take a few seconds for the keypad/button backlight to turn
> + * on or off.

Is this really the situation we has found on real device?

::: hal/gonk/GonkHal.cpp
@@ +501,5 @@
>  bool sScreenEnabled = true;
>  
> +// We can write to screenEnabledFilename to enable/disable the keypad/button
> +// backlight, but when we read, we always get "mem"!  So we have to keep track
> +// ourselves whether the screen is on or not.

I think sKeyLightEnabled is the different case then sScreenEnabled.
sScreenEnabled is relevant to /sys/power/state and it will return values about what states this device supports.
Therefore it always return "mem" if device is a mobile device.

But in case of sKeyLightEnabled, I think we can just use GetLight(...) to check whether key or button light is enabled or not.

@@ +533,5 @@
>  
> +bool
> +GetKeyLightEnabled()
> +{
> +  return sKeyLightEnabled;

use GetLight() to check one of eHalLightID_Buttons/Keyboard.

@@ +560,5 @@
> +    aConfig.color() = 0x00000000;
> +  }
> +
> +  hal::SetLight(hal::eHalLightID_Buttons, aConfig);
> +  hal::SetLight(hal::eHalLightID_Keyboard, aConfig);

This function is already under namespace of mozilla::hal_impl,
therefore I think it is no nesseary to call version from mozilla::hal which will check sandbox again.

Moreover, I found that only functions in namespace of mozilla::hal_impl called Set/GetLight.
This means that we even don't need to add Set/GetLight into PHal.ipdl but just static functions inside GonkHal.cpp

@@ +599,5 @@
>    aConfig.color() = color;
>    hal::SetLight(hal::eHalLightID_Backlight, aConfig);
> +  if (GetKeyLightEnabled()) {
> +    hal::SetLight(hal::eHalLightID_Buttons, aConfig);
> +    hal::SetLight(hal::eHalLightID_Keyboard, aConfig);

Same here.
Attachment #8429147 - Flags: review?(mchen)
Attached patch Patch attempt 3 (obsolete) — Splinter Review
Updating based on Marco's comments.
Attachment #8429147 - Attachment is obsolete: true
Attachment #8430575 - Flags: review?(mchen)
Attached patch Patch attempt 4 (obsolete) — Splinter Review
Fixing build failures.
Attachment #8430575 - Attachment is obsolete: true
Attachment #8430575 - Flags: review?(mchen)
Attachment #8430609 - Flags: review?(mchen)
Comment on attachment 8430609 [details] [diff] [review]
Patch attempt 4

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

It looks good to me. Nice work.
Then please ask :dhyland to review the implentation and :sicking to do superreview.

By the way,
Could you help to open a new bug for removing Get/SetLight() from PHal.ipdl?
Thanks.

::: hal/gonk/GonkHal.cpp
@@ +551,5 @@
> +    uint32_t color = (0xff<<24) + (val<<16) + (val<<8) + val;
> +
> +    aConfig.color() = color;
> +  } else {
> +    aConfig.color() = 0x00000000;

You can move this line before "if (aEnabled)" then "else" can be removed.
Attachment #8430609 - Flags: review?(mchen) → feedback+
(In reply to Marco Chen [:mchen] from comment #13)
> By the way,
> Could you help to open a new bug for removing Get/SetLight() from PHal.ipdl?
> Thanks.
Sure. I've added bug 1017463 to follow up this concern.
Attached patch Patch attempt 5 (obsolete) — Splinter Review
Adding hal:: back before Set/GetLight to resolve build failures.
Attachment #8430609 - Attachment is obsolete: true
Attachment #8430644 - Flags: superreview?(jonas)
Attachment #8430644 - Flags: review?(dhylands)
Comment on attachment 8430644 [details] [diff] [review]
Patch attempt 5

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

r=me with nits addressed.

::: dom/power/test/test_power_set_key_light_enabled.html
@@ +42,5 @@
> +
> +    SimpleTest.finish();
> +  }
> +
> +  SimpleTest.expectAssertions(0, 9);

Why is this here?

If we're expecting assertions, then I think you should document what assertions you're expecting and why.

::: hal/Hal.cpp
@@ +385,5 @@
> +  AssertMainThread();
> +  RETURN_PROXY_IF_SANDBOXED(GetKeyLightEnabled(), false);
> +}
> +
> +void SetKeyLightEnabled(bool enabled)

nit: This should be aEnabled (and SetScreenEnabled is wrong too)

::: hal/fallback/FallbackScreenPower.cpp
@@ +23,5 @@
> +  return true;
> +}
> +
> +void
> +SetKeyLightEnabled(bool enabled)

nit: This should be aEnabled (and SetScreenEnabled is wrong too)

::: hal/gonk/GonkHal.cpp
@@ +528,5 @@
>  
> +bool
> +GetKeyLightEnabled()
> +{
> +  hal::LightConfiguration aConfig;

nit: Shouldn't be aConfig since it isn't an argument

@@ +536,5 @@
> +
> +void
> +SetKeyLightEnabled(bool aEnabled)
> +{
> +  hal::LightConfiguration aConfig;

nit: Shouldn't be aConfig since it isn't an argument

@@ +547,5 @@
> +    // Convert the value in [0, 1] to an int between 0 and 255 and then convert
> +    // it to a color. Note that the high byte is FF, corresponding to the alpha
> +    // channel.
> +    double brightness = GetScreenBrightness();
> +    int val = static_cast<int>(round(brightness * 255));

nit: I would use 255.0 here since you're doing a floating point multiply.
nit: You're storing val into a uint32_t below, so I would declare val as uint32_t here.

::: hal/sandbox/SandboxHal.cpp
@@ +156,5 @@
> +  return enabled;
> +}
> +
> +void
> +SetKeyLightEnabled(bool enabled)

nit: should be aEnabled

@@ +624,5 @@
>      return true;
>    }
>  
>    virtual bool
> +  RecvGetKeyLightEnabled(bool *enabled) MOZ_OVERRIDE

nit: should be bool* aEnabled (so both placement of * and name of argument)

@@ +634,5 @@
> +    return true;
> +  }
> +
> +  virtual bool
> +  RecvSetKeyLightEnabled(const bool &enabled) MOZ_OVERRIDE

nit: should be bool& aEnabled
Attachment #8430644 - Flags: review?(dhylands) → review+
Attachment #8430644 - Flags: superreview?(jonas) → superreview+
Hi,

Even there is a follow up bug for removing Get/SetLight() from PHal.ipdl, I think we still can directly call hal_impl::Set/GetLight() instead of calling hal::.
Attached patch Patch attempt 6Splinter Review
Minor updates based on Dave and Marco's comments (argument renaming, adopting hal_impl name space, etc.) I'd like to directly mark the patch as 'checkin-needed' since it was already granted for review+ and superreview+.
Attachment #8430644 - Attachment is obsolete: true
Hi Sean, do you have a link to a recent Try run by chance? :)
Keywords: checkin-needed
Sean, the sheriffs now require a try run of the patch if you're going to use checkin-needed.
Sean,

I think that this is the proper link for the try run:
https://tbpl.mozilla.org/?rev=69e9b6e4056e&tree=Try

I retriggered the failures, since they might be caused by one of the intermittent bugs.
https://hg.mozilla.org/mozilla-central/rev/3dd59dda51fc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
\o/.  Is there a bug for the Gaia portion of this?
Blocks: 1172345
You need to log in before you can comment on or make changes to this bug.