Step counter history added #2120

Hunman · 2024-09-08T23:15:35Z

I changed the MotionController to remember the past N (currently 2) day's step counter history after the midnight reset. And I also changed the Steps screen to display (and update) yesterday's step count in addition to today's.

I didn't feel comfortable changing the return type of MotionService::NbSteps() yet without feedback, instead I wrote a new method to return the whole history.

I've tested this only using InfiniSim, testing with devkits would be welcome!

In the future I would like to increase the history size from 2 to 7 and transfer the whole history to the companion applications, but I haven't really looked into how bluetooth communication or "API"s work, so feedback would be most appreciated!

github-actions · 2024-09-08T23:25:53Z

Build checks have not completed. Possible reasons for this are:

The checks need to be approved by a maintainer
The branch has conflicts
The firmware build has failed

minacode · 2024-09-09T20:07:00Z

Nice! You should be able to test this on your sealed watch without problems. As long as you don't verify the firmware, a reboot will be enough to get it back to working. And if you do verify it, a force reboot into the backup firmware still enables you to install another firmware.

mark9064

Cool feature, looking good :) haven't tried it yet on my PT yet but hopefully will soon

mark9064 · 2024-09-10T23:26:09Z

src/components/motion/MotionController.cpp

-  if (this->nbSteps != nbSteps && service != nullptr) {
+void MotionController::AdvanceDay() {
+  --nbSteps;           // Higher index = further in the past
+  nbSteps[today] = -1; // Ensure that the `this->nbSteps[today] != nbSteps` condition in `Update()` will be FALSE


I think it'd be better to assign zero here and do service->OnNewStepCount() inside this function. Reasoning about -1 applied to an unsigned int is complex and so is reasoning about the delta steps calculation

mark9064 · 2024-09-10T23:26:19Z

src/components/motion/MotionController.h

@@ -18,7 +19,17 @@ namespace Pinetime {
        BMA425,
      };

-      void Update(int16_t x, int16_t y, int16_t z, uint32_t nbSteps);
+      enum Days {
+        today = 0,


Enum members should be capitalised

mark9064 · 2024-09-10T23:26:28Z

src/components/motion/MotionController.h

+        yesterday = 1,
+      };
+
+      using step_t = uint32_t;


I'm really split on this one. On one hand it's nice to have this as a type. On the other, I can't see this type ever being changed (nobody is ever gonna exceed 4 billion steps in a day :P) and it is slower to read and more verbose than uint32_t. Also IIRC C++ still will allow using step_t and uint32_t interchangeably right? (and perhaps any larger/smaller integer type depending on the context). If that's true then we don't get any safety benefits either

I was torn when I wrote this too :D

Yeah, it is unlikely to change, since the hardware returns a uint32_t through bma423_step_counter_output() and since it's an alias, step_t is completely interchangeable with uint32_t, so having this does not make the code any safer or faster.

But on the other hand, it is more verbose, potentially making the code easier to read (at least for me) that this function returns or expects a type in which steps are stored (even though the function/parameter name should be enough), much like how size_t is used for most if not all functions where a container size is used.

At least that was my rationale when I wrote this, but I can change it back to uint32_t if needed (or move it out of the MotionController class).

Or I could make it a real type instead of an alias, so they wouldn't be interchangeable.

I think when it comes to verboseness what I mean is having to reference Controllers::MotionController::step_t instead of uint32_t. To me uint32_t is easier to read, with the operations available on the type being clearer rather than having to look it up and go "oh, it's just an unsigned int, right". But your point about the semantics of the variable being clearer is fair.

I don't know, maybe someone else will have an opinion? I don't strongly oppose it either way

It's just a count, right? Maybe keep it simple. I don't see, what problem would be solved, or is there any critical code, where confusion must not happen (e.g. temperature, where we handle values with different units)?

Yeah, it's just a counter, I just didn't feel important to know the real underlying type everywhere in the code.

Even if step_t makes sense, it definitely shouldn't be defined within MotionController, so I'm removing it

mark9064 · 2024-09-10T23:27:31Z

src/components/motion/MotionController.h

@@ -32,15 +43,19 @@ namespace Pinetime {
        return zHistory[0];
      }

-      uint32_t NbSteps() const {
+      step_t NbSteps() const {


I think it'd be better if NbSteps took a member of Days as an argument (as you suggest actually)
The step counter history method is still needed though if you plan on exposing this over BLE

Done

I've set Today as default parameter.

Should I revert most of the changes to Steps.cpp, where I'm storing a const reference to nbSteps?

I would personally as I think calling NbSteps() is a bit clearer than using the buffer reference

mark9064 · 2024-09-10T23:29:30Z

src/systemtask/SystemTask.cpp

@@ -440,6 +440,7 @@ void SystemTask::UpdateMotion() {

  if (stepCounterMustBeReset) {
    motionSensor.ResetStepCounter();
+    motionController.AdvanceDay();


Can I suggest removing stepCounterMustBeReset and the associated message as they aren't needed anymore? (as I2C is never slept) This would cause the step count handling to be done immediately when a new day starts. It's a little out of scope for this PR technically but it needs to be refactored anyway and I reckon now is a sensible time to do it

It indeed sounds a bit out of scope

I can open a pull request only with this after this one if that's alright

No worries, I'll get round to it

mark9064 · 2024-09-11T17:15:40Z

src/displayapp/screens/Steps.h

        lv_obj_t* stepsArc;
        lv_obj_t* resetBtn;
        lv_obj_t* resetButtonLabel;
        lv_obj_t* tripLabel;

-        uint32_t stepsCount;
+        const char* yesterdayStr;


Could yesterdayStr be a constant?
If that's fine, you could stick it in an anonymous namespace in the implementation as constexpr const char * (I think!) and then it will live in just flash rather than flash and RAM

In theory that's how it should work right now too, because I'm not copying the string, just taking the address of a string literal (that's why I don't need to free it either)

Unless I'm misunderstanding something

It doesn't matter here too much as it's heap memory rather than stack, but you can avoid storing the pointer at all by effectively inlining the constant

mark9064 · 2024-09-12T11:18:44Z

src/components/motion/MotionController.h

+        Last,
+      };
+
+      static constexpr size_t stepHistorySize = static_cast<std::underlying_type_t<Days>>(Days::Last); // Store this many day's step counter


NbSteps(Days::Last) is out of bounds with this implementation right? I think that could be better, I'd expect every entry of the enum to be valid. Perhaps do LastEntry = Yesterday and then static_cast<std::underlying_type_t<Days>>(Days::LastEntry) + 1? Also probably assign an integer value explicitly to every enum member as well for clarity (except LastEntry as above)

My other concern is that there's no way to get all of the days without having an enum entry for each one. If we want to expose 7 days over BLE, at the moment all 7 days would need to be named in the enum which feels a bit odd. Not a dealbreaker though

Yeah, theoretically out of bounds. In practice the CircularBuffer handles it internally by doing modulo (%) operation on the size, so Days::Last would be just index 0 again.

While it is nice to refer to the days with Today and Yesterday, maybe having to name 5 more enum values is not that nice.

One option is that the Days enum can be removed altogether. Or alternatively it can remain with only Today and Yesterday values, and NbSteps() function could get an overload that expects a uint8_t.

That second option sounds good to me, that way we have a nice human readable enum while also allowing specifying a number of days ago for more flexible usage

mark9064 · 2024-09-12T11:19:56Z

Sorry for the seemingly endless review! It's looking really good now, almost there

Hunman · 2024-09-12T11:32:14Z

It's alright, I'm actually enjoying this opportunity to "nerd out" with people on a project 😄

Since most if not all calls expect today, I set it as default value

mark9064 reviewed Sep 10, 2024

View reviewed changes

mark9064 reviewed Sep 11, 2024

View reviewed changes

mark9064 reviewed Sep 12, 2024

View reviewed changes

Hunman force-pushed the step-counter-history branch from 20168fa to 7f7189e Compare September 16, 2024 06:30

Hunman force-pushed the step-counter-history branch from 7f7189e to 85c1f4e Compare September 23, 2024 12:54

Hunman added 15 commits October 28, 2024 11:47

Introduce an alias for the step counter's type

07fc444

Inverted the circular dependency of MotionService and MotionController

a1a45df

Converted MotionController::nbSteps to CircularBuffer internally

cd1815b

Added an enum for the index of today and yesterday

71cef51

Display yesterday's step counter on the Steps screen

f07a16f

At midnight, "shift" the step counter history

35129e9

Clang-format changes

24eab6f

Do not set unsigned step count to -1

cc8a381

Capitalising Day enum's members "Today" and "Yesterday"

a0b468d

MotionController::NbSteps should take a Days enum as parameter

275c70e

Since most if not all calls expect today, I set it as default value

Removed step counter history getter

144c7d0

Removed step_t

fcfdea7

Reverted to the original MotionController and MotionService dependency

0f2c2ff

Changed MotionController::Days from enum to enum class

a5f2d2c

Changed MotionController::stepHistorySize to hard-coded value again

e973125

Hunman force-pushed the step-counter-history branch from 85c1f4e to e973125 Compare October 28, 2024 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step counter history added #2120

Step counter history added #2120

Hunman commented Sep 8, 2024

github-actions bot commented Sep 8, 2024

minacode commented Sep 9, 2024

mark9064 left a comment

mark9064 Sep 10, 2024

Hunman Sep 11, 2024

mark9064 Sep 10, 2024

Hunman Sep 11, 2024

mark9064 Sep 10, 2024

Hunman Sep 11, 2024

Hunman Sep 11, 2024

mark9064 Sep 11, 2024 •

edited

Loading

minacode Sep 12, 2024

Hunman Sep 12, 2024

mark9064 Sep 10, 2024

Hunman Sep 11, 2024

mark9064 Sep 11, 2024

mark9064 Sep 10, 2024 •

edited

Loading

Hunman Sep 11, 2024

mark9064 Sep 11, 2024

mark9064 Sep 11, 2024

Hunman Sep 11, 2024

mark9064 Sep 11, 2024

mark9064 Sep 12, 2024

Hunman Sep 12, 2024

mark9064 Sep 12, 2024

mark9064 commented Sep 12, 2024

Hunman commented Sep 12, 2024

Step counter history added #2120

Are you sure you want to change the base?

Step counter history added #2120

Conversation

Hunman commented Sep 8, 2024

github-actions bot commented Sep 8, 2024

minacode commented Sep 9, 2024

mark9064 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark9064 Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark9064 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark9064 commented Sep 12, 2024

Hunman commented Sep 12, 2024

mark9064 Sep 11, 2024 •

edited

Loading

mark9064 Sep 10, 2024 •

edited

Loading