search

joeycastillo / Sensor-Watch

A board replacement for the classic Casio F-91W wristwatch
Other
1.02k stars 210 forks source link

Multiple fixes and improvements #380

Closed matheusmoreira closed 3 months ago

matheusmoreira commented 3 months ago

The following pull requests have been merged and tested on hardware:

  1. https://github.com/joeycastillo/Sensor-Watch/pull/329
  2. https://github.com/joeycastillo/Sensor-Watch/pull/340
  3. https://github.com/joeycastillo/Sensor-Watch/pull/344
  4. https://github.com/joeycastillo/Sensor-Watch/pull/295
  5. https://github.com/joeycastillo/Sensor-Watch/pull/316
  6. https://github.com/joeycastillo/Sensor-Watch/pull/373
    • With 24h only feature
  7. https://github.com/joeycastillo/Sensor-Watch/pull/371
  8. https://github.com/joeycastillo/Sensor-Watch/pull/369
  9. https://github.com/joeycastillo/Sensor-Watch/pull/376

The latest commits were fetched and merged, preserving authorship and date information. I also extensively credited all contributors in the merge commits.

theAlexes commented 3 months ago

Thanks for the thorough commenting and testing :) Once #373 is updated on this branch, we'll be happy to merge this.

matheusmoreira commented 3 months ago

Something I forgot to mention about this branch: the clock face replaces the simple clock face in it. I thought it was alright since I've been running it for weeks now with no issues, and I also tested alarms and hourly chimes too. I can split it up again if needed though.

theAlexes commented 3 months ago

@joeycastillo did request that the simple clock face remain in place, so i think restoring that will make this branch mergeable :)

matheusmoreira commented 3 months ago

Yes. More thorough testing of the new clock face was required before it could replace the original, so I created this branch with the clock face replaced in order to flash it to my device for testing. Eventually I started merging all the other PRs as well and it turned into a Linux style next branch.

Not sure how much testing is enough to be honest. Been running it every day for weeks now, tested every function and have found no issues. What do you think @joeycastillo?

matheusmoreira commented 3 months ago

I'm on mobile at the moment and don't have access to a computer right now. When I get home I'll split the faces up in order to unblock this, assuming Joey hasn't changed his mind.

theAlexes commented 3 months ago

the extensive testing and thorough annotation is quite appreciated, for sure. I think the goal behind having the simple clock face separate from the advanced one is that the simple face serves as a low-complexity example for others to build new faces from, but we'll have to defer to Joey on this.

matheusmoreira commented 3 months ago

Yeah, I have this propensity to making small functions. I personally find it easier to understand but others found to be "Lisp-like", a quality which I didn't even try to refute due to my love of lisp. In my mind the refactored code is easier to understand, but I'll respect it if others don't find it so.

814d3 commented 1 month ago

@matheusmoreira, I checked simple clock face and "your" clock face in the online firmware builder. They act different in indicating alarm and hourly chime. Refering to https://github.com/joeycastillo/Sensor-Watch/pull/373#issuecomment-2001936555 I thought they should be identically?

matheusmoreira commented 1 month ago

@814d3

They act different in indicating alarm and hourly chime.

Yes. It appears the hourly chime and alarm indicators had been swapped in movement. I noticed this and swapped them back. So clock_face differs from simple_clock_face but matches the stock watch's behavior. This appears to have been the original intention of the maintainers.

814d3 commented 1 month ago

Well, what I read about the original watch behavior is, that the bell indicates a hourly signal und the waves a alarm - it's the same for the simple_clock_face, but not in clock_face. But you are right, in the documentation (https://github.com/joeycastillo/Sensor-Watch/pull/373#issuecomment-2001022142) it's swapped and should be corrected.

matheusmoreira commented 4 weeks ago

@814d3

You're absolutely right about that. I based the decision to switch the icons on the watch library documentation. Turns out the documentation was swapped all along!!

https://github.com/joeycastillo/Sensor-Watch/blob/439843f56b1cd6f8976b2ecabb27457a3b3e233c/watch-library/shared/watch/watch_slcd.h#L44-L51

Screen elements descriptions, Module No. 593 manual

qw593.pdf

There's the alarm-on-mark and the time-signal-on-mark. The watch library documentation says the bell indicates alarms, but on the original module the bell indicates the time signal.

So this should definitely be fixed. Perhaps the fix should happen inside movement at the API level. The constants WATCH_INDICATOR_BELL and WATCH_INDICATOR_SIGNAL refer to the same thing.