KDEV - #1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive watchface and UI subsystem for what appears to be a smartwatch project running on Zephyr RTOS. It adds display driver support, graphics functionality, font assets, and watchface implementations with both digital and information display modes.
Key Changes
- Display driver implementation for LPM013M126A LCD (176x176 pixels, 4-bit color)
- Watchface manager with digital clock and information notification displays
- Multiple font assets (TomThumb, Org_01, Picopixel, FreeSerif variants, Roboto, Tiny3x3a)
- Graphics framework integration for rendering text and UI elements
Reviewed changes
Copilot reviewed 61 out of 98 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/watchface/face_manager.h | Header defining watchface manager API for display initialization and updates |
| src/ui/watchface/face_manager.c | Implementation managing digital watchface and notification information display |
| src/ui/watchface/face_main/face_main.h | Header for main watchface with time, date, weather, and status indicators |
| src/ui/watchface/face_main/face_main.c | Implementation rendering main watchface frame, time, and date displays |
| src/ui/watchface/face_information/face_information.h | Header for information/notification display screen |
| src/ui/watchface/face_information/face_information.c | Implementation with commented-out duplicate code from face_main |
| src/ui/watchface/face_digital.h | Header for digital watchface with comprehensive time and date display |
| src/ui/watchface/face_digital.c | Implementation with two display modes - minimal and detailed with icons |
| src/ui/fonts/*.h | Font asset files (TomThumb, Org_01, Picopixel, FreeSerif, Roboto, Tiny3x3a) |
| src/ui/driver/LPM013M126A.h | LCD driver header with display specifications and color definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // void face_main_frame(kw_gfx_t *g) { | ||
| // kw_gfx_fill_screen(g, LCD_COLOR_BLACK); // black background | ||
| // kw_gfx_fill_rect(g, 0, 30, 176, 90, LCD_COLOR_WHITE); // white panel | ||
| // } | ||
|
|
||
| // void face_main_time_update(kw_gfx_t *g, uint8_t hour, uint8_t minute) { | ||
| // char time_str[8]; | ||
| // snprintf(time_str, sizeof(time_str), "%02u:%02u", hour, minute); | ||
| // int w_time = text_width_gfxfont(&FreeSansBold24pt7b, time_str); | ||
| // int x_time = (176 - w_time) / 2; | ||
| // int y_time = 78; | ||
| // kw_gfx_draw_text_gfxfont(g, x_time, y_time, time_str, &FreeSansBold24pt7b, LCD_COLOR_BLACK); | ||
| // } | ||
|
|
||
| // void face_main_date_update(kw_gfx_t *g, const char *weekday, const char *date_str) { | ||
| // char date_line[32]; | ||
| // snprintf(date_line, sizeof(date_line), "%s %s", weekday, date_str); | ||
| // int w_date = text_width_gfxfont(&FreeSans9pt7b, date_line); | ||
| // kw_gfx_draw_text_gfxfont(g, (176 - w_date)/2, 105, date_line, &FreeSans9pt7b, LCD_COLOR_BLACK); | ||
| // } |
There was a problem hiding this comment.
Remove commented-out code. These functions appear to be copied from face_main.c and serve no purpose here. Commented-out code reduces maintainability and creates confusion about the intended implementation.
|
|
||
| /* Large time display - HH:MM format */ | ||
| char time_str[8]; | ||
| snprintf(time_str, sizeof(time_str), "%u:%02u", hour, minute); |
There was a problem hiding this comment.
Inconsistent time formatting between functions. In face_digital_update line 28 uses %02u:%02u for both hour and minute, but here only minute is zero-padded. This creates inconsistent display between the two watchface modes. Use %02u:%02u for consistent formatting.
| kw_gfx_init(&g, 176, 176, face_draw_cb, face_refresh_cb, NULL); | ||
|
|
||
| cmlcd_init(); | ||
| cmlcd_backlight_set(30); /* 100% brightness */ |
There was a problem hiding this comment.
Comment incorrectly states '100% brightness' but the value passed is 30. If this represents 30% brightness, update the comment to '30% brightness'. If 30 represents 100% on a different scale, clarify the scale in the comment.
| #include "../../driver/KW_GFX_font.h" | ||
| #include "../../driver/KW_GFX.h" | ||
|
|
||
| // === include fonts) === |
There was a problem hiding this comment.
Inconsistent comment syntax. The closing parenthesis should match the opening format. Change '// === include fonts) ===' to '// === include fonts ==='.
| #include "../../driver/KW_GFX_font.h" | ||
| #include "../../driver/KW_GFX.h" | ||
|
|
||
| // === include fonts) === |
There was a problem hiding this comment.
Inconsistent comment syntax. The closing parenthesis should match the opening format. Change '// === include fonts) ===' to '// === include fonts ==='.
| /** @def | ||
| *ID for setBlinkMode | ||
| */ |
There was a problem hiding this comment.
Missing space after asterisk in multi-line comment. Change ' *ID' to ' * ID' to maintain consistent comment formatting.
No description provided.