Replies: 16 comments 72 replies
-
You hit a good vibe here, I guess that would be a nice clean approach.
Here a former C/C++ programmer speaking :)
In all honesty, I think some responsibilities are placed wrongly. For example: RowingStatistics is aware of the update interval of both BLE and web interfaces and tailors the communication for it. Why not refresh the metrics on their side and let the endpoints decide they should act on it (and how). Like you indicate: all these user actions in Server.js are only a complicated way of telling RowingStatistics to stop or start (which RowingStatistics should tell the rest of the world). Some twists are odd (and created by me to fit the excisting pattern, I admit): RowingStatistics concludes the interval ends, tells server.js, server.js tells RowingStatistics the session is complete so it can stop, after which RowingStatistics will tell server.js it has stopped. IMHO, this should be handled internally by RowingStatistics. Server.js should tell RowingStatistics of user input, but not act upon it itself.
That is one way of looking at it. For RowingStatistics down I use a different pattern: I get a currentDt, send it downwards and I inspect the result. When all recipients know when they want to update (some update type has to be passed along, as the PM5 requires a stroke start event), stuff becomes more easy I think. So you have a GPIO input and the reciepients should be notified of a metric and possibly a resultingstrokestatus/sessionstatus update. Similar for an HR metrics update: HR metric gets sent, pheriperals should decide wether to act upon it. When BLE or UI get user input, only RowingStatistics can do something with it. So after that processing, the recipients should be notified. Currently, server.js basically processes the UI input, by telling RowingStatistics to do something.
I agree. But there are four: RowingStatistics is in itself just another event recipient. When recieving a 'stopped' event, it can do its things (instead of being told to write files by server.js).
I think Server.js should do that. Although main.js sounds comfortably familiar :), I don't know if node.js expects a server.js.
Its been a while since I used MVC pattern. But a small step towards it would be to move responsibilities to places where they should reside: update UI and peripherals when updates are available and make it more event-driven.
Yeah, I could look at RowingStatistics as well, as all chaos originates there...
I agree here, and GUI and BLE should/could fire identical events.
I agree, but you have several states changed (on stroke, interval and session level), but those are minor derails to be worked out. |
Beta Was this translation helpful? Give feedback.
-
What I meant by the above is that you mentioned somewhere that for instance PM5 protocol requires specific message to be sent when the stroke state changes (e.g. when the drive phase is completed, or the recovery started). So the app needs to know when that change happen so it can trigger the right message. Based on the proposed solution, every time something changes (regardless if its a metric or a state) an event is fired, but in this case the client needs to track what has changed so it can act filter the state change. So basically its easier to have a separate state change event that is broadcasts when drive is finished, so the client know this is the moment, it can reach out to the data store in that right moment to get the most current data (or metrics can be passed with the state change event). Me personally like the first option (client reaching out to data) rather but it would depend whether a client would like to act on specific events (e.g. on state changes, or every data change) or would like to do something based on their own interval. I trust the above clarifies what I meant.
We need to separate the BLE and the ANT+ as those can work in a slightly different manner. BLE: it can broadcast based on an interval (like broadcast every second) or any data change (like what ever changes immediately fire a broadcast with new event), or on specific datachange (like when a new rotation is detected, or a the stroke finished). Currently it does broadcast every second, plus when there is a state change of the rowing. ANT+: it can only do interval as it has a very tight scheduling to which it needs to stick. Based on the above there are two options: we allow the store to be queried by the clients on their own terms (like getData() function), or we can limit them to only get data when the store allows. My proposal was to have it both: broadcast data on the data change, but still allow clients to query for the most up-to-date data in the store. This would liberate any client to keep local copy and would help with the single source of through (and make less prone to bugs). JS and node is actually syncornous under the hood (its event loop anyway) so it is not really a problem that data get corrupted (its not like when you do an interrupt in an embedded system where if you read and write at the same time a multi byte variable its content can get corrupted). So there is no risk that while we are reading the data from the store it gets changed half-way through, since the read request will finish before the new data is written.
Not necessarily, as I said its a design descision. On my ESP32 MCU implementation I tried both (i.e. sending update on every wheel rotation, on the completion of a stroke and periodically). The only difference is how spiky the data, the more you broadcast the less smooth the pace is (at least on the CPS and CSC, FTMS is not affected by frequency of course as it broadcast its own speed/pace calculation).
Or the third, we can have both option and the Client can decide which to use :) Don't get me wrong I think choosing either can work too, I just dont see the issue of making both available. Nevertheless, if we need to choose I would go for option 2. only broadcast the event and let the client decide if it wants to query the store. The reason is simple:
I agree in a sense that this has more steps, as once the event is received the client needs to query the store. But this is the main reason that I would add both options. I.e. a dataChange event includes all data, stateChange event includes all data that is valid for that specific moment. If any client would like to stuff on their own terms, let them.
I see your point, but time stamp and tracking with such is not the answer. It ads significant complexity, while the things that could happen is fairly linear. Things always happen in the same sequence tied to the rotation
You are right about that, have not thought about the paused timer. Still comparing timestamps all the time is a pain and will carry a lot of issues, while I am not sure what they really solve.
My intention was never to move these logics away. My point is that these information (i.e. the new state along with the metrics data etc.) get aggregated in the store. When it is set by the function in the store, the store know that moment and can broadcast notification. So for instance when the engine detects that the stroke ended in addition to what it does now, it calls an update method on the store that is somewhat specific (like state, or pure metric data, or action) and the store while saves the new data into the store, emits on the specific channel "that guys, I have new data, or new state"
I think you are trying to couple the engine too tight to these features. For instance, a smart watch can do interval training. It can record laps, splits, what ever. It does this on its own terms, It receives data frequently but runs its own timer (independent from the machine), triggers laps etc. I dont understand why its necessary to tie the interval and session to the physical machine. (a side note: this is one thing I dont like about the real PM5, that if it detects a bit of a motion its starts its timer, and clutters my workout session data with moves like me getting the handle, sitting down, setting the damper but not really doing workout etc.)
I am not sure how much data degradation this would cause. I mean from a consumer perspective its probably total irrelevant if you have a data that is one impulse delayed (if it actually even delayed). but may be there are use cases. |
Beta Was this translation helpful? Give feedback.
-
Yes indeed. The only question is whether that is a single message or two messages (with or without a request by the feature). The PM5 interface seems to broadcast every second and at the start of the stroke. Currently these are decoupled, but ideally we would reset the timer upon the last message (thus the start of the stroke message will be followed by an metricsupdate after x ms).
I like simple and single mechanisms. Providing multiple ways of doing the same thing typically makes life more complex and confusing. I rather choose a good design pattern and stick with it. I think making RowingStatistics the sole datarepository and let the features decide whether to request data based on time or state is the cleanest approach: the features themselves are clearly responsible to get the data they want and RowingStatistics only delivers upon request. So indeed you get a state channel (where a metricsupdate wouldn't trigger it, as that is ongoing business that is actually irrelevant for the features as when they are timebased they will ask based on their timer) where a settingschange might be a cause for requesting update as well. Given that the features can determine the timing themselves, this could clean up the code and logic quite a bit.
Sounds good. Let's assume we won't run into issues until somebody sends us bugreports :)
As long as time and stroke/interval/session state are the only triggers, I think our approach should work.
As said above: doing both typically adds complexity and you inherit the bad habits of both approaches. I think adding specific status broadcasts and let the feature based on a timer or such status trigger a datarequest is by far the easiest approach.
I would go for option two: less code is easier to understand and a consistent approach is easier to follow.
I would limit to a small set of getters, for example a "basic set" and "complete set". As I see it, the FMTS, Web and TCX file all use the very simple set of core metrics (i.e. HR, distance, time, speed, watts, SPM, strokenumber, Calories, dragfactor) and the RowingData and PM5 interface use the complete set. Come to think of it, I think we might to split up the WorkoutRecorder for this as well: typically you would want three workout recording features: one for each recording. raw recording is pretty close to the gpio datastream (thus requiring its own subscription to that stream), tcx is pure drive start and session end driven (i.e. events based), where RowingData can be identical to tcx, but also time driven (once per second).
Yeah, but we want to reduce complexity, so making things simpler is an aspect here as well. Option two looks the most consistent and pretty clean to me.
Agreed.
Hitting a pause is an event, triggering a request for update to most features. If the features are in full control and keep their own timers to request data when they need it, I think this should work.
I think we could limit the state to "the stroke state has changed", etc.. Thinking about it, there is no need to tell you have new data. If you are purely waiting for metricsupdates, the feature probably is time driven (and thus a timer in the feature should solve this). RowingStatistics could be fitted with two getter datafunctions, and you would be done, IMHO. Only thing it needs to provide is a set of current metrics when asked for it.
Getting further from the Raspberry or PM5 will reduce accuracy as the high frequency part of the monitor will monitor passing a certain limit. When you have the high end use of Rowing Machines (like the PM5), you want to measure with high accuracy. So at every pulse, you need to evaluate whether you are passing a time or distance limit. This way, on a Concept2, you reach precision in .1 sec accuracy. If you report distance every second, you might be almost a second late in reporting you passed the finish line.
It could mess up things with data recordings, especially in the RowingData and PM5 section there are metrics that exist for a short time (Power Curve, Force Curve, Velocity Curve) as the end of the stroke empties their contents, so we need to watch those. |
Beta Was this translation helpful? Give feedback.
-
I've played around with a Garmin Epix connected to a PM5 through ANT+ last two weeks, and I must say I'm extremely disappointed by the results. Time and distance measurement are sloppy at best: the Epix doesn't seem to respect the time and distance measurement from the PM5 (let alone split calculation both compared to the PM5 and ORM). On a 15K row, I missed over 300 meters. Splits that should be equal in length (steady state rowing) weren't. Stroke length was all over the place where the raw data from ORM showed a nice smooth increase. I hope our solution is more robust than Concept2's, but I guess missed messages and rounding have quite some bad effects. Could also be that I configured the Epix wrongly, but it confirms my suspicion that when it comes to accurate recording, you need to stay close to the hardware. |
Beta Was this translation helpful? Give feedback.
-
That is very strange. Its an epix gen 2 right (EDIT: Of course its a gen 2, the gen 1 did not have FE-C) :)? Especially as distance (as per the ANT+ specification) is directly reported by the "server" (i.e. PM5) so its not like the watch calculates this, and hence their could be a deviation. It should not be packet loss as distance is reported as a byte and accumulates (so 256m is available to make corrections) unless there is like more than a minute of disconnect:
So basically the watch looks for the delta change and adds it to the total. But I checked when I recorded PM5 and Garmin FR255 in august: This is C2 logbook. You can see that its stroke detection is sometimes gets off for some reason... (ORM is superior in this sense :)) But the recorded distance is indeed different: But I remember that I switched off my watch recording while the C2 was still spinning so I suppose PM5 was recording longer on the end side. I suppose a correct test would be to wait with the stopping of the watch recording until the PM5 stops recording as well. The difference in splits (very minor though) I can, theoretically, understand as there is a lap toggle bit that is broadcasted and it is sent twice or so per second, so there might be some delay in registering the split. My workout was not an interval session though, actually I triggered the lap manually on the watch so I dont have info on this one. The time measurement will not be accurate like ever, as you have to start the watch timer separate from the PM5 so they will never be able to record the same time. The watch will run its own timer independent from the PM5, time is not taken from the PM5. This should mean that the first split time will never be equal on the watch and the PM5 but subsequent once should be close. On you suspicion re. being close to the hardware, I think its a matter of implementation of the data transfer. E.g. if a data is broadcasted and handled properly by the recipient it should not be relevant how close you are to the hardware. What I mean is that if the data is copied and re validated on every broadcast (so corrections as necessary is made) it should not matter as the potentially incorrect data may be corrected by the subsequent packet. And once everything stops final data can be broadcasted. Actually the accumulated distance approach in ANT+ is very good as it should enable handling of data loss (this is noted by the specification). Also in the CPS and CSCS BLE profiles the translation of the distance to "rotations" are self correcting too and the watch should be in sync very much. The only issue there is that for some reason there is latency on the Rpi (approx 2 sec...), meaning that you should wait for the flywheel to stop and wait a few tick so the final distance is received. The ESP32 MCU is much more accurate in this sense. But these issues are with the transfer layer. |
Beta Was this translation helpful? Give feedback.
-
That is what I assumed as well, but missed messages, at least with the PM5 results in missed distance (had the same issue today). I'll probably switch to your fork and see what ORM does. Could be that Concept2 messed up somewhere....
Took a bit of time, but it is pretty reliable :). In working on the validation of the engine I also discovered the PM5 has strokes with a 0.5 seconds length, followed by a 1,5 sec stroke. They do seem to buffer all metrics (i.e. no extreme SPM's), but they do mess up somehow.
I agree. Even splits and times shouldn't be an issue. EXR depends on the BLE messages, but accepts time and distance travelled as-is, and thus has no timing issues. As you say, the rest becomes interpolation. RowsAndAll does the same thing after the fact based on the provided xls, works pretty well. But an interesting experiment. And I must say it delivered some very useful data: in rowing with the Epix I noticed I was overheating (temperature went up and up). So I bought a fan (oddly enough, not a big seller in winter....), and rowing became much less stressful. So that is a big improvement for me. I'll try to switch to the ORM ANT+ server in a couple of weeks and see what happens. |
Beta Was this translation helpful? Give feedback.
-
|
@Abasz , come to think of it: why should RowingStatistics even be aware of the HR data? Wouldn't it make more sense to have features ask a HR datastore this (and simultanously ask |
Beta Was this translation helpful? Give feedback.
-
|
Finally I had some time and I am reviewing our discussion. Playing with the design ideas. I was thinking about how would you feeling about introducing a library for the data stream management: rxjs (https://rxjs.dev/). I've been using this with angular for a while now and it is very convenient and allows the removal of eventemitter and enables a very robust and future proof way to implement communication between components via data streams. It is also able to wrap native nodejs events so introducing it should not be that big of a challenge. I am not saying this is strictly necessary. The aimed setup can be done without the need for an external library. But I believe that on the long term it would make a lot sense as the API for handling the stream of data is much cleaner and flexible compared to node event emitter. |
Beta Was this translation helpful? Give feedback.
-
Looks very promising! I'll have to take a dive into it as well to understand how to connect the various components of ORM, but from what I've seen so far, it looks quite suitable. I don't know if @laberning has additional perspectives on this? |
Beta Was this translation helpful? Give feedback.
-
|
So I have put together a discussion starter class for the data service. What I did
So basically here whenever an update to any data is made the corresponding notification will be dispatched and subscribers will be notified as well as the data at that moment is sent along with the notification. An alternative approach would be that we dont send the data (just the notification), but rather provide an API (getter) on the data service that allows getting the data. I dont mind either approach (or can be both). The drawbacks of the first approach (only sending data along the notification) is that non-event based features (like a feature that gets data based on an internal timer) would need to keep a copy of the data. The drawback of the latter (i.e. signal the event and allow a getter for getting the data) is that it could happen (though it is very very unlikely) that by the time the feature reaches out to the data service to get the data it changes. Actually for this reason (but I know we discussed this several times :)) I would allow both approaches (especially since the code complexity for this and option 2 would be identical, the only difference that we send data along with the event not just the plain event). class RowingDataService {
#strokeState = "WaitingForDrive"
#workoutSessionData = {
sessionType: 'JustRow',
sessionStatus: "WaitingForStart",
intervalNumber: 0,
intervalMovingTime: 0,
intervalLinearDistance: 0,
}
#strokeCycleMetrics = {
driveLastStartTime: 0, //seconds
driveDuration: 0, // seconds
driveLength: 0, // meters
driveDistance: 0, // meters
driveAverageHandleForce: 0,
drivePeakHandleForce: 0,
driveHandleForceCurve: [0],
driveHandleVelocityCurve: [0],
driveHandlePowerCurve: [0],
recoveryDuration: 0, // seconds
strokeWork: 0, // Joules
strokeCalories: 0, // kCal
cycleDuration: 0, // seconds
cycleDistance: 0, // meters
cycleLinearVelocity: 0, // m/s
cyclePower: 0, // watts
dragFactor: 0,
instantPower: 0,
}
#heartRateMetrics = {
heartRate: undefined,
heartRateBatteryLevel: undefined
}
#overallMetrics = {
totalMovingTime: 0,
totalNumberOfStrokes: 0,
totalLinearDistance: 0, // meters
totalCalories: 0, // kcal
}
#lastRotationImpulseDataPoint = 0;
onOverallMetricsChange$ = undefined // we will provide access to an Observables via these public properties to which clients can subscribe; this will be triggered by overallMetricsSubject
onStrokeCycleMetricsChange$ = undefined // we will provide access to an Observables via these public properties to which clients can subscribe; this will be triggered by updateStrokeCycleMetricsSubject
onWorkoutSessionDataChange$ = undefined // we will provide access to an Observables via these public properties to which clients can subscribe; this will be triggered by updateWorkoutSessionDataSubject
onHeartRateMetricsChange$ = undefined // we will provide access to an Observables via these public properties to which clients can subscribe; this will be triggered by heartRateMetricsSubject
onStateChange$ = undefined // we will provide access to an Observables via these public properties to which clients can subscribe; this will be triggered by strokeStateSubject
onNewRotationImpulse$ = undefined // we will provide access to an Observables via these public properties to which clients can subscribe; this will be triggered by impulseDataPointSubject
onAction$ = undefined // we will provide access to an Observables via these public properties to which clients can subscribe; this will be triggered by actionSubject
// Taking advantage of dependency injection, and injecting config via constructor
constructor(_config) {
this.#strokeCycleMetrics.dragFactor = _config.rowerSettings.dragFactor ? _config.rowerSettings.dragFactor : 0 // Dragfactor
}
dispatchAction(action) {
// we trigger notification
// actionSubject.next(action)
// new data will broadcasted via onAction$ that is an observable. any client that subscribed will be notified with the new data
}
addNewRotationImpulse(dataPoint) {
this.#lastRotationImpulseDataPoint = dataPoint
// we trigger notification
// impulseDataPointSubject.next(dataPoint)
// new data will broadcasted via onNewRotationImpulse$ that is an observable. any client that subscribed will be notified with the new data
}
updateStrokeState(state) {
this.#strokeState = state
// we trigger notification for the observable. This will notify any subscribed client that there is new data
// strokeStateSubject.next(this.#strokeState)
}
updateOverallMetrics(metrics) {
// with the application of the spread operator partial update is allowed, needs to be decided if this is necessary
this.#overallMetrics = { ...this.#overallMetrics, ...metrics }
// we trigger notification for the observable. This will notify any subscribed client that there is new data
// overallMetricsSubject.next(this.#overallMetrics)
}
updateStrokeCycleMetrics(metrics) {
// with the application of the spread operator partial update is allowed, needs to be decided if this is necessary
this.#strokeCycleMetrics = { ...this.#strokeCycleMetrics, ...metrics }
// we trigger notification
// updateStrokeCycleMetricsSubject.next(this.#strokeCycleMetrics)
}
updateWorkoutSessionData(sessionData) {
// with the application of the spread operator partial update is allowed, needs to be decided if this is necessary
this.#workoutSessionData = { ...this.#workoutSessionData, ...sessionData }
// we trigger notification
// updateWorkoutSessionDataSubject.next(this.#workoutSessionData)
}
updateHeartRateMetrics(heartRateData) {
// with the application of the spread operator partial update is allowed, needs to be decided if this is necessary
this.#heartRateMetrics = { ...this.#heartRateMetrics, ...heartRateData }
// we trigger notification
// heartRateMetricsSubject.next(this.#heartRateMetrics)
}
} |
Beta Was this translation helpful? Give feedback.
-
|
Hi Abasz, Looks interesting. A lot of points, but not to discourage you in any way. I think some stuff should be interpreted differently (i.e. a state based approach instead of a metrics based approach) and getting back to an old debate :). But I like the direction.
I'm not used to this notation, but if @laberning agrees, I'll learn to live with it :)
Indeed I would focus on some key metrics and skip derived metrics like pace etc.. I would push them to features as well. But please realize that LinearVelocity is quite a nasty calculation as it is only updated every cycle, and one that has to be consistent (as some BLE-clients like EXR use it to trigger their behaviour). So many of these metrics need to stay close to the rower and session state, as they are defined and updated at very specific moments.
It looks messy as you are approaching it the wrong way. A rower is state driven, and the metrics are updated as a byproduct of the state transition of the machine (standard petri-net/Finite-State-Machine based architecture, as described in the architecture document). The state machine looks like this: stateDiagram-v2
[*] --> WaitingForStart
WaitingForStart --> Rowing: strokeState<br>is 'Drive'
state Rowing {
strokeState=Drive --> strokeState=Recovery
strokeState=Drive --> strokeState=Drive
strokeState=Recovery --> strokeState=Drive
strokeState=Recovery --> strokeState=Recovery
}
Rowing --> Paused: strokeState<br>is 'WaitingForDrive'
Paused --> Rowing: strokeState<br>is 'Drive'
Rowing --> Stopped
Stopped --> [*]
Each arrow leads to a subset of metrics being updated. This is what RowingStatistics implements from the Finite state machine. So the state of the machine is leading, and metrics updates are just a byproduct of that state changing or not. As most metrics can only be captured/generated at a specific moment, so we have to capture them at these specific moments. And to be able to answer questions at random times, you need to buffer it somewhere. The "messy" structure is because we have to interpret and buffer/disconnect from the underlying state of the Rower.js and maintain a session state. RowingStatistics is that place, as it also prepares data for data consumption (like smoothing), and an extremely strong focus on the state machine. I admit there is some formatting going on that should be moved to GUI/etc., but most metrics need to be calculated there, driven by the state machine as the calculation needs to be done at a very specific state transition. So maintaining the state machine here is essential, as that drives the entire refresh of the metrics. The state determines what metrics are to be refreshed. I would organize the broadcasting of messages in the main loop of the Finite State Machine, as that is the ONLY place where session state is clearly determined (the rest of the functions all have multiple entry and exit points, as an interval end, etc. is always a possibility). If you look at V1Beta_Updates, you see the
As I see it, the store would lean on exposing internal metrics, but updating a specific set of metrics should not trigger any event. It is the change of state (or the absence of a state change) that only could trigger updates.
You shouldn't trigger on metrics updates at all. We currently trigger on state machine state changes to broadcast metrics. I can imagine a new update when metrics are refreshed somehow, but it isn't the metric changes driving the broadcasts, it is the rower/session state changes. In essence every arrow in the state diagram should trigger a message of some kind, even the ones returning to their own state (typically a metrics only update). The key is an event, not a data change of a data subset (please realize that I do see that processing a currentDt, updating metrics but not having a session/rower state change as a valid state change).
I'm not too familiar with object oriented parts of Java, although an object oriented, more class based, approach would be welcomed by me. Will be a bit more of a learning curve, but as an old C++ programmer, how hard can it be?
The issue is that control over the control/dataflow goes both ways, and in my experience it becomes a mess in the end. Either you:
I'm fine with either model, but to keep code clean, consistent and understandable we really shouldn't mix these models. If features start to define their own way of getting data, you can easily break features as the consistency is missing, and then you have to find out where it all went wrong. Having one way of getting data, also guarantees that if you break the data supply to one, you break them all (that probably doesn't go unnoticed in real-life testing). The current model is pretty close to option 1, albeit that server.js is too much hands on. From that perspective, I'd be inclined to go for option 1, but option 2 certainly has its charm.
As said, the distinction you make between OverallMetrics, WorkoutSessionData, StrokeCycleMetrics etc. will not work. This distinction is internal, to facilitate updating specific metrics more effectively without duplicating some code 8 times. But this partial approach will expose a state dependency to all features, were most features are incapable of understanding this difference. That is actually why RowingStatistics is there as a buffer: most metrics are only valid at very specific moments (like LinearVelocity and Watts only can be updated at the end of the Recovery and the end of the Drive), but the WebGUI and several BLE clients need them each and every broadcast. To prevent a lot of complexity handling all these different metrics at different times (basically copying the "messy" setup of RowingStatistics to all features!!!), it is centralised at RowingStatistics. A distinction you could make is broadcasting at specific events (which is actually the current setup, although server.js should be more hands-off): at specific changes in the rower or session state, as that triggers broadcasts from some of the features (i.e. the PM5). These events are now triggered by a Other time driven metrics could be updated by a I would probably also remove the HR data out of rowingStatistics and into its own store. That would clean up some disconnected data in RowingStatistics as well. Not a big issue, but one to think of. |
Beta Was this translation helpful? Give feedback.
-
|
While I was reading your comment I just realized that despite me thinking that I understand the basis of the direction of the architectural design we are not on the same page in terms of starting point. Before I elaborate on this let me provide some theory that will help understand why I was proposing thing in the above discussion (also just a note: you should not call javascript as Java, just the name is confusing but the two languages are far from each other, Java being complied, javascript being non-compiled as one, while there is also no object oriented and non-object oriented part in JS, these "classes" are just syntactic sugar for the prototype based inheritance so people coming from true object oriented languages - like C++, C#, Java, etc. - can acquainted with the language more easily, they are "fairly" new, introduced in 2015). The current code is a complicated and hard to extend/maintain as it breaks two very important coding principles: 1. separation of concerns and 2. single responsibility principle. This means that some of the components try to do too many things and responsible for several things that should not be in their scope. And here I am not just talking about formatting values but much more fundamental things. In order to derive to a good architecture that is maintainable (by anyone that looks at the code without prior knowledge), easily extensible etc. the code should be compartmentalized with plug and play elements (separation of concern). These can be called services, components or whatever, but it should be written in a way that if something is swapped out, or changed only very limited code should be changed. Also every component (or class or module) in a program should only provide one specific functionality. This should mean that a data store should never be responsible for calculating data as well as a metric calculator service should not be responsible for storing data and maintain state. These are two different tasks for two different elements (or service, or components, but for now I will call these services) that are wired together via "public" API on these services. These "public" facing methods and properties should be fixed in way that any consumer of these can trust that they get what they expect. On the other hand this allows that the consumers can ignore how this data or function is created. To illustrate via an example. A good example is logging. The consumer of the logger should generally not care where one logs (console, file, cloud) it provides a way to do logging that is the same for each type of logging destination. So actually one can change the logging destination without affecting the consuming end with a snap of a finger (or a few press of the keyboard). Now lets say that we want to swap out the rowing engine and go back to the old one. If the data store is coupled to the metric calculation that is by virtue of state driven nature of the actual rowing engine (i.e. because of its state driving the metrics), we will not be able to do it unless we reprogram the data store as well. However, if these functional components are separated either part can be swapped out without the other caring about the changes. What I am trying to say is that a person that for instance implements BLE Peripherals should be able to program that with zero understanding of how metrics are calculated since for this person the only relevant thing is that distance or power or last stroke event time, etc. is available some way. Now with the above in mind my proposal is/was to start breaking up the spaghetti code into components one of which is a new data store. A data store that is decoupled from the engine and the metric calculation and knowledge of next or previous state, and one that receives data updates and keeps the current state (that is factually the last state) on one sides and broadcasts/enables query (i.e. push or pull, that is irrelevant for now as the principle is the relevant) on the other side. My data store design does not care how the metrics are calculated (honestly I dont fully understand that either, but again I should not need to understand what drives these metrics really) or what state are available. It should just be responsible to store the most current data as as single source of truth (including the state) and serve that data to any consumer that wants data in structured and backwards compatible way. Such consumer can be a data producer it self, like the engine that needs the information on the current state, and also updates the state on the data store side. |
Beta Was this translation helpful? Give feedback.
-
No offence, and I know you mean well, but I'm an IT architect with over 25 years of experience, former Microsoft MVP for embedded systems development for 7 years, which literally has written books about software architecture that are used in university classes. As a dinosaur in IT, I've seen most stuff being introduced :(. And I found out the hard way JS isn't decently object oriented: a year ago, I thought using a nice abstract class for the regressors would be fun, but JS wouldn't let me. Now all regressors just implement the same interface without the real fun.
I agree with you in theory, but please note that being too rigid with such approach actually brings you further from your goals than closer.
Self-explaining code is a utopia. Code may look simple, but it will never be self-explanatory. You need the design philosophy and design rationale before you can understand code. As a rower is a discontinous system (i.e. behaves totally different in different states), it is annoyingly present and you can't understand the code without understanding the states.
It isn't: Rower.js calculates all linear metrics. I think you miss the crux here: state and data are too intertwined to be seperated. State determines what can and can't be updated, state can invalidate data, data drives state. That is a domain property that you must respect when designing something. Rower.js calculates all linear metrics, but will update them based on the state. RowingStatistics collects them, and determines the session state (as this is intertwined with the data) and buffers them to make sure there always is a metric (like LinearVelocity) to speak of to the outside world. That is RowingStatistics purpose: make the state less dominant, easing the pain for features.
There is a limit what one can abstract. As described in the architecture document, we abstract away layer for layer from the raw data: from currentDt to flywheel properties in Flywheel.js, from Flywheel properties to linear metrics and rowing state in Rower.js, and that is inspected and buffered by RowingStatistics.js, to present a complete set of metrics regardless of the inner state of the Rower/FLywheel. But the intertwining between state and data is inevetible here: state determines what you are "allowed" to ask, not because it of design choices, but because of fundamental physics. To put it simply: by convention the linear velocity simply isn't a defined entity when you aren't in the end or begin of a drive-state. More fundamentally, even total distance travelled isn't defined unless you are at the end of a recovery as the drag present isn't known. I can't change the physics, I only can make the code respect it and look pretty decent.
That is not by design choice, but because of fundamental physics. Most metrics, aside total time, aren't defined when a rower isn't in a very specific state (see http://eodg.atm.ox.ac.uk/user/dudhia/rowing/physics/ergometer.html). You might consider it a violation of seperation of concerns, I consider it fundamental physics that is in the domain that can't be ignored. As said: you can't ignore the physics. For V1Beta, I actually designed by contract, and replaced layer for layer, in the way you describe. This allowed me to redesign each layer from scratch with the possibility to return to the old design if it didn't work. But in that process, you have to respect the contract between the layers, which includes a lot of fundamental physics: you can't ask for anything aside a few metrics unless you are in a very specific state. I admit it isn't a simple contract, but it is one that respects the domain and its physics. I tried simplifying it by letting Rower.js do more work, but that became too complex as calculation and persistence became too intertwined. I tried another layer to manage session state on top of RowingStatistics, but as many events are needed to communicate ro the outside world, you end up with duplicating the state machine, not simplifying it. So if you want, I can look at an old backup and I can replace Rower.js (together with Flywheel.js) with its older implementation as that still adheres to the same service contract.
That is exactly what RowingStatistics currently provides (see https://github.com/JaapvanEkris/openrowingmonitor/blob/v1beta_updates/docs/Architecture.md#rowingstatisticsjs), where the buffering and abstraction away from the strong stroke and session dependence for the consumers of such data. But some consumers (tcx recorder, rowingdata recorder, pm5 BLE) actually require a state present, and specify their assumptions upon state quite clearly.
An essential part of abstracting away from rower and session state, requires buffering of metrics that aren't defined at that moment.
That is what RowingStatistics does. But several features explicitly demand state as well, you can't ig ore that. |
Beta Was this translation helpful? Give feedback.
-
Apologizes, my aim was not to offend you. The reason did not assume this much experience is that you mentioned several times that you do not know how to implement certain designs. I probably misunderstood those statement.
Well, the above (i.e. not decently object oriented) is half true half wrong, while the statement that it does not have the ability to create abstract class is wrong. As I mentioned JavaScript has this prototype-based inheritance thing that makes conventional approaches to base and derived class slightly complicated. This is why ES6 classes (and the support for the For the ES6 have a look at this stack overflow example: https://stackoverflow.com/questions/597769/how-do-i-create-an-abstract-base-class-in-javascript For non-ES6 to do it from scratch is a bit more involved but this is one way to do it: function Animals (name, age) {
const newAnimal = Object.create(animalConstructor)
newAnimal.name = name
newAnimal.age = age
return newAnimal
}
const animalConstructor = {
sing: function () {
return `${this.name} can sing`
},
dance: function () {
return `${this.name} can dance`
}
}
function Cats (name, age, whiskerColor) {
const newCat = Animals(name, age)
Object.setPrototypeOf(newCat, catConstructor)
newCat.whiskerColor = whiskerColor
return newCat
}
const catConstructor = {
whiskers () {
return `I have ${this.whiskerColor} whiskers`
}
}
Object.setPrototypeOf(catConstructor, animalConstructor)
const clara = Cats('Clara', 33, 'purple')
console.log(clara.sing())
console.log(clara.whiskers())
I dont understand why you are stating that these cannot be separated. Basically the way I see it is that there is an impulse that triggers the reassessment of the state (rowing, pause, etc.) and when state changes based on the data metrics are (re)calculated/updated. Actually I have looked at the code and it is very simple to put the data store in the middle. Basically you have the gpio currentDt communicated to the data store, that dispaches a notification to all subscribers that there is a new impulse. than everyone does its job to calculate what ever they are responsible in the sequence they are doing it right now and update the metrics and state stored in the data store. Currently what is being done is that in server.js gpioTimerService.on('message', handleRotationImpulse)
function handleRotationImpulse (dataPoint) {
workoutRecorder.recordRotationImpulse(dataPoint)
rowingStatistics.handleRotationImpulse(dataPoint)
}We are calling these functions and triggering the recalculation. Instead of this I would delegate the handling of the subscription to the features via the data store. So for instance in rowingStatistics we would have something like this: dataStore.onNewRotationImpulse.subscribe(handleRotationImpulse)
function handleRotationImpulse (currentDt) {
// Provide the rower with new data
rower.handleRotationImpulse(currentDt)
....
}And then rowingStatistics would instead keeping datat internal would update things on the store, for eg.: function handleDriveEnd () {
dataStore.updateMetrics({
driveDuration: driveDuration.push(rower.driveDuration())
driveLength: driveLength.push(rower.driveLength())
driveDistance: driveDistance.push(rower.driveLinearDistance())
driveAverageHandleForce: driveAverageHandleForce.push(rower.driveAverageHandleForce())
drivePeakHandleForce: drivePeakHandleForce.push(riower.drivePeakHandleForce())
driveHandleForceCurve: driveHandleForceCurve.puswhh(rower.driveHandleForceCurve())
driveHandleVelocityCurve: driveHandleVelocityCurve.push(rower.driveHandleVelocityCurve())
driveHandlePowerCurve: driveHandlePowerCurve.push(rower.driveHandlePowerCurve())
})
}After which update an of event 'new metric is available' would be dispatched and any subscriber notified.
I am not sure if this is true but I might be missing something. For instance
If you see it that way than I dont think there is significant room for redesign as it seems that the architecture works as intended and only minor rather refactoring type of changes are necessary like moving out things that should not be there. In my view doing more would allow better maintainability, extensibility and testability e.g. due to the possibility to do any data subscription or update internally from the feature effectively decoupling the feature and make it a fully independent module (unlike now, where for instance currentDt cannot be accessed by any part of the app without editing server.js and adding the callback). |
Beta Was this translation helpful? Give feedback.
-
Absolutely no offence taken. I usually am at C/C++ projects, and honestly I find JavaScript (and Java for that matter) a bit cumbersome. As an architect I manage teams that work in Java or .Net, but usually only for extending functions in larger middleware systems. Especially implementing a decent structure where a combination of command-push and directed data-pull would be created is challenging (I've done so for commercial customers, but that was with a decent service oriented architecture).
The functions I think it would be easier to replace emitMetrics with something like (i.e. combining the function of the current emitter and the getMetrics function):
This would present a single, consistent dataset, where all metrics are contained (and yes, we need to clean out derived metrics).
But, as the name suggests,
Quite simple: rowing is a discontinuous sport, the rower does totally different things during recovery and during drive.
Asking rower.js for data outside these windows will result in garbage. As mentioned: handlevelocity isn't defined outside the drive phase. Another example is power: it can only be calculated across the stroke, but ONLY when the drive or recovery ends. This is a hard requirement as all work is done in the drive, and in the recovery no work is done. So when calculating the power, you need to include both the total drive and recovery to get a decent average. But asking for power outside these exact moments will result in garbage as underlying flywheel and rower metrics aren't at their final value yet. The goal of RowingStatistics is to remove the phase dependence of metrics being present, inspect Rower.js at the right moments and present the last known good state to the outside world, also respecting the desired smoothing. This way, rowingstatistics acts as a buffer between the event and phase driven world of Rower.js and can it always present data. RowingStatistics already provides a consistent set of metrics independent of time: due to historic choices, RowingStatistics triggers the time driven updates of both the peripherals and web interface, and it can't suffice with a half-empty dataset as one of the consumers is the PM5 interface which consumes most metrics as well and pushes it to EXR (causing total chaos there when stuff is wrong).
I think in the setup of Flywheel - Rower - RowingStatistics, there is little to improve from an architectural point, except moving some historic/odd responsibilities out of RowingStatistics (i.e. making it responsible for pushing data timedriven to the features, managing the HR data and managing the HRR updates). I'm not entirely happy about the entanglement of the two state machines, but removing it makes either rower.js overly complex or just makes a bigger mess elsewhere. But that need is driven by the need of specific transitions to be communicated externally as well. However, I might get better insights (and you might help me :)) once the code is maturing based on the changes proposed. Cleaning up code make it easier to see patterns as well. Sometimes it is seeing the code change that leads to better ideas. One of the things I'm still toying with is splitting the session management part and the persistence layer. Not a big issue at the moment as the states managed overlap 90%, but when sessions become more complex (so they can include a pause timer, etc.), it could qualify for its own layer as well. But that is dependent on what the sessions might include as functionality.
I don't see that as much of an issue with your specific example, as I see currentDt as an internal variable that actually shouldn't be in server.js in the first place. I rather see it internally to RowingStatistics (or better, flywheel.js) and hidden from the rest of the application. I'm rather fond of the more abstract approach you have chosen with the HR data, where the data is there, delivered to server.js without much interference. I rather see such a clean approach with the rower data as well. I'd agree that for most features this isn't the case: they need a lot of code to get going. On the other hand, writing a simple test framework can help a lot. In developing the new engine, I started with writing test cases against the old contract, and then build the new implementation, which allowed me to test for regression issues. So for the entire stack of Regressors, Flywheel, Rower I created a bunch of test cases. Only thing I couldn't get it to work with RowingStatistics.js (it was too much work for getting in depth to get the message-based structure to work. I couldn't be bothered, still on my ToDo list). |
Beta Was this translation helpful? Give feedback.
-
|
Looking at the above discussion, I think we let the solution lure us into a Big Design Upfront approach, and abstract discussions about this type of change ahead of implementation are impossible without a whiteboard anyways. As we are both intelligent people with a great deal of experience in building systems, let's assume we can approach this more agile and trust ourselves to able to pull a change through without changing everything everywhere in one huge undertaking. Let's focus on small steps that result in a working solution each time. What I see is that
What I propose is making small steps and grow to a better structure:
Each step should result in a working solution, but allows us to test this extensively before taking the next step, and find potential issues. By making these steps small, we don't have to make huge decisions now, but we can see what happens to the code and take it from there, making many discussions much less abstract. |
Beta Was this translation helpful? Give feedback.
-
|
Sorry I ahve not seen this one before posting an answer. I will read this and edit my above if necessary. |
Beta Was this translation helpful? Give feedback.
-
What difference does the fact that new metrics are emitted when state does not change? What I mean is that when state changes there must be a new currentDt as well. So basically state change happens on specific new currentDt change.
So this is what I would try to avoid. I think on the long term its important that features can be implemented without the need to edit other files (that is why I proposed in my answer to make the design so features can subscribe internally without the need to edit other files). This would help to avoid merge conflicts, duplication of work and many other issues etc.
I think we are on the same page here, i.e. exposing the state change event and then the features could filter the once they would like to act on. So wtr. workoutRecorder it will ignore statechange events other than RowingStopped.
I dont understand what workoutRecorder has to do with heartRateMeasurement. Basically workoutRecorder would need heart rate data (current or avg over the cycle) at the end of each stroke cycle when it gets logged but for the rest of the time it does not care about heart rate. So why would a move to WorkoutRecorder be better than rowering statistics?
Yes, I agree. Not sure what by "all other message types" is meant and what case the need for such a catch all for events is necessary. What I mean is that in the implementation you draw up (by the MetcisUpdate call) either a feature updates the local copy of the the data on every MetricsUpdate and uses it on its own term (e.g. broadcasts it based on a timer), or going to carry out an action on a specific state change (e..g. WorkoutRecorder save csv on RowingStopped event).
Yes, I agree with the ceveat that I wolud not split the HR management (that is actually only keeping the HR data that is updated from the peripheral).
I am not sure how much better having a GUI commands handlers in RowingStatistics than in server.js. In my view with this just increases RowingStaticscs responsibilities making it more complex (for instance now RowingStatiscs will need to be aware of webserver). I would move this to a new, separate module that either exposes event hooks for which other parts can subscribe for (e.g. subscribe for a resetWorkout event) or inject the necessary services into this new module (like peripheralManager, workoutUploader etc.) and call the functions on these. The first approach requires more work but results in a cleaner solution, the second is less work but does not really solve the decoupling issue rather tries to hide it, but easy to implement. |
Beta Was this translation helpful? Give feedback.
-
But not the other way around. When a currentDt is submitted, the old state can be
You need some temporary plumbing in these processes. Stuff probably removed afterwards, but before you lay down new pipes, you have to keep the old ones working.
workoutrecorder also needs it for the recovery heartrate (HRR), which is part of the tcx comments (but is also part of fit-files which I want to do some time), which is defined as the heartrate AFTER rowing has stopped. There are no strokes to broadcast, but HR needs to be conveyed. So currently you get an artificial construct there that updates the HR from rowingstatistics to the workoutrecorder after rowing has stopped. It sucks, but there is no practical way at the moment, unless HR is directly communicated to the workoutrecorder. Aside that: why shouldn't the workout recorder receive HR directly from the ANT/BLE and process it as if sees fit? When decoupling, that is a good place to start to be honest.
In essence, when metrics change, all subscribers should be notified regardless of the reason for change. They should be aware that there are specific classes of messages, for example
I would let the peripheral manage the HR, but also let the peripheral be the single source for that data, broadcasting it to its subscribers whenever it sees fit, without the intervention of RowingStatistics. The peripherals are smart enough to decide whether they see an HR update as a reason to update their displayed status.
Yes and no. Currently, server.js tells RowingStatistics that setIntervalParameters, pause, stop, resume or reset is needed. It does so by directly triggering the responsible function. This results in one finite state machine, and 5 backdoors that can do whatever they like. Here, it leads to an inconsistent design, as some states are managed from the state machine itself, and some aren't as they might be triggered externally as well. And that removes the consistency that you need (IMHO, one currentDt should always directly lead to a visibly emitted message in the main loop). And that backdoor influence is obscured (you can only see it in the public advertised functions), making its effects unexpected. I rather have a single command handler on top (lets say I rather have RowingStatistics abstract away from the webserver. I think that the webserver is one of the sources that could generate a command that can be handled by the exposed handleExternalCommand or listener. But I think the ANT+ or BLE interface should be able to generate identical commands as well. For RowingStatistics or the features, it shouldn't matter where the command originates from. I can imagine that some commands are only of interest of a specific part of ORM. For example, changing the heartrate setting from BLE to ANT would be irrelevant for RowingStatistics, but interesting for the peripheral manager, who in turn can signal the GUI of a settings change as well. But that is the reciever's responsibility to ignore such commands. |
Beta Was this translation helpful? Give feedback.
-
Yes, I agree. And this was the main reason I included a separate event for the state change in the data store mock and also mentioned some time ago that having a separate event for the state change is necessary and time stamps will not cut it (as this would liberate the recipient from keeping "old" state and compering on every metrics update).
Im probably missing something here as an empty handler really is not an old pipe going :) its an empty handler. Or do you have some specific client that would go in there?
So does the GUI need HR data, so this would mean that the GUI would need access to the workoutrecorder to retrieve HR.
That would mean that workoutrecroder (as well as the GUI) would need access to the peripheral manager. Honestly I think this is very easily centralized. It can have a data store with an update method to receive the HR data from the peripherals and a getter for any feature that needs the data. Then this data store can be injected as service dependency into the parts that use HR data (i.e. workout recorder and GUI and peripheral manager for the update). This approach was proposed by you actually in one of the post. HR data is independent from the rowing machine and will be updated from the peripheral after the rower stopped so getting recovery heart rate should not be an issue.
But these can be implemented in a separate events (on the same eventemitter) and subscirbed by the clients separately. I.e. one event that emits on every currentDt change (i.e. metric change) and one event that emits on state change/transition. Then a client can decide if they want to subscribed either of these or both. I dont like catch all subsrciption since it could cause issues if a new event for some reason with a different data structure is implemented on the same eventemitter (event that would not be relevant for the subscriber of the catch all). There would need to be guard clauses against this in the feature so actually when extending a feature already working functionality can be broken.
But that means that you need provide access to the peripheral manager to parts of the app that depend on HR data. You are essentially giving access to features to parts of the app that has no right to have access to (e.g. workout recorder to switch peripheral mode, it could do it since peripheral has this method public, it has to have it as that is main task, manage the modes). So the question is why should peripheral store HR data, when indeed they could easily send updates to a central data service that handles HR (and then the peripheral manager does not need to be accessed by different parts of the app) but rather this data store can be accessed the same way as would the peripheral manager (but with only functionality available that is relevant for that part). The difference here is that peripheral manager is liberated from storing HR data (its not an HRmanager, its aim is to manage the peripheral, starting stopping changing cleaning up).
Now that I think as well that it is a good idea (I mean to have only two public commands on RowingStat). That would make rowingstat better injectable (safer). But there is a BUT.
Yes I agree, hence rowing stat is a good start, but then again externalCommand Handler could be a new service/module that has input from these two sources (peripheral and webserver) and one output as an event to which Rowingstat subscribes. Essentially achieving the same thing you propose but abstracting one step further (laying down the initial plumbing of potential future centralization of event handling). And eventually at some point it would be possible that non of the features would need rowingstat injected anymore only the central handler.
Now this will be one of the challanges with having to integrate into rowingstat. That communication will go both ways (i.e. when peripheral is updated it should signal back that it was successfull so the GUI can update the view, or not if failed). So in rowingstat there would need to be a differentiation made on the direction of the command. Like an event that fires when changed peripheral mode successfully. While I understand the little steps approach this is once again a new responsibility that will make (in my view) rowerstat have to do more than it should. |
Beta Was this translation helpful? Give feedback.
-
Hahaha, fair point. I would use it as a temporary construction and fill it with subscribers while moving control. In the current set-up, features expect RowingStatistics to tell them to do something. In essence, we want to move that control from RowingStatistics to the features. I'd rather transfer control to the features one by one, and then introduce the datastore as an optimisation to clean up a lot of code, than do it all at once.
I think I explained my thinking wrong. I think that the peripheralmanager should communicate HR to everyone who is interested, workoutrecorder just being one of the many interested parties here. The GUI being another. When looking at ORM from a microservices perspective, I think that the HR data is its own independent microservice, where you have multiple subscribers as well (so you have two services: the rowingdata and the hrdata).
It would subscribe to the HRData coming from the peripheral manager. Don't think it should be able to command it.
Wouldn't you start duplicating messages, which could to all kind of odd behaviours? For example, on a recoveryFinished you would also send a metricsUpdate (as it is both). A feature like the PM5 needs both (as it is both time and event driven). One of the things I saw in EventEmitter2 (not saying we should use it, but as a feature interesting) is a hierarchical messaging structure, where you could use wildcards in your subscription. And they allowed messages to be processed once (removing the duplicates problem, although in our case that might just be CPU time being wasted and nothing else). So I can imagine the ANT+ (being pure time driven) to subscribe to all updates, where the workoutrecorder subscribing to all but only processing specific subtypes.
True, But we could agree that peripherals and the workoutrecorders can/should subscribe to everything (basically making it a general broadcast to all subscribers), but are explicitly responsible to filter the message type that is relevant to them. Or, closer to your approach, they subscribe explicitly to specific events, where there is a hierarchical model in the message classes and an option is to subscribe to all rowingdata updates. This can be done either by using something like a message structure naming and allowing rowingdata.* or by explicitly mentioning all accepted messages. But please note: the dependency you describe will always exist: when you introduce a new message type to handle a specific case (say signaling the end of a session pause by a new specific message instead of a metricsUpdate), subscribers must be made aware of this new type as well otherwise it will be missed by them. In practice, you can't avoid having to modify stuff at more than one place. Only thing you can manage is how obvious this work is. As ORM isn't that big, I consider it an acceptable trade-off.
We could separate the command flow from the dataflow. But, it is not that we are using ORM distributed across a complex network, so the workoutrecorder theoretically being able to do something isn't much of an issue as in practice we control the workoutrecorder as well (and all agree that it would be silly to have commands coming from there).
I think that features should subscribe to HR data, and if the features find it relevant store a last known good value as they see fit (like workoutrecorder store all historic values).
There always is, isn't it ;)
I'm not certain what you mean here. Could you elaborate?
I don't know. As an independent service, the peripheral manager could report a status on startup and when it sees fit (upon a change of state). For most subscribers it wouldn't be relevant, but for the GUI it would, so it would subscribe to that message as well. |
Beta Was this translation helpful? Give feedback.
-
That is a valid point, in this sense the "native" EventEmitter is indeed limited (i.e. would not be able to help on the event side). This would need to be handled, either the emitter side: we dont emit currentDt change on state change but that would create a hard requirement from the subscribers to subscribe to both, so this is a crappy solution. Or we handle on the recipient side depending on the feature's requirements which seems much better solution, here what needs to be avoided is that there a race condition arises (i.e. that it cannot be guaranteed that state change first then new currentDt event is received). I never played with this on the EventEmitter so I dont know if this can be sorted.
I have never used EventEmitter2 but it would be interesting how the issue of duoble events can be solved. The reason is that I am not sure if the order of the event on the recipient side can be guaranteed (i.e. that following a state change event always a currentDt event is followed but with identical data). I would need to give more thought to this, but one solution could be to signal the state change via the event payload. Meaning that only the new currentDt event is emmited (i.e. emmit on every new currentDt since state change is essentially triggered by specific new currentDt) and have a property with a boolean of
well what I meant by new message is not an "instead" situation, but rather a new situation. Lets say we want to add a fan that speed is triggered by the effort you put in and we need to monitor its rotation speed, so a new event with the
What I was trying to explain that I would create a command manager (or router if you will) instead delegating the commandhandling to rowingstat (similarly to the HR data service maanger I provide more info below). But dont worry about this for now (disregard my comment), as I think starting the refactoring with the HR part (please see further details below) is the best way, and the command handling would come at a later stage. So we can come back to the actions later
In general I would start with the restructuring of the HR data management. As you described, this is a well defined and reasonably simple flow. Here I would go all in, in terms of creating a separate HR data manager service (that receives HR data from peripheral manager via an Actually half of the approach is already used in the current implementation, in server.js, when updating the hr data in rowingstat: if (config.heartrateMonitorBLE) {
const bleCentralService = child_process.fork('./app/ble/CentralService.js')
bleCentralService.on('message', (heartrateMeasurement) => {
rowingStatistics.handleHeartrateMeasurement(heartrateMeasurement)
})
}
if (config.heartrateMonitorANT) {
const antManager = createAntManager()
antManager.on('heartrateMeasurement', (heartrateMeasurement) => {
rowingStatistics.handleHeartrateMeasurement(heartrateMeasurement)
})
}Instead of the above (i.e. call to NOTE: that in the current v1beta the ant+ and ble hr is separately reporting hr as shown above, this means that until these two abstracted away into peripheralManager (as has been done on my extend-peripheral manager branch), it would be hard to remove server js. Actual the rework of the peripheral management is done on my side, only some minor clean up is needed, and rebase to your v1beta_updates branch, I can do it over the weekend and then it can be the basis for the HR rework. Then taking workoutRecorder as an example I show you what (actually very limited) changes needed to be: Current HR is added as part of the metrics received from the function recordStroke (stroke) {
if (startTime === undefined) {
startTime = new Date()
}
strokes.push({...stroke, heartrate: heartRateDataStoreService.getHeartRateData()})
}If Hr is 0 in the data store it will add zero if other than that other value. The rest of the workoutrecorder can be left unchanged (though I have not tested this, but since the Of course the above to work workoutRecorder (as well as peripheralManager in my extened-peripherals branch or in AntManager.js and createCentralManager.js in the current v1beta) would need heartRateDataStoreService as dependency. This can be injected via a function argument (or constructor argument) when server.js sets it up: const heartRateDataStoreService = heartRateDataStoreService() // or can be = new heartRateDataStoreService()
const peripheralManager = createPeripheralManager(heartRateDataStoreService)
const workoutRecorder = createWorkoutRecorder(heartRateDataStoreService)Now we have the mediator datastore service in between the two features without server.js having to know anything about actual heartrates (apart from initilizing the service dependency tree). What needs to be handled is the disconnect or if no HR data is received by peripheral manager for a while (i.e. to avoid the GUI stuck on showing a value that is invalid and old). But this is already done by the RowingStatistics (with a 6sec interval, so this can be moved the peripheralManager so it sets the HR to 0 on the data store service if HR peripheral is enabled but no data has been received): // set the heart rate to zero if we did not receive a value for some time
if (heartrateResetTimer)clearInterval(heartrateResetTimer)
heartrateResetTimer = setTimeout(() => {
heartRateDataStoreService.updateHeartRateData({
heartrate: 0,
heartrateBatteryLevel: 0
})
}, 6000)Actually I would happy to do the refactor of the HR data management should you agree with the above approach. Especially since its real power would depend on the reworked and centralized peripheralManager. |
Beta Was this translation helpful? Give feedback.
-
Actually the rxjs library may be able to guarantee to keep the order of two emitting streams (i.e. ensure that two merged streams are emitted in the order the next emission call is made). I created a stackBlitz: https://stackblitz.com/edit/typescript-iqycoc?devtoolsheight=100&file=index.ts to present this But the issue with this is that the event type is lost (you can see that on the subsribing end the type is either number or string, meaning that the subscriber after merging the two streams will not know if this is a state change event or a simple new currentDt event.). So check of the payload at the subscribing end would be necessary (to determine if this is a state change or a simple new currentDt event). So I am not sure if this is the best solution, as this essentially would result in the need to differentiate based on the payload. But to the potential rescue in the rxjs library it is possible to emmit pairs: https://www.learnrxjs.io/learn-rxjs/operators/combination/pairwise in this case the client can simply check the two last value from the two event (one from the previous and the new and simply compere the state property on the payload. If there is a change its a state change event. I created another stackBlitz: https://stackblitz.com/edit/typescript-iqycoc?devtoolsheight=100&file=index.ts The potential drawback (but I think this is pretty much acceptable) is that at least two emissions is necessary to have the first trigger which makes the need on the subscribing end to handle the first emission as special case as both the values are new. The good part in the rxjs approach that they have a pipe operator that can change the the data stream on the receiving end. So actually it is possible to set up the two emitters on the source side separately and only merge or change the data emitted on the subscribing end where it is needed. It gives significant flexibility. These are just ideas I came up within a short period of time. I am pretty sure that the double emission issue can be solved easily. |
Beta Was this translation helpful? Give feedback.
-
I played around and I think the rxjs library would probably help with the duplications. I will need to do more test but at a first glance its stream emission mechanism keeps the order of the "emit" calls (which is next in their library). I created a stackBlitz to show this: https://stackblitz.com/edit/typescript-iqycoc?devtoolsheight=100&file=index.ts Here you can see that the second emitter is always second on the emission when next is called on it after the next call on the first. This example utilizes The other possibility is their pairwise operator: https://www.learnrxjs.io/learn-rxjs/operators/combination/pairwise Basically this emits the previous and current values as an array. This way the subscriber would be able to compare the last and the pervious data and determine if there is a state change (without the need to actually keep an internal copy of the last data) this is the StackBlitz I created: https://stackblitz.com/edit/typescript-fbu3vk?devtoolsheight=100&file=index.ts The potential drawback I see right now is that this needs at least two data emission get started i.e. the subscribing end needs to handle the first emission as a special case as there are actually two new values. This is not an issue per se. EDIT: Actually this can be solved by providing an initial value on the stream with the startsWith() operator! I updated the stackBlitz The good part is that rxjs allows the tweaking of the data streams on the subscribing end (via the pipe operator). That allows that one client can do pariwise and not care about the very first value being late and another subscirbe normally and have every value. Anyway, I only played with this with a very limited time but I am positive that there is a good solution to the double emission vs. being aware of state change issue |
Beta Was this translation helpful? Give feedback.
-
|
Sorry about the doubel replies... I did the first post than It disapeared so a I took a deepbreth and rewrote it (which turned out to be great as I found solutions for some problems when redoing the code :) But Now I can see that there are two posts.... |
Beta Was this translation helpful? Give feedback.
-
|
@Abasz: I've been working on the modified architecture. Essentially, I combined the two "case....switch" commands of server.js into a single list, and moved this single list to the RowingStatistics and WorkoutRecorder as a single "HandleCommand" function, which will respond to the command for their area. I like the code a lot more, as it cleans up code quite nicely. In Server.js, the case statements could be removed, and be replaced by the various components being told to handle a command, regardless of the type of command (the commandhandler of the recipient will sort out if and how it will respond to a specific command). Especially in the RowingStatistics, you get a lot more symmetry as the multiple exit points are gone now. I also followed your lead in the revised recorder section. I introduced a recordingManager on top of the three individual recorders, which controls all actions of the three recorders. It makes the recorder code a lot easier to read. On hindsight, @laberning already said on top of the file "Todo: split this into multiple modules". I guess that is done :) I also moved the RecoveryHR to the WorkoutRecorder by directly pushing HR data to the recorder, removing that from the RowingStatistics. Only real issue is the Strava uploader: it does GUI stuff, and low level commands on the tcxRecorder. I haven't found an easy way to split this one up. I've added an additional branch (see https://github.com/JaapvanEkris/openrowingmonitor/tree/V1Beta_Architecture_Revision) to post the current status, and I'll be posting my current status tomorrow. There still is some work to be done, but perhaps you could apply a same pattern (especially the HandleCommand() thing, which could eliminate the case-statements from server.js and provide the underlying components with single command entry points). |
Beta Was this translation helpful? Give feedback.
-
|
Hi @Abasz, I uploaded my changes in https://github.com/JaapvanEkris/openrowingmonitor/tree/V1Beta_Architecture_Revision. Code should still work (copied it by hand, to get rid of some experiments). Key element is that RowingStatistics and RecordingManager have a 'handleCommand' function, that contains the following options: // This function handles all incomming commands. As all commands are broadasted to all application parts,
// we need to filter here what this part will react to and what it will ignore
function handleCommand (commandName) {
switch (commandName) {
case ('start'):
break
case ('startOrResume'):
break
case ('pause'):
break
case ('stop'):
break
case ('reset'):
break
case 'blePeripheralMode':
break
case 'switchBlePeripheralMode':
break
case 'antPeripheralMode':
break
case 'switchAntPeripheralMode':
break
case 'hrmPeripheralMode':
break
case 'switchHrmMode':
break
case 'uploadTraining':
break
case 'stravaAuthorizationCode':
break
case 'shutdown':
break
default:
log.error(`Recieved unknown command: ${commandName}`)
}Here, we can filter more effectively and make sure we keep one entry point (remove a lot of chaotic bits of code in RowingStatistics). One question: blePeripheralMode, antPeripheralMode and hrmPeripheralMode all result in the same action: the GUI needs a fresh set of settings. Shouldn't we have a command that says GUI_Refresh? What I noticed is that when the config is handed over to another application part explicitly, it is always by reference. Thus instead of having server.js do all kind of ugly tricks, why not hand both a reference to the central config, and change the setting there, and tell the GUI to refresh? |
Beta Was this translation helpful? Give feedback.
-
|
I will need more time with this to review. But one thing I noticed is that with the non-realtime simulation the raw data file does not get written properly. I suppose the issue may relate to the immediate file write (but I ahve not looked at any code, I am just testing the total number of stroke issue and noticed this). In real time everything works fine. |
Beta Was this translation helpful? Give feedback.
-
No problem. Took me a while to make this step as well. I think that a better separation of functionality would make stuff a lot less messy. That is my experience now with how RowingStatistics and the recorders work. Historically things have grown, but sometimes you have to refactor to get a more futureproof solution. I don't think the introduction of the commandHandler will be the last step we will take. I can imagine that it will be replaced by a more appropriate subscriber model in the future, but cleaning up the surrounding code in this way will allow us to eat this elephant in small pieces while moving in the right direction. I think, a similar thing will happen to the rowingMetrics and HRdata: first make sure the underlying code and its responsibilities are well-placed (removing server.js as the central decision spot for deciding when something is relevant for a recieving feature), and then move to a subscriber model there as well.
I noticed this with the RowingData file just now as well (did you mean to say that instead of Raw file, or do we have two issues...). In real time there are no issues (I ran the old and new recorder next to each other for five sessions). I'll look at it more detailed, otherwise I'll make the RowingData recorder also a record first and write later solution (as is the current one). I liked the idea that the data is always written directly, so might the app crash, you still have your metrics. ps: I recommended the ESP32 version you maintain to a Concept2 owner (see https://www.c2forum.com/viewtopic.php?f=10&t=207088) |
Beta Was this translation helpful? Give feedback.
-
Sorry, yes, I meant the RowingData file.
wow, thanks appreciate it :) I have to push out some fixes there (the flattened slope of slope concept does not work if there is not enough data point :), I guess I had to try to be cleaver :)). |
Beta Was this translation helpful? Give feedback.
-
|
I will have to think through how efficient code review can be done here :) there are a lot of changes to certain parts. I think I will start with the recorder at some point. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @Abasz , great idea. Some ideas are complex to explain, but easy to show. What I do propose is that I'll bring the branch to the last known state on my side. I'll do that tonight. Last week I've been playing with both the WebServer and some parts of the peripherals as I am also cleaning up code. My idea is that RowingStatistics will create a continuous stream of metricsUpdates, and all the features will receive them as is, and filter it themselves. I am moving all the decisions made by RowingStatistics and Server.js to the clients (ideally at the same level as the commandHandler, but given the complex scope of the PeripheralManager, that would be unrealistic), so one can easily see when a client should report something, from the code of the client itself (instead of all layers on top of it making some decision somewhere). To do this, I am removing the message-type base structure, and I've added a "metricsContext" object to the rowing metrics with some binary values (so for example, metrics.metricsContext.isDriveStart can be tested). Another modification I'm taking along is that I now have the HR reporting by the Peripheralmanager as single source for that data. So I moved sanity checking to that part of the peripheralmanager as well. And the WebServer and recorders now receive HR data directly, and can handle it as they see fit (for example, the GUI could decide to always update HR data when it comes in). I think you might have some better/cleaner approaches to some issues. For example, I'm still not happy about how I implemented how the GUI tells peripheralmanager that the state has changed, and then reports it back to the GUI. Either I replace the 3 individual commands by a single "ConfigRefresh" command, or I let the GUI change the config and let peripheralManager refresh on that. And I'm playing with the idea that perhaps you want to extend it so that people can off recorders as well (which could be done dynamically now as well, although the GUI for that is missing). I am open for better approaches to that. I've looked at the peripherals, and that is pretty complex to sort out. I've played with the PM5 Service manager, which I might put in the branch, or otherwise publish here in the discussion, as it shows the filter structure. A tricky one is the Strava uploader. It is a weird beast that probably has to be split up as it does GUI things and directly talks to the tcx-recorder. As I don't use Strava, I have no clue what hoops @laberning had to jump through. But that one is for later concern. |
Beta Was this translation helpful? Give feedback.
-
Sure, take your time
Yes that is a very plausible idea actually. Actually, back at the beginning of the year this was one of my proposed approach (I would have used a library, rxjs, that is specifically built for managing filtering, manipulating streams of data, but there is a learning curve to that library for sure). The pluming is already done with the handleCommand function. I dont fully understand the binary nature of the metricsContext yet (I suppose I would need to see code for that :)) but I would think about using specific enums like: driveIsStarting, inDrive, driveIsEnding etc. Note, I have not thought this through so this may not be a feasible options, But having the context with several true/false values will be hard to read while the message that one may watn to send is that
I agree, this will clean staff up. I think this is also inline with the single responsibility principle that is being intorduced for the rowing statistics.
" can off recorders as well " what does this mean? I am not familiar with the term. the rest I agree. (assuming I understand you aim correctly). Normally communication with GUI and server is bidirectional as the GUI needs confirmation that the server processed the request. so in normal HTTP request flow, you make the HTTP call and then based on the response code you update the GUI (change the value, indicate error etc.). Its just a matter of consequence that certain gui actions have been implemented via the websocket connection that is not a direct channel, rather the message bus that has all sorts of messages so both side need to filter. Actually I would be much happier if the GUI would get metrics via WS, but would use standard HTTP api endpoint communications for sending changes. Its simpler and cleaner. If we are changing this part, I would go down this road. As this would create direct communication line that is fully separated from the metrics with the backend and can have immediate reponse. Of course the peripheralState (i.e. ble on, ant+ off etc.) would still need to be sent as part of the message but on the back end there will be no need for filtering for commands. Once you done, I will add an example code. it will be cleaner, much cleaner. when you make a request to a server on the GUI you do not update
I dont use strava either. But from the code the point there is that it uses the browser to authenticate with strava get the token after that the only gui staff it does is sending the command to upload. the actual upload is done on the back end. This is a great example of moving this out from the websocket and have a separate http call to the back end to trigger upload. when the upload is clicked the gui can authentiacte and send the token as a JTTP post request for upload and server does the upload. it does the same auth process and can send the token via a normal HTTP Post call. Currently the strava upload is a bit messy as the flow is inefficient:
But actually these steps can be simplified or streamed lined rather, greatly if the order of the steps are changed:
So there is no more messing with ws commands. Honestly, in the GUI the WS data stream can be only from server to GUI. There is no real staff that would really require to communicate staff back on the WS stream as all things on the GUI side is tide to specific user actions. EDIT: As a note, the HTTP request-response pattern is well suited to operations that require synchronization or that must act in a serialized fashion (much better than WS). These are events when user clicks something and expects an action to be performed and signal success or failure on the GUI side (which can take specific action). this is due to the fact that the HTTP response represents a definitive conclusion to a specific request, allowing subsequent actions on the client side. In WebSockets, this detail is left up to the messaging layer design. The WebSocket protocol offers no guarantee a message will be acknowledged in any form so that is way we need to wait and actually make a |
Beta Was this translation helpful? Give feedback.
-
I was able to upload it. Please realise I copied by hand as my code is still a bit experimental, so if it doesn't work upon download, please say so as I might have forgotten to copy some code :).
I agree, this is not the final form. I see moving code to the place where it belongs, aligning responsibilities, is the first step. Then indeed a library like rxjs could easily be fitted on top of that. This approach allows us to take smaller, more manageable steps towards that goal.
The key issue @laberning ran into is that some states overlap: typically the start of the session is also a the start of the drive. You also see this in interval ends: the interval can coincide with the start of a drive or recovery. Some consumers of the metrics only want the "end of drive" signal (WebServer and PM5 for example), others like to see an explicit interval or session end (see the tcx and RowingData recorder). The binary structure allows RowingStatistics to send both flags, and let the consumer handle how it sees fit. When you look at the updated WebServer, you can see how it plays out: function presentRowingMetrics (metrics) {
if (metrics.metricsContext === undefined) return
addHeartRateToMetrics(metrics)
switch (true) {
case (metrics.metricsContext.isSessionStart):
notifyClients('metrics', metrics)
break
case (metrics.metricsContext.isSessionStop):
notifyClients('metrics', metrics)
break
case (metrics.metricsContext.isIntervalStart):
notifyClients('metrics', metrics)
break
case (metrics.metricsContext.isPauseStart):
notifyClients('metrics', metrics)
break
case (metrics.metricsContext.isPauseEnd):
notifyClients('metrics', metrics)
break
case (metrics.metricsContext.isDriveStart):
notifyClients('metrics', metrics)
break
case (metrics.metricsContext.isRecoveryStart):
notifyClients('metrics', metrics)
break
case ((Date.now() - timeOfLastMetricsUpdate) > config.webUpdateInterval):
// Normal metrics update, only config.webUpdateInterval ms after the last broadcast
notifyClients('metrics', metrics)
}
lastKnownMetrics = metrics
}Essentially, it checks the various conditions in a specific order. I'd agree that this could be made more compact quite easily, but I used it as a template for all places, so I like its ease of recognition for now. But I'm more than open for improvements, as the current binary implementation is a bit clunky. Only thing going for it is that it works and it provides me the room to push forward the changes in responsibilities.
Sorry, missed a word there. I meant to say "Turn off recorders". I think some things should be doable from the GUI, like your implementation of turning on and off the HRM, Ant+ and BLE functions, which is extremely practical. I think one of the things that a user should be able to enable/disable from the GUI are the various recorders. Current tile is a bit too small for that, but it could be a future option for users.
Interesting. I don't know why @laberning chose the current implementation, what his considerations were for the socket-based approach. Speed would come first to mind, but my impression is that it isn't that traffic heavy (and I run at 75ms webUpdateInterval).
Yeah, it confirms my suspicion that stuff was bouncing back and forth, where authentication should be pretty straightforward. I assumed that Strava required some display of a 2FA token on the GUI (QR code), but if it is just a simple token being picked up by a simple direct call, than it is overkill.
I have the impression that @laberning was worried a lot about performance and load, which lead to some suboptimal design choices on hindsight. I don't know how this performs on a Pi Zero, but on a Pi4 I can easily stream all data to all consumers (with BLE and ANT+ working, and all recorders working). Even a simulation with the not-real-time flag flies by. |
Beta Was this translation helpful? Give feedback.
-
I think at the time it was a simple solution, but the code base passed that and actually now it is re-implementing a request/response pattern within the messaging layer of the WS that is just making clutter. For changing peripherals speed is not that relevant. I cannot really thinkg anything time critical in the GUI. So I still stand by by using this dual approach (i.e. http for gui actions and ws for sending data). We need to include nodejs express library for this but that is very much standard.
This can be done from the settings page. There was a request from one of the users to move these things there and I think it makes a lot of sense. I had a high level review of the recording manager and I would approach this from a slightly different angle: currently there is a great amount of duplication with the handleCommand implmenetations as the individual data creators receive the same data rendering, in my view, upper layer manager redundant. I think the recordingManager is basically a controller, a router that handles incomming request and orchestrates things. But in the current approach the lowsest level handlers (the actual recorders) are fully aware of all the staff so actually the recording manager can be skipped. I mean there is really no reason for the lowlevel data writers to be aware of the full picture. Those should be services that do staff based on specific instructusions. I would picture this as these services are the pupets and the puppetier is recording manager that calls init if recording is enabled, calls the specific write functions, save function etc. I think these low level services should have no notion of rowing phases and things like that. They should know how to record the specific data and the data should be passed to them. Also the recordingManager handlecommand is basically a flow through function. I think this flow would make the whole flow much simpler. |
Beta Was this translation helpful? Give feedback.
-
That is my idea as well.
Interesting perspective. I tried to keep the RecordingManager as simple as possible, as life will complicate things along the way. One of these more complicated things that will come is that I want to upload to RowsAndAll and Intervals.icu automatically as well. RowsAndAll can handle RowingData files, but also tcx and fit files. intervals.icu accepts tcx and fit files. So the "Upload" command will require some logic at the level of the recordingManager to decide what to upload to who and when (wait for HRR data?). Especially when we could also create fit files. So in the end, the recordingManager will create the recordings, but also handle upload of these files and the playback of raw recordings.
I'll look into it. It is an interesting approach, I'll implement it and see what happens. But I do see some current and future needs that might complicate things to take that approach. Current implementation is basically a reshuffle of the old WorkoutRecorder with some minor improvements. In the current implementation, the reactions to the MetricsContext and the resulting data structure is indeed kept identical (as it all originated from the same WorkoutRecorder). I'm not certain how this should work when implementing the notion of intervals and pauses in the recorders as well. In tcx, an interval can split a stroke, and the boundary of an interval must be recorded explicitly. So, strokes are an attribute of an lap/interval, and the interval has its own dataset of metrics (totals). In the current version of RowingData, the interval is an attribute of a stroke, so there is no semantics to record the boundary of a interval (might be coming, but isn't there yet) or any associated data. So in the short term I'm not certain it would value. One could simply record everything (perhaps skipping metricUpdates you know for sure you can skip) and then hand it over to the recorders to write it when needed, but then you are basically doing the same thing the current case statements are doing, in two steps with no additional value. The logrecorder will be updated with a more finegrained control (this is one of the reasons to create the current structure): for example recording a "stroke 0" in the log at the start of a session is confusing at best (as technically, no stroke has been completed yet), but providing better info reduces user confusion. So using the MetricsContext structure, we can now report different messages for these situations (and similar the transition to new intervals, etc.). This will contain handling of situations where there is a start of the drive at the beginning of an interval, etc.. Aside improving logging, it is playground to understand what to report when, which is the basis for the tcx and RowingData recorder. Another step I'm considering is rewriting the tcx and RowingData recorders in such a way that the datastructure can be automatically extracted directly from the recorded strokes. Putting data in a structure with the right structure and names might simplify the recorders further. But that is an improvement down the road. |
Beta Was this translation helpful? Give feedback.
-
|
Jaap, I have been looking at the recorders and it occured to me that I dont fully understand the "business needs" for all of the recorders e.g. the raw and rowingdata recorder does saving/creating file. Then further down you are implementing a guard that do not create if it has already been created ( My understanding of the flow of the rowing statuses is that there is a set of states that happen (e.g. before stop we always have pause, when we hit reset we will have pause (and or stop). So I would try to find a common event when file is created as that would make life simpler in my view, but I am affraid that I dont fully understand the flow of state changes. Also I am slightly confused when is done what. In previous code I saw an instant writing of the raw and rowing data files but now it seems it is written to memory. So would you mind briefly summarize what are the specs of this feature? Also I noticed that you are doing some resetting in the reset event but on on stop and pause. But for instance when one stops, the rowingdata is written to file. But since the memory is not resetting when one restarts then stops again, the full memory is rewritten in its entirety to the file (effectively duplicating stuff in the csv). I appreciate your help. |
Beta Was this translation helpful? Give feedback.
-
That is in fact one of the big issues here: there are multiple scenario's where a recorder should start writing (state transition diagram can be found here, which explains the state transitions for RowingStatistics). And users can always force a stop or reset via the GUI, even midstroke. So you get some scenario's:
This is one of the reasons I like the centralized state management and command management: it makes much clearer who can trigger what and when. There are effectively only two entry points to both the rowingengine and recorders. The 0.8.4 is way too fuzzy to get a clear picture what happens when and why.
Initially we recorded directly to disk. I still want to do that, because it makes sure you'll never lose much data regardless of any issues that ORM might have. As a long distance rower, I've seen the reaction of people when they were on a halfway break on a full marathon, and the C2 PM5 timed out and reset (losing 21K of data...). Let's say the PM5 can take a lot of physical abuse for a reason. Somehow the direct to disk recording stopped working well (we used this in the earlier versions of ORM, and AFAIK only the last version introduced the write to memory first approach for raw). Both RowingData and Raw should be easily written directly to disk, but when you turn on the simulator without the real-time option, it becomes a mess. The raw file got delayed writes, so the order of the recorded delta's was mixed up, and the RowingData missed the first 60 rows. as my main priority is good recordings, I reverted to the 0.8.4 memory based implementation for the moment. But for me it is a goal to make it write to disk directly if possible as it increases robustness of the app, even under crash conditions or a loose power connector of the Pi (had that one on a long row....).
Yep, but as the filename (as set by the recordingManager) isn't changed, so it will overwrite. So when somebody pauses, the file will be written to disk as we have no clue if the human has paused or that he quit the session entirely. So there always is the option he can continue. When the human continues, ORM will indeed add the new data to the existing dataset and when the time comes it will be written to the disk again, now with the additional data but with the same filename, effectively overwriting the existing file. Here some additional work has to be done, both for forced pauses (as part of a training schedule) and spontaneous pauses (rower stops moving for a while) should result in some recording of stop and start times by the recorders for their files. For tcx it even should result in aggregating some additional data as well (as laps have their own bookkeeping). But I need to dive in how all these things are defined, what Garmin normally does and what it means for ORM to administer. |
Beta Was this translation helpful? Give feedback.
-
|
on a different note: in the docs you mention this: https://github.com/JaapvanEkris/openrowingmonitor/blob/b7c72213d6c2d462a2e08d6d9f360678b684d3a9/docs/Architecture.md?plain=1#L209 I am not sure I can fully follow this explanation. You are injecting config as a dependency into the features. which I think is a very good approach. If you do not want all parts of the application to see changes instead of mutating the object you can create a deep copy (I generally use the If the intent is to have the config object behave as a service (or rather a store of state of the config) I dont see much of a regret or a downside that cannot be cured as there are only two major problems with this specific implementation in my view: when you inject a pure data object instead of a service class, it makes every consumer a "god" enabling them to do what ever they want. They can remove, add properties, they can act without control. The above while not ideal is less of a problem then the other issue that is since this is a pure object you cannot indicate/detect changes in the consumers of this dependency. This means that when a consumer changes a property, specific code (a function) is needed in every consumer that either queries the config object or the consumer changing the property needs a hacky way to tell others that hey, I changed something (which would be server.js as the top level function). The solution is extremly simple and both can be sorted with one change: instead of injecting the pure config data object a singleton config manager service (with getter and setter functions) should be injected as dependency with helper functions to allow tapping into changes. This would enable direct communication between components without the need for server.js routing. Of course the request for changing a config property will still need to flow through server.js as that is a command but then there will be no need for every component to implement an indication function when it makes changes to the config in my view effectively simplifying the flow So when peripheralManager changes the ble mode, it calls the configManager's Merging this with the HTTP request response pattern, when the user clicks change, we make a HTTP request and indicate the request for changing the BLE mode via the appropriate method (this is already done), then create a promise that resolves when configManager's Actually once I understand your intent slightly better I can draw up some code to show this, I think that would make it probably easier to digest. |
Beta Was this translation helpful? Give feedback.
-
One the one hand it is. And I like it a lot as we can adopt the idea that the config can tell the current config of ORM (and not just the state at the start). As an invariant that config and app are always in the same state, which seems like a good idea to me. On the other hand I constantly have the feeling I'm playing with a hidden side-effect: we talk to the PeripheralManager, and as a side-effect the config changes and the rest should pick that up. We trigger it explicitly by sending a signal to the webclient. So it is more a maintenance issue, as it might obscure how things behave. But making that more explicit might help here, and using it consistently as a pattern across ORM might help here as well.
I agree with you here on principle, but in practice it can easily be managed in checking the code of the features before promoting it to the main branch.
To me, that sounds like the cleanest approach I'll hope to arrive in the end. How to get there, no idea yet. But you might have.
Saw a new toy :) And I like to draw schematics instead of writing :)
Hope this helps. |
Beta Was this translation helpful? Give feedback.
-
I will draw up some code next week to show an example |
Beta Was this translation helpful? Give feedback.
-
|
I move the conversation on the architecture data layer design here where it belongs so everything is at one place: #167 (reply in thread)
Based on my experience since we last discussed, I also agree that pure pushing on every DT is probably the way to go. I was originally against the caching because it leads to code duplication and the need for filtering logic and its not the most performant as there will be a lot of unnecessary broadcast. Also it forces the subscribers to track the changes of the metric context to track changes to it. But where the cost of such performance loss and increased memory need is acceptable (which in case of the Rpi it is) probably this provides the biggest freedom for peripherals. Also, having the metric context distributed with every data broadcast the subscribers will have all information to decide what they need to do. My only design issue now with the bidirectional communication where the clients sends some command (like start, stop shutdown, etc.). I think client decision should be purely on the metrics context and not on these commands directly (these are the switch statements you included in one of the proposals). Actually only the engine and the originator of the command should be aware that a command was sent. This would allow simplification of the data streams that there is one stream for data and the related context which goes out from engine and one callable command function on the engine that goes inwards and then the engine decides what to do with the command and make the necessary changes and send an updated metric context accordingly. Now of course commands that are not engine related (like shutdown or strava upload) should not go back to engine but rather handled by a separate service (like AppManagement may be?). This way responsibilities would be cleaned up. |
Beta Was this translation helpful? Give feedback.
-
The trick is that in the proposed approach, the RowingEngine or the SessionManager will raise the appropriate flags (i.e. isMoving, isDriveStart, isPauseStart) in one central place. The consumers see the stream and can decide what they keep and what they ignore based on their own set of rules. So far, I haven't seen issues there (I already modified server.js to mimmic the old approach to the peripheralmanager based on these flags).
I agree we should have a clear responsibility here: commands like 'stop', 'pause' or 'resume' should clearly be handled by the engine alone, and the clients should ignore these. Looking at the current code, it doesn't take that approach, with possible some double action on a forced stop from a client, which is undesireable. So I agree that is an improvement. However, there are some housekeeping commands like 'reset' and 'shutdown' which must be seen by all clients. For example, the recorders all see this as a trigger to instantly write all data to disk. As this might be caused by a crash, it could be a hail-Mary attempt to rescue all data from the session. The peripheralmanager should use it to shutdown its claims to devices. The recodringmanager uses the 'reset' command to reset the starttime of the recordings (as that timer is triggered by the first currentDt reported, and not the first report from the engine, as it awaits filling up the flank). Current code in the recorders are placeholders, just to make sure I don't forget to handle any possible commands. These unneeded commands probably will be mapped as a single option, or to default in the production code. I already cleaned up some, as the peripheralmanager already has a command mapping that includes the ant+ and BLE mode changes in it, where all other clients already ignore these. So I'm not consistent, and some further cleanup of unneeded code is indeed required.
Yes and no. I agree with the single data stream. I think the callable function on server.js just broadcasts the commands to all managers, and some will react, others will ignore or wait for the official response of the engine. The clients have their own responsibilities, for example writing to file and cleaning up after themselves after a 'shutdown' or 'reset'. When looking at the recorders and peripherals, I can imagine them completely ignoring 'stop', 'pause' and 'resume' as the engine will tell them that, but they will listen to the 'shutdown' and 'reset' command as these typically are given in situations where the engine might have died.
The Strava upload is a weird beast, and its implementation has a lot of room for improvement. I think the approach like I implemented in the server.js and peripheralmanager of the architecture branch is pretty straightforward: any command given is told to all managers (peripheralManager, recordingManager, sessionManager) and they will handle it, and decide if they tell their clients. Recordingmanager just multiplexes the relevant command to its clients, which will handle them as they see fit, but also chooses to ignore quite a lot of them. |
Beta Was this translation helpful? Give feedback.
-
You are right here. I have not considered these requirements actually.
which branch should I look at? is it the architectural revision? |
Beta Was this translation helpful? Give feedback.
-
See https://github.com/JaapvanEkris/openrowingmonitor/tree/V1Beta_Architecture_Revision , and more specifically https://github.com/JaapvanEkris/openrowingmonitor/blob/V1Beta_Architecture_Revision/app/server.js , where
Server.js will become pretty straightforward if all consumers have the same structure, and data and commands can just be forwarded. |
Beta Was this translation helpful? Give feedback.
-
|
I cleaned up the code, as we discussed: https://github.com/JaapvanEkris/openrowingmonitor/tree/V1Beta_Architecture_Revision I moved the control over when to broadcast what to the peripherals. Even the PM5 works now. Only ones not tested are BLE CSC and CPS. Some things to do:
|
Beta Was this translation helpful? Give feedback.
-
|
I get a Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'openrowingmonitor/app/recorders/WorkoutUploader.js' imported from openrowingmonitor/app/server.js I think the WorkoutUploader.js file is missing. When you commited into Archivetcture_Revision did the project run? EDIT: I found it it was moved to engine just the path has not been changed in server.js. |
Beta Was this translation helpful? Give feedback.
-
Yeah, that sums up my relation with Strava upload quite well. In my setup it is commented out as I still haven't been able to position it well in the new architecture.... |
Beta Was this translation helpful? Give feedback.
-
I think HR is not in rowingstatistics but rather in SessionManager. I started looking at the structure, and all in all I think this is a good setup. SessionManager is essentially became the central data store. Though I could find ways to split it up a bit (like separating the general data function and the actual session management, split monitoring etc.) as it still does too many things but things are already cleaner, I think. In the sense I see a pattern where I would not actually move HR out but rather have every other data consumers depend on it (sessionManager). I have not made my mind up whether this is better structure as in this case an additional broadcast would be necessary (i.e. breaking the one data per impulse pattern) on every new HR data and probably indicate via metric context that this is a new HR data (but then probably two new context would ne necessary like, In addition now that we broadcast on every impulse, actually this is redundant: function handleRotationImpulse (dataPoint) {
recordingManager.recordRotationImpulse(dataPoint)
sessionManager.handleRotationImpulse(dataPoint)
}Here In terms of todo I would like to do the below things (you did not leave that much :)):
Do you have anything other in mind? |
Beta Was this translation helpful? Give feedback.
-
True, but it is a hassle. I think the webclient and PM5 already accept a seperate stream now, it is just checking that the recorders also handle it well.
I think that allowing different streams (HR, rowingmetrics) to coexist is easier, as I think there are some cases where you want to add additional data sources (like air temperature, CO2 level, etc.), also being relevant to some consumers.
It is a safety measure. If the sessionmanager crashes due to a specific currentDt value, it is still recorded and written down, allowing it to be replayed.
Good idea
Apart from the distance and time, metrics don't change in-stroke. Some change on theend of a drive or recovery. I can't speak for CPS and CSC, but FTMS works well now when connected to EXR. I don't have the impression it results in spiky charts.
I'll take a look at the webserver and recorders to see on what datastream they depend. |
Beta Was this translation helpful? Give feedback.
-
Please note: I'll probably make a new branch this weekend to make the production version of the Architecture revision. The current Architecture_Revision is based on the main branch, where the Regression_Completeness should be its base. Rebasing is something that probably causes issues (as RowingStatistics is modified for both Regression_Completeness and Architecture_Revision). |
Beta Was this translation helpful? Give feedback.
-
|
By spiky I meant the below on the pace: This is due to the way distance information is passed in CSC and CSP. You are right, that the FTMS does no suffer from this pace issue as pace there is specifically sent by the central (i.e. ORM). while for the other two this is calculated by the client based on distance and time data sent. The spikyness comes from the fact that ORM braodcasts the new distance in every second which includes an acceleration and a deceleration part. I over came this in ESPRM by only updating the underlying data on every completed stroke or 4 second. This means that one get an average pace for that stroke. This mirrors the way ANT+ works on the C2 PM5.
I agree, leaving HR in a separate stream is probably better. Though I would remove it from the metrics object completely so consumers would need to chose the separate stream. That would make things cleaner on the long term.
Actually that is a good idea! Can I ask you to squash the commits to into one or two commits when you add it to the main branch? Reason is that it is very difficult to see what has actually changes that is relevant when its split into 20 one liner commits with the same commit message :) |
Beta Was this translation helpful? Give feedback.
-
Good point, I'll change the update mechanisms of the metrics to filter on that.
I'll look at all the clients and see where their HR originates. I also have a small task of improving robustness of metrics. I changed startup behaviour slightly (to resolve an issue with startup noise of a very slow flywheel triggering a start), and prevent weird strokerates (as the first stroke is picked up pretty late), so for some metrics the clients can recieve an 'undefined' in those cases.
Yeah, not to forget the mess that a small change in startup behaviour caused :(. I'll check the clients and squash the result to a new branch that is mereable with the other branches. I want to keep the engine changes and architecture changes seperate, as the engine changes are well tested, and can be merged into main in a couple of days (just needs some time on my test machine). I haven't been able to test the architecture revision yet. But both will be squash merges. |
Beta Was this translation helpful? Give feedback.
-
I added logic to the CSC and CSP implementations: they will now only update metrics when a stroke or recovery ends, or when the sessions tends to stop. You might want to finetune that behaviour.
That is still on my ToDo list.
Done: they are squashed in a new branch https://github.com/JaapvanEkris/openrowingmonitor/tree/V1Beta-Architecture_Revision As an aside, "Regression_Completeness" has been squashed into "Main" already. So when we complete and test this branch, we can directly push into "main" as well, completing V1 (finally....) |
Beta Was this translation helpful? Give feedback.
-
|
@Abasz I have introduced the equipmentstate transitions into FEPeripheral.js based on this description of its behaviour. I see some pages seem to be missing, and we might need to make the lap bit an individual bit. Any thoughts on this one? |
Beta Was this translation helpful? Give feedback.
-
Thought the same, this is the second run and the watch was on the top of the Rpi :) Disconnect happened around the same time. I will do a real row and check what is going on there.
I did a clean rebase on top of your changes. No conflicts. thanks |
Beta Was this translation helpful? Give feedback.
-
|
Did more tests. No more dropouts (zero speed is when I stopped actually). 2x500 m sprint. Bonus, this is KayakFirst Blue machine. I will share the rower profile too.
The only thing I dont like is the spikes at the beginning when rowing is restarted. I think it relates to how start and stop is handled (CSC uses total moving time which does not increment in the stop state but not sure, this is a bit cumbersome to debug as I have to replay longer sessions ). I will have to tackle that during the weekend. |
Beta Was this translation helpful? Give feedback.
-
Handling the isPauseStart and isPauseEnd flags might help, as it broadcasts the latest state (this is how ANT+ handles it). Could also be that might mess with this. I am considering changing all their initialize actions to 'undefined', but I have to look for side effects there. |
Beta Was this translation helpful? Give feedback.
-
|
I am a total idiot... I used The issue with including So for instance the last distance data at the pause is 77.63046496076602 meter, at 30.65082099999999 seconds. Of course the difference is too short to calculate any data reliably since this could include measurement errors. The speed here is 34kmph :) which is an error of course: Theoretically having
|
Beta Was this translation helpful? Give feedback.
-
|
Alternatively, it would be possible to remove the small spike too by going with EDIT: Just for reference, this is how the same spike looks with
There is only a very subtle bump. Considering that I used to have speed update at the end of the drive for me having feedback only before starting a new drive is a bit strange, because at the recovery its more comfortable to look at metrics and now when I look at a metric during the recovery it is actually lagging behind and showing the metrics related to the stroke before. Hence I would go with the middle ground i.e. |
Beta Was this translation helpful? Give feedback.
-
|
I pushed the updated code to my fork https://github.com/Abasz/openrowingmonitor/tree/V1Beta-Architecture_Revision If you are ok with the changes I will create a pull request to the V1Beta-Architecture_Revision branch. Please note that I went with the isRecoveryStart option as it was very annoying in practice that data on the watch was so much behind (it lags by default any way and having a full cycle delay made things even worse). |
Beta Was this translation helpful? Give feedback.
-
This needs to be split:
These can be included but essentially these will be included when the 4 second timeout is reached any way so essentially it will not make a difference because if there is a broadcast at sessionEnd/pauseStart there will be a value displayed for approx 4 second after that anyway. Here are the output of the console for the only
output-onlyIsRecoveryStart.txt If you put them into a diff, you can see that there is no difference (which is totally expected in my view). There is one exception from this, and that is when the 4 second timeout does not elapse and the rower restarts (and completes a drive as well) before the final metric is pushed to the client. In this case the speed in the Any way I can include these endings no problem. Actually I will :)
If I include these there will be the spikes. Reason is that in some cases there is a very small difference between in the last data and the data broadcasted. I suppose this due to the fact I explained above (namely that the totalMovingTime and totalLinearDistance will be very short and any measurement errors will be magnified when calculating speed). I added an output log where you can search for
ANT+ works slightly differently as it uses the pace value calculated by ORM and does not calculate its own pace at the client. This means that the errors in the different totalMovingTime and totalLinearDistance will not be visible in the pace shown. In CSC and CPS the client calculates the speed based on the diffs of the number of rotation (distance in our case) and the time elapsed since the last calculation in line with this formula: |
Beta Was this translation helpful? Give feedback.
-
For normal setups I'd agree. But some setups have a shorter (3 second) timeout due to their extreme drag.
Great!
That explains that. Let's include sessionStop and pauseStart, and see what happens in the field. |
Beta Was this translation helpful? Give feedback.
-
|
done |
Beta Was this translation helpful? Give feedback.
-
|
The PR is more than welcome! By The Way: my daughter recruited some user interface design students to make a better GUI. Hopefully they will deliver soon, and that will be V1 :). |
Beta Was this translation helpful? Give feedback.
-
Awesome!
I will do it tomorrow |
Beta Was this translation helpful? Give feedback.
-
@JaapvanEkris I moved this to a new discussion to keep the other separated as this is a completely different topic
I have not noticed the top comment but now I read it, its a good assessment :):)
What you are saying sounds like an eventbus, but I haven't given much thought to this honestly just felt when I edited the file for the graceful shutdown. I like to use a main method as an entiry point and use that to wire up things (making it responsible only for wiring up dependencies for services and inject these services into the classes). But this is probably due to the fact that I originaly came from C# and dotnet core and used angular on the front end only.
For me an app at this scale is hard to oversee with its event based approach (not to mention the error prone nature of the string event names :)) but refactoring this app away from events to dependency injection and direct method calls in these singleton services for instance would be a pian (if even possible).
Altough I need to think about this more, but I think we have 3 mjour parts:
Based on the above, I beleive that the server.js should include (esentially from a public API perspective these are very similar in nature) only direct communiction between these 3 parts on a high level like: control event was fired and this is relayed to the other two so it can decide what to do with it, but the logic what sholud be done should be within the feature part.
I would add a layer on top, the main method (we can call it main.js :)) that is reponsible for wiring up things (starting these parts of the app, manage their and the app liftime etc.)
But this is just a quick thought, I would need to draw a chart to see what parts we have and what communication we have between each other.
There are simple things that can be done like you have peripheralManger:
So here we fire a control event then we bubble it up to server just to carry out an action on the periopheralManager (notifyStatus). This is unneccessary (I could have refactord this actually but I did not). or the event.res= true: not sure what that is but I left it in because did not want to break something :)
Summary (sorry the above became too long):
EDIT:
+1
It would be possible to build a central eventbus that holds/aggregates all the data and keeps state and notifies subscribers on changes of this state/data. So instead of having several different event types only a hand full would be remain (just out of my head I would can see about 3, maybe 4: dataChanged, stateChanged, controlAction, and maybe UI event can be separated out but I need to think this through) and clients could subscribe to one of these and receive notification on change and act appropriately. The main eventbus could be injected into every part a singleton and can call update method related to the events.
Beta Was this translation helpful? Give feedback.
All reactions