-
-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add averaging on cycling power and cadence sensors #1839
Comments
If you have the time, go ahead. This also affects heartrate as well as running cadence. And I am not sure how to extract this functionality in a generic way - it might that this is not that simple and may be done afterwards. And one more thing: the implementation needs to be covered with test cases. Duplicate of #1029. |
OK, so how do you run tests on OpenTracks? Notice, I'm a Java and OpenTracks noob :) I do not know how much of this will be done by me and how much it will stay unimplemented or done by somebody else. As for as I'm concerned, it depends how much it will be fun and how much it will be chores (in the rather limited Java language). I can provide a PR for an imprecise averaging of cycling power&cadence(*) even now. I have it already implemented in my public branch. Aside from that I will look at it more and maybe provide a (possibly even precise) generic solution along the lines as described in the alternative 1. Maybe the challenge of it will capture my attention. (*) as I specified it in the ticket without the "alternatives" |
Null problemo :) You can run Tests will be executed either on a connected Android device or an emulator. PS/ just open a PR, then I can support you. |
As for as the tests: it failed to run at all. The log is attached. If you do not know from the top of your head then do not bother. I'm postponing my introduction to java tests for now. |
@hercek do you use JDK8? If yes, then please upgrade JDK11 or above. That should fix the problem. |
Thanks, that did help a bit. I was using jre8, upgrade to jdk11 told me to upgrade at least to jdk17. I upgraded to jdk21 which led to:
Downgrade to jdk17 helped quite a bit but way too many tests are failing. Edit: replaced the log file. The original one contained errors because of my added averaging in agregators. This new one contains only the original errors. |
These results look okayish - I only execute UI tests on a physical device. PS/ are you using AndroidStudio? |
I do but I'm kind of a "command line guy" whenever I can. I did not find yet where to run the test from android-studio. I read the first 2/3 of the web page you pointed me to but the page seemed to describe some different version of android studio :-/ |
Ach, right, that is it. Thanks! I can run them from GUI now. I get 6 failures though. The clever test-in-android-studio web page asks to select Android at the beginning and then only continues with "in the project view" ... bla bla bla. No nice picture with context menu on src folder in the project view. |
Is your feature request related to a problem? Please describe.
Cycling cadence and power logs are slightly imprecise since they take the last instantaneous value.
Describe the solution you'd like
Add weighted averaging for cycling power and cadence in aggregators. I have an alpha implementation which adds this for classes
AggregatorCyclingCadence
andAggregatorCyclingPower
. This is not a totally precise solution since it does not properly divide BLE data point into the adjacent track points based on the precise location where the track point boundary lands into the BLE data point time span. This information is not easily available in the aggregators but I guess it can be obtained from thereset()
call timing. Anyway, my approximate solution is better than just taking the last instantaneous value (as it is currently done).Describe alternatives you've considered
AveragedAggregator
so that the average computation is shared. A bit more work and there are questions how much it would help: e.g. cadence average is computed quite differently from power average since the input data are different and using more native input data for cadence is more optimal.Aggregator.value
should not be computed with each BLE data arrival but only once when requested by a new track point. This would be more efficient but likely not worth the additional work.Additional context
My power meter sends BLE data about 2 times a second. Well actually I'm not sure how quickly it send the data but this is the rate how they are sent to aggregators by OpenTracks.
This typically gives me 1 to 3 BLE data points in the default track point length of 10 m. So averaging does not help that much with the default setting. The longer the track point length (which I believe is indicated as "record distance interval GPS setting") the more my patch helps. It is useful for cadence and power since these values can change a lot very quickly. So selecting one instantaneous value can lead to a significant error. Other channels could use averaging as well even when they are changing more slowly. The next candidates in the order of importance might be speed and heart rate.
Here is a sample how it looks like with 20 m long track point. The "heart beat" channel does not contain heart beat but a ten multiply of the number of samples averaged into one track point. If I provide a PR then of course the heart beat channel change will be reversed so that it works well for people with heart beat sensor. I only put it there temporarily for my debugging purposes.
I can provide PR quickly for the solution as described if it is accepted. If alternative solutions 1, 2 or 3 are requested then I may (or may not) provide a PR in the future.
The text was updated successfully, but these errors were encountered: