Advanced clock

[matheusmoreira]

Completely refactored the clock face of the sensor watch. It has been reorganized and optimized.

I also implemented some compile-time customization features for 24h only mode as well as the ability to force 24h mode movement-wide.

[wryun]

I appreciate the effort you've gone to in separating out the code and making it much more 'self-documenting'; i.e. that the function names accurately reflect what's going on.

Two concerns:

• I think major refactoring is best done when keeping the functionality equivalent (i.e. I wouldn't mix the 24-hour forcing with your refactoring).
• I worry you've gone a little too far; IMO too many functions adds cognitive overhead as well, making it harder on first glance to tell what the code is actually doing (lots of jumping backwards and forwards). Though I used to be a fan, I no longer find as much value in very small functions that are only used once (e.g. things like 'clock_indicate_alarm'); I think they end up making this clock face look more complex than it actually is. The LoC has almost doubled! But probably what makes me most wary is that your coding style feels almost Lisp-ish and sits oddly with the rest of the C code in the repository.

[matheusmoreira]

@wryun Thanks for the review.

• I think major refactoring is best done when keeping the functionality equivalent (i.e. I wouldn't mix the 24-hour forcing with your refactoring).

Absolutely. I added this feature in because at least one other person wanted it. If it is deemed too specific to warrant inclusion, I can easily remove it.

I think the 24 hour only mode is a valuable addition to the face though. It will reduce code size, making room for more functionality.

IMO too many functions adds cognitive overhead as well, making it harder on first glance to tell what the code is actually doing (lots of jumping backwards and forwards). Though I used to be a fan, I no longer find as much value in very small functions that are only used once (e.g. things like 'clock_indicate_alarm'); I think they end up making this clock face look more complex than it actually is.

I understand what you mean. I think I had a good rationale for factoring out these functions though.

• clock_indicate centralizes the if statement in one place. As you mentioned, the more ifs, the more complex. I've even seen similar bits of code in other faces. It should be moved to the utilities file.
• The other functions make explicit which bit is setting which indicator.
  • For example, the WATCH_INDICATOR_SIGNAL and clock->time_signal_enabled symbols are right next to each other with no other noise around.
• They read like imperative English commands in the rest of the watch face code.

The LoC has almost doubled!

Low line count does not imply low complexity. The number of lines may have increased but the logic in each function has been significantly reduced.

But probably what makes me most wary is that your coding style feels almost Lisp-ish and sits oddly with the rest of the C code in the repository.

I can't deny the foreign language influences. I've learned plenty of languages, including Lisp and Scheme and Ruby. C was my first language though, is what I write most and where I feel at home.

My coding style is informed by the Linux kernel coding style:

6. Functions

Functions should be short and sweet, and do just one thing. They should fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24, as we all know), and do one thing and do that well.

Use helper functions with descriptive names (you can ask the compiler to in-line them if you think it's performance-critical, and it will probably do a better job of it than you would have done).

I'm particularly motivated by this point here:

if you have a complex function, and you suspect that a less-than-gifted first-year high-school student might not even understand what the function is all about, you should adhere to the maximum limits all the more closely

I learned to program when I was 13 so I know what that's like. Also, a high school student approached me privately on Discord seeking help with making a custom watch face. I didn't see the message in a timely manner due to life and they seem to have disappeared. Hopefully this will make it easier for them.

[joeycastillo]

I love this effort, and this refactor does look like a dramatic improvement to my old “simple clock” face. Can I make a suggestion, though? I think it would be preferable for this new clock_face to exist alongside the classic simple_clock_face, and for the moment to leave simple_clock_face in the default configuration. This would allow a larger group of folks to test out your new clock_face before pushing the change to everyone. Especially for a watch face that's so core to the experience of the device, I think more testing would be valuable.

Your PR is sufficiently well segmented that I think it would be as simple as restoring the simple_clock_face header and source files, and removing the change in movement_config.h. This would keep the standard build the same while letting folks opt into the new and improved version. If testing goes well, we can aim toward eventually removing simple_clock_face and replacing it with your refactored version.

Also, I do agree that the “Force 24H” / “24H only” seems a bit out of scope. I think it would be valuable to have a compiler flag to set 24H mode once, at first boot. But after that, the wearer should have the option to change back to 12H mode without disassembling their device. It's fairly out of the way in the Preferences watch face; you're unlikely to be frustrated by an accidental change of mode, so I don't see a benefit to removing this customizability entirely.

[matheusmoreira]

@joeycastillo Thanks for the review.

I think it would be preferable for this new clock_face to exist alongside the classic simple_clock_face, and for the moment to leave simple_clock_face in the default configuration.

Absolutely. I will make it so.

This would allow a larger group of folks to test out your new clock_face before pushing the change to everyone. Especially for a watch face that's so core to the experience of the device, I think more testing would be valuable.

Indeed, that is a good idea. I'm open to any and all feedback and will assign myself to any issues found.

I think it would be valuable to have a compiler flag to set 24H mode once, at first boot.

Great! I think so too. I'll implement this feature in a separate pull request so it can be evaluated independently of the clock face.

so I don't see a benefit to removing this customizability entirely

The rationale for the feature is reduction of binary size.

I wrote it so that the runtime customizability remains the default: all features of the simple_clock_face are maintained and it remains able to deal with both 12h and 24h modes. If the user explicitly chooses to, they can invoke make CLOCK_FACE_24H_ONLY=yes in order to delete the 12h support.

The code is structured so as to facillitate dead code elimination by the compiler: I expect the compiler to delete from the resulting binary all the code needed to handle 12 hour mode, leading to a smaller memory footprint. I haven't actually measured the impact. I will do so and follow up here with the data.

[matheusmoreira]

I haven't actually measured the impact. I will do so and follow up here with the data.

I got an 88 byte reduction in code size: 103892 - 103804 = 88.

$ make clean
$ make
# ...
Memory region         Used Size  Region Size  %age Used
      bootloader:           0 B         8 KB      0.00%
             rom:      103892 B       240 KB     42.27%
          eeprom:           0 B         8 KB      0.00%
             ram:       12792 B        32 KB     39.04%
# ...
size:
   text	  data	   bss	   dec	   hex	filename
 103892	  1788	 11004	116684	 1c7cc	build/watch.elf
 103892	  1788	 11004	116684	 1c7cc	(TOTALS)
UF2CONV build/watch.uf2
Converting to uf2, output size: 211456, start address: 0x2000
Wrote 211456 bytes to build/watch.uf2
$ du -h movement/make/build/watch.uf2 
208K	movement/make/build/watch.uf2

$ make clean
$ make CLOCK_FACE_24H_ONLY=yes
# ...
Memory region         Used Size  Region Size  %age Used
      bootloader:           0 B         8 KB      0.00%
             rom:      103804 B       240 KB     42.24%
          eeprom:           0 B         8 KB      0.00%
             ram:       12792 B        32 KB     39.04%
# ...
size:
   text	  data	   bss	   dec	   hex	filename
 103804	  1788	 11004	116596	 1c774	build/watch.elf
 103804	  1788	 11004	116596	 1c774	(TOTALS)
UF2CONV build/watch.uf2
Converting to uf2, output size: 211456, start address: 0x2000
Wrote 211456 bytes to build/watch.uf2
$ du -h movement/make/build/watch.uf2 
208K	movement/make/build/watch.uf2

Forced 24h mode results in a reduction of 80 byes: 103892 - 103812 = 80.

$ make clean
$ make CLOCK_FACE_FORCE_24H=yes
# ...
Memory region         Used Size  Region Size  %age Used
      bootloader:           0 B         8 KB      0.00%
             rom:      103812 B       240 KB     42.24%
          eeprom:           0 B         8 KB      0.00%
             ram:       12792 B        32 KB     39.04%
# ...
size:
   text	  data	   bss	   dec	   hex	filename
 103812	  1788	 11004	116604	 1c77c	build/watch.elf
 103812	  1788	 11004	116604	 1c77c	(TOTALS)
UF2CONV build/watch.uf2
Converting to uf2, output size: 211456, start address: 0x2000
Wrote 211456 bytes to build/watch.uf2
$ du -h movement/make/build/watch.uf2 
208K	movement/make/build/watch.uf2

Results might be better with whole program or link time optimization. See PR #326.

[matheusmoreira]

OK, I have made the clock_face a separate face for independent evaluation. I have also dropped the commits for 24 hour forced/only modes for the time being.

[matheusmoreira]

I am now running this clock face in my watch. No issues so far.

[theAlexes]

The sensor watch differs from the stock watch in this regard, using the 0,17 [)))] segment for indicating the alarm is set, and the 0,16 🔔 segment for indicating hourly chime. This is probably worth a compile-time toggle STOCK_INDICATOR_STYLE, defaulting to sensor-watch "swapped" style.

[matheusmoreira]

The sensor watch differs from the stock watch in this regard

Oh, so it was an intentional change? I consulted the documentation and was under the impression it had been swapped unintentionally or something.

enum WatchIndicatorSegment

    WATCH_INDICATOR_SIGNAL 
        The hourly signal indicator
        also useful for indicating that sensors are on

    WATCH_INDICATOR_BELL 
        The small bell indicating that an alarm is set

This is probably worth a compile-time toggle STOCK_INDICATOR_STYLE, defaulting to sensor-watch "swapped" style.

I'll add this feature!

[theAlexes]

Yeah, it got put that way in pr #96. There was some discussion on the forum, iirc, about putting it back the other way, but having it be a compile-time configurable seems ideal.

[joeycastillo]

The documentation might be out of date but I thought we switched it back to match the stock behavior some time ago. I’d rather not have the complexity of a compile time constant; ideally the indicators should match what the stock F91W firmware does, which I thought we did in the code even if I failed to update the docs…

[theAlexes]

huh, we stand corrected, sorry!

[matheusmoreira]

ideally the indicators should match what the stock F91W firmware does

The current state of this PR does that!

I’d rather not have the complexity of a compile time constant

Okay.

[matheusmoreira]

Included in PR #380, not closed as merged because it was rebased.