Opened 6 months ago

Closed 4 months ago

Last modified 4 months ago

#12798 closed defect (fixed)

Screenshot does not take every time

Reported by: tonyforster Owned by: suyouxin
Priority: normal Milestone: 13.2.0-android
Component: android Version: Development build as of this date
Keywords: Cc: pgf@…
Blocked By: Blocking:
Deployments affected: Action Needed: no action
Verified: no

Description

"press the power button and then X game key and hold them for a second, then release", I can only get it to work about 1 in 10 tries. I am pressing power first.

Change History (22)

comment:1 follow-ups: Changed 6 months ago by Quozl

  • Action Needed changed from never set to design
  • Cc pgf@… added
  • Milestone changed from Not Triaged to 13.2.0-android
  • Version changed from not specified to Development build as of this date

When you say "does not take every time", what do you mean?

My tests on 1GB and 2GB units show several possible "does not take" results:

  • the volume control dialog appears instead,
  • the power control dialog appears instead,
  • there is no response at all.

The first of these occurs if the X game key is seen first, probably due to a race. See below.

The second of these occurs if the X game key is not seen before the power button has been down long enough. Being more careful with the finger timing solves this.

The no response at all only ever occurs for me if the Saving screenshot... notification is still in the top left of the screen.

Now, about the race. The underlying cause is that the data path for the two keys are separate; the power button down event is from the embedded controller power control subsystem, and the X game key event is from the security processor PS/2 keyboard scan algorithm. These two keys were never intended to be used in combination in the original design, and so there is no attempt to ensure timing consistency between them. @pgf, any comment?

Looking at the schematic,

  • PWR_BTN# on page 25 connects to GPIO 14 of the EC, and the EC debounces and delays the signal and delivers events to the SoC via an OLPC specific protocol,
  • KEY_R_DN on page 25 connects to GPIO 18 of the SoC, and is handled by CForth running on the security processor, which debounces in a different way and delivers events to the main processor via shared memory.

@Ben, can we have a duplicate key binding for screenshot to overcome this design incompatibility?

comment:2 Changed 6 months ago by suyouxin

Actually I disagree with hard to take a screenshot. I can pretty much trigger it 9/10.
Once you get use to its sequence, it can be easily triggered.

I understand Quozl's comment above, however I don't think there is human detected-able delay on power button pressing. This can be proved when you pressing power button let system go to sleep. It just happens instantly after pressing.

The problem is that because of the button hardware structure, sometime you will feel that power button has already been pressed, but indeed it hasn't been pressed somehow. It also happens when I press power button let the system go to sleep or wake up system from sleep. Always has chance that power button hasn't been triggered properly.

On another side, if I can overcome this design incompatibility, which combination would you recommend?

comment:3 Changed 6 months ago by Quozl

Square and tick game keys at same time?

comment:4 in reply to: ↑ 1 Changed 6 months ago by tonyforster

Replying to Quozl:

When you say "does not take every time", what do you mean?

  • the volume control dialog appears instead, about 2/3 of the time
  • the power control dialog appears instead, about 1/3 of the time

comment:5 in reply to: ↑ 1 ; follow-up: Changed 6 months ago by pgf

once the power button event and/or the game-key event hit the kernel, they're
treated pretty similarly, i think. so without doing more research, i'm not sure i'm buying the "race condition" argument.

but as james points out, the debounce mechanisms, and timings, are quite different.

the power button needs to be pressed for at least 150ms before an event will be generated. this is somewhat long, intentionally. (from power.c:336 in the ec-cl4-mmp3 repo.)

the game keys only need pressing for (the code says) 60ms, though since it's really implemented as 3 x 20ms, and the cycle-count to ms conversion might not be completely accurate, i'm not going to swear to that number -- it could be longer. (this is from
src/platform/arm-xo-1.75/consoleio.c:1039 in the cforth repo.)

so: be sure you're holding the key combo long enough so that the kernel can see that both "down" events arrive before either button is released.

comment:6 Changed 6 months ago by tonyforster

The power key must be seen before X? It must be pressed 150-60=90ms before X? X must be received before a power off request is interpreted, any estimate of time in mS? From James I take it that a power off dialog means that the system is no longer waiting for X. If this happens while both buttons are still pressed then the issue of early release is not relevant?

comment:7 Changed 6 months ago by pgf

i'm not sure i understand the "power key before X" part. is that really part of the requirements for a screen shot? i would have expected it to simply be "hold the two together, and release before you get the power dialog". (i might have hoped that android is smart enough to suppress the power dialog if there's another key down at the same time, but from what james said, i guess that's not the case.) in either case, nothing will happen until 150ms after you push the power button, and until 60ms after you push X.

comment:8 Changed 6 months ago by Quozl

thanks. yes, i stretched the definition of race condition to include the fingers, 'cause i wasn't sure if the debounce time would be predictable.

yes, i would have expected something simpler, but the X game key also controls a volume dialog, and the power key also controls a power dialog. quite a bit of functional overloading.

i speculate. the problem might be solved if the event handler could be changed to allow X game key then power key also as a trigger for the screenshot, but that might delay the volume control response. ben would know.

comment:9 Changed 6 months ago by pgf

ah -- so you're saying there's currently an ordering requirement: X before power. i didn't realize that before. it seems like the longer latency on power (due to the longer debounce interval) should actually help make the presses happen in the correct order.

what's the screenshot combo for a phone? (so i can play with it.)

comment:10 Changed 6 months ago by Quozl

press and hold power and volume down button at the same time for 1-2 seconds.

comment:11 Changed 6 months ago by Quozl

actually, the ordering appears to be power before X, and maybe it is the different debounce that is causing the ordering.

comment:12 Changed 6 months ago by suyouxin

As the code said in Android

Time to volume and power must be pressed within this interval of each other.
private static final long SCREENSHOT_CHORD_DEBOUNCE_DELAY_MILLIS = 150;

Could be either X or power in front if within 150ms.

So that if we ignore these transfer cost between cpus and kernel to android as Quozl comments. And apply pgf's data "60ms generate X and 150ms generate power", In this case:

       Press X 61ms early than power.        not trigger
       Press X 60ms early than power.        trigger
       Press X 60ms and power some time.     trigger
       Press power 90ms early than X.        trigger
       Press power 240ms early than X.       trigger
       Press power 241ms early than X.       not trigger

So that if you press power button little bit early will increase the successful of trigger screenshot taken.


comment:13 follow-up: Changed 6 months ago by tonyforster

Thanks. So increasing SCREENSHOT_CHORD_DEBOUNCE_DELAY_MILLIS will make it easier to take screenshots? Is that something that I can patch?

comment:14 Changed 6 months ago by tonyforster

Your timings are probably consistent with my observations. Either I try to press both simultaneously or the pwr first. When I press simultaneously, the recessed pwr button probably takes 60 mS longer to connect because of the longer distance my finger must travel. When I try to press the pwr first, I probably introduce more than 240mS delay. Adults with big fingers and slow reaction times will probably find this harder than children.

comment:15 in reply to: ↑ 5 Changed 6 months ago by Quozl

Replying to pgf:

the power button needs to be pressed for at least 150ms before an event will be generated.

i've measured this with an arduino uno driving the power button to ground through a diode. the reliable point was 155ms, for 100% event reporting. 154ms gave 80%, 153ms gave 28%, and 152ms gave no events. the arduino uno has a crystal oscillator.

the game keys only need pressing for (the code says) 60ms, ...

i've not measured this; i have to restack components to get a keyboard connected, otherwise the game key buttons are not given an event stream.

comment:16 Changed 6 months ago by Quozl

verified 60ms for game keys.

comment:17 in reply to: ↑ 13 Changed 6 months ago by suyouxin

Replying to tonyforster:

Thanks. So increasing SCREENSHOT_CHORD_DEBOUNCE_DELAY_MILLIS will make it easier to take screenshots? Is that something that I can patch?

Code is here:
https://android.googlesource.com/platform/frameworks/base/+/43d84518f2380e45659fbb31455479ef018b9329/policy/src/com/android/internal/policy/impl/PhoneWindowManager.java?autodive=0%2F#462

Should not make it too long, it will also effect power button respond time. How many time you add here also means how many time you have to wait for a common power button reaction(shut down screen).

No, you can't patch them right now. The source code currently is not ready to release, due to it has Marvell's code which is under NDA. We can only release binaries not source code of that part.

comment:18 Changed 6 months ago by pgf

to be honest, it would probably be okay to reduce the power debounce time, perhaps even all the way to 60ms. 150 always seemed a bit too long to me. it's safer, of course (the user should really mean it when they push power), but from a mechanical standpoint, if 60ms is okay for the rotate button, then it should be okay for power as well.

but messing with the power-up sequence is not to be taken lightly.

comment:19 Changed 6 months ago by Quozl

i've experimented further with an arduino driving the keys, and verified the thresholds for correct response as described by ben;

  • the x key must be pressed no more than 60ms before the power key, otherwise the volume dialog occurs,
  • the power key must be pressed no more than 240ms before the x key, otherwise the volume dialog occurs,
  • if struck together, both keys must remain down with no release for at least 660ms,
  • if power is struck before x key by 90ms (the difference in firmware debounce timers), both keys must remain down for 580ms.

even if we messed with the power key debounce, or increased the game key debounce, this would only move the boundaries from -60ms:240ms to -150ms:150ms. this is a remarkably narrow window, and is obviously designed for hardware that has nearly identical physical structure for both buttons, such that a finger can cover both.

reviewed the source PhoneWindowManager.java. there does not seem to be a good reason why both keys have to be down for 580ms. it depends on whatever getGlobalActionKeyTimeout returns.

i suggest next time the code is edited, that we try:

  • increasing the 150ms boundary by 90ms, so it will be -150ms:330ms,
  • decrease getGlobalActionKeyTimeout significantly.

comment:20 Changed 4 months ago by Quozl

  • Action Needed changed from design to no action

Fixed in build released on 2014-08-18.
http://build.laptop.org/android/2014-08-18/

comment:21 Changed 4 months ago by Quozl

  • Resolution set to fixed
  • Status changed from new to closed

comment:22 Changed 4 months ago by tonyforster

much better now, thanks

Note: See TracTickets for help on using tickets.