Skip to content
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

Open
hercek opened this issue Jan 24, 2024 · 10 comments
Open

add averaging on cycling power and cadence sensors #1839

hercek opened this issue Jan 24, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@hercek
Copy link
Contributor

hercek commented Jan 24, 2024

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 and AggregatorCyclingPower. 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 the reset() 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

  1. Putting weighted average into a separate class 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.
  2. 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.
  3. Precisely weight BLE data time intervals onto track point data time intervals. This would be as precise as possible but is it worth it?

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.
avgSample

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.

@hercek hercek added the enhancement New feature or request label Jan 24, 2024
@dennisguse
Copy link
Member

If you have the time, go ahead.
It would be great if you have the resources to also account for TrackPoint boundaries (can be determined by reset() calls).
You can also pass in the TrackPoint.

This also affects heartrate as well as running cadence.
It would be great, if you could take this into account.
Suggestion: let's start with power and see how it works out?

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.
It would be pretty tricky to maintain this kind of code without test coverage.

Duplicate of #1029.

@hercek
Copy link
Contributor Author

hercek commented Jan 26, 2024

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"

@dennisguse
Copy link
Member

Null problemo :)

You can run ./gradlew connectedCheck or start them via AndroidStudio:
https://developer.android.com/studio/test/test-in-android-studio

Tests will be executed either on a connected Android device or an emulator.
In the code base, you can search for @Test for inspiration.

PS/ just open a PR, then I can support you.

@hercek
Copy link
Contributor Author

hercek commented Jan 28, 2024

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.
fail.log

@dennisguse
Copy link
Member

@hercek do you use JDK8? If yes, then please upgrade JDK11 or above. That should fix the problem.

@hercek
Copy link
Contributor Author

hercek commented Jan 29, 2024

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:

Execution failed for task ':compileDebugJavaWithJavac'.
> Java compiler is not available. Please check that /usr/lib/jvm/java-21-openjdk contains a valid JDK installation.

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.
fail1.log

@dennisguse
Copy link
Member

These results look okayish - I only execute UI tests on a physical device.

PS/ are you using AndroidStudio?
That would save you quite some trouble..

@hercek
Copy link
Contributor Author

hercek commented Jan 29, 2024

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 :-/
I have version 2023.1.1.28-1 on Arch Linux.

@dennisguse
Copy link
Member

:D

image

@hercek
Copy link
Contributor Author

hercek commented Jan 29, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants