Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow toggling an alarm via clkinfo #3579

Merged
merged 12 commits into from
Oct 7, 2024

Conversation

bobrippling
Copy link
Collaborator

No description provided.

@thyttan
Copy link
Collaborator

thyttan commented Sep 22, 2024

Thanks!

The intended function seems to work well. But activating repeatable timers is a bit wonky.

Bug with repeatable timers

If a newly set timer has run out and is turned off, if I start it via the clock info entry it will run out again immediately. If I start it after a minute or two it will instead say it will run out in "24h".

Reproduce

  1. Set a new timer in the alarm app for 15 seconds. Uncheck "Delete After Expiration".
  2. Navigate to a clock face with clock infos on it (I used 'bwclk').
  3. Wait for the timer to run out.
  4. Click "Stop" on the timer alert screen.
  5. Click the now inactive 15 s timer on the clock info to restart it.
  6. The clock info says "0m" (or "-1m") and the timer runs out immediately.
  7. Click "Stop" on the timer alert screen.
  8. Wait a minute or two.
  9. Click the inactive timer on the clock info again.
  10. The clock info will say it will run out in "24h" and the timer will indeed not run out in the specified 15 seconds.

Other considerations

  • Events set with "Delete After Expiration" unchecked will be toggleable via the clock info entry.
    • This may be confusing. A user may see the entry on the clock info and toggle it on and expect an alarm to go off at that time the next day but it will not go off until the next year (depending on configuration of the event).
    • I think the icon for events should be different from the one for standard alarms.
    • Instead of clock time, maybe the date of the event should be displayed?
  • Similar to the situation with events, maybe we want to differentiate between standard alarms that will not fire soon (the next time the clock is the time displayed on the clock info entry) and those that will not go off as soon?
    • Maybe writing a letter on the alarm icon corresponding to the weekday it will fire on next?

@bobrippling
Copy link
Collaborator Author

Nice finds! For events, perhaps we ignore these for now so the user can't toggle them, to avoid any confusion. For timers, I've made it so we restart the timer. What do you think?

@thyttan
Copy link
Collaborator

thyttan commented Sep 23, 2024

I think that's a good idea to make events not toggleable for now - longer term maybe they shouldn't even be displayed in the same clock info? I don't know.

Restarting the timer makes sense!

Some bugs

  • Toggling a timer or alarm on through the clock info removed all other alarms, timers and events completely!
  • Toggling off an alarm through the clock info didn't actually disable it.

@bobrippling
Copy link
Collaborator Author

I think that's a good idea to make events not toggleable for now - longer term maybe they shouldn't even be displayed in the same clock info? I don't know.

Yeah I don't know either - I don't use them enough to have a good opinion.

Restarting the timer makes sense!

Sweet!

Some bugs

  • Toggling a timer or alarm on through the clock info removed all other alarms, timers and events completely!

I can't reproduce this - I had the IDE running, tapped on a disabled alarm and noticed that it became enabled - widalarmeta also activated, showing a countdown. It then fired at the time - this was created (but disabled) via the alarm app, is that what you're using?

  • Toggling off an alarm through the clock info didn't actually disable it.

Similarly, I created an alarm set for 1 minute's time, switched back to the clock and disabled it via clkinfo. The alarm didn't fire and I confirmed (with require("sched").getAlarms()) that it was off. Perhaps I have an odd setup? Do you have a sched.json that has some repros?

@thyttan
Copy link
Collaborator

thyttan commented Sep 30, 2024

Perhaps I have an odd setup?

Or perhaps I do 🤔🙃 I can do more testing in a day or two.

@bobrippling
Copy link
Collaborator Author

bobrippling commented Oct 1, 2024

Thanks, and thanks for your testing so far too :)

If it helps, I've deployed to my gh-pages too

@thyttan
Copy link
Collaborator

thyttan commented Oct 1, 2024

I did some more testing - the two problems seem to arise:

  • independent of the used clock face.
    • Tried with bwclk and pebblepp.
  • independent of firmware.
    • Tried with firmwares stable 2v24 and 2v24.61.
  • independent of using my setup or doing a factory reset before trying.
  • independent of pretokising apps or not.

I don't see errors in the Web IDE.

EDITS:

  1. test without pretokenized apps
  2. Could it be something with timezones? That could be one difference between us two maybe? I'm at GMT+2.

@bobrippling
Copy link
Collaborator Author

Interesting - I'm GMT+0, shall take a look at whether that could be doing anything. These are the steps I'm following - perhaps I've missed something that brings up the bug?

Disabling an alarm

  1. Create an alarm for 2 minute's time (via the alarm app)
  2. Return to clock face
  3. Confirm alarm is active (via both widalarmeta and seeing the alarm in clkinfo as active)
  4. Tap alarm in clkinfo

Outcomes:

  1. Observe it transitions to the inactive icon
  2. Wait 2 minutes, confirm alarm doesn't fire

Enabling an alarm

  1. Create a disabled alarm for 2 minute's time (via the alarm app)
  2. Return to clock face
  3. Confirm alarm is disabled (no widalarmeta shown, icon is disabled / has the little 'x')
  4. Tap alarm in clkinfo

Outcomes:

  1. Active icon is shown
  2. widalarmeta appears, showing time to alarm (~1 min)
  3. Wait 2 mins, alarm fires

@thyttan
Copy link
Collaborator

thyttan commented Oct 1, 2024

Disabling an alarm

1. Create an alarm for 2 minute's time (via the alarm app)

2. Return to clock face

3. Confirm alarm is active (via both `widalarmeta` and seeing the alarm in clkinfo as active)

4. Tap alarm in clkinfo

Outcomes:

1. Observe it transitions to the inactive icon

2. Wait 2 minutes, confirm alarm doesn't fire

My outcomes for that disabling flow on factory reset, non-pretokenized apps, fw 2v24, bwclklite differs from yours:

  1. The clock info transitions to the inactive icon.
  2. widalarmeta does NOT stop displaying the eta.
  3. Wait 2 minutes - the alarm fires anyway.

Enabling an alarm

1. Create a disabled alarm for 2 minute's time (via the alarm app)

2. Return to clock face

3. Confirm alarm is disabled (no `widalarmeta` shown, icon is disabled / has the little 'x')

4. Tap alarm in clkinfo

Outcomes:

1. Active icon is shown

2. `widalarmeta` appears, showing time to alarm (~1 min)

3. Wait 2 mins, alarm fires

For this enabeling flow I get the same result as you.

@thyttan
Copy link
Collaborator

thyttan commented Oct 1, 2024

Test for "Toggling a timer or alarm on through the clock info removed all other alarms, timers and events completely!"

  1. Connect to Web IDE.
  2. Create two alarms and leave them enabled, I choose 12:00 and 14:00.
  3. Return to clock face
  4. Run require("sched").getAlarms() to see both alarms in the Web IDE console field. Both have on:true.
  5. Toggle off the 12:00 alarm through the clock info.
    • notice widalarmeta does not change.
  6. Run require("sched").getAlarms() to see both alarms in the Web IDE console field. Both have on:true.
  7. Toggle on the 12:00 alarm through the clock info.
  8. Run require("sched").getAlarms() to see ONLY THE 12:00 alarm in the Web IDE console field. The 14:00 alarm has been removed.
    • Also verifiable by opening the alarm app, then only showing the 12:00 alarm.

apps/sched/clkinfo.js Outdated Show resolved Hide resolved
@bobrippling
Copy link
Collaborator Author

My outcomes for that disabling flow on factory reset, non-pretokenized apps, fw 2v24, bwclklite differs from yours:

1. The clock info transitions to the inactive icon.

2. `widalarmeta` does NOT stop displaying the eta.

3. Wait 2 minutes - the alarm fires anyway.

Interesting, I'll have a go at that - thanks for the IDE route as well!

@bobrippling
Copy link
Collaborator Author

I believe I have this fixed now, back to the original code but with timers working - thanks for your patience, thorough testing and advice, I appreciate it :)

To go back to your original questions:

  • Events set with "Delete After Expiration" unchecked will be toggleable via the clock info entry.
    • This may be confusing. A user may see the entry on the clock info and toggle it on and expect an alarm to go off at that time the next day but it will not go off until the next year (depending on configuration of the event).

Agreed - I think our two options are to hide the "Delete After Expiration" ones from clkinfo, or leave for now and see?

  • I think the icon for events should be different from the one for standard alarms.

Yes I think so, I'm no good with icons but I could have a whirl

  • Instead of clock time, maybe the date of the event should be displayed?

Sounds good!

  • Similar to the situation with events, maybe we want to differentiate between standard alarms that will not fire soon (the next time the clock is the time displayed on the clock info entry) and those that will not go off as soon?

    • Maybe writing a letter on the alarm icon corresponding to the weekday it will fire on next?

I like that idea, I could give that try too - maybe as a separate PR?

@thyttan
Copy link
Collaborator

thyttan commented Oct 5, 2024

Yes - it seems to work as expected on my end as well! Thanks 😃

Regarding events I think we go a real long way by changing the icon (both in the app and clock info) to something like
image
or
image
from https://icons8.com/icons/set/event .

So maybe we could bring that into this PR and then merge? Or we could do that + change to displaying date in a later PR.

What do you think?

I also think "day letter" on alarms fits better in a separate PR! :)

@bobrippling
Copy link
Collaborator Author

Sweet - I like the look of those icons, gone with the latter which seems to fit in with the others but don't mind changing. I can't seem to get it to show though, I've encoded it as a 1-bit black/white Image String but it doesn't show up for me 🔍

(sorted the text too - left off the year for breverity, let me know what you think)

@thyttan
Copy link
Collaborator

thyttan commented Oct 6, 2024

I also don't see the icon 🤔

Otherwise - looking good!

apps/sched/clkinfo.js Outdated Show resolved Hide resolved
@thyttan
Copy link
Collaborator

thyttan commented Oct 6, 2024

What do you say we merge this?

@bobrippling
Copy link
Collaborator Author

Sounds good, yes please!

@thyttan
Copy link
Collaborator

thyttan commented Oct 7, 2024

Thanks 🌸☺️

@thyttan thyttan merged commit 8ae59c8 into espruino:master Oct 7, 2024
1 check passed
@bobrippling bobrippling deleted the feat/alarm-clkinfo branch October 7, 2024 16:53
@bobrippling
Copy link
Collaborator Author

My pleasure - out of interest, do you know how to go from an image string to the image itself? (For getting at some of these images)

@thyttan
Copy link
Collaborator

thyttan commented Oct 7, 2024

I didn't test this myself but seems like a solution: https://onlinepngtools.com/convert-base64-to-png#tool

@bobrippling
Copy link
Collaborator Author

Thanks - it seems I might need a subscription but I've found another method (guessing at the dimensions - I've tried multiple):

$ base64 -D < alarm_on.b64 > alarm_on.raw
$ magick -depth 1 -size 8x8 rgb:alarm_on.raw alarm_on.png

Except I get back:

magick: unexpected end-of-file 'alarm_on.raw': No such file or directory @ error/rgb.c/ReadRGBImage/249.

Despite:

$ ls alarm_on.raw 
alarm_on.raw 

@gfwilliams how do you go from the base64 strings to an image?

@gfwilliams
Copy link
Member

The image format itself is covered in https://www.espruino.com/Graphics#strings - but it's unique to Espruino. You could use imagemagick after skipping the few bytes of header...

But what I do is just use Espruino. In the simplest case just draw the image to the screen and dump the contents of the screen (which you can then copy/paste from the IDE console), then manually crop it out:

let img = atob("GBiBAAAAAAAAAAAAAA//8B//+BgAGBgAGBgAGB//+B//+B//+B/++B/8+B/5+B8z+B+H+B/P+B//+B//+B//+A//8AAAAAAAAAAAAA==")
g.clear(1).drawImage(img).dump();

Or you can create a Graphics instance of the right size and dump that:

let img = atob("GBiBAAAAAAAAAAAAAA//8B//+BgAGBgAGBgAGB//+B//+B//+B/++B/8+B/5+B8z+B+H+B/P+B//+B//+B//+A//8AAAAAAAAAAAAA==")
let m = g.imageMetrics(img);
let t = Graphics.createArrayBuffer(m.width,m.height,m.bpp,{msb:true});
t.drawImage(img).dump();

@bobrippling
Copy link
Collaborator Author

Ah I see, so the first four bytes are a header in the string-only version, got it. Thanks!

@gfwilliams
Copy link
Member

Ah I see, so the first four bytes are a header in the string-only version, got it. Thanks!

Yes, but it can vary. If it's not transparent it's only 3 bytes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants