laberning / openrowingmonitor

A free and open source performance monitor for rowing machines
https://laberning.github.io/openrowingmonitor
GNU General Public License v3.0
98 stars 19 forks source link

Redesign of the rowing engine #84

Closed JaapvanEkris closed 1 year ago

JaapvanEkris commented 1 year ago

This new version will bring some major changes to the rowing engine:

Abasz commented 1 year ago

Hi @JaapvanEkris,

This is great work.

I had some issues with starting up, as there is a typo in the WorkoutRecorder.js file: VO2max.js is referenced incorrectly (there is a typo as the M should be lower case). Otherwise it was nearly plug and play. Even setting up the settings was rather straight forward (though, I already had some idea about the right values for some of the settings).

I also did some initial basic testing 20 strokes side by side with the ESP32 micro controller. Please find below the chart of the raw delta times. Orange is ORM and blue is the ESP32.

image

You can see that the Rpi is quite spot on with some occasional inaccuracies. Note, my rower has 3 magnets and this is without any optimization on the Rpi (i.e. all general services are running in the background, etc.). Also its quite surprising that the Rpi it self handles reed switch debounce quite well on higher rotation speeds (you can see that it starts to struggle when rotation speed decreases). My take here is that the higher latency here helps, as the Rpi actually misses those <0.01sec debounces from the reed switch (that needs more cleaning up on the ESP32).

I will do more testing with proper workouts but this seems really promising.

JaapvanEkris commented 1 year ago

Abasz

Thanks for trying and giving the feedback! Fixed the typo.

Jaap

laberning commented 1 year ago

Thanks for submitting the PR. I can see there is a lot of passion and hard work in there! I'm looking forward to review and stress test code. This is quite a big one so this will take some time.

Gordon-Shumway2 commented 1 year ago

Hi,

I ran into the same typo issue when installing the Backed_redesign branch and additionally another one which might be due to my hardware. I installed on a Pi Zero 2 W with 512 KB RAM. Once the install.sh script hit the npm section RAM maxed out, swap was used and eventually the installation hung with a load of 8 and maxed-out swap (default size for an out-of-the-box installation of 64bit lite was 100MB if I remember correctly). I aborted the installation. extended swap to 1024MB (following [https://pimylifeup.com/raspberry-pi-swap-file/]) and re-ran the installation. Swap grew to close to 400MB during the npm installation phase but the installation succeeded.

I am not sure whether it is down to the new branch or the 64bit OS but it might be worth mentioning this issue within the documentation.

Cheers, Michael

JaapvanEkris commented 1 year ago

I am not sure whether it is down to the new branch or the 64bit OS but it might be worth mentioning this issue within the documentation.

Michael,

Thanks, I'll look into it. Might be the setup on 64Bit indeed that is problematic.

Jaap

JaapvanEkris commented 1 year ago

I didn't yet dig very deep into the math involved and also did not yet have the time to stress test this in different environments.

Small steps will get us there. No problem.

Thanks to Abasz and Gordon-Shumway2 for already giving feedback on this 👍

Dito! I love the feedback and I already found a new nice challenge.

This is great work which makes the rowing engine much more powerful and universal. Also thanks for taking the time to expand a lot of stuff from the forum and emails into a proper documentation!

You are more than welcome. It is fun to contribute to well-designed clean code and see people use it on totally different machines.

This is just a first pass of feedback, and I assume that this will take a few round as this is a rather big and complex change.

The code wasn't written in a couple of days either :)

As this PR also introduces some breaking changes (i.e. configuration) I would propose the following approach:

  • start by iterating with feedback and fixes on the PR (that's what we are doing now)
  • at some point move this into a new branch (maybe v1beta) and announce that one. This would give more users the possibility to try it out since its easy to switch branches with the installer
  • hopefully get feedback from more users and catch problems
  • once we are confident that this versions works good for most users, move it into the main branch

That is a very good approach, love it. That will give us the room to integrate things like the arcade as well, and potentially even the changes proposed by @Abasz.

Abasz commented 1 year ago

I have been testing the new algorithm on my machine. I did about a 100km, different type of exercises to found out how the new algorithm performs. These were the main types:

I tested ORM against the micro-controller that was specifically tweaked to my machine over like 6+ month. Its important to note that I was mainly focusing om the stroke detection, DF and Power. I excluded distance as its calculation is based on the number of impulses, and until there are noise in the recorded DTs, it will never really mach be correct as it will be slightly over estimated (due to the higher number of data points. Of course for this reason there will be a certain amount of deviation on the other metrics (e.g. the total power is higher on ORM) but those can be nicely visualize on a graph that enables comparison.

Stroke count validation

In my view this is the most important part. The way I did it is that I set up logging in a way way that DTs are logged in microsec and when a stroke is detected I logged the current total number strokes so I can create a graph where a straight line towards 0 shows a detected stroke. Please see the excel file: Stroke-verification.xlsx

My conclusion is that ORM was very (100%) accurate after setting the detection settings up correctly. I was very impressed especially since the different type of exercises not always had nice strokes (especially the recovery after the harder rowing, there are longer, shorter, with bad technique etc. These were always detected correctly.

DF an Power

In the excel RowerModelData3.xlsx you can see that ORM lines up nicely against the MCU, produces identical data. The only thing that seems a bit off is drive time. It seems very volatile.

So all in all the regression based stroke detection worked great.

JaapvanEkris commented 1 year ago

Thanks to the great work of @Abasz, I was able to add the pigpio changes, allowing a much more finegrained detection of the pulses, including debounce.

There is one outstanding improvement from the discussion with @Abasz: changing the startup behaviour so that ORM ignores the first noisy part of the first stroke and the crazy values pigpio seems to produce in the first CurrentDt. The needed code is already in testing on my own machine, but it needs a bunch of starts and stops before I am confident it will work under all circumstances.

JaapvanEkris commented 1 year ago

While trying to get a grip on my flywheel issues, I've played around with the smoothing parameter. And my conclusion based on that test, is that it creates more issues with the subsequent regression analysis, than that it resolves. It also feels odd: the regression analysis is designed to be robust against noise, so why filter it beforehand?

So I consider removing this, as it makes code simpler and configuring ORM easier. WHat are other's thoughts here?

Abasz commented 1 year ago

While trying to get a grip on my flywheel issues, I've played around with the smoothing parameter. And my conclusion based on that test, is that it creates more issues with the subsequent regression analysis, than that it resolves. It also feels odd: the regression analysis is designed to be robust against noise, so why filter it beforehand?

So I consider removing this, as it makes code simpler and configuring ORM easier. WHat are other's thoughts here?

Sorry, which smoothing parameter are you referring to? the debounce filter or the running average size?

JaapvanEkris commented 1 year ago

Sorry, which smoothing parameter are you referring to? the debounce filter or the running average size?

The running average: in the rowerprofile, it is mentioned as follows:

// Smoothing determines the length of the running average for filtering the currentDt, 1 effectively turns it off
 smoothing: 1,

It was changed to a median as it is more robust to outliers, but when I tried to correct my flywheel issues with it, it just made a mess of all data.

Abasz commented 1 year ago

Sorry, which smoothing parameter are you referring to? the debounce filter or the running average size?

The running average: in the rowerprofile, it is mentioned as follows:

// Smoothing determines the length of the running average for filtering the currentDt, 1 effectively turns it off
 smoothing: 1,

It was changed to a median as it is more robust to outliers, but when I tried to correct my flywheel issues with it, it just made a mess of all data.

So theoretically it is possible that the time between impulses are vary on a systematic bases due to very slight inaccuracies of the magnet placements. Like one magnet is closer to the other so there will be a shorter diff followed by a longer etc. So in my view this was the purpose of this smoothing (at least I used it to smooth these variations out) so the non-robust simple comparison version does not get thrown off by these insignificant differences. But I think this is not necessary with the regression version as it handles this by design. Actually I checked and for all of my tests on the new back-end I set this to 1. Currently I see no use for it, but may be there are some edge cases.

Nevertheless, I would expect that such smoothing setting should not interfere with the stroke detection as it generates a nicer line so the goodness of fit should be better. So if someone uses this should increase goodness of fit as well. When you did testing, did you increase goodness of fit?

JaapvanEkris commented 1 year ago

Nevertheless, I would expect that such smoothing setting should not interfere with the stroke detection as it generates a nicer line so the goodness of fit should be better. So if someone uses this should increase goodness of fit as well. When you did testing, did you increase goodness of fit?

That is something that I expected as well, although it fits a pattern: the drag calculation is fed raw data by design, as the initial tests quickly showed that using cleaned data resulted in an artificially higher Goodness Of Fit, but a much more unstable drag factor. A similar thing is found in the stroke detection algorithm: it has to be fed dirty data to remain useable. Here I see a similar issue: clean data seems to kill the workings of the Quadratic regression algorithm, as data seems to become too clean?

Abasz commented 1 year ago

I dont think that from a mathematical theory perspective the data can be "too clean". I can think only a regions where the issue could lie: 1) the averaging smoothens and delays the tipping point of the flywheel acceleration and that causes issues with detecting the actual time for the stroke. 2) feeding averaged data to the regression or the median filter causes inconsistencies with its output, because for instance the effect of one great outlier is stretched across to multiple impulses (e.g. if you have DTs like 1000, 1000, 1, 1000, 1000, 1000, you get over the inaccurate impulse nearly instantly, while with the running average of 3 you would have, 1000, 1000, 667, 667, 667, 1000)

I think the 1. option is less likely from a theoretical point of view, as such smoothness would be handled by the regression (detecting stroke change with a slight delay due to the longer plateau). So I think its more likely that due to the averaging and the resulted lengthening of the effect of outliers the median filter or the regression model gets confused (which one I dont know, but I would bet for the median filter as theoretically the regression model would be able to handle this).

laberning commented 1 year ago

@JaapvanEkris, thanks for all the effort you put into this! I've now moved it into a beta branch on the main repository (https://github.com/laberning/openrowingmonitor/tree/v1beta). So that will already make it more easy to install this from the main repository by issuing updateopenrowingmonitor.sh -b v1beta. I'll do some more checks and then it will be time to announce this 🥳

Also a lot of thanks to @Abasz and @Gordon-Shumway2 for reviewing and contributing to the redesign

JaapvanEkris commented 1 year ago

Thanks for merging and your support!

Also thanks to @Abasz for pushing the envelope of what is possible. And some great new things are under way thanks to him and @Carlito1979!