Settings and Actions Toolbar

[DXCanas]

WIP, first code contribution! Trying out an idea I had based on need expressed here:

Functionally: I hate wasting a tile on command buttons I use once per training (at a maximum).

#168 (reply in thread)

Also corrected a quirk in heigh calculation that led to the tiniest bit of scrolling.

Testing TBD, will try it on next workout.

[Abasz]

This is brilliant, and its hilarious why I have not thought of this when I worked on the tiles years ago :D We could even hide it dynamically like windows task bar (though this will require some gesture support for touchscreens or we just show it on any click anywhere that also works) on smaller screens.

[JaapvanEkris]

Great work! What a space saver! I'll test this evening and accept the request when no issues are found

[Abasz]

Can it be that something is wrong with the slot tracker:

I can see that there is no option for action yet its here. I am suspecting we will need to clear (preferably update) local storage where these settings are saved. So we should run a simple migration I suppose where we swap out the actions to something else.

[DXCanas]

I think new defaults are needed. And the local storage carries over. I could have sworn I removed it from the list, and I still haven’t removed the component from the code base 😅

Tested it myself during a workout today and I did notice some quirks. But overall seemed mostly functional! 12x on the tablet was glorious.

[Abasz]

You removed it from the defaults but since localStorage has it it will happily load it back:

https://github.com/DXCanas/openrowingmonitor/blob/6bde3fa16425c2c3e07dab39a7069685a214cc79/app/client/components/SettingsDialog.js#L160C4-L160C62

Here basically we just get whatever is in the store.

https://github.com/DXCanas/openrowingmonitor/blob/6bde3fa16425c2c3e07dab39a7069685a214cc79/app/client/index.js#L29

here we load it from the store.

    const config = this._appState.config.guiConfigs
    Object.keys(config).forEach(key => {
      config[key] = JSON.parse(localStorage.getItem(key)) ?? config[key]
    })

We need a settings migration here, namely that if config has actions we need to override it and write this new config back to local storage.

[DXCanas]

Heads up that I am also unhappy with the state of the CSS here; it looks ok but it's not very understandable atm. So I will probably be tinkering and testing that along with the state reset ^

[JaapvanEkris]

Before I forget: One thing to look at is the update behaviour of the force curve. We only send new data on the flag isRecoveryStart is true (all subsequent updates repeat the same curve), but it keeps drawing CPU cycles when displayed. Perhaps it is redrawing too often?

[DXCanas]

Alright y'all — sorry, I got a little carried away with the CSS. If the changes are out of scope, will happily pull into another PR.

Tried to make the CSS more self explanatory, and addressed an iOS bug I ran into while working on something else.

It's stricter/more defined now, and no longer has what I believe was unintentional overflow/scrolling.

So. Here's the before (Saw this while testing #171 and prior):

And here it is now (only tested locally):

[DXCanas]

Before I forget: One thing to look at is the update behaviour of the force curve. We only send new data on the flag isRecoveryStart is true (all subsequent updates repeat the same curve), but it keeps drawing CPU cycles when displayed. Perhaps it is redrawing too often?

Relying heavily on the AI for this, but the solution makes sense to me!

[JaapvanEkris]

We need a settings migration here, namely that if config has actions we need to override it and write this new config back to local storage.

Or we need to clear the browser cache on update.

There are some PR's regarding the cache and the browser anyway (if the user isn't named 'pi' there are install issues, and we might switch browser to something lighter as on the Zero2W there are memory issues), so clearing it will kill any CSS and caching issues?

[Abasz]

Or we need to clear the browser cache on update.

I think that is a bit heavy. I like the current migration. I think it is sufficient.

We only send new data on the flag isRecoveryStart is true (all subsequent updates repeat the same curve),

So this is one of the things that proper data layer would solve. When I was experimenting with rxjs it has distintcUntilChanged which is a breez (so I was checking data on each tile for changes and only update/render).

Actually you should not worry about this for now, as I will review the updates to the lit framework and rework this data layer which will improve change detection (and trigger less rendering). I did more reading and it is going to be gradual but still robust. It will remove this passing down global state and make it more focused, that will reduce re-rendering.

The issue currently is that we use the global state which is fully replaced on every new incoming data packet (250ms as per default gui update interval I think), hence triggering all components update. I think moving to update() will not help but you can check that with a logging statement (i.e. how many times the update() method runs).

[JaapvanEkris]

A practical remark/question: currently we have lit pinned to 2.8, as upgrading to 3.x will break the FE. If you want a higher version, just drop a line. If we can uograde to 3.x pkease also tell as it might unlock a bunch of dependent package updates.

[JaapvanEkris]

Or we need to clear the browser cache on update.

I think that is a bit heavy. I like the current migration. I think it is sufficient.

You realize we might change browser or cach store completely due to the outstanding issues (thus clearing any cache anyway)?

[Abasz]

Or we need to clear the browser cache on update.

I think that is a bit heavy. I like the current migration. I think it is sufficient.

You realize we might change browser or cach store completely due to the outstanding issues (thus clearing any cache anyway)?

Yes that is true and essentially it will result in the same effect for those that use a screen with the Pi. But we need to keep it mind that

1. this migration code will remain there for later and potentially be extended with other migration
2. what if someone would already use the GUI on a phone and access it via wifi (i.e. not locally), or from another devices. For them full clear is total overkill.

[JaapvanEkris]

2. what if someone would already use the GUI on a phone and access it via wifi (i.e. not locally), or from another devices. For them full clear is total overkill.

Forgot about that one. Thanks!

[DXCanas]

Alright, given how much back and forth is going on after trying to address this:

One thing to look at is the update behaviour of the force curve. We only send new data on the flag isRecoveryStart is true (all subsequent updates repeat the same curve), but it keeps drawing CPU cycles when displayed. Perhaps it is redrawing too often?
#170 (comment)

I think we should just tighten the scope here and pull out all of that code for another PR. Seems like there are lots of things that need to be validated or confirmed, including what constitutes "fighting the framework" and benchmarking performance.

[DXCanas]

Side note, just to note while I'm thinking about it: the toolbar has the added benefit of giving us that blank space in the center for some more interface; I was thinking maybe segment information for workouts? "Just Row" by default? We'll cross that bridge when we get there. But I was reflecting while testing another PR without the UI updates.

[JaapvanEkris]

I guess I'm just thinking of the use case — peak force is a neat bit of information; coupled with the wattage, it answers the question "am I going as hard as I'm supposed to be?".

It is a bit more subtle than that. In essence, the force curve represents force over distance (the X axis is actually the drive length). The "volume under the curve" is the work done (essentially Joules). If you divide it by the stroke time, you get the power. So there are some parameters in between.

Looking at the force curve, a high peak in the force curve is actually quite inefficient. Most efficient is a flatter curve where the force peak is close to the average.

But what I'm (personally) looking at when using the force curve is the shape of it; whether or not it's "virtuous". I don't need to see every data point necessarily, just that the peak is closer to the left half than the right half.

Based on your experience, would you say that's the typical case? Or am I making assumptions?

It isn't a bad assumption. When you look at RP3's force curve they actually display that alongside the peak force: the percentage where the peak is. The position of the peak is actually relevant for the type of boat.

Because if the shape is the priority, I personally would rather have as much vertical room as possible to visually gauge where (along the horizontal access) the peak is. The less vertical room it has, the less differentiated the peak is from the rest of the graph.

Pretty good point. Perhaps remove the labels completely?

[JaapvanEkris]

Side note, just to note while I'm thinking about it: the toolbar has the added benefit of giving us that blank space in the center for some more interface; I was thinking maybe segment information for workouts? "Just Row" by default? We'll cross that bridge when we get there. But I was reflecting while testing another PR without the UI updates.

I think that keeping a lot of whitespace is more beneficial: people move up and down the slide, and that makes text pretty hard to read. On the standard setup (integrated 7" display) most text aside the large metrics isn't too readable. An extra icon that opens the workout schedule would be usefull, but we borrowed a trick from Concept2: metrics that are set as a (time/distance/calories) target in the interval will start at their target and count down. This has the huge benefit that this is displayed in a large font, provides a clear indication that the workout is loaded and active (this is something that went wrong quite often when not doing that), and people are mentally more inclined to count down to a finish anyways.

[DXCanas]

Looking at the force curve, a high peak in the force curve is actually quite inefficient. Most efficient is a flatter curve where the force peak is close to the average.

Oh, interesting. I never really understood the numbers here, but this makes sense. I would just use the peak "relatively"; is it higher or lower than the last time I did something like this workout?

Perhaps remove the labels completely?

sure 🤷 I lean toward keeping it in some way just because removal would be feature removal which… is a law my workplaces historically don't like breaking lol.

Anyway — I'll go ahead and remove in an individual commit. Can be reverted if y'all change your mind down the line.

I did remove it, and brought it back as evidenced in the commits. It will dissapear once data starts getting populated into the chart.

I thought it might be confusing to sit down at your rower and just see a large blank rectangle after update. So it will show at start, and dissapear as the data starts getting populated.

Another tangent:

the percentage where the peak is.

Should this also be theoretically be possible? ${((index of max)/(array.length)) * 100}%? Am I oversimplifying? haha

[DXCanas]

Heads up that this should be ready for testing again @JaapvanEkris — Adding some uniform padding to the tiles (so that the chart isn't right up against the bottom), made the appearance of the title dyamic (so that it's only visible when data isn't; mostly a UX thought for when someone hasn't started rowing and loads up the GUI), and kept the expanded height of the chart.

The layout of the titles, and the 2-tile length of the chart actually sets an uninstentional little "halfway" marker right above (or below) the chart, making it easier to identify what "side" of the force curve you're on. Personally found this really nice!

Also found a bug during my own test session having to do with the battery icon (removed a seemingly useless line of styles earlier). That's fixed now too.

[JaapvanEkris]

I'll start testing tomorrow (have to complete some tests for the CyclicErrorCorrection filter)

[Abasz]

Sorry for being late to this party FY closing is very busy.

I have been also experimenting with changing (removing/swapping) the title on the force chart as well and I think:

1. I would remove the "Force Curve" as title as it is obvious and does not add but takes space.
2. I would make the displaying of the Peak in the title optional and if disabled I would show the peak on the force curve (but without title)
3. I would align the Peek title to the left (or the right) so it is less likely in the way (because force values tend to be smaller on the start of the stroke it is less likely to be overlapped).

Alternatively we can have the option to completely remove the peak but that cannot be done easily with a checkbox (drop down maybe with options: off, title, chart?)

What do you think? I will actually do the same for ESPRM GUI (I realised that on my phone I dont see the peak too small, jumping is irritating indeed. but sometimes I am interested about this)

[DXCanas]

So right now the behaviour is that there's a title at the very start, before any data is picked up. Just for clarity on the layout before a workout is begun.

Once data starts coming in, title goes away.

I like the idea of including peak, but maybe placing it somewhere consistent, and other than where title was. Settings would be a bit more work, and we'd have to think through where that config would live. I'm inclined towards a sane fixed setting (top right aligned?), but open to the idea of config if we can settle on the design.

But if there are no objections with the current behaviour, can we save any big changes for a followup PR? To be worked on/discussed after #171 and #176?

[DXCanas]

So here's the current state:

What it looks like before start:

After:

---

Threw together right alignment styles:

If you guys like it, I can commit and push.

[DXCanas]

Thinking about use cases a bit more, I removed the Y axis scale. The numbers are too small to be useful during regular rowing, I think, and having the graph span across the entire length, both vertical and horizontal, makes it more useful I think? Can see what side it leans more towards, etc.

Also, decided to just push the right alignment styles. Can remove the commit if we don't like it.

So this is the current state, with latest commit:

The chart does go to the end (except for padding); it's just that the recording replay doesn't consistently go all the way across 🙃

[JaapvanEkris]

Guys,

Ket's not go this route. It is quite cramped and on a decent force curve this will overlap. Why not remove the peak force from the graph and give it its own tile. That is most consistent from a GUI perspective, and prevents overlap. I would keep the axis.

As a side note: the settings for this machine need some attention: 4kw power 3kN force and a spikey force curve suggest something is way off.

Jaap

[DXCanas]

Agreed! Will revert to dd47014 tomorrow

Curves are not from a real machine. Just commented out the replay line in server.js 🤷

[Abasz]

Ket's not go this route. It is quite cramped and on a decent force curve this will overlap. Why not remove the peak force from the graph and give it its own tile. That is most consistent from a GUI perspective, and prevents overlap. I would keep the axis.

I think this is just a font size matter and alignment. I mean we can have the Peak: and the number on two lines right aligned and reduce the font size as much as it does not interfere.

Why not remove the peak force from the graph and give it its own tile.

I mean we can have a peak tile but that will take a space of a tile. I think this is a decision of the user whether it accepts a slightly smaller peak font (that does not overlap) or sacrifice as full tile for something that is readable.

Axis

I would also keep the axis. For instance on kayak sometimes one applies greater force on one side than the other. And the fact that the axis re-scales is visible. Yes peek would help here (substitute to some extent) too as that would increase and decrease but if someone uses it to improve technique it helps. I mean this is again something that may be controlled from the settings if you want to give the option.

So right now the behaviour is that there's a title at the very start, before any data is picked up. Just for clarity on the layout before a workout is begun.

Once data starts coming in, title goes away.

Yes. I agree with this. I was planning to the same in ESPRMGUI

But if there are no objections with the current behaviour, can we save any big changes for a followup PR?

Yes sure, I think implementing settings and stuff (if this is the way) is the next step PR.

[JaapvanEkris]

Ket's not go this route. It is quite cramped and on a decent force curve this will overlap. Why not remove the peak force from the graph and give it its own tile. That is most consistent from a GUI perspective, and prevents overlap. I would keep the axis.

I think this is just a font size matter and alignment. I mean we can have the Peak: and the number on two lines right aligned and reduce the font size as much as it does not interfere.

The key issue is that on a really efficient stroke, you have a significant flat area on top of the curve. And if you want text to be readable on a 7" screen while people are moving up andd down the slide, it is pretty hard to read if the font becomes smaller. It gets in the way of that curve and distracts.

There are more key indicators here that are relevant. RP3 and EXR show the drive length on the X-axis. Physically correct, but for our current GUI not ideal. So it is an additional tile. I think when you want to really do a nice job, you'd make the force curve even bigger, put the peak force on the y-axis and the drive distance on the x-axis, and show what the peak is in percentage on the x-axis as well. But I think that should be part of a fundamental rethink how the GUI is designed, as you want a more integrated (but more flexible) approach.

As @DXCanas rightfully indicated, it is about the shape itself, so it should focus on that. Making that as easily readible as possible is key. Adding large readible text in the same tile would go against that. So removing large texts that block that hopefully smooth curve is a good step in my book. I guess perhaps a slightly smaller font on the axis might also be appropriate.

Why not remove the peak force from the graph and give it its own tile.

I mean we can have a peak tile but that will take a space of a tile. I think this is a decision of the user whether it accepts a slightly smaller peak font (that does not overlap) or sacrifice as full tile for something that is readable.

We just saved them a tile as well by this PR. In essence, if people are keen to see it, they can do now for "free".

Key thing here is being able to read it when returning to the catch. The old solution was interesting but never good to read (in all honesty, as my PM5 still is connected as well, I read that data via the PM5-ErgData interface). On ErgData it has its own tile. I think having it on a place where people can see it without obstruction from other elements (a curve intersecting) would make it a cleaner interface. I actually would introduce two tiles: Peak Force and Average Force, as their combination essentially determines the stroke efficiency (the closer they are together, the better). If people really are focussed on stroke efficiency I would put the force curve the Peak and Average force tiles next to each other. Especially on a larger tablet with 12 rows, that would work well.

If you look at more elaborate setups (i.e. using EXR) pace, distance, time, strokerate, force curve, drive distance etc.. is all in the HUD, essentially offloading them from the monitor. This would free up the monitor's tiles to show the more detailed metrics. So I would focus more on that scenario.

[DXCanas]

Alright: reverted, as promised!

So we are at the no-peak + no-title while rowing setup, with Y axis scale visible.

As far as the new tile, etc, I am thinking that's a followup PR :) I'd like to clear out my current PR queue before taking it on, if y'all don't mind! Also probably best to get a new conversation thread going; this one is rather long haha.

[JaapvanEkris]

Alright: reverted, as promised!

So we are at the no-peak + no-title while rowing setup, with Y axis scale visible.

As far as the new tile, etc, I am thinking that's a followup PR :) I'd like to clear out my current PR queue before taking it on, if y'all don't mind! Also probably best to get a new conversation thread going; this one is rather long haha.

No problem. I'll test tomorrow. I want to make some progress as well. I have to reinstall my Pi, as I FUBARed my test machine (some issue with HCI socket not being found, which I hope is not OS or hardware related...).

[Abasz]

some issue with HCI socket not being found, which I hope is not OS or hardware related...).

i had the same issue!!! rfkill for some reasons random disabled Bluetooth.

You need to enable Bluetooth if disabled.

[JaapvanEkris]

some issue with HCI socket not being found, which I hope is not OS or hardware related...).

i had the same issue!!! rfkill for some reasons random disabled Bluetooth.

You need to enable Bluetooth if disabled.

Interesting. Hope this helps in my case (saves me setting up a new test environment).