Skip to content
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

Face for tracking the menstrual cycle #250

Merged
merged 36 commits into from
Sep 18, 2024
Merged

Conversation

jokomo24
Copy link
Contributor

I discovered the Casio F-91W through my partner, appreciated the retro analog aesthetic of the watch, and got one for myself. Soon afterward I discovered the Sensor Watch project and ordered two boards, thank you!! I introduced the Sensor Watch to my partner who inquired whether she could track her menstrual cycle. So I decided to implement a menstrual cycle watch face that also calculates the peak fertility window using The Calendar Method.

@joeycastillo
Copy link
Owner

This is such a great idea, thanks for making this! And monumental apologies for the delay in reviewing it.

All looks good, except for one warning that I need your guidance to resolve. The switch statement on line 326, handing the alarm long press, can fall through. I'm not sure if that is intentional or not. Can you resolve by either adding a break; at the end of that case, or if the fall-through is intentional, by adding a // fall through comment where the break would be? Either of these things will resolve the warning.

Thanks again!

@jokomo24
Copy link
Contributor Author

Of course and thank you for your hard work, this project is awesome! My apologies as well for taking so long to respond, I just completed a move and a job search so times were a bit hectic.

But yes, nice catch! I did not intend to let this EVENT_ALARM_LONG_PRESS case to fall through. Also, I uncommented my use of the beep function for "production". The simulator would freeze during testing of the watch face if I did not comment it out and I could not determine exactly why.

Thanks again for your feedback and the chance to contribute!

@jokomo24
Copy link
Contributor Author

jokomo24 commented Sep 2, 2023

Now that I've gotten this face on a physical watch, I've noticed a few bugs. Mainly, the value for "Period in # days" and for "Average Cycle" becomes zero after a short period of time. I will be looking into it!

@jokomo24
Copy link
Contributor Author

jokomo24 commented Sep 4, 2023

I've identified that the issue occurs when reseting the watch face using the reset page. So the bug must reside in my reset function...

@CarpeNoctem
Copy link
Contributor

I've identified that the issue occurs when reseting the watch face using the reset page. So the bug must reside in my reset function...

Nice work on this face! Just checking - has this all been resolved as of c1ac01d?

@jokomo24
Copy link
Contributor Author

jokomo24 commented Sep 7, 2023

I've identified that the issue occurs when reseting the watch face using the reset page. So the bug must reside in my reset function...

Nice work on this face! Just checking - has this all been resolved as of c1ac01d?

Yes, this has been resolved! Was a simple typo mistake where I used watch_store_backup_data(state->dates.reg, state->backup_register_cy) instead of watch_store_backup_data(state->cycles.reg, state->backup_register_cy).

Also, in reset_tracking(), I tried to clear the entire register using state->dates.reg = 0 and state->cycles.reg = 0 but that did not work as expected, so I went back to setting the bits to 0 explicitly just like in menstrual_cycle_face_setup().

Thank you for the review and acknowledgements!

@@ -28,6 +28,7 @@
#include "movement_faces.h"

const watch_face_t watch_faces[] = {
menstrual_cycle_face,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should get removed — we don't want to change the default set of faces.

state->cycles.bit.longest_cycle = cycle_length;
}

void menstrual_cycle_face_setup(movement_settings_t *settings, uint8_t watch_face_index, void ** context_ptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This triggers a warning, since the settings parameter isn't used. To silence the warning, you can add the line:

(void) settings;

next to the (void) watch_face_index; that's already there.

if (event.subsecond % 5 && state->dates.reg) { // blink active for 3 quarter-seconds
first_day_fert = get_day_pk_fert(state, first_day);
last_day_fert = get_day_pk_fert(state, last_day);
sprintf(buf, "Fr%2d To %2d", first_day_fert, last_day_fert); // From: first day | To: last day
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line gives the following warning:

../watch_faces/complication/menstrual_cycle_face.c: In function 'menstrual_cycle_face_loop':
../watch_faces/complication/menstrual_cycle_face.c:431:40: warning: '%2d' directive writing between 2 and 3 bytes into a region of size between 2 and 3 [-Wformat-overflow=]
../watch_faces/complication/menstrual_cycle_face.c:431:30: note: directive argument in the range [0, 255]
../watch_faces/complication/menstrual_cycle_face.c:431:17: note: 'sprintf' output between 11 and 13 bytes into a destination of size 11

This isn't actually a problem, since the day number will always be between 1 and 31, but it's good to get rid of the warning anyways — a simple way to do that is to just write:

sprintf(buf, "Fr%2d To %2d", first_day_fert % 100, last_day_fert % 100);

// Store the date of the 'first' and the total cycles since to calulate and store the average menstrual cycle.
// Store the date of the previous, most recent, period to calculate the cycle length.
// Store the shortest and longest cycle to calculate the fertility window for The Calender Method.
// NOTE: Not thrilled about using two registers, but could not find a way to perform The Calender Method
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be preferable to store this data in LFS instead of in the backup registers, just since there are a limited number of backup registers and a lot of LFS space. Then again, currently we have unused backup registers, so it seems fine to merge this as-is and change it to use LFS later if we want.

#322 has a example of how to use LFS

@joeycastillo
Copy link
Owner

Sincere apologies for the delay in merging your pull request, and once again thank you so much for making this!

@joeycastillo joeycastillo merged commit 0f5defe into joeycastillo:main Sep 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants