Advanced clock - Githubissues

matheusmoreira commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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.

matheusmoreira commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

matheusmoreira commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

huh, we stand corrected, sorry!

matheusmoreira commented 3 months ago

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 commented 3 months ago

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

joeycastillo / Sensor-Watch

Advanced clock #373