Force Curve in GUI

[Abasz]

Somthing I thought would be usefull:

A few minor things are needed (options for showing force curve vs. other metrics). I will create a separate branch for testing once that is done.

[JaapvanEkris]

The big question is whether you'd like it near-real time (as the PM5 does) or just at the end of the stroke. The latter is already possible, the real-time option requires some rethinking of the filter algorithms in RowingStatistics (they trim all curves to exclude noise). Please also note, the number of datapoints in a force curve varies quite a lot: Lars' his waterrower has 5 datapoints in the drive, a concept2 typically has 80. So that is something to keep in mind.

One practical point: if you have to sacrifice two tiles, it would be the calories and the watts. Total time is quite a crucial one. In practice, Watts are just another presentation of pace, so not a big deal if you lose it (on a PM5, the are never on the same screen......).

[JaapvanEkris]

Sorry I might have phrased my idea incorrectly. Separating the web and server settings is a prerequisite (actually the first step should be to separating settings domains like server, flywheel, GUI). Than change the way settings are persisted (like having a json file that is loaded on startup and saved on edit etc.)

I think that is a pretty good approach. I think some fundamental settings (like GPIO pin, process priority, flywheel inertia, etc.) shouldn't be accessible via the GUI, but others should (like metrics smoothing). I guess that we could make the features responsible for exposing settings and persisting them (thus the GUI would request the settings, and would push them back). That would require dividing the settings into the features, but it would make a lot of sense: some settings apply to the PM5 settings, but not the ANT+ settings, but currently we use one single updatefrequency for all peripherals.

What I meant is that I would have a settings page on the GUI, where everything can be edited and saved to persistent storage rather than editing stuff directly in code. This would be much more user friendly.

I largely agree. I think we need to add levels (i.e. distinguishing the individual features) and then the underlying settings (so you first select the ANT+ feature before modifying the ANT+ settings). As said, I think some settings should only be editable from the commandline. Especially the GPIO and flywheel settings are prone to this (which are currently mixed into the normal settings). We could display them, but making them too easy to edit by normal users would be odd, as the GPIO pin is also intertwined with the OS parameters.

[Abasz]

I largely agree. I think we need to add levels (i.e. distinguishing the individual features) and then the underlying settings (so you first select the ANT+ feature before modifying the ANT+ settings). As said, I think some settings should only be editable from the commandline. Especially the GPIO and flywheel settings are prone to this (which are currently mixed into the normal settings). We could display them, but making them too easy to edit by normal users would be odd, as the GPIO pin is also intertwined with the OS parameters.

Yes I agree with levels (like, general, advance, expert), but I would not have settings that are not settable via the GUI. Direct edit of files does not allow input validation and hence error prone. Settings that are changed via the GUI can be validated upon entry and useful validation errors can be thrown. If a setting is set via the GUI all sorts of input validation can be added and this would enhance consistency.

The above would not impact the settings domain approach as levels exits purely for representation purposes.

I guess that we could make the features responsible for exposing settings and persisting them (thus the GUI would request the settings, and would push them back).

I see little reason for a feature to be responsible for exposing/managing settings. In general this should be done by a separate service. Allocating such responsibility to a feature would create a requirement of exposing the whole feature to random parts (like the GUI which should have nothing to do with the GPIO service it self) of the app. I think settings should be kept within config manager and the different domains exposed via separate getters/setters (but this latter is just an idea, the settings domains can have their own config manager, but probably this would create unnecessary overhead, but you get the point).

[JaapvanEkris]

I largely agree. I think we need to add levels (i.e. distinguishing the individual features) and then the underlying settings (so you first select the ANT+ feature before modifying the ANT+ settings). As said, I think some settings should only be editable from the commandline. Especially the GPIO and flywheel settings are prone to this (which are currently mixed into the normal settings). We could display them, but making them too easy to edit by normal users would be odd, as the GPIO pin is also intertwined with the OS parameters.

Yes I agree with levels (like, general, advance, expert), but I would not have settings that are not settable via the GUI. Direct edit of files does not allow input validation and hence error prone. Settings that are changed via the GUI can be validated upon entry and useful validation errors can be thrown. If a setting is set via the GUI all sorts of input validation can be added and this would enhance consistency.

Well, we validate settings now as well in V1Beta-updates. And I put in a lot of thought what can reasonably be expected. In all honesty, I don't see how you can validate a setting like dragfactor, as the range of real encountered values is already collosal for the known machines. That is one of the reasons to keep it out of the GUI: normal users are totally clueless what the settings should be, a typo is made far too easy there and you can't filter for bad settings in a meaningful way. Only thing you create by exposing it to the GUI is that a user or his kid plays with it and messes up these complex to debug settings. And the only way to debug these settings is through the syslog anyway, so if you want to change anything related to the engine, you are stuck with SSH to begin with. When you can only reach it through SSH and during the setup procedure, you can reasonably expect someone to follow a manual. For some settings it is ridiculous as well, as the GPIO pin has to be set in OS files as well.

The above would not impact the settings domain approach as levels exits purely for representation purposes.

I think there are only two types of users: users and owners. The latter has access to the machine related settings (i.e. GPIO settings, flywheel settings), without changing the core of what is machine related. Other more user focussed settings (i.e. GUI, device interaction, file creation) should be made accessible through the GUI.

I guess that we could make the features responsible for exposing settings and persisting them (thus the GUI would request the settings, and would push them back).

I see little reason for a feature to be responsible for exposing/managing settings. In general this should be done by a separate service. Allocating such responsibility to a feature would create a requirement of exposing the whole feature to random parts (like the GUI which should have nothing to do with the GPIO service it self) of the app. I think settings should be kept within config manager and the different domains exposed via separate getters/setters (but this latter is just an idea, the settings domains can have their own config manager, but probably this would create unnecessary overhead, but you get the point).

I think that when you want truly independent services with minimal dependencies, a service will tell the rest of the world what settings it has. Otherwise you have a settings manager that somehow magically guesses what settings are appropriate for a feature. That approach will still split up knowledge about what a feature needs in two modules (as is the current case, as the current config manager already does validation and activation of settings). The GUI should be able to ask a feature what the possible settings are, and it should be able to send selected choices (and allowing the feature to persist and activate these settings).

[Abasz]

Well, we validate settings now as well in V1Beta-updates. And I put in a lot of thought what can reasonably be expected. In all honesty, I don't see how you can validate a setting like dragfactor, as the range of real encountered values is already collosal for the known machines. That is one of the reasons to keep it out of the GUI: normal users are totally clueless what the settings should be, a typo is made far too easy there and you can't filter for bad settings in a meaningful way.

I am not sure what you are trying to say here. If you validate the input on as part of the form validation process, you can provide instant feedback to the user. You can even restrict the input values and ranges (i.e. only numbers that are greater than or equal to zero are accepted). Actually the point of validation in the GUI is to push a real time feedback on the validation. Its essentially the same when you fill out a order form online and you are not entering your phone number correctly. So wtr. your statement that "a typo is made far too easy there and you can't filter for bad settings in a meaningful way" the program does not even get to that point in case of a typo as the user would not be able to submit/save/persist the changes until the input is correct. The issue with editing files is that its error prone. If you have a look at the issue list that has been submitted by users to the discussion half of them hit a comma error or a wrong structuring of the settings (like incorrect object properties).

Only thing you create by exposing it to the GUI is that a user or his kid plays with it and messes up these complex to debug settings. And the only way to debug these settings is through the syslog anyway, so if you want to change anything related to the engine, you are stuck with SSH to begin with. When you can only reach it through SSH and during the setup procedure, you can reasonably expect someone to follow a manual. For some settings it is ridiculous as well, as the GPIO pin has to be set in OS files as well.

Well if the worlds solution against invalid input would have been to force ssh and manual tinkering with settings files than probably online shopping would not strive. I think you are quite missing the point of input validation. The main aim is to prevent malformed values to be entered to prevent bugs and crashes as well as to ease the learning curve for changing settings (like if someone never seen JS object structure should be liberated from this). Also like bugs that derive from entering a negative number as dragfactor, or entering text where number is expected, or preventing chasing down issues when a comma is missed in the settings file (which came up multiple times in the discussion).

I think that when you want truly independent services with minimal dependencies, a service will tell the rest of the world what settings it has. Otherwise you have a settings manager that somehow magically guesses what settings are appropriate for a feature. That approach will still split up knowledge about what a feature needs in two modules (as is the current case, as the current config manager already does validation and activation of settings). The GUI should be able to ask a feature what the possible settings are, and it should be able to send selected choices (and allowing the feature to persist and activate these settings).

While I can see your reasoning on the origination of the settings, I think there is a few aspects that it does not consider. First of all managing settings has a very well defined set of functionality such as saving, editing creating (basically it has CRUD operations, maybe not delete). Having the features managing settings completely would require them to have these functionality each and every that would allocate more responsibility and repetition (of course functionality of these can be abstracted to a base class that can be inherited by the feature which is a possible solution, but at the end of the day in order to avoid code repetition some centralization will be necessary).

Second you are not considering that it is not that hard to implement a dynamic settings manager actually. Since settings management is mostly CRUD, new settings can be migrated when a new feature is registered or added (i.e. the relevant settings storage files, lets say json) is generated. Here the settings manager would not need the actual settings but the metadata of the setting (default value, type, name, validation etc.). The GUI would not just magically guess the settings but ask the settings manager for what is the current settings object and dynamically render the settings (it is even possible to add/register metadata that describes the characteristics of a given setting, like validation rules types etc). I am not saying that dynamic settings management is the easiest solution from a coding perspective and I am not saying that this is the way.

The issue with the GUI asking features is that actually in this case the GUI would either have a dependency on each feature that wants to expose settings (which would thighly couple every feature to the GUI) or a middle layer is necessary as a kind of config manager.

[JaapvanEkris]

Well, we validate settings now as well in V1Beta-updates. And I put in a lot of thought what can reasonably be expected. In all honesty, I don't see how you can validate a setting like dragfactor, as the range of real encountered values is already collosal for the known machines. That is one of the reasons to keep it out of the GUI: normal users are totally clueless what the settings should be, a typo is made far too easy there and you can't filter for bad settings in a meaningful way.

I am not sure what you are trying to say here. If you validate the input on as part of the form validation process, you can provide instant feedback to the user. You can even restrict the input values and ranges (i.e. only numbers that are greater than or equal to zero are accepted). Actually the point of validation in the GUI is to push a real time feedback on the validation. Its essentially the same when you fill out a order form online and you are not entering your phone number correctly.

The issue is that the logic (what is a valid range for a setting) is put in the GUI, and the use is in the feature. I rather have a feature tell the GUI what a valid range is for a setting, than a GUI deciding for a feature what the valid ranges are. If you want to follow a clear separation of concerns, this is an extremely obvious approach. The GUI might be used to present stuff and select options, and it might even check if it is in a specific range, but it is the feature that tells the GUI what parameters can be set and what the valid ranges are.

Please note, the basis is already present in the current config manager, as all variable types and their plausible ranges are already coded there.

So wtr. your statement that "a typo is made far too easy there and you can't filter for bad settings in a meaningful way" the program does not even get to that point in case of a typo as the user would not be able to submit/save/persist the changes until the input is correct. The issue with editing files is that its error prone. If you have a look at the issue list that has been submitted by users to the discussion half of them hit a comma error or a wrong structuring of the settings (like incorrect object properties).

And these errors are now all fixed by the current config manager. The current config manager will fix almost any error found by using sane defaults, and log what it did in the syslog.

The problem is that with real validation you need understanding of the machine and want to make sure people follow the manual when changing them. You don't want people to change settings of the flywheel. You see the same distinction in modern cars: as a user you can change a lot of parameters in a car, but certainly not all. Changing the upper speed limit for example, is a feature that is only available to mechanics with the right equipment and training.

Only thing you create by exposing it to the GUI is that a user or his kid plays with it and messes up these complex to debug settings. And the only way to debug these settings is through the syslog anyway, so if you want to change anything related to the engine, you are stuck with SSH to begin with. When you can only reach it through SSH and during the setup procedure, you can reasonably expect someone to follow a manual. For some settings it is ridiculous as well, as the GPIO pin has to be set in OS files as well.

Well if the worlds solution against invalid input would have been to force ssh and manual tinkering with settings files than probably online shopping would not strive.

See https://edition.cnn.com/2023/02/01/business/grubhub-child-one-thousand-dollar-order/index.html

I think you are quite missing the point of input validation. The main aim is to prevent malformed values to be entered to prevent bugs and crashes as well as to ease the learning curve for changing settings (like if someone never seen JS object structure should be liberated from this).

In all honesty, some settings should not be available to users in a normal situation. If you can't use SSH and edit the config.js file, you're probably incapable of debugging the settings to begin with as it requires access to syslog to see what the engine does under the hood. Unfortunately, it is the only way to create a decent settings file. When people are making their own custom setting, they should use SSH and syslog to see what is going on under the hood, to be able to perfect the settings.

Also like bugs that derive from entering a negative number as dragfactor, or entering text where number is expected, or preventing chasing down issues when a comma is missed in the settings file (which came up multiple times in the discussion).

You can't, as this line in ConfigManager.js explicitly blocks that as it does:

checkFloatValue(configToCheck.rowerSettings, 'dragFactor', 1, null, false, false, null)

This is one of the key steps taken in the ConfigManager.js of V1Beta-Updates. It checks whether the value found is numeric, whether it is in a valid range (dragfactor > 1), and whether it is allowed to be zero. It will nicely log an error to syslog if it does. It does so for all settings aside the file locations. For each and every setting, there is a check whether the setting even exists, if the type is correct, whether ranges are plausible, if it can be zero and if it is allowed to repair it if the check fails.

I think that when you want truly independent services with minimal dependencies, a service will tell the rest of the world what settings it has. Otherwise you have a settings manager that somehow magically guesses what settings are appropriate for a feature. That approach will still split up knowledge about what a feature needs in two modules (as is the current case, as the current config manager already does validation and activation of settings). The GUI should be able to ask a feature what the possible settings are, and it should be able to send selected choices (and allowing the feature to persist and activate these settings).

While I can see your reasoning on the origination of the settings, I think there is a few aspects that it does not consider. First of all managing settings has a very well defined set of functionality such as saving, editing creating (basically it has CRUD operations, maybe not delete). Having the features managing settings completely would require them to have these functionality each and every that would allocate more responsibility and repetition (of course functionality of these can be abstracted to a base class that can be inherited by the feature which is a possible solution, but at the end of the day in order to avoid code repetition some centralization will be necessary).

I think it should be a base persistency class, or a shared component (currently it is a repeatedly shared component I think). But the knowledge what valid ranges are and should be, should be checked when reading in the settings file (to prevent file corruption) and when getting new settings before persisting and activating them. The current set-up works, but centralises the logic to ConfigManager.js, which is awkward if you add functionality that requires (a modified use of) a setting.

Second you are not considering that it is not that hard to implement a dynamic settings manager actually. Since settings management is mostly CRUD, new settings can be migrated when a new feature is registered or added (i.e. the relevant settings storage files, lets say json) is generated. Here the settings manager would not need the actual settings but the metadata of the setting (default value, type, name, validation etc.). The GUI would not just magically guess the settings but ask the settings manager for what is the current settings object and dynamically render the settings (it is even possible to add/register metadata that describes the characteristics of a given setting, like validation rules types etc). I am not saying that dynamic settings management is the easiest solution from a coding perspective and I am not saying that this is the way.

See ConfigManager.js, but I already had to fix issues where you added functionality to the BLE/ANT+, but forgot to update the ConfigManager.js checks. This stuff really needs to be kept together or people will miss the relation here.

The issue with the GUI asking features is that actually in this case the GUI would either have a dependency on each feature that wants to expose settings (which would thighly couple every feature to the GUI) or a middle layer is necessary as a kind of config manager.

I think you can include an almost static JSON response on a settings function call, for each feature. There it can tell (similar to my current implementation of ConfigManager.js) what the types are, what the valid ranges are, and if there are sane defaults. Possibly it could even tell what the current values are.

[Abasz]

Based on the code I understand that the forceOnHandle array is filled with the _torqueBeforeFlank instead of the _torqueAtBeginFlank
openrowingmonitor/app/engine/Flywheel.js
Line 209 in 9422716
return _torqueBeforeFlank
What is the reason for this? I would expect that actually when the data for the force curve is collected, the torque related to the current impulse's deltaTime should be used. So would not be the _torqueAtBeginFlank value better?

Nope. The flank isn't exposed to the outside world: all exposed metrics (angular position, angular velocity, angular acceleration, time) are based on the BeforeFlank (thus excluding any knowledge about the contents of the flank itself). The only exception is that flank data is used to determine the flywheel state (i.e. powered, unpowered, dwelling). That is essentially the flank's use, you need to determine whether you are in a drive or recovery, so in the next processed currentDt, you can pick up the data in the BeforeFlank position and process it in the correct drive state,

Please note: the flank is also used to create a Theil-Senn regression of the Angular Velocity and Angular Acceleration, where the algebraic properties are used to stabilise these measurements further, which both are essential ingredients to the Torque calculation, This is why we can't use a series of torques to determine flywheel state, but we must rely on a single measurement at the head of the flank.

While this explanation is clear I think wrong force value gets included in the force curve. Let me explain:

the flywheeel.torque() function (

openrowingmonitor/app/engine/Flywheel.js

Line 207 in 9422716

function torque () {
) is called only in the startDrivePhase() and updateDrivePhase() to save that into the force curve. In this moment ORM knows that the current flank is drive. So when we are in startDrivePhase() function I would expect that here the _torqueBeforeFlank is actually part of the recovery.
To provide the specific issue, I noticed frequent small negative value as the first value in the force curve in driveHandleForce when running ORM in debugging mode that in my view should not have been there (I use no recovery slope and stroke quality of 1, so basically recovery detection is purely torque based). Based on the code those negative forces are actually the last force values before the drive, hence I would not expect those to be included in the force curve.

This was a very conscious design decission I made over a year ago. Really revisitting this would require me to go back to my notes. Key concept here is that for the data in the flank we don't know what phase it belongs to, and we do know for the data in front of the flank.

That is correct, but the createCurveAligner in (see https://github.com/JaapvanEkris/openrowingmonitor/blob/v1beta_updates/app/engine/RowingStatistics.js) is there to remove the head and tail values if there is noise in them. As torque is based on potentially volatile metrics and thus can be noisy, it is a necessary step that can't be skipped. That is what I meant with that the current forcecurve implementation requires some serious thinking before turned into a live forcecurve viewer on the GUI (as cleaning up noise live can have weird visual effects).

If you use the exposed driveHandleForceCurve from the RowingStatistics with a decent minumumForceBeforeStroke, that shouldn't happen. One can debate if the curvealigner should always prefix the curve with a starting base value (zero for force, but what to do with handle speed), but that is the thing that produces force curves for over a year now in quite a reliable fashion (and yes, I check them all each row).

And please, let's not have discussions about extensions in a pull request. This reduces the clarity of the status of this pull request for the package maintainer.

[Abasz]

Sorry my intention was to put it here, just did not notice that I commented under the PR when referenced the line in the code. I may try to delete the comments from there but not sure that GitHub allows this without trace.

This was a very conscious design decission I made over a year ago. Really revisitting this would require me to go back to my notes. Key concept here is that for the data in the flank we don't know what phase it belongs to, and we do know for the data in front of the flank.

I am not saying this needs to be revisited, I just wanted to understand the background as my logic would have dictated for this specific use case atBeginningFlank would have been the appropriate value. Actually from a consistency perspective beforeFlank is the right value, and for most of the metrics its clear that beforeFlank is the appropriate in ORM model.

Anyway this, from a purely force curve perspective, is not relevant, and I am not challenging the choice, as it can be cleaned out easily and there would be no loss of information (e.g. when negative values at the beginning are zeroed out).

[JaapvanEkris]

Sorry my intention was to put it here, just did not notice that I commented under the PR when referenced the line in the code. I may try to delete the comments from there but not sure that GitHub allows this without trace.

No issue. GitHub isn't the clearest environment for me either. I'm trying to convince @laberning to merge the updates into V1Beta (and hopefully even make it main), as I see a lot of people struggling with it.

This was a very conscious design decission I made over a year ago. Really revisitting this would require me to go back to my notes. Key concept here is that for the data in the flank we don't know what phase it belongs to, and we do know for the data in front of the flank.

I am not saying this needs to be revisited, I just wanted to understand the background as my logic would have dictated for this specific use case atBeginningFlank would have been the appropriate value. Actually from a consistency perspective beforeFlank is the right value, and for most of the metrics its clear that beforeFlank is the appropriate in ORM model.

It is a tough choice, hence the notes. I agree it feels a bit weird to have cleanup functionality for the curve, but it works well. Issue is that on a decent data rich machine, you have different issues than on a simpeler machine. I think once we have a lot more data and experience, I should look at it again. As the design isolates the impact of such design changes quite well, it can be done afterwards.

Anyway this, from a purely force curve perspective, is not relevant, and I am not challenging the choice, as it can be cleaned out easily and there would be no loss of information (e.g. when negative values at the beginning are zeroed out).

I agree. What are you trying to achieve: a single curve after the stroke, or a dynamically presented one during the stroke. The later is achieved by the PM5, but I don't know if the combination of flanklength (thus delay in torque calculation) and noise filtering (again potentially delaying results) will be a great user experience. But there is only one way to find out.

[Abasz]

So I have another question on this topic (sorry about the amount of questions, the reason I am asking them as I am looking at the code and I dont want to reinvent the wheel in terms of design decision) that relates to the force curve.

On the initial start i.e. when we are in the WaitingForDrive for the first time after starting the program, logically ORM waits until the deltaTime Series is filled up with data, then a power is detected and then starts counting totalNumberOfImpulses and certain other metrics. But this is not the case for the force/torque data as it is calculated for every impulse.

now my question is that would not ORM be slightly out of sync (totalSpinningTime and totalNumberOfImpulses) with one impulse at the start that impacts (not materially I suppose) distance calculation as it is possible like with this data. What I am trying to say is that here it is clear that the whole rowing session starts with a drive, but (flankLength: 7) the impulses for distance calculation as well as for the data for force curve gets collected only after 0.06313 (i.e. the first datapoint for which data is considered is 0.058266).

So for your to understand the background of my question: with a "cold" start of ORM I noticed that the force curve for the very first stroke (assuming that it is started with Drive immediately) starts with rather high values.

Its clear from the data that the drive started 7 impulses before, and the code? for deltaTimeBeforeFlank is very much correctly gets the data from at the beginning of the flank (_deltaTime.yAtSeriesBegin() -> seriesArray[0]):

      totalNumberOfImpulses += 1
      _deltaTimeBeforeFlank = _deltaTime.yAtSeriesBegin()
      totalTimeSpinning += _deltaTimeBeforeFlank
      _angularVelocityBeforeFlank = _angularVelocityAtBeginFlank
      _angularAccelerationBeforeFlank = _angularAccelerationAtBeginFlank
      _torqueBeforeFlank = _torqueAtBeginFlank

The totalNumberOfImpulses would be 1 and lag behind exactly with 1 impulse compared to the totalSpinning time: when the totalNumberOfImpulses gets the first increment, the totalSpinning time will be 0.315088 (which is not the beforeFlank - that is 5.3 but rather at the beginning of the flank). But the _torqueBeforeFlank value will equal to the value calucated at 0.06313 which for me would be the beginning of a different flank than the data for totalNumberOfImpulses and totalSpinningTime.

I suppose you have considered this (and you may concluded (and I dont want tear up old wounds so to speak :), that it is fine based on a cost/benefit approach, or I am simply missing something :)), but I was wondering whether this creates a slight out of sync state.

(please note that the below torque values are for representation purposes only)

5.331447 | _torqueBeforeFlank = 0 | _torqueAtBeginFlank = 0
0.315088 | _torqueBeforeFlank = 0 | _torqueAtBeginFlank = 0
0.154906 | _torqueBeforeFlank = 0 | _torqueAtBeginFlank = 1
0.110027 | _torqueBeforeFlank = 0 | _torqueAtBeginFlank = 2
0.089455 | _torqueBeforeFlank = 0 | _torqueAtBeginFlank = 3
0.077505 | _torqueBeforeFlank = 0 | _torqueAtBeginFlank = 4
0.069448 | _torqueBeforeFlank = 0 | _torqueAtBeginFlank = 5
0.06313 <--- Drive registered | _torqueBeforeFlank = 5 | _torqueAtBeginFlank = 6
0.058266 first datapoint for which metrics are collected | _torqueBeforeFlank = 6 | _torqueAtBeginFlank = 7
0.054404
0.050983
0.048365

[JaapvanEkris]

So I have another question on this topic (sorry about the amount of questions, the reason I am asking them as I am looking at the code and I dont want to reinvent the wheel in terms of design decision) that relates to the force curve.

No problem. Some design decisions were really tough as there were only suboptimal choices either way, and they involved a lot of subtle details. Luckily I wrote down a lot of considerations, but it requires a lot of thinking before I really understand my own ideas from a year ago.

But I also realise we need to get transparency on this. Key element for Open Source is that other people can read and understand the code, and hopefully improve or extend it. But this is some tough code with lots of edge cases and decisions that could have gone either way. But explaining it in the docs is critical.

And don't stop pushing the boundaries. We haven't come this far with ORM without people wanting more.

On the initial start i.e. when we are in the WaitingForDrive for the first time after starting the program, logically ORM waits until the deltaTime Series is filled up with data, then a power is detected and then starts counting totalNumberOfImpulses and certain other metrics. But this is not the case for the force/torque data as it is calculated for every impulse.

Yeah, you need Torque for the Torque-based force detection as well. For data-rich machines it isn't much of an issue, but for example in a water rower like @laberning has, you have the end of the drive already after 5 datapoints, and with a flanklength of 3, things are going to be tight.

In all honesty, we should look at how to handle the force curve for the first stroke. AFAIK, the PM5 doesn't even provide one. In that first stroke, it is total chaos from a force curve (and handle force and power curve) perspective. Dragfactor is a guess (as it is only calculated after the first stroke, with quite some margin for error), and after you clear both the flanklength and the maximumTimeBetweenImpulses threshold (you need to reach flywheel.isAboveMinimumSpeed()) you probably lost some datapoints along the way. On my Concept2, I typically lose 20 (of the usually 80) datapoints for my drive in the first stroke.

As your flywheel is at a standstill to begin with, lots of metrics don't make that much sense yet (as radial velocity is relatively low, some physical aspects like drag aren't optimally calculated either). As the ratio of angular velocity and angular acceleration are quite different from regular strokes, instantaneous torque and power calculations also do weird things anyway when noise enters the calculations. I never considered it a big issue, and looking at the concept2 PM5 they don't either. Looking at the research from the Ulm University team, Concept2 also has huge issues there (the first 5 strokes are complete nonsense).

now my question is that would not ORM be slightly out of sync (totalSpinningTime and totalNumberOfImpulses) with one impulse at the start that impacts (not materially I suppose) distance calculation as it is possible like with this data. What I am trying to say is that here it is clear that the whole rowing session starts with a drive, but (flankLength: 7) the impulses for distance calculation as well as for the data for force curve gets collected only after 0.06313 (i.e. the first datapoint for which data is considered is 0.058266).

Yeah, but I looked at most monitors with some reputation, and they seem to do the same thing. You need to have some bar where you block measurements from triggering a start. In essence, if you stop rowing, you want the monitor to stop. When you pick up the handle and really start to do some rowing, you want it to pick up. Some filtering on noisy readings is inevitable there I'm afraid: you don't want the noise of a spinning down flywheel to start a next drive again, messing with all kind of metrics.

So for your to understand the background of my question: with a "cold" start of ORM I noticed that the force curve for the very first stroke (assuming that it is started with Drive immediately) starts with rather high values.

It does as it skips quite a bit from the first stroke from moving. For comparison: ORM skips about 20 datapoints before it starts to move. The PM5 typically lags 0.5 to 1.0 seconds behind ORM in starting. For me, startup-behaviour is a subject of investigation for me, as it looks that it is the big difference between the current ORM and PM5 data on a concept 2 (currently, our data deviates around 0.03% from the Concept2 data). When ORM starts earlier than the PM5, thus including an incredibly slow portion of a row where the PM5 ignores it, it might explain this deviation.

There is some discussion among competitive rowers whether this startup delay is a good thing, as it would "delay" the really slow part of a start, and if you know what you are doing you could even get a flywheel up to a certain speed before a start is counted (thus allowing cheating in qualifications and races).

I suppose you have considered this (and you may concluded (and I dont want tear up old wounds so to speak :), that it is fine based on a cost/benefit approach, or I am simply missing something :)), but I was wondering whether this creates a slight out of sync state.

Well, let's look at bugs and decisions when we need to, but these are quite complex. One of the key issues is that you want to represent a analog system with discrete measurements. So at a totalSpinning time of 0.315088, it could very well be that the angular displacement is 1 (times something with pi).

Carefully looking at how these curves behave is interesting, but requires some drawings for me. Sometimes we trigger knowing that the drive must have happened somewhere before the flank, as the current flank has a certain shape and direction. Writing that stuff down well, based on a discussion, might help in understanding the decisions made, allowing the code to mature. But that isn't an easy process, I know I've drawn quite some graphs to get a grip onto things.

[JaapvanEkris]

@Abasz, also looking at this discussion as well: #131

I think what we clearly agree that Weblient settings should be kept in the webclient, and that a good first step would be to allow the client to configure itself :). Discussions about the GUI ordering other features to change settings and these features persisting their own settings etc. could come in a later stage (also looking at future plans where the datarecorder for fit and tcx potentially providing data second-based). I think that the GUI managing its own settings is a good first step which will fit that plan regardless, and we could do that anyway.

[Abasz]

I agree, actually I had the same thinking when having read that discussion. I have one problem here wtr. persisting the settings. Namely that settings persistence cannot be done efficiently on the client side (well it is possible to use the browser storage api, but that ties the settings to the specific browser). Nevertheless, it could be an intermediary solution, and once settings management complete in this sense it is not that hard to swap stuff out, assuming that the settings object structure the client code needs to deal with is the same (and differences are handled on the server side).

EDIT: of course ORMs use case is a slightly special as generally you do not swap browsers when you use it (apart from a few people who use reverse proxy in apache to be able to access to gui from laptop and other devices, as the Rpi is completely headless).

[JaapvanEkris]

I agree, actually I had the same thinking when having read that discussion. I have one problem here wtr. persisting the settings. Namely that settings persistence cannot be done efficiently on the client side (well it is possible to use the browser storage api, but that ties the settings to the specific browser). Nevertheless, it could be an intermediary solution, and once settings management complete in this sense it is not that hard to swap stuff out, assuming that the settings object structure the client code needs to deal with is the same (and differences are handled on the server side).

Client side storage would also allow us to learn without messing up server side right away. One thing I'm looking at is what people expect from the GUI settings. I noticed that Concept2's ErgData app allows you to change the GUI (they have several templates), but you can't put any field to any location (can't find Distance Per Stroke for example). Seeing what issues people encounter could help us in developing the final solution.

[Abasz]

While I was fiddling with the force curve to see what is the best solution (I dont think real time is viable actually, I gave it a quick spin by updating the UI on every new impulse and it was not that smooth, the chart.js was not able to handle the updates - which is possible was due to the animation, so its possible that it could be tweaked, but I am not rooting for real time). Anyway, so I was playing with the damper settings on my machine and tried to see how higher DF would impact on force curve and technique. I noticed that with plain torque stroke detection (i.e. where I set the minimumStrokeQuality to 1 and minumumRecoverySlope to 0) when I set a higher damper and then back to lower ORM was not able to recognize any strokes.

I realized that this is due to the fact that we are using the dragfactor of the last recovery to detect the next where that DF is no longer valid (as the damper was lowered and the air drag decreases, the torque actually never dips under zero). This is just an observation I wanted to share, not sure if you have ever thought about this.

Now the main reason or question I would like to ask is that while I was debugging this (naturally first I thought its a bug of the stroke detection but really this is a bug of the settings) I noticed that when you initialize the Flywheel you call reset() which pushes two zeros to the _deltaTime and _angularDistance (https://github.com/JaapvanEkris/openrowingmonitor/blob/bd97723f8b969082e79c8fd6ad303ed9f69b280c/app/engine/Flywheel.js#L332-L333):

    _deltaTime.push(0, 0)
    _angularDistance.push(0, 0)

Why is this necessary? What is the purpose of having one initial element that is zero in the deltaTime OLS and the TS Quadratic series? The interesting part is that you dont have the same for the recoveryDeltaTime, i.e. after reset the first element in that is an actual data point.

[JaapvanEkris]

While I was fiddling with the force curve to see what is the best solution (I dont think real time is viable actually, I gave it a quick spin by updating the UI on every new impulse and it was not that smooth, the chart.js was not able to handle the updates - which is possible was due to the animation, so its possible that it could be tweaked, but I am not rooting for real time). Anyway, so I was playing with the damper settings on my machine and tried to see how higher DF would impact on force curve and technique. I noticed that with plain torque stroke detection (i.e. where I set the minimumStrokeQuality to 1 and minumumRecoverySlope to 0) when I set a higher damper and then back to lower ORM was not able to recognize any strokes.

As I understand you correctly, this is in one rowing session, so say the first 100 strokes on DF150, followed by a bunch of strokes at DF 100

I realized that this is due to the fact that we are using the dragfactor of the last recovery to detect the next where that DF is no longer valid (as the damper was lowered and the air drag decreases, the torque actually never dips under zero). This is just an observation I wanted to share, not sure if you have ever thought about this.

I did. Concept2 thought about it as well as it was abused in races (you row a bit in high DF, and then drop the DF mid-row, and enjoy a bunch of easy strokes where the PM3 considers them heavy strokes). In races, and in the PM5, it is detected and as I understand it actually blocks uploading validated results. I know that changing the damper in a race, you will get disqualified.

Now the main reason or question I would like to ask is that while I was debugging this (naturally first I thought its a bug of the stroke detection but really this is a bug of the settings) I noticed that when you initialize the Flywheel you call reset() which pushes two zeros to the _deltaTime and _angularDistance (https://github.com/JaapvanEkris/openrowingmonitor/blob/bd97723f8b969082e79c8fd6ad303ed9f69b280c/app/engine/Flywheel.js#L332-L333):

    _deltaTime.push(0, 0)
    _angularDistance.push(0, 0)

Why is this necessary? What is the purpose of having one initial element that is zero in the deltaTime OLS and the TS Quadratic series? The interesting part is that you dont have the same for the recoveryDeltaTime, i.e. after reset the first element in that is an actual data point.

recoveryDeltaTime is only used for the calculation of drag, and starts at the begin of the recovery and ends at the end of the recovery (i.e., the Linear series that determines the slope for the dragfactor). So there is no "default value" there, and even then, we want to prevent including datapoints that might belong to a drive into this series, as it might poison the drag calculation.

_deltaTime and _angularDistance are continous running metrics which start of when the flywheel is standing still. They must be zeroed in, as after inserting the first datapoint, its slope is important. If you look, their slopes are used constantly to check the flywheel status. So providing a good initialisation is key to getting the slope away from its fail-safe value as quickly as possible (and thus the metrics actually telling something), as some machines only produce four or five datapoints during a drive. These zero's are actually valid datapoints, which are "missing" because a stationary flywheel does not trigger a datapoint. Otherwise you call a slope on a single value, which is undefined.

[Abasz]

So I completed an update of the GUI it adds the following features: https://github.com/Abasz/openrowingmonitor/tree/improve-gui

• Added force curve component
• Created a settings dialog component that can manage GUI settings
• Made the metric tiles settable including the metrics to show and the order as well
• Added the option to hide the icons while making the font increase (as per request Give the user the option on which field to overwrite with heart rate data #131)

some Screenshots:

There are a couple of notes as at now:

• I hate CSS :) I spent too much time with fine tuning the CSS and in certain screen sizes it is still not the best (has limitations). In order to save dev time I left these as is and when a user reports a specific issue with a specific screen size I can implement a fix.
• Current settings are saved to browser localStorage that is tied to the browser and the specific url (i.e. changing the browser or the port etc. will clear the settings). Nevertheless, persistence works over browser refresh and restart of course. But the settings saving API can be swapped out.
• I am not sure if this code is the most efficient and most modular but I will try to see if it can be improved in some way. The issue is that I dont really have much experience with Lit-html so I was not really able to take full advantage of the framework. The one thing I am not 100% happy with is the way changes are propagated to the top level and "service" calls (e.g. saving settings) is made. Currently this is done via events but I think using services would be more appropriate. But because the original code used this pattern I sticked to this as well. Until the app functionality is this simple this should be probably fine but once it starts to grow I am not sure this is the most maintainable pattern.
• Actually I noticed that if I decrease the font size and play around, on a decently sized screen 12 tiles (i.e. 3 rows) can be fitted very nicely giving the ability to show more metrics. I will explore this as I want to extend the available metrics (e.g. distance per stroke, drag factor, maybe the other charts for handle power and what have you).

Next step:
I will add showing peak value to the force curve at the weekend.
Do a code review and refactoring if necessary and finalize. Ones tested can be merged into your v1beta_update branch I suppose and tested by a broader audience.

[JaapvanEkris]

Wow, that is cool! Looking forward to the pull request.

It actually makes sense to have a per browser setting of metrics: in theory you could add multiple screens and have them display different things.

Could you add some additional datafields as well. More specifically:

• driveDuration (usually displayed in miliseconds, so needs *1000)
• driveLength (length of handle travel during the drive, usually displayed in meters)
• cycleDistance (Distance per Stroke, also in meters)

[Abasz]

Yes, I will add those. While I was reviewing the code, I realized that this would be a very good time to clean up the GUI metrics API. What I mean is that I have sufficient understanding of the components to remove the dependency on the server formatting and use solely raw data and format it on the browser side. This will make the extending of the metrics much better on the long term also would enable clean up on the back end quite a bit.

EDIT:
There is not a lot actually:
These will be the properties that will no longer be needed:

"totalMovingTimeFormatted"
"totalLinearDistanceFormatted"
"cyclePaceFormatted"

Need confirmation:
Can the below two calculated from the totalCalories? Does it make sense to do the calculation on the server side instead of the client?

"totalCaloriesPerMinute"
"totalCaloriesPerHour"

EDIT2:

I noticed that in the RowingStatistics "totalMovingTimeFormatted" is a completely different metric than the "totalMovingTime". As far as I understand this shows remaining time from the interval if time interval is set. From an API perspective this is not a very good approach especially as interval related metrics can have their own metric tile in the future. I propose to add a separate property for "remainingIntervalTime" (or something)

[JaapvanEkris]

Yes, I will add those. While I was reviewing the code, I realized that this would be a very good time to clean up the GUI metrics API. What I mean is that I have sufficient understanding of the components to remove the dependency on the server formatting and use solely raw data and format it on the browser side. This will make the extending of the metrics much better on the long term also would enable clean up on the back end quite a bit.

Yeah, and it greatly improves consistency. It is quite odd that the linear distance is formatted in the GUI, where time is formatted server side.

I noticed that in the RowingStatistics "totalMovingTimeFormatted" is a completely different metric than the "totalMovingTime". As far as I understand this shows remaining time from the interval if time interval is set. From an API perspective this is not a very good approach especially as interval related metrics can have their own metric tile in the future. I propose to add a separate property for "remainingIntervalTime" (or something)

I agree that totalMovingTimeFormatted is indeed an akward naming as it indeed has totally different behaviour than totalMovingTime. And as setting expectations from an API perspective, you are completely right: it is a bad decision. What I propose is to go a slightly different route in anticipation of interval based functionality: let's provide features both with the interval target and the current progress (and the projected end). This way, the features can decide how to handle interval data themselves (the PM5 interface has some provisions for this as well, that I want to add later on as well).

EDIT: There is not a lot actually: These will be the properties that will no longer be needed:

"totalMovingTimeFormatted"

I totally agree this logic should be moved. On hindsight, we need to split this in its fundamentals, so that features can use it as they see fit. What I propose is that RowingStatistics produces two new time indicators intervalTargetTime (target time to be spent in the current interval, excluding time spent in previous intervals) and intervalMovingTime (time spent inside the current interval, excluding time spent in previous intervals).

So, in the GUI you get some code that will determine the time to be displayed via something similar to:
totalMovingTimeFormatted: intervalTargetTime > 0 ? secondsToTimeString(intervalTargetTime - intervalMovingTime) : secondsToTimeString(intervalMovingTime)

"totalLinearDistanceFormatted"

Here, I also propose to move to more fundamental parameters, similar to the time. What I propose is that RowingStatistics produces two new distance indicators intervalTargetDistance (length of the current interval, WITHOUT including distance travelled in previous intervals) and intervalLinearDistance (distance travelled inside the current interval, WITHOUT including distance travelled in previous intervals).

So, in the GUI you get some code that will determine the time to be displayed via something similar to:
totalLinearDistanceFormatted: intervalTargetDistance > 0 ? intervalTargetDistance - intervalLinearDistance : intervalLinearDistance

I also propose to rename the cycleProjectedEndTime and cycleProjectedEndLinearDistance to cycleIntervalProjectedEndTime and cycleIntervalProjectedEndLinearDistance. This seems to be the basis for all inteval based metrics for the features.

"cyclePaceFormatted"

Completely agree, let's remove this sucker.

Need confirmation: Can the below two calculated from the totalCalories? Does it make sense to do the calculation on the server side instead of the client?

"totalCaloriesPerMinute"
"totalCaloriesPerHour"

They are calculated based on the curve of calories through time, which the feature could also easily do server side. Currently, this data is used only by the FTMS features, so the IndoorBikeDataCharacteristic.js and RowerDataCharacteristic.js. This isn't done client side, but it would reduce the number of unnecessary metrics in RowingStatistics. Although it would result in a bit of code duplication, the two FTMS profiles are copies already, so it wouldn't matter that much IMHO.

[Abasz]

I actually created a factory for the metric tiles that simplifies adding more tiles. I will push these commits shortly. Basically adding a new tile should be as easy as adding a property to an object in code that compiles with a set interface.

For example:

{
  distance: {
    displayName: 'Distance',
    size: 1,
    template: (metrics, showIcon) => {
      const linearDistance = formatDistance(metrics?.totalLinearDistance)

      return simpleMetricFactory(linearDistance.distance, linearDistance.unit, showIcon ? icon_route : '')
    }
  },
 calories: { displayName: 'Calories', size: 1, template: (metrics, showIcon) => simpleMetricFactory(formatNumber(metrics?.totalCalories), 'kcal', showIcon ? icon_fire : '') },
}

Any rendering logic (both in the settings tab and in the dashboard) will be driven by this object and constructed automatically.

So any metric that we would like to add can be added simply by adding a new property on this object, add dispalyName (for the settings tab) size and the factory function to create the template. This can be a complex lit element, or with the created simpleMetricFactory a general tile, which would allow people with no knowledge of web components to create new metric tile.

Since I removed any dependency on server side formatted data and reworked tile creation logic (that enabled to remove the odd object property filtering that I did not really understand), for these factories all metric is available that is available as payload via the websocket from the server.

What I am trying to say here is that if we want a metric to be available it just need adding to one object with the metadata. Also if a new metric is added on the server side to the broadcasted metrics, it should be available to be added to a new metric tile. Any complex calculation logic can be performed in the factory before passing the value to either simpleMetricFactory or to the webcomponent that creates the component in the view.

This should mean that there is no need to add ternary operators and logic paths to the displayed metrics as one can crate a tile for the remaining interval time, the elapsed interval time as well as the total moving time and select to display those that the user want to see.

[Abasz]

I actually created a factory for the metric tiles that simplifies adding more tiles. I will push these commits shortly. Basically adding a new tile should be as easy as adding a property to an object in code that compiles with a set interface.

For example:

{
  distance: {
    displayName: 'Distance',
    size: 1,
    template: (metrics, showIcon) => {
      const linearDistance = formatDistance(metrics?.totalLinearDistance)

      return simpleMetricFactory(linearDistance.distance, linearDistance.unit, showIcon ? icon_route : '')
    }
  },
 calories: { displayName: 'Calories', size: 1, template: (metrics, showIcon) => simpleMetricFactory(formatNumber(metrics?.totalCalories), 'kcal', showIcon ? icon_fire : '') },
}

Any rendering logic (both in the settings tab and in the dashboard) will be driven by this object and constructed automatically.

So any metric that we would like to add can be added simply by adding a new property on this object, add dispalyName (for the settings tab) size and the factory function to create the template. This can be a complex lit element, or with the created simpleMetricFactory a general tile, which would allow people with no knowledge of web components to create new metric tile.

Since I removed any dependency on server side formatted data and reworked tile creation logic (that enabled to remove the odd object property filtering that I did not really understand), for these factories all metric is available that is available as payload via the websocket from the server.

What I am trying to say here is that if we want a metric to be available it just need adding to one object with the metadata. Also if a new metric is added on the server side to the broadcasted metrics, it should be available to be added to a new metric tile. Any complex calculation logic can be performed in the factory before passing the value to either simpleMetricFactory or to the webcomponent that creates the component in the view.

This should mean that there is no need to add ternary operators and logic paths to the displayed metrics as one can crate a tile for the remaining interval time, the elapsed interval time as well as the total moving time and select to display those that the user want to see.

[Abasz]

All clear.

[JaapvanEkris]

Hi @Abasz,

I've integrated your PR into the V1Beta_Updates branch. I can't get past the testing.

When I install the V1Beta_Updates branch on a Pi and run sudo npm test inside the /opt/openrowingmonitor/app directory, it fails with the same error:

> openrowingmonitor@0.8.2 test
> uvu

SyntaxError: Invalid or unexpected token
    at ESMLoader.moduleStrategy (node:internal/modules/esm/translators:119:18)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:468:14)

I agree that the error message isn't the clearest one, but I can't find any errors (ORM seems to start normally on the Pi). Do you have a clue what is going on?

[Abasz]

Its an import statement issue. There are some incompatibility between the module loader for the server and the client.

I am pretty sure there is an import statement issue in the client folder... Not sure why this was not caught during my dev. it run just fine. If you remove the helper.test.js (or its content) it will work.

I will find the incorrect import statement and correct it and do a PR.

EDIT:

I fund the issue. For some reason uvu cannot handle the lit template import of import '../components/DashboardMetric.js'... I was not aware of this, not sure how it got passed the git hook though... any way there are two options:

1. Factor the simpleMetricFactory (value = '--', unit = '', icon = '') out into a separate file (or into dashboardMetric.js that is not loaded by uvu when running tests
2. We remove the test (it includes one test of the filter method)

I am leaning towards option 1).

[JaapvanEkris]

I'll follow your lead as you know this code best.

[JaapvanEkris]

Its an import statement issue. There are some incompatibility between the module loader for the server and the client.

Thanks for the quick fix! I accepted the PR and I'll indicate to @laberning that V1Beta is such a huge improvement over 0.8.4 that it should become the main line.

[carlito1979]

wow guys! I've been away from the rower for a couple of months with an injury (and general laziness) and I've come back to a huge amount of work that you two have done with open rower! I'm loving the GUI enhancements!
One request from my end - would it be possible to move the options that show on the actions tile to be a part of the settings dialog?
I was using the experimental 12 tile screen on my 7" monitor, and it would be awesome if I didn't have to use a tile for the actions, but I do need them to shut down the Pi after a workout, and also to turn on/turn off the Ant broadcasting.

[JaapvanEkris]

wow guys! I've been away from the rower for a couple of months with an injury (and general laziness) and I've come back to a huge amount of work that you two have done with open rower! I'm loving the GUI enhancements!

Welcome back! I hope you recovered well from your injury.

One practical note: you were aiming to improve the PM5 interface, is that still your ambition?

One request from my end - would it be possible to move the options that show on the actions tile to be a part of the settings dialog? I was using the experimental 12 tile screen on my 7" monitor, and it would be awesome if I didn't have to use a tile for the actions, but I do need them to shut down the Pi after a workout, and also to turn on/turn off the Ant broadcasting.

Yeah, I typically turn on FTMS Rower manually as well, as EXR seems to respond better to it. I would be fine with a tabbed settings interface, it could even allow the buttons stop from cycling through the options and present all options at once.

[carlito1979]

I would like to work on the PM5 interface (I guess reverse engineering the custom C2 CAFE commands) but I haven't had any time to look into it

[JaapvanEkris]

I would like to work on the PM5 interface (I guess reverse engineering the custom C2 CAFE commands) but I haven't had any time to look into it

No problem. I'll remove it from the list from the V1 functionality.

[Abasz]

e if I didn't have to u

Technically this is possible of course. This was left as is from original ORM GUI. But can be changed should this be the request.

I would actually do this differently sightly and move the power down button out from the tile similarly to the settings menu icon along with the full screen and other non peripheral controlling icons.

Eventually a speed dial FAB button (https://m2.material.io/components/buttons-floating-action-button#types-of-transitions) would be the best. But I am not a UI designer so I need to find a ready component to do this :) I have similar in Angular but this project does not uses that unfortunately...