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

After the flywheel stops the GUI gets no updates any more #109

Open Abasz opened 1 year ago

Abasz commented 1 year ago

On the v1beta, once the flywheel sops, the GUI does not update anything anymore. This includes HR data as well as the modes for BLE and peripherals when changed. The GUI dispatches the peripheral change event correctly, and ORM acts accordingly, its just the GUI that does not update the view.

I am not sure if this is intentional or not, but in my view this should be considered as a bug as these parts of the GUI should always be responsive to specific events.

JaapvanEkris commented 1 year ago

That is odd. On the Backend_Redesign branch I tested this numerous times to determine HRR1, HRR2 and HRR3 by hand, where HRR3 is 3 minutes after reaching the endpoint, so the flywheel should have stopped by then. Works flawlessly. In RowingStatistics, the broadcast keeps on going, regardless of state.

Abasz commented 1 year ago

I debugged this further, here are the updates and steps to reproduce:

Run the following csv in real time loop: 2023-01-15_10-11-17_raw.csv

After 1m11s ORM detects full stop and saves logs etc. That is the time when the GUI gets unresponsive. In the devconsole of chrome the following error is shown:

PerformanceDashboard.js:167 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'value')
    at PerformanceDashboard.calculateFormattedMetrics (PerformanceDashboard.js:167:30)
    at PerformanceDashboard.render (PerformanceDashboard.js:127:30)
    at PerformanceDashboard.update (lit-element.lit-element.v3.1.2.js:29:20)
    at PerformanceDashboard.performUpdate (@lit.reactive-element.v1.2.2.js:300:16)
    at PerformanceDashboard.scheduleUpdate (@lit.reactive-element.v1.2.2.js:286:17)
    at PerformanceDashboard._$EC (@lit.reactive-element.v1.2.2.js:281:20)

The error relates to a null value of the cyclePaceFormatted in the broadcasted metrics object after the PAUSE state is made.

Error comes from here:

 for (const [key, value] of Object.entries(metrics)) {
      const valueFormatted = fieldFormatter[key] ? fieldFormatter[key](value) : value
      if (valueFormatted.value !== undefined && valueFormatted.unit !== undefined) {

if (valueFormatted.value !== undefined && valueFormatted.unit !== undefined) {

No error on the server side:

stroke: 4, dist: 8.2m, speed: 1.56m/s, pace: 5:21/500m, power: 11W, cal: 0.6kcal, SPM: 29.8, drive dur: 0.85s, rec. dur: 1.16s, stroke dur: 2.02s df: 110.31545528130754
*** RECOVERY phase started at time: 8.0464 sec
*** DRIVE phase started at time: 9.0145 sec
*** Calculated drag factor: 111.8848, no. samples: 24, Goodness of Fit: 0.9954
*** Calculated recovery slope: 0.003210, Goodness of Fit: 0.9954, not used as autoAdjustRecoverySlope isn't set to true
stroke: 5, dist: 11.0m, speed: 1.73m/s, pace: 4:50/500m, power: 14W, cal: 0.8kcal, SPM: 36.3, drive dur: 0.68s, rec. dur: 0.97s, stroke dur: 1.65s df: 111.88480109175087
*** RECOVERY phase started at time: 9.6092 sec
*** DRIVE phase started at time: 10.5303 sec
*** Calculated drag factor: 111.4892, no. samples: 24, Goodness of Fit: 0.9953
*** Calculated recovery slope: 0.003199, Goodness of Fit: 0.9953, not used as autoAdjustRecoverySlope isn't set to true
stroke: 6, dist: 13.8m, speed: 1.84m/s, pace: 4:31/500m, power: 18W, cal: 0.9kcal, SPM: 39.6, drive dur: 0.59s, rec. dur: 0.92s, stroke dur: 1.52s df: 111.48917512916441
*** RECOVERY phase started at time: 11.0251 sec
*** WARNING: currentDt of 0.51119 sec is above maximumTimeBetweenImpulses (0.5 sec)
*** WARNING: currentDt of 0.52367 sec is above maximumTimeBetweenImpulses (0.5 sec)
*** WARNING: currentDt of 0.537051 sec is above maximumTimeBetweenImpulses (0.5 sec)
*** WARNING: currentDt of 0.566509 sec is above maximumTimeBetweenImpulses (0.5 sec)
*** WARNING: currentDt of 0.583305 sec is above maximumTimeBetweenImpulses (0.5 sec)
*** WARNING: currentDt of 0.601476 sec is above maximumTimeBetweenImpulses (0.5 sec)
*** PAUSED rowing at time: 71.8702 sec, rower hasn't moved in 61.3399 seconds and flywheel is dwelling
*** Calculated drag factor: 184.7818, not used because reliability was too low. no. samples: 533, fit: 0.9208
*** Paused rowing ***
*** PAUSE MOVING command recieved by RowingEngine at time: 71.8702 sec, distance: 52.86 meters
stroke: 6, dist: 52.8m, speed: 0.00m/s, pace: Infinity/500m, power: 0W, cal: 0.9kcal, SPM: 0.0, drive dur: NaNs, rec. dur: NaNs, stroke dur: NaNs df: 111.48917512916441
saving session as raw data file data/recordings/2023/01/2023-01-15_10-19-14_raw.csv...
saving session as RowingData file data/recordings/2023/01/2023-01-15_10-19-14_rowingData.csv...

my settings:

export default {
  // example: change the default log level:
  loglevel: {
    default: 'debug',
    RowingEngine: 'debug'
  },

  heartRateMode: 'ANT',
  bluetoothMode: 'CSC',
  antplusMode: 'FE',
  numOfPhasesForAveragingScreenData: 1,
  createTcxFiles: false,
  createRawDataFiles: true,
  gzipRawDataFiles: false,

  rowerSettings: { ...rowerProfiles.Generic_Air_Rower, numOfImpulsesPerRevolution: 3 }
}

There are two solutions for this depending on the intent 1) quick, assuming that these values can in fact be null: adding optional chaining operator ? to valueFormatted: valueFormatted?.value making it null safe (probably other variables need the same operator). 2) checking where these properties are being set to null and change it to something that is empty (e.g. in pace :)

JaapvanEkris commented 1 year ago

I would do the latter, and set it to infinity. It is odd, I have some paused rows lately, and I never saw this behaviour. But letting it default to the infinity symbol or --- would be safest.

Abasz commented 1 year ago

I would do the latter, and set it to infinity. It is odd, I have some paused rows lately, and I never saw this behaviour. But letting it default to the infinity symbol or --- would be safest.

I agree

JaapvanEkris commented 1 year ago

I must say, I've been working on the new setup for more complex workouts, and I'm getting more and more convinced that the webserver should contain more intelligence in order to match the status of a feature. Stuff like time formatting and distance selecting/formatting (have timers count down when there is a target set) should be at the display-end, not at the engine-end. This fits that pattern: the webserver should become more robust and more independent.

Abasz commented 1 year ago

ff like time formatting and distance selecting/formatting (have timers count down when there is a target set) should be at the display-end, no

of course it should! actually data that is passed to the web GUI should comply with REST API best practices to some extent. As a minimum it should only pass raw data to the web interface and let the interface deal with formatting and everything. Its not the server side that should decide what happens to the data. That fits the separation of concerns approach that the new "store" setup enables. I was not aware that the server and the GUI is coupled so tightly together.

So what needs to be done is clear: we need to go through the metrics data that RowingStatistics broadcasts and any data that is not raw should be moved the feature to deal with (be it GUI, peripheral etc.) The only decision point here is what the type of the raw data should be (e.g. milliseconds or seconds, meters or centimeters etc.).

These changes can be made along with the implementation of the architectural changes.