allow non-wake-binded inputs to propogate when is screen off

d3nd3 commented 1 year ago

So to keep it short & brief, the idea is to allow inputs that aren't wake-key-binded inputs, to not be blocked when screen is off. Believing that the if locked dont push is already enough control.

Also put lcdPowerTimeout && in some places where missing, so it consistently behaves the same, requiring a timeout if its going to perform any auto-wake-action.

Thought it'd be nice to have a backlight api to go along with the timeout.

d3nd3 commented 1 year ago

I have not tested any of these code changes, proper care needs to be taken to ensure its bug free.

gfwilliams commented 1 year ago

I have not tested any of these code changes, proper care needs to be taken to ensure its bug free.

Do you think you could test them?? As long as GitHub actions are enabled then the firmware is automatically built - for example https://github.com/espruino/Espruino/actions/runs/5307613873?pr=2384

d3nd3 commented 1 year ago

I have not tested any of these code changes, proper care needs to be taken to ensure its bug free.

Do you think you could test them?? As long as GitHub actions are enabled then the firmware is automatically built - for example https://github.com/espruino/Espruino/actions/runs/5307613873?pr=2384

Sure, I would love to. I only have a Bangle1 here atm, that might change soon if Poolitzer is gifting me his old one.

d3nd3 commented 1 year ago

Okay , I have tested it and I haven't found anything non-functional about it. The behaviour I wanted is working, the key presses go through when screen is off.

The backlight api seems to be working fine too, it didn't change much so that is what I expected, the function was already there. Unable to test on a bangle 2, perhaps you could do that, all you would look for is if the backlight behaves correctly from the 3 functions: .on("LCDPowerBacklight" , isBacklightOn() , setLCDPowerBacklight(). Expect to see the sun-read display separate from backlight. But ye its a small change to code, so everything should be fine.

gfwilliams commented 1 year ago

Thanks for the explanation in #2383 - the backlight stuff looks great, and it's really good to have better control of that.

I misunderstood earlier - I thought you wanted to get events through even when the device was locked - but what you want is to be able to be unlocked with the LCD/backlight off?

So my only concern is the line I linked above - I think for the Q3 wake on tap (which uses the accelerometer) we probably only want to reset the inactivityTimer if we've got wake_on_tap enabled - plus, should if (bangleFlags & JSBF_LOCKED) handled = false; be the =true? as far as I can see setting handled=false actually pushes the event rather than ignoring it?

d3nd3 commented 1 year ago

Yes, it should be = true, and it also should be within the WAKE_TOUCH, correct x2. I did a force push --amend.

d3nd3 commented 1 year ago

I just had a realization that the on Locked code should not be on the WAKEON_TWIST, WAKEON_FACEUP or WAKEON_TOUCH. Those are user settings which ignore LOCK.

gfwilliams commented 1 year ago

I just had a realization that the on Locked code should not be on the WAKEON_TWIST, WAKEON_FACEUP or WAKEON_TOUCH. Those are user settings which ignore LOCK.

I'm not entirely sure I understand?

d3nd3 commented 1 year ago

I mean, every input like on tap, on touch, on twist, on faceup should be filtered by is Locked in general, but not for the WAKE part. That was only in the BtnCommonFunction because that function handles 3 button and some might not have setting wake active in settings.

d3nd3 commented 1 year ago

Some inputs don't have to be behind if Locked, it would be better to not have them behind if locked then they have access input events if they need to, right. If we put if locked on every input there, its restricting access. Its better if they use .on then run the javascript Bangle.isLocked(). So the idea of locking every input down in the firmware is not such good idea.

I mean because of the desire of them wanting partial lock. not lock for every input type, its too restrictive. edit: I'm asking the question : should the users be denied .on events, when they have locked set. I think they shouldn't. but its a tough call. We need to define what set of inputs does locked block and which ones does it not.

d3nd3 commented 1 year ago

Or accept that LOCKED means: gesture twist faceup tap drag touch twist events do NOT fire, unless they have WAKE_ON flag applied in settings. And thats just going to be how LOCKED works

d3nd3 commented 1 year ago

We have the choice to put the !(bangleFlags & JSBF_LOCKED) check before jshHadEvent(), is that preferred, to not send the task instead of putting if statement in jswrap_banglejs_idle function?

gfwilliams commented 1 year ago

should the users be denied .on events, when they have locked set. I think thats ok, then the people who want ultimate control will disable auto-lock, then they will hook into every .on and control locking themselves within javascript. What do you think?

Yes, I think for inputs that interact with a UI they should only be available when unlocked, even when they are the WAKEUP flags. If you press the middle button to wake the watch, you don't want it to jump straight to the launcher - it should unlock the watch and the next press goes to the launcher.

But I think not all should be behind locked:

Not when locked: buttons/tap/drag/touch/stroke
All the time: twist/faceup/health/HRM/accel/etc
Maybe all the time? gesture

I'm not entirely sure if that's what's happening with your changes at the moment?

gfwilliams commented 1 year ago

We have the choice to put the !(bangleFlags & JSBF_LOCKED) check before jshHadEvent()

No, don't do that - if you're setting bangleTasks or similar then you should always call jshHadEvent. If you don't then the next time the Bangle does go to the event look for something it'll execute the event then

d3nd3 commented 1 year ago

I mean like :

if (!(bangleFlags & JSBF_LOCKED)) {
    faceUpSent = true;
    bangleTasks |= JSBT_FACE_UP;
    jshHadEvent();
}

Or let the task created and put the locked check in the other function where the task is handled.

Also I had idea for future possibility, if we ever really need more direct access to the inputs even when locked, could be a .on("touch-raw" etc... which doesn't check locked condition. Probably will never be requested though, and those die-hards could always fall back to pin reads.

d3nd3 commented 1 year ago

https://github.com/espruino/Espruino/blob/491288d9ffb88150e4213f94c4ed30c99380b90b/libs/banglejs/jswrap_bangle.c#LL1447C1-L1457C8

Is it intentionally that WAKE_ON_TWIST doesn't absorb the event like buttons do? It creates the same issue you described above.

If you press the middle button to wake the watch, you don't want it to jump straight to the launcher - it should unlock the watch and the next press goes to the launcher.

But for twist, if the app has programmed twist feature.

Yes, I think for inputs that interact with a UI they should only be available when unlocked, even when they are the WAKEUP flags.

I think you mean unless they are the WAKEUP flag, but its just hard to communicate, I think we understand each other. Its just there are :

Inputs

gesture [ wants always work ] JSBT_GESTURE_DATA
tap [ not when locked ] JSBT_ACCEL_TAPPED
drag [ not when locked ] JSBT_DRAG
stroke [ not when locked ] JSBT_STROKE

WakeInputs

twist [ wants always work NOT_CONSUME WAKE] JSBF_WAKEON_TWIST JSBT_TWIST_EVENT
faceup [ wants always work NOT_CONSUME WAKE] JSBF_WAKEON_FACEUP JSBT_FACE_UP
touch [ not when locked CONSUME WAKE ] JSBF_WAKEON_TOUCH b2 - JSBT_ACCEL_TAPPED b1 - JSBT_TOUCH_RIGHT
- buttons [ not when locked CONSUME WAKE ]
Since only 3 of them are wake, and so far it seems only the wake inputs can reset the inactivityTimer to 0. The twist and faceup should 1) pierce through lock and 2) not consume the event when woken(unlike buttons). I assume the same for faceup because you categorised it similar to twist in above.

Agree?

d3nd3 commented 1 year ago

The workflow action auto-builder not applying anymore? Ah its run on my repo, so its fine.

d3nd3 commented 1 year ago

The tap location for top bottom and right left are reverse to the docs description. Wanna double check? Its crazy how many different ways there are to input with the device. I never knew there were so many tap directions.

Unless my accelerometer is mis-placed internally ;)

gfwilliams commented 1 year ago

I mean like :

Ahh, ok - but as I said above the faceup event doesn't want a locked check on it.

Is it intentionally that WAKE_ON_TWIST doesn't absorb the event like buttons do?

Yes - you wouldn't normally use the twist event in the UI - you probably care more about receiving it all the time (maybe you want to show seconds on a watch face when twisted, even when the watch is locked).

I think you mean ....

Yes, that sounds exactly right. So is that what happens after your changes?

The workflow action auto-builder not applying anymore?

It's because you have conflicts with the master branch now (stuff changed in jswrap_bangle and the CHangelog) but I'll try and fix that

The tap location for top bottom and right left are reverse to the docs description

I just checked and on Bangle.js 2 they are correct.

So please can you change https://github.com/espruino/Espruino/pull/2384/commits/4e1403f40e1b572ec770d5dd8afb57efef3cc4c6 to only change it for the Bangle.js 1, or you'll inadvertantly break the 2.

Also just to check - you're not running it in left-handed mode or anything? It looks like we don't take account of that currently.

Please can you try and keep the changes in each PR to a minimum? Just as I thought we were getting to a consensus on the event handling there's now this tap change which definitely breaks Bangle.js 2

d3nd3 commented 1 year ago

I am sorry about putting too much into one PR. I dont know what left handed mode is, never knew it exist ,can't find reference to it, I move the left right back into the #ifdef BANGLEJS_Q3 area.

d3nd3 commented 1 year ago

inactivityTimer might be better inside that condition too.

if (lastBangleTasks != bangleTasks) {
    jshHadEvent();
    // ensure we don't sleep if touchscreen is being used
    inactivityTimer = 0;
}

gfwilliams commented 1 year ago

inactivityTimer might be better inside that condition too.

Sounds like a great plan, yet

d3nd3 commented 1 year ago

What sort of inputs do you want to trigger the inactivityTimer to reset, or should it only be triggered for inputs that have WAKE_ON in settings ?

Most of the code currently is resetting it inside the if ((bangleFlags&JSBF_WAKEON_ condition, but that line in internalTouchHandler is an exception. I'm wondering if it shouldn't be an exception.

Should : Twist, Faceup, Touch only be the input events which can reset inactivityTimer, and only if the user has WAKE_ON set for each of them?

gfwilliams commented 1 year ago

What sort of inputs do you want to trigger the inactivityTimer to reset,

I think just buttons and touchscreen (not 'tap' as that uses accelerometer)

d3nd3 commented 1 year ago

In the current master branch, the inactivityTimer is reset like this :

Currently:

TWIST - IF settings.WAKE_
FACEUP - IF settings.WAKE_
BUTTONS - ANY - settings.WAKE_ not required.
Bangle1 TOUCH - IF settings.WAKE_
Bangle2 TOUCH - ANY - settings.WAKE_ not required

notice

bangle2 doesnt require WAKEON_TOUCH, but bangle1 does. From the above post, I think you want touch event and BTN event to reset inactivityTimer without requiring WAKEON_TOUCH or WAKEON_BUTX for both Bangle1 and Bangle2. But yet for TWIST/FACEUP, they require WAKEON_TWIST and WAKEON_FACEUP to reset the timer.

Require the specifics on this before I can change anything.

gfwilliams commented 1 year ago

I think you want touch event and BTN event to reset inactivityTimer without requiring WAKEON_TOUCH or WAKEON_BUTX for both Bangle1 and Bangle2

Yes, that sounds like a good idea.

But yet for TWIST/FACEUP, they require WAKEON_TWIST and WAKEON_FACEUP to reset the timer.

Yes, I think so - otherwise maybe you're doing some kind of exercise that moves your arms around a lot and your Bangle won't ever sleep.

d3nd3 commented 1 year ago

It should be described like :

FaceUp/Twist have their inactivityTimer measured from the last time the device woke up.

Touch/Buttons have their inactivityTimer measured from the time of last input.

So FaceUp/Twist have their inativityTimer reset in code like :

bool wokeup = false;
if (lcdPowerTimeout && !(bangleFlags&JSBF_LCD_ON)) {
    jswrap_banglejs_setLCDPower(1);
    wokeup = true;
}
if (backlightTimeout && !(bangleFlags&JSBF_LCD_BL_ON)) {
    jswrap_banglejs_setLCDPowerBacklight(1);
    wokeup = true;
}
if (lockTimeout && (bangleFlags&JSBF_LOCKED)) {
    jswrap_banglejs_setLocked(false);
    wokeup = true;
}
if (wokeup) inactivityTimer = 0;

Touch/Buttons have their inactivityTImer reset like this:

inactivityTimer = 0;

gfwilliams commented 1 year ago

Yes, that sounds good to me

d3nd3 commented 1 year ago

What sort of inputs do you want to trigger the inactivityTimer to reset,

I think just buttons and touchscreen (not 'tap' as that uses accelerometer)

The question is equivalent to asking What sort of inputs are allowed to extend the wake state. Isn't a tap a legitimate input for ui? The .on.touch() for bangle2 even uses it, so its considered touch for bangle2. I think that tap should extend the wake state. Is the justfication for not using it that it might never sleep if moving around?

gfwilliams commented 1 year ago

Are we getting confused between Tap and Touch?

Tap comes from the accelerometer, so conceivably if you were in a very bouncy environment it'd get triggered. It's never used for UI input

d3nd3 commented 1 year ago

Are we getting confused between Tap and Touch?

Tap comes from the accelerometer, so conceivably if you were in a very bouncy environment it'd get triggered. It's never used for UI input

Ah what I was referring to is :

#ifdef BANGLEJS_Q3
    if ((bangleFlags&JSBF_WAKEON_TOUCH) && (tapInfo&2)) {

Which is just the WAKEON_TOUCH setting being associated with a tap event, where as on bangle1 it doesn't do that.

d3nd3 commented 1 year ago

I've created a table to make things a bit more clear.

Event	Ignores Lock?	Can Wake?	Extends Wake in Unlocked?	Consumes Wake Event?
Drag	No	No	Yes	No
Swipe	No	No	Yes	No
Stroke	No	No	Yes	No
Tap	Yes	Yes(B2)	No	No
Twist	Yes	Yes	No	No
Faceup	Yes	Yes	No	No
Button	No	Yes	Yes	Yes
Touch	No	Yes(B1)	Yes	Yes

gfwilliams commented 1 year ago

Ahh - yes. Since we can't leave the touchscreen on due to power consumption on Bangle.js 2 we fake it with the accelerometer, so we have to treat that event differently on Bangle.js 1/2

On your table:

'Tap' should ignore Lock
swipe/stroke are not super important for extending wake, since they will all generate a Touch/Drag at some point as part of them - but I guess adding the code doesn't hurt

But yes, apart from that it looks good

d3nd3 commented 1 year ago

https://github.com/espruino/Espruino/blob/8f6bb0fdfc915454ee4b3734db8a0dacfb236dca/libs/banglejs/jswrap_bangle.c#LL1646C1-L1650C4

Can you remember why this returns false here, instead of true. Why doesn't it consume the event if locked for BTN4/5 which are pseudo touch buttons. I think i might have guessed at one point, but i forgot.

Ah i think my guess in the past was that you might had made a WAKEON_BUT4 WAKEON_BUT5 setting. But so far it doesnt' exist?

Maybe it just means : treat it like a button, so pass it to the button handler. It could happily return true there, which is an efficiency gain, not sure if you interested.

gfwilliams commented 1 year ago

I can't remember - but why don't you leave it as-is if it works?

If you change it you might save a microsecond when someone presses something, but there's a good chance it might break something.

So are you confident that the code now does what's in that table?

d3nd3 commented 1 year ago

I can't remember - but why don't you leave it as-is if it works?

If you change it you might save a microsecond when someone presses something, but there's a good chance it might break something.

So are you confident that the code now does what's in that table?

Not just yet. I am preparing it, give me a day.

d3nd3 commented 1 year ago

I am going to test it my device. Take care when merging it because I have duplicated: JsBangleTasks lastBangleTasks = bangleTasks;

if (lastBangleTasks != bangleTasks) {
    jshHadEvent();
    inactivityTimer = 0; // extend wake - only unlocked reaches here
  }

from your other commit. Because I've added inactivityTimer into there. And wanted it to test.

d3nd3 commented 1 year ago

I am confident that the code does what is in that table. Its ready to merge. If there is a 1% error, it should be spotted quickly and an extra commit can always fix it. Difficulty for programmers to be 100% certain ;)

gfwilliams commented 1 year ago

This looks good apart from that code I just mentioned in the wakeup handler... If there's no reason for calling those functions direct I'm happy to swap it to bangleTasks |= JSBT_LCD_ON/etc here.

d3nd3 commented 1 year ago

Yes, I moved the code and didn't realise the implication there. Should be bangleTasks |= JSBT_LCD_ON etc.

gfwilliams commented 1 year ago

Looks good - thanks!

espruino / Espruino