-
Notifications
You must be signed in to change notification settings - Fork 245
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
Added ability to make default time on the watch be PC clock's during … #414
base: main
Are you sure you want to change the base?
Conversation
movement/movement.c
Outdated
for (int i = 0, count = sizeof(movement_timezone_offsets) / sizeof(movement_timezone_offsets[0]); i < count; i++) { | ||
if (movement_timezone_offsets[i] == MAKEFILE_TIMEZONE) { | ||
movement_state.settings.bit.time_zone = i; | ||
break; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there should be a way to set this statically, without looping over the time zones at runtime. We need to figure out the index of the timezone. Perhaps the makefile can figure out or call a script that figures it out, then it can pass the index via preprocessor definition.
watch-library/hardware/main.c
Outdated
#ifdef MAKEFILE_CURR_YEAR | ||
date_time.unit.year = MAKEFILE_CURR_YEAR; | ||
#else | ||
date_time.unit.year = 3; | ||
#endif | ||
#ifdef MAKEFILE_CURR_MONTH | ||
date_time.unit.month = MAKEFILE_CURR_MONTH; | ||
#endif | ||
#ifdef MAKEFILE_CURR_DAY | ||
date_time.unit.day = MAKEFILE_CURR_DAY; | ||
#endif | ||
#ifdef MAKEFILE_CURR_HOUR | ||
date_time.unit.hour = MAKEFILE_CURR_HOUR; | ||
#endif | ||
#ifdef MAKEFILE_CURR_MINUTE | ||
date_time.unit.minute = MAKEFILE_CURR_MINUTE; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest defining static functions that return the initial time components and putting the #ifdef
s in them. This avoids adding complexity to the initialization function. They'll just return constants so the compiler is likely to eliminate the functions when they are compiled.
static uint16_t get_initial_year(void)
{
#ifdef MAKEFILE_CURR_YEAR
return MAKEFILE_CURR_YEAR;
#else
return 3;
#endif
}
// ...
date_time.unit.year = get_initial_year();
make.mk
Outdated
CFLAGS += -DMAKEFILE_TIMEZONE=$(TIMEZONE) | ||
CFLAGS += -DMAKEFILE_CURR_YEAR=$(CURRENT_YEAR) | ||
CFLAGS += -DMAKEFILE_CURR_MONTH=$(CURRENT_MONTH) | ||
CFLAGS += -DMAKEFILE_CURR_DAY=$(CURRENT_DAY) | ||
CFLAGS += -DMAKEFILE_CURR_HOUR=$(CURRENT_HOUR) | ||
CFLAGS += -DMAKEFILE_CURR_MINUTE=$(CURRENT_MINUTE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest not mentioning MAKEFILE
in these variables. Users might add them to a header file and include it instead. The makefile therefore wouldn't be the only source of these values. See also PR #295 for example.
I suggest INITIAL_TIMEZONE
, INITIAL_YEAR
, and so on.
CURRENT_MONTH := $(shell date +"%-m") | ||
CURRENT_DAY := $(shell date +"%-d") | ||
CURRENT_HOUR := $(shell date +"%-H") | ||
CURRENT_MINUTE := $(shell date +"%-M") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a direct dependency on GNU coreutils for building the firmware. I consider that to be perfectly OK but then again I've been running Linux for over a decade. I have no idea how this impacts Windows users. Maintainers might want to double check this.
date_time.unit.month = get_initial_month(); | ||
date_time.unit.day = get_inital_day(); | ||
date_time.unit.hour = get_inital_hour(); | ||
date_time.unit.minute = get_inital_minute(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is worth much, but it's been helpful for me. For Linux users who only add these 4 lines and don't change any other files, sed
can do all the work except setting the timezone offset (though it's kind of hacky).
From movement/make
:
sed -Ei "s/(unit.year = ).*(;)/\1$(( $(date +%-g) - 20 ))\2/;s/(unit.month = ).*(;)/\1$(date +%-m)\2/;s/(unit.day = ).*(;)/\1$(date +%-d)\2/;s/(unit.hour = ).*(;)/\1$(date +%-H)\2/;s/(unit.minute = ).*(;)/\1$(( $(date +%-M) + 1))\2/" ../../watch-library/hardware/main.c && make COLOR=YOURCOLOR && make install COLOR=YOURCOLOR
I know it's not pretty, but it is handy. (It also adds 1 minute to the time to allow you to put the watch back together.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!!
…ole to avoid confusion on M/D/Y vs D/M/Y
…ey aren't needed otherwise
da59f6b
to
79a1b98
Compare
…e hardcoded for the wanted color
This is a good PR but since movement 2.0 will bring about a new build system it seems pointless to merge this in right now. Let's wait and revisit this after the refactoring is done! |
When running
make
, you can includeDATE=
, which will set the default time on the watch to the PC's.Below are example outputs of the setting.
Context in Discord