[SleepLog] Fix important bugs with HRM being undefined #4023
Conversation
Removed unnecessary whitespace and comments in health function.
|
Not for this PR, but would you find it useful to have typescript check this code, to stop similar errors in future? I wouldn't mind making the change if so |
|
@bobrippling That would be good! Just a question: how does typescript checking differ from the checks that are already done? |
It'd be able to find typos such as |
|
@ondras12345 Any updates from testing for you? For me it seems as though only deep sleep is being compared in consecutive sleep, and even when the light sleep Hrm threshold is 100, it does not register. Light sleep is still registered, but doesn't look like it is accurate. When the deep sleep threshold is 100 it works as expected and shows everything as sleep, as it should. |
|
Sorry for the late reply.
That definitely isn't the case for me. E.g. today, it shows consecutive sleeping 9h 50min, true sleeping 9h 50min, deep sleep 5h 10min, light sleep 4h 40min. My HRM thresholds are 60 for deep sleep and 64 for light sleep. |
|
@ondras12345 can you try something out? Try setting the light sleep threshold to 100. Theoretically, it should report that you sleep all the time, but when I try it does not register. Changing deep sleep as stated is what works for me. Test that out... |
|
You're right, it registers as awake when I set light sleep threshold to 100. Will experiment with this some more and let you know what I find. |
|
I think this behavior is by design: Light sleep is corrected to awake if it did not start with a deep sleep. |
|
Hmm, is that accurate though? From what I have researched, a daily sleep starts with light sleep, and then alternates between deep sleep and light sleep again. I'm tagging @storm64, the creator, to see what the decision behind this was. |
|
As the programming is some time ago I can't tell you what the decision process was, but I tried to document all behaviour and settings in the readme: https://banglejs.com/apps/?id=sleeplog&readme |
|
I think it is working well enough. We should probably get this PR through so that users who update don't get a completely broken app. A change to the light sleep behavior can be added later if deemed necessary. |
|
True. I'll work on fixing checks so we can hopefully get this merged in. |
|
This looks good to go right now! |
|
Two review comments waiting for answers in the files view :) |
|
Should be resolved now |
|
This is good to go, so if we can merge this in, that would be great for users to fix the currently broken app :) |
5 participants
Fixed bugs relating to HRM data being incorrectly pulled in and comparing with undefined.
This comes from the discussion at #4010, and has been tested by me and @ondras12345
From the looks of it, this also fixes #4012