Heartrate measurements in background #1718
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -50,6 +50,17 @@ namespace Pinetime { | |||||
| int colorIndex = 0; | ||||||
| }; | ||||||
|
|
||||||
| enum class HeartRateBackgroundMeasurementInterval : uint8_t { | ||||||
| Off, | ||||||
| Continuous, | ||||||
| TenSeconds, | ||||||
|
There was a problem hiding this comment. Ten seconds is not a good option to expose: the PPG takes at least 6.4s to get a value and there's honestly no point turning the sensor off for 3.6s, just leave it on and enjoy much more stable heart rate values as the spectral averaging won't get reset every cycle There was a problem hiding this comment. If that's the case then it would make sense to have a different interval instead. Which interval would you propose instead? There was a problem hiding this comment. I think having 30 as the minimum is OK. The sensor is already allowed to run for up to 30s in the background if it can't find a good signal, in which case 30s mode becomes the same as continuous There was a problem hiding this comment. The formatting of the settings screen only really works with 2*n items There was a problem hiding this comment. Hmm how does it look with an odd number of items? If there's a rendering bug it should be fixed, I could take a look I misinterpreted the timings actually. It's not the measurement-measurement interval as I thought but actually the measurement finished (got value or given up) to measurement start waiting interval Is that intended? If I enabled this feature at say 1 minute, I would expect a reading to arrive every 1 minute rather than every 1 minute 20 seconds (if it took 20s to find the HR) If it's not intended then I think 15s is also a bit pointless, but if it is then I think it could just about make sense There are two reasons I think short measurement times are bad:
So sensor duty cycles (% of time passing with sensor on) between say 50-100% are a bit pointless IMO, just run the sensor all the time and receive the benefits of always having a value, and that value being more stable and reliable There was a problem hiding this comment. It is not a perfect interval setting (meaning setting it to 1 minute = one value every minute). It's just a quick and easy solution to get the feature of repetitive measurements in the background. I think it is difficult to make it more accurate. Since it can't be determined beforehand how long the sensor needs to measure until enough data is captured right? I mean there is an upper limit of 30s (for which the heart rate task just skips the current measurement) and there is a lower limit of around 6s, but that's still a wide range of measuring time. I think the current implementation is easy and quite robust in the regards that you know "If there was a measurement, then after x time another measurement is started" I'm fine with having a more complicated algorithm, but it seems a lot of effort for probably not a small positive effect, since I assume most people don't need a very precise interval. Also it only really matters for smaller values. If you use a 5 minute interval, another 6-15s is not really that important. I'm also assuming, that having the sensor running all the time wastes battery. If you just want a HR roughly every 15 seconds, then having 15s wait + 6s measurement means having the sensor 71% of the time off. Which is quite a lot better than having it run all the time. Even if it degrades to 15s wait + 30s measurement, it's still 33% of the time deactivated. Instead of always on. For longer interval times, this would mean that the sensor is restarted for every measure, but then you again just get the bad "warm-up" data, so if the imprecise data that is retrieved from the sensor that has just been started is a problem, then the longer interval times shouldn't exist anyway (or the the whole PR needs to be a lot more complicated). Because having the sensor running all the time for measurements every 10 minutes seems like a waste of energy. This PR is open quite for a long time already and I think small improvements can be discussed much more thoroughly and implemented way better after more people had the chance to test it. But again I'm open for suggestions for making the code better and if anything (like an inaccurate interval time) is a deal breaker, I will of course fix it! There was a problem hiding this comment. Ah sorry, what I was thinking was actually something different. I agree that trying to predict measurement time would be a bad idea (too complex), and having cases where runtime degrades to continuous is not ideal (for longer periods). What I meant was storing the start time of the measurement, and then starting the next measurement after What do you think of this? If you're hitting memory limits, shrinking the heap wouldn't be unreasonable BTW, there is currently very little static memory free and the heap will probably be shrunk by 1K or 512B soon anyway There was a problem hiding this comment. Yes, please start a measurement every x seconds. That is what users will expect to happen. |
||||||
| ThirtySeconds, | ||||||
| OneMinute, | ||||||
| FiveMinutes, | ||||||
| TenMinutes, | ||||||
| ThirtyMinutes, | ||||||
| }; | ||||||
|
There was a problem hiding this comment. I think that this is the wrong data structure for what you want to do. Maybe just use a const array of integers representing seconds? Or at least set the enums values to that? There was a problem hiding this comment. I tried an array of integers before, but there was a problem when storing something longer than a There was a problem hiding this comment. Weird, but I guess it's alright then 😄 |
||||||
|
|
||||||
| Settings(Pinetime::Controllers::FS& fs); | ||||||
|
|
||||||
| Settings(const Settings&) = delete; | ||||||
|
|
@@ -283,10 +294,21 @@ namespace Pinetime { | |||||
| return bleRadioEnabled; | ||||||
| }; | ||||||
|
|
||||||
| HeartRateBackgroundMeasurementInterval GetHeartRateBackgroundMeasurementInterval() const { | ||||||
| return settings.heartRateBackgroundMeasurementInterval; | ||||||
| } | ||||||
|
|
||||||
| void SetHeartRateBackgroundMeasurementInterval(HeartRateBackgroundMeasurementInterval newHeartRateBackgroundMeasurementInterval) { | ||||||
| if (newHeartRateBackgroundMeasurementInterval != settings.heartRateBackgroundMeasurementInterval) { | ||||||
| settingsChanged = true; | ||||||
| } | ||||||
| settings.heartRateBackgroundMeasurementInterval = newHeartRateBackgroundMeasurementInterval; | ||||||
| } | ||||||
|
|
||||||
| private: | ||||||
| Pinetime::Controllers::FS& fs; | ||||||
|
|
||||||
| static constexpr uint32_t settingsVersion = 0x0008; | ||||||
|
There was a problem hiding this comment. Does this need to be bumped to
Suggested change
There was a problem hiding this comment. Yes you are right. Thank you! I bumped the settings version a few times already and I don't want to do it every time there is the settings file in main anymore. |
||||||
|
|
||||||
| struct SettingsData { | ||||||
| uint32_t version = settingsVersion; | ||||||
|
|
@@ -308,6 +330,8 @@ namespace Pinetime { | |||||
| uint16_t shakeWakeThreshold = 150; | ||||||
|
|
||||||
| Controllers::BrightnessController::Levels brightLevel = Controllers::BrightnessController::Levels::Medium; | ||||||
|
|
||||||
| HeartRateBackgroundMeasurementInterval heartRateBackgroundMeasurementInterval = HeartRateBackgroundMeasurementInterval::Off; | ||||||
FintasticMan marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| }; | ||||||
|
|
||||||
| SettingsData settings; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| #include "displayapp/screens/settings/SettingHeartRate.h" | ||
| #include <lvgl/lvgl.h> | ||
| #include "displayapp/screens/Styles.h" | ||
| #include "displayapp/screens/Screen.h" | ||
| #include "displayapp/screens/Symbols.h" | ||
| #include <array> | ||
| #include <algorithm> | ||
|
|
||
| using namespace Pinetime::Applications::Screens; | ||
|
|
||
| namespace { | ||
| void event_handler(lv_obj_t* obj, lv_event_t event) { | ||
| auto* screen = static_cast<SettingHeartRate*>(obj->user_data); | ||
| screen->UpdateSelected(obj, event); | ||
| } | ||
| } | ||
|
|
||
| constexpr std::array<Option, 8> SettingHeartRate::options; | ||
|
|
||
| SettingHeartRate::SettingHeartRate(Pinetime::Controllers::Settings& settingsController) : settingsController {settingsController} { | ||
|
|
||
| lv_obj_t* container1 = lv_cont_create(lv_scr_act(), nullptr); | ||
|
|
||
| lv_obj_set_style_local_bg_opa(container1, LV_CONT_PART_MAIN, LV_STATE_DEFAULT, LV_OPA_TRANSP); | ||
| lv_obj_set_style_local_pad_all(container1, LV_CONT_PART_MAIN, LV_STATE_DEFAULT, 5); | ||
| lv_obj_set_style_local_pad_inner(container1, LV_CONT_PART_MAIN, LV_STATE_DEFAULT, 5); | ||
| lv_obj_set_style_local_border_width(container1, LV_CONT_PART_MAIN, LV_STATE_DEFAULT, 0); | ||
|
|
||
| lv_obj_set_pos(container1, 10, 60); | ||
| lv_obj_set_width(container1, LV_HOR_RES - 20); | ||
| lv_obj_set_height(container1, LV_VER_RES - 50); | ||
| lv_cont_set_layout(container1, LV_LAYOUT_PRETTY_TOP); | ||
|
|
||
| lv_obj_t* title = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_label_set_text_static(title, "Backg. Interval"); | ||
| lv_label_set_text(title, "Backg. Interval"); | ||
| lv_label_set_align(title, LV_LABEL_ALIGN_CENTER); | ||
| lv_obj_align(title, lv_scr_act(), LV_ALIGN_IN_TOP_MID, 10, 15); | ||
|
|
||
| lv_obj_t* icon = lv_label_create(lv_scr_act(), nullptr); | ||
| lv_obj_set_style_local_text_color(icon, LV_LABEL_PART_MAIN, LV_STATE_DEFAULT, LV_COLOR_RED); | ||
| lv_label_set_text_static(icon, Symbols::heartBeat); | ||
| lv_label_set_align(icon, LV_LABEL_ALIGN_CENTER); | ||
| lv_obj_align(icon, title, LV_ALIGN_OUT_LEFT_MID, -10, 0); | ||
|
|
||
| for (unsigned int i = 0; i < options.size(); i++) { | ||
| cbOption[i] = lv_checkbox_create(container1, nullptr); | ||
| lv_checkbox_set_text(cbOption[i], options[i].name); | ||
| cbOption[i]->user_data = this; | ||
| lv_obj_set_event_cb(cbOption[i], event_handler); | ||
| SetRadioButtonStyle(cbOption[i]); | ||
|
|
||
| if (settingsController.GetHeartRateBackgroundMeasurementInterval() == options[i].interval) { | ||
| lv_checkbox_set_checked(cbOption[i], true); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| SettingHeartRate::~SettingHeartRate() { | ||
| lv_obj_clean(lv_scr_act()); | ||
| settingsController.SaveSettings(); | ||
| } | ||
|
|
||
| void SettingHeartRate::UpdateSelected(lv_obj_t* object, lv_event_t event) { | ||
| if (event == LV_EVENT_CLICKED) { | ||
| for (unsigned int i = 0; i < options.size(); i++) { | ||
| if (object == cbOption[i]) { | ||
| lv_checkbox_set_checked(cbOption[i], true); | ||
| settingsController.SetHeartRateBackgroundMeasurementInterval(options[i].interval); | ||
| } else { | ||
| lv_checkbox_set_checked(cbOption[i], false); | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <lvgl/lvgl.h> | ||
|
|
||
| #include "components/settings/Settings.h" | ||
| #include "displayapp/screens/ScreenList.h" | ||
| #include "displayapp/screens/Screen.h" | ||
| #include "displayapp/screens/Symbols.h" | ||
| #include "displayapp/screens/CheckboxList.h" | ||
|
|
||
| namespace Pinetime { | ||
|
|
||
| namespace Applications { | ||
| namespace Screens { | ||
|
|
||
| struct Option { | ||
| const Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval interval; | ||
| const char* name; | ||
| }; | ||
|
|
||
| class SettingHeartRate : public Screen { | ||
| public: | ||
| SettingHeartRate(Pinetime::Controllers::Settings& settings); | ||
| ~SettingHeartRate() override; | ||
|
|
||
| void UpdateSelected(lv_obj_t* object, lv_event_t event); | ||
|
|
||
| private: | ||
| Pinetime::Controllers::Settings& settingsController; | ||
|
|
||
| static constexpr std::array<Option, 8> options = {{ | ||
| {Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval::Off, " Off"}, | ||
| {Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval::Continuous, "Cont"}, | ||
| {Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval::TenSeconds, " 10s"}, | ||
| {Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval::ThirtySeconds, " 30s"}, | ||
| {Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval::OneMinute, " 1m"}, | ||
| {Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval::FiveMinutes, " 5m"}, | ||
| {Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval::TenMinutes, " 10m"}, | ||
| {Pinetime::Controllers::Settings::HeartRateBackgroundMeasurementInterval::ThirtyMinutes, " 30m"}, | ||
| }}; | ||
|
|
||
| lv_obj_t* cbOption[options.size()]; | ||
| }; | ||
| } | ||
| } | ||
| } |
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.
Accidental whitespace removal?
Or if this is on purpose, also remove from below
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.
Removed the other whitespace