search

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

Errors due to incomplete rowingProfiles #103

Closed Abasz closed 1 year ago

Abasz commented 1 year ago

I noticed with the v1beta that if a rowing profile is not fully implemented (i.e. one or two settings are missing) ORM does not fall back to the default. I think this is an undesirable and misleading and could create issue. For instance the Generic_Air_Rower profile (the one I use and created) does not have all the rower profile settings (e.g. maximumTimeBetweenImpulses, or minumumRecoverySlope), because the DEFAULT profile values work fine for those. Now ORM did not work correctly as these values became undefined making the ORM unable to detect drive, recovery or other state.

For instance old profiles (e.g. DKN_R320) implement only 3 changes to the default.

JaapvanEkris commented 1 year ago

Hi Abasz, I'm away from my machine for a couple of days, so this isn't as thorough as you'd expect.

When I look at https://github.com/laberning/openrowingmonitor/blob/v1beta/app/tools/ConfigManager.js, it hasn't changed over a year. So I assume the error isn't in that code. Are you certain that your config.js and rowerprofile.js are valid? I noticed that when one comma or bracket is missing or surplus, everything goes sour without any indication. I looked at the current V1Beta configs, and I haven't spotted any obvious errors. But I haven't checked the validity of the config.

Could you run 'sudo npm test' in the app directory? This triggers the automated test locally and my guess is that the DKN R-320 Air Rower should have similar issues then (where the V1Beta this test is passed succesfully).

Abasz commented 1 year ago

Ok, this is my fault when I was checking in and out different branches and testing deleted the config.js and instead of recreating it changed the rowerProfile in the default.settings.js so the deepMerge in the manager did not work... It was strange that it worked before any way closed this.

JaapvanEkris commented 1 year ago

I guess we still have some things to do with error detection and reporting for V1Beta.

JaapvanEkris commented 1 year ago

I'll add some code to verify the validity of the settings at startup, and log when things are out of bounds. As users also can mess with every setting, some checks might be handy and impove the experience. This can also create an upgrade path from V0.8.x to V1 and beyond for the config file.