[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 :) |
|
@thyttan I'll let you merge if you're happy, being the main reviewer here :) |
|
I'll get back to this saturday I hope. |
|
I'll merge this now - thanks! One thing I'm thinking is, should the user be able to prefer movement data over hrm data? The code now defaults to heart rate and falls back to movement (correct me if I'm wrong). For some people movement data may be more reliable and it has been the standard for a long time. In my mind, the heart rate option should have been introduced as an option and not the new default. On the other hand, if a user find heart rate readings unreliable they probably keep it disabled generally? thus getting the movement data version anyway. @RKBoss6 @ondras12345 @bobrippling, thoughts? |
|
I think it's fine as is, the code only uses hrm data if it's available, but yes, it does default to it if present. We can make a pr for this, I think it's a good setting to have |
|
Cool, sounds like a reasonable approach to me |
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