This update handles stopping and pausing ORM

This update handles stops and pauses from ORM by cutting off the RowingEngine.js from the currentDt-stream. As RowingEngine.js needs to work in absolutes, a free spinning flywheel (that could go on for minutes) wouldn't stop updates to time and distance, even when an end-criterium is reached or the rower has stopped. This change makes stops and pauses more clean.

To generate clean breaks when a certain end-criterium is reached, RowingStatistics.js will check the end-condition for processed impulse. End goal here is to be able to set these end-criteria in the GUI at a later stage. A future improvement would be to use these targets in RowingStatistics as well to let the displayed time or distance count down in the display during rowing (similar to the PM5 does).

Ideally, there will be only one rowing state, and the movementAllowed bit wouldn't be needed. I'm not certain that it would work without this semaphore on the highest level (as I fear there would be timing issues somewhere in stopping a session).

Please feel free to rename areWeThereYet to sindWirSchonDa :)

I'm a fan of releasing Minimum Viable Features (MVF) early, however those should not conflict with the overall quality of a product. The proposed way of implementing an initial version of workout criteria in this PR comes with quite a few pitfalls and side-effects.

Defining and managing workouts is something that will come to Open Rowing Monitor at some point. But to do so I think that we need a solid architecture for managing and observing the training state. If we spread the logic throughout the code base, then this will be very difficult to maintain and test in the future. Also all end user interactions during the rowing session should not require the user to change something in the core code base of the application.

Some aspects that I would like to tackle, with a training session feature are (not saying that all this will be implemented, but the chosen approach should be extendable to all these in the future):

Training sessions with multiple intervals based on Distance, Number of Strokes and Time
Setting goals for each Interval (i.e certain SPM, Watt, Pace or Heart Frequency)
Saving and Loading Training Session Presets
The BLE FTMS protocol has a "Fitness Machine Control Point" which allows setting and getting the training goals with other apps, might be interesting to integrate those
- Set Target Speed
- Set Target Power
- Set Target Heart Rate
- Set Targeted Expended Energy
- Set Targeted Cadence (SPM)
- Set Targeted Distance
- Set Targeted Training Time)
- Start, Stop, Reset Sessions
The BLE PM5 has a similar functionality
The exported TCX files can contain Workouts with different Laps and Activities, exported Training sessions should make use of this so the training intervals are properly recorded

Hi Lars,

I am a big fan of maintaining architecture as well, but I can't see how this update conflicts with the code quality. I don't think this code has side effects at all, and it is a step toward more functionality, but this is a step I can't make without your help from a GUI perspective. I rather have it GUI settable as well, but as I'm not familiar with java GUI work, I think that would be left for people who do know what they are doing. So all I can provide is a hook for functionality.

I think this fits within the architecture quite well, but I also think that we shouldn't be afraid to refactor code when needed. Getting things to work and frequnelty see where the architecture contains odd constructions should be a common approach (just like I did with the RowingEngine).

I think that being able to set a single interval with a target time or distance is already a huge improvement with respect to the current functionality and many monitors. It is in fact needed for testing, as validation with the PM5 can only be done based on identical end-criteria, thus there is a strong need to be able to nicely stop an interval and record all metrics. To me, it currently has the same status as the replay functionality, but it would become much more usefull if you could make it user accessible.

Checking an end-condition can only be done in two points in the architecture: rowingEngine.js and RowingStatistics.js. When this is moved to server.js the granularity is too big (RowingStatistics is updated every impulse, server.js isn't), which results in significant deviations (over 5 meters). And you also introduce very bad effects (as metrics.sessionstate will say "rowing" while you alrady have concluded you are finished) which can only be fixed by violating the architecture (i.e. server.js rewriting metrics.sessionstate) to have the PM5 interface signal the right things to EXR.

To me, the only logical place is RowingStatistics, as it is more involved in session management and several criteria anyway. It is already calculating many metrics, and it could well abstract away from the absolute state of the rower maintained in RowingEngine.

As I see it, aside the practical perspectives, moving such decission to server.js would result in a too monolithic architecture where server.js is too seeply involved into too detailed decissions. It is better to server.js orchestrate all streams and only coordinate between the processes when the indicate they see a need, more managing the session and the intervals than the detailed determination wether an end-condition has been reached. That would provide a clear seperation of concerns.

As I see it, RowingStatistics should report that an interval end-criterium has been reached, and Server.js should than decide wether it starts a new interval directly (give RowingStatistics new orders and keep processing currentDt's), stop for a pause (stop processing currentDt's) or stop completely. That is much cleaner role, where also user input from the GUI could easily fit in. Here server.js could also orchestrate with the workoutrecorder to record another lap or finish the recording.

I'm the first to admit that movementAllowed is ugly and shouldn't be in server.js. However, this semaphore is needed at some level as the flywheel keeps generating currentDt's, so somehow either the GPIO or RowingNegine must start to ignore these signals. Otherwise the metrics keep moving minutes after the rower has stopped moving, as the airrowers typically have a three-minute spindown. My initial proposal a while back in a PR was to extend RowingEngine with this semaphore (this actually is how my own machine still works), but you removed this from the PR despite me explicitly stating this goal. For me, this was the only option left. But I'm more than happy to implement my current solution into this PR, as I think this would make server.js more an orchestrator (for a pause or full stop, server.js tells rowingengine.js to ignore currentDt's, and server.js can also activate it again). This would allow server.js to orchestrate, without any knowledge of the streams.

This could be a good basis for session plans, but also keeps server.js from becoming too heavy. I think the things you mention would typically handled by server.js, telling rowingstatistics.js to guard that criterium for the current interval. As rowingStatistics escalates back to server.js when the interval is complete, that would be the moment where new orders would be set by server.js. That way, rowingStatistics is only dealing with maintaining the current interval state and the accompanying metrics based on raw data from RowingEngine.js., and server.js will only have to act upon escalation. This would be a nice clean seperation of concerns that is in fact found in most industries (see for example ISA-95 for industrial processing, which is my professional background).

I think it is wise to distinguish targets and constraints. The PM5 can only handle three types of targets: time, distance and calories. ErgZone also does the same thing. I must say, the PM5-BLE interface is quite sloppy, and bad things happen when you change a split to an different value (i.e. a sprint at the end of steady state rowing). I spontainously had 2.5 sec pauses (time to the next drive) between splits. We can and must do better here, and the RowingStatistics abstracting away from the raw RowingEngine data would allow seamsless transitions similar to EXR's approach.

I hope this provides you with a clear picture where I see this fit within the overall architecture. Please let me know if you want to adjust the implementation of movementAllowed to my current impleemtation, and I am always happy to discuss the architecture with you in a Skype call.

Hi Jaap,

thanks for sharing your thoughts. I'm sorry, but I won't add a hacky solution for session management like this to the main codebase. I agree that replayRowingSession is also lacking in that way, but thats already on the list for extraction.

As I said I haven't made up my mind on how I want to address sessions in the future (there are still some other features that I want to sort out earlier). But managing the session will eventually be a separate module and will require a suite of solid test cases to get and keep it working as expected. This feature will also somehow change the role of the RowingStatistics module.

The issue with rowers with long spindown times is an interesting one and yes that could be handled better. We sure can discuss this further. My first thought on this is that the impulse stream should not be cut off before the RowingEngine, as otherwise there would be no way to detect when the rowing continues. Maybe we can define a maximumRecoveryTime (or maximumStrokeTime) and then stop counting the totals until we see the next rising flank? Anyway, lets discuss this at another place, I'll close this PR for now...

Hacky solution? I think the RowingStatistics escalating nicely that it reached an end criterium is quite in line with what the code should do. The semaphore is needed as you need to signal the rest of ORM that the rower intens to stop.

The issue is that not signalling RowingEngine will keep all metrics going and poisons dragcalculation, which causes havoc when restarting a session. That is why I proposed to make movementAllowed part of the RowingSession as I proposed in the earlier PR. The initial implemantation is quite blunt, but the key is that specific paameters aren't updated, but those are details that should be added when we need a pause.

replayRowingSession is quite useful, as I use it to test new algorithms, like stroke detection, and the combined distance/drag calculation. So it is a bit shocking to see it go.

Not including this makes validation and keeping the codebases in sync extremely complex. Your updates already broke the possibility to do a fetch request on my (newly created) C2 validation branch that I used to copy my improvements. With an average lead-time of often months for a PR I have numerous changes in my local copy which by your request are fed to the main base one-by-one, but copying them to the main is a nightmare as I have several interwoven improvements in my code, along with testcode. Rejecting this, which cost me considerable time to copy on the main branch, makes the complexity even bigger. To be honest, this makes contributing to this project quite frustrating. Iti s your project, so you should do what you see fit, but I'm losing my enthousiasm here quickly.

Open Rowing Monitor is a spare time project that I started to improve the performance of my rowing machine and to experiment with some concepts and technologies that I find interesting.

Still, I intend to keep the code base clean and maintainable by following some standards. So before working on new features, first discuss the intended changes so you don't spend time on something that does not get accepted. This actually is a common practice for Open Source projects and so far I just did not spend the time to write a Contribution guide. I just did that now, so maybe that will make it a bit more clear: Contribution Guidelines.

I'm sorry that you find contributing to this project frustrating, but I hope that you understand that Open Source also requires some rules to work.

Don't worry, replayRowingSession is not getting deleted. Having everything in Open Rowing Monitor depend on the raw measured dt's of impulses was probably one of the best design decisions with this project, so I'm not throwing that away. With 'extraction' I mean that this functionality will go to some place where it will be more practical to work with it.

Having local feature branches diverging heavily from the upstream and then using copy and paste to get stuff in sync really sounds like a terrible idea - you will easily miss something if things get more complicated. It is much safer to use git's capabilities to do so (rebasing, merging, maybe cherry picking). If all those don't work then that might be a sign, that the new feature is not abstracted properly. Not really sure what you are working on, but it's probably your improved physics model based on torque? The interface for that should be quite slim (just a handful of calls and events going in and out of RowingEngineTorque or however it is called). Let me know if you want support here - just maybe not in this PR...

Having local feature branches diverging heavily from the upstream and then using copy and paste to get stuff in sync really sounds like a terrible idea - you will easily miss something if things get more complicated. It is much safer to use git's capabilities to do so (rebasing, merging, maybe cherry picking).

That is what you get when PR's aren't processed swiftly, and thus the current PR's I sent you are based on code that I've implemented about nine months ago. Renaming functions and variables in the main branch at random didn't help there either, so I'm stuck in version 0.7.2 with a lot of improvements and a very painful process to get them to the main line.

A lot of improvements are intertwined: they are build on top of each other as the I was waiting for RowingEngine 2.0 to be accepted. They are based on work of the university of Ulm, the BT documentation and some notes of Prof. van Holst and Nomath. I know how the PM5 does its math internally, so the new model copies it when usefull, but takes a different approach where they go wrong. Even powercurves work, although the output isn't verified yet. Things like session handling, interval handling etc.. are done sloppy by Concept2's implementation, while my solution has been proven to be seamless for months: RowingStatistics can start a new interval without disturbing any metric. But that implementation requires the architecture I developed based on 0.7.2 in the summer while waiting for the RowingEngine to be accepted as PR. The implementation I'm feeding you in small bits, per your request. In all honesty, the RowingEngine works as I need to, and much better than the current RowingEngine, being consistently within 1% of the PM5's results, but i don't see an easy way to integrate it into the current version of ORM.

Contributing to a project where people work together requires rules, but also clarity what the intended purpose is of specific components, and some vision where components should be going. And some flexibility to accept deviations from a plan and see where that road takes you: a project is organic and it is build with code, not concrete, and the complexity isn't that bad that code can't be refactored. Discussing the architecture is needed in projects, where all viewpoints need to be heard and thoroughly discussed, especially the ones the lead architect doesn't agree with (at least, that is what my development teams tell me in the retrospects).

The way you phrase this is not very constructive. Basically you are saying that you have a much better implementation of a rowing physics engine lying around somewhere and you want to contribute it to the project (which great btw.). But you don't know how because your code base has diverged from the main development branch too far.

As you say a software project is organic and so it is not uncommon that an unmentioned huge feature branch lying around for a long time will eventually diverge.

I already offered you a few times to assist you with sorting this out. But without actually knowing what code base we are talking about and why this physics model has so different dependencies that it can not be integrated with the current code base it is hard to make any progress here. There have only been very few changes to the current physics model in the past few months, so I currently don't think that there should be any difficulties that are impossible to overcome.

This whole ping pong discussion about how to integrate your physics model does not seem to go anywhere and is in my eyes also unrelated to this PR about a workout session. So I stop writing in this thread here.

I'm still open for discussions about how to integrate the model, but please in a constructive manner.

laberning / openrowingmonitor

This update handles stopping and pausing ORM #60