[sensors] [mac] Fix ambient light sensor not updating its value when closing the lid with a monitor…

[sensors] [mac] Fix ambient light sensor not updating its value when closing the lid with a monitor connected.

Right now we use IOServiceAddInterestNotification with kIOGeneralInterest
as a way to get value changes from the AppleLMUController whenever
the ambient light condition changes.

However when closing the lid rapidly and a monitor is connected (which
prevents the computer to go to sleep) we don't receive a notification
and callback from the controller the whole way (until the lid is fully
closed). It means that the latest value received in JavaScript is not what
would be expected : as a developer point of view, lid closed = dark = 0.0
as a lux value.

The patch now listen kIOBusyInterest as well because when closing
the lid the AppleLMUController sends busy events which we can use
to fetch the value of the sensor and report it back in JavaScript.

BUG=606766
Committed: https://crrev.com/8fd4ba018ddfebcfc47966eea9ea3a75ed0b5fa0
Cr-Commit-Position: refs/heads/master@{#431046}

[darktears, 2016-10-25 22:09:34]

alexis.menard@intel.com changed reviewers:
+ rsesek@chromium.org

[darktears, 2016-10-25 22:09:34]

Let's discuss the above patch.

[darktears, 2016-10-31 16:22:04]

On 2016/10/25 at 22:09:34, darktears wrote:
> Let's discuss the above patch.

Any feedback?

[Robert Sesek, 2016-11-01 21:48:31]

Sorry I didn't have time to look at what AppleSMCLMU was doing last week to
properly review this. What is the message_type value for when you receive the
busy callbacks?

[darktears, 2016-11-02 15:33:36]

On 2016/11/01 at 21:48:31, rsesek wrote:
> Sorry I didn't have time to look at what AppleSMCLMU was doing last week to
properly review this. What is the message_type value for when you receive the
busy callbacks?

kIOMessageServiceBusyStateChange

[Robert Sesek, 2016-11-04 19:23:33]

On 2016/11/02 15:33:36, darktears wrote:
> On 2016/11/01 at 21:48:31, rsesek wrote:
> > Sorry I didn't have time to look at what AppleSMCLMU was doing last week to
> properly review this. What is the message_type value for when you receive the
> busy callbacks?
> 
> kIOMessageServiceBusyStateChange

How often does the sensor send busy-state notifications? From my reading it
looks like this could get called somewhat frequently.

[darktears, 2016-11-04 21:30:22]

On 2016/11/04 at 19:23:33, rsesek wrote:
> On 2016/11/02 15:33:36, darktears wrote:
> > On 2016/11/01 at 21:48:31, rsesek wrote:
> > > Sorry I didn't have time to look at what AppleSMCLMU was doing last week
to
> > properly review this. What is the message_type value for when you receive
the
> > busy callbacks?
> > 
> > kIOMessageServiceBusyStateChange
> 
> How often does the sensor send busy-state notifications? From my reading it
looks like this could get called somewhat frequently.

It does get called more frequently than say the other type of notification (e.g.
GeneralInterest). Also just listening kIOBusyInterest is not enough to have
accurate report of the ambient light condition.

[Robert Sesek, 2016-11-04 21:30:57]

On 2016/11/04 21:30:22, darktears wrote:
> On 2016/11/04 at 19:23:33, rsesek wrote:
> > On 2016/11/02 15:33:36, darktears wrote:
> > > On 2016/11/01 at 21:48:31, rsesek wrote:
> > > > Sorry I didn't have time to look at what AppleSMCLMU was doing last week
> to
> > > properly review this. What is the message_type value for when you receive
> the
> > > busy callbacks?
> > > 
> > > kIOMessageServiceBusyStateChange
> > 
> > How often does the sensor send busy-state notifications? From my reading it
> looks like this could get called somewhat frequently.
> 
> It does get called more frequently than say the other type of notification
(e.g.
> GeneralInterest). Also just listening kIOBusyInterest is not enough to have
> accurate report of the ambient light condition.

How frequently is it, roughly? Every few minutes? Seconds? More than that?

[darktears, 2016-11-07 23:16:33]

On 2016/11/04 at 21:30:57, rsesek wrote:
> On 2016/11/04 21:30:22, darktears wrote:
> > On 2016/11/04 at 19:23:33, rsesek wrote:
> > > On 2016/11/02 15:33:36, darktears wrote:
> > > > On 2016/11/01 at 21:48:31, rsesek wrote:
> > > > > Sorry I didn't have time to look at what AppleSMCLMU was doing last
week
> > to
> > > > properly review this. What is the message_type value for when you
receive
> > the
> > > > busy callbacks?
> > > > 
> > > > kIOMessageServiceBusyStateChange
> > > 
> > > How often does the sensor send busy-state notifications? From my reading
it
> > looks like this could get called somewhat frequently.
> > 
> > It does get called more frequently than say the other type of notification
(e.g.
> > GeneralInterest). Also just listening kIOBusyInterest is not enough to have
> > accurate report of the ambient light condition.
> 
> How frequently is it, roughly? Every few minutes? Seconds? More than that?


I don't get any kIOMessageServiceBusyStateChange while idle (lighting conditions
do not change whether 0 or some other values). I can't get any
kIOMessageServiceBusyStateChange while moving my laptop with "normal" light
conditions (say a lux range from 10 to 50). I see this event fired (~9 times in
a row) when the light conditions are very low or extremely high. For most use
cases it will never actually get fired.

[Robert Sesek, 2016-11-08 22:56:08]

LGTM

[darktears, 2016-11-08 23:07:37]

alexis.menard@intel.com changed reviewers:
+ reillyg@chromium.org, timvolodine@chromium.org

[darktears, 2016-11-08 23:07:38]

timvolodine@chromium.org, reillyg@chromium.org : please review.

[Reilly Grant (use Gerrit), 2016-11-09 00:34:11]

I am concerned that trying to make a request to the LMU when it is in the busy
state will cause the request to block for a while. Can you do some experiments:

a) Look at the value of message_type in the calls to IOServiceCallback during
normal operation and when the lid is being closed so we understand better what
events the system is actually sending us.

b) Measure the amount of time spent in IOConnectCallMethod to make sure there
isn't a spike when the lid is closed.

For example, maybe we should only try to read the sensor when we get a busy
message saying that the service has set the busy flag back to 0.

[Reilly Grant (use Gerrit), 2016-11-09 00:36:42]

On 2016/11/09 at 00:34:11, Reilly Grant wrote:
> I am concerned that trying to make a request to the LMU when it is in the busy
state will cause the request to block for a while. Can you do some experiments:
> 
> a) Look at the value of message_type in the calls to IOServiceCallback during
normal operation and when the lid is being closed so we understand better what
events the system is actually sending us.
> 
> b) Measure the amount of time spent in IOConnectCallMethod to make sure there
isn't a spike when the lid is closed.
> 
> For example, maybe we should only try to read the sensor when we get a busy
message saying that the service has set the busy flag back to 0.

Sorry, I looked back at the comments and see you've already looked into some of
these questions. I'm still concerned about avoiding calls when the IOService
goes into the busy state. Can we limit reading the sensor to only when it
becomes unbusy?

[darktears, 2016-11-09 19:03:07]

On 2016/11/09 at 00:36:42, reillyg wrote:
> On 2016/11/09 at 00:34:11, Reilly Grant wrote:
> > I am concerned that trying to make a request to the LMU when it is in the
busy state will cause the request to block for a while. Can you do some
experiments:
> > 
> > a) Look at the value of message_type in the calls to IOServiceCallback
during normal operation and when the lid is being closed so we understand better
what events the system is actually sending us.
> > 
> > b) Measure the amount of time spent in IOConnectCallMethod to make sure
there isn't a spike when the lid is closed.
> > 
> > For example, maybe we should only try to read the sensor when we get a busy
message saying that the service has set the busy flag back to 0.
> 
> Sorry, I looked back at the comments and see you've already looked into some
of these questions. I'm still concerned about avoiding calls when the IOService
goes into the busy state. Can we limit reading the sensor to only when it
becomes unbusy?

I'm not sure how I could achieve this. IOService interest notification system
doesn't seem to make a difference with busy/unbusy states. Using kIOBusyInterest
notification I only get kIOMessageServiceBusyStateChange and as far as I can see
I can't tell if the sensor is entering in a busy or unbusy state.
message_argument passed in the callback doesn't give me anything that I can use
(in some case I get a _NSAtom but what one can do with this?). Unless I use some
dubious assumptions like I've already got a kIOMessageServiceBusyStateChange
message, the next one is probably telling me that's it's not busy anymore.

For the record calling IOConnectCallMethod from the callback when we get a
kIOMessageServiceBusyStateChange is always successful (which explains why the
value gets updated until the lid is physically closed). It's still a mystery to
me why we get kIOMessageServiceBusyStateChange calls only when the luminosity is
way high (e.g. I use my smartphone flashlight) or very low (when closing the lid
or entering in a dark condition) and not while in normal ambient light
conditions (even when it changes a bit). I can't get much more info opening the
LMU sensor kernel module in Hoppper.

[Reilly Grant (use Gerrit), 2016-11-09 19:20:20]

On 2016/11/09 at 19:03:07, alexis.menard wrote:
> On 2016/11/09 at 00:36:42, reillyg wrote:
> > On 2016/11/09 at 00:34:11, Reilly Grant wrote:
> > > I am concerned that trying to make a request to the LMU when it is in the
busy state will cause the request to block for a while. Can you do some
experiments:
> > > 
> > > a) Look at the value of message_type in the calls to IOServiceCallback
during normal operation and when the lid is being closed so we understand better
what events the system is actually sending us.
> > > 
> > > b) Measure the amount of time spent in IOConnectCallMethod to make sure
there isn't a spike when the lid is closed.
> > > 
> > > For example, maybe we should only try to read the sensor when we get a
busy message saying that the service has set the busy flag back to 0.
> > 
> > Sorry, I looked back at the comments and see you've already looked into some
of these questions. I'm still concerned about avoiding calls when the IOService
goes into the busy state. Can we limit reading the sensor to only when it
becomes unbusy?
> 
> I'm not sure how I could achieve this. IOService interest notification system
doesn't seem to make a difference with busy/unbusy states. Using kIOBusyInterest
notification I only get kIOMessageServiceBusyStateChange and as far as I can see
I can't tell if the sensor is entering in a busy or unbusy state.
message_argument passed in the callback doesn't give me anything that I can use
(in some case I get a _NSAtom but what one can do with this?). Unless I use some
dubious assumptions like I've already got a kIOMessageServiceBusyStateChange
message, the next one is probably telling me that's it's not busy anymore.
> 
> For the record calling IOConnectCallMethod from the callback when we get a
kIOMessageServiceBusyStateChange is always successful (which explains why the
value gets updated until the lid is physically closed). It's still a mystery to
me why we get kIOMessageServiceBusyStateChange calls only when the luminosity is
way high (e.g. I use my smartphone flashlight) or very low (when closing the lid
or entering in a dark condition) and not while in normal ambient light
conditions (even when it changes a bit). I can't get much more info opening the
LMU sensor kernel module in Hoppper.

The NSAtom may indicate YES, with null indicating NO, thus telling you the busy
state. Does that jibe with what you are seeing? Anyways, if you observe that the
IOConnectCallMethod() function is always returning immediately then I'm
satisfied so lgtm.

[darktears, 2016-11-09 20:49:53]

On 2016/11/09 at 19:20:20, reillyg wrote:
> On 2016/11/09 at 19:03:07, alexis.menard wrote:
> > On 2016/11/09 at 00:36:42, reillyg wrote:
> > > On 2016/11/09 at 00:34:11, Reilly Grant wrote:
> > > > I am concerned that trying to make a request to the LMU when it is in
the busy state will cause the request to block for a while. Can you do some
experiments:
> > > > 
> > > > a) Look at the value of message_type in the calls to IOServiceCallback
during normal operation and when the lid is being closed so we understand better
what events the system is actually sending us.
> > > > 
> > > > b) Measure the amount of time spent in IOConnectCallMethod to make sure
there isn't a spike when the lid is closed.
> > > > 
> > > > For example, maybe we should only try to read the sensor when we get a
busy message saying that the service has set the busy flag back to 0.
> > > 
> > > Sorry, I looked back at the comments and see you've already looked into
some of these questions. I'm still concerned about avoiding calls when the
IOService goes into the busy state. Can we limit reading the sensor to only when
it becomes unbusy?
> > 
> > I'm not sure how I could achieve this. IOService interest notification
system doesn't seem to make a difference with busy/unbusy states. Using
kIOBusyInterest notification I only get kIOMessageServiceBusyStateChange and as
far as I can see I can't tell if the sensor is entering in a busy or unbusy
state. message_argument passed in the callback doesn't give me anything that I
can use (in some case I get a _NSAtom but what one can do with this?). Unless I
use some dubious assumptions like I've already got a
kIOMessageServiceBusyStateChange message, the next one is probably telling me
that's it's not busy anymore.
> > 
> > For the record calling IOConnectCallMethod from the callback when we get a
kIOMessageServiceBusyStateChange is always successful (which explains why the
value gets updated until the lid is physically closed). It's still a mystery to
me why we get kIOMessageServiceBusyStateChange calls only when the luminosity is
way high (e.g. I use my smartphone flashlight) or very low (when closing the lid
or entering in a dark condition) and not while in normal ambient light
conditions (even when it changes a bit). I can't get much more info opening the
LMU sensor kernel module in Hoppper.
> 
> The NSAtom may indicate YES, with null indicating NO, thus telling you the
busy state. Does that jibe with what you are seeing? Anyways, if you observe
that the IOConnectCallMethod() function is always returning immediately then I'm
satisfied so lgtm.

Yes it seems like it does alternate NSAtom, nullptr indeed.

[darktears, 2016-11-09 20:53:38]

The CQ bit was checked by alexis.menard@intel.com

[commit-bot: I haz the power, 2016-11-09 20:54:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 22:03:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-09 22:07:08]

Description was changed from

==========
[sensors] [mac] Fix ambient light sensor not updating its value when closing the
lid with a monitor connected.

Right now we use IOServiceAddInterestNotification with kIOGeneralInterest
as a way to get value changes from the AppleLMUController whenever
the ambient light condition changes.

However when closing the lid rapidly and a monitor is connected (which
prevents the computer to go to sleep) we don't receive a notification
and callback from the controller the whole way (until the lid is fully
closed). It means that the latest value received in JavaScript is not what
would be expected : as a developer point of view, lid closed = dark = 0.0
as a lux value.

The patch now listen kIOBusyInterest as well because when closing
the lid the AppleLMUController sends busy events which we can use
to fetch the value of the sensor and report it back in JavaScript.

BUG=606766
==========

to

==========
[sensors] [mac] Fix ambient light sensor not updating its value when closing the
lid with a monitor connected.

Right now we use IOServiceAddInterestNotification with kIOGeneralInterest
as a way to get value changes from the AppleLMUController whenever
the ambient light condition changes.

However when closing the lid rapidly and a monitor is connected (which
prevents the computer to go to sleep) we don't receive a notification
and callback from the controller the whole way (until the lid is fully
closed). It means that the latest value received in JavaScript is not what
would be expected : as a developer point of view, lid closed = dark = 0.0
as a lux value.

The patch now listen kIOBusyInterest as well because when closing
the lid the AppleLMUController sends busy events which we can use
to fetch the value of the sensor and report it back in JavaScript.

BUG=606766

Committed: https://crrev.com/8fd4ba018ddfebcfc47966eea9ea3a75ed0b5fa0
Cr-Commit-Position: refs/heads/master@{#431046}
==========

[commit-bot: I haz the power, 2016-11-09 22:07:09]

Patchset 1 (id:??) landed as
https://crrev.com/8fd4ba018ddfebcfc47966eea9ea3a75ed0b5fa0
Cr-Commit-Position: refs/heads/master@{#431046}