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

Get maximum of heart rate and use 110% for y axis of heart rate graph #873

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vanseforge
Copy link

The maximum of the heart rate graph in the detail view was set by the graphview. I changed it to 110% of the maximum heart rate from settings. Now the graph is better scaled.

Copy link
Collaborator

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If max is set, min must be set too.
However, I do not find the presentation better with just the zone max/min.
Limiting max min if exceeding the zones could be useful, to filter uninteresting bad values. Was that what you really tried to do here?
Crash as well.

Please provide screenshots as well in an updated version.

Screenshot_20200111-143244
Screenshot_20200111-145752

HRZones hrZones = new HRZones(context);
HRZoneCalculator hrZoneCalculator = new HRZoneCalculator(context);
int zoneCount = hrZoneCalculator.getZoneCount();
Pair<Integer, Integer> values = hrZones.getHRValues(zoneCount);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no zone is set, this will return null, must add a check.

@vanseforge
Copy link
Author

The goal was to get a better scaled graph for the heart rate (and always the same scaling).

I totally forgot the min. Thank you. I added also the check if a zone is defined.

ScreenHeartRatepng

Copy link
Collaborator

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The presentation if below the low zone just looks like a bug to users.
This from a walking activity, not the primary use of RU, but still a use case.
Screenshot_20200830-224746

If zones are not setup, the presentation is a little stretched out:
Screenshot_20200830-231516

Presenting zero values should be considered a bug. This is how nozone presentation look after changing that:
Screenshot_20200831-005953

This could be made a configurable, but RU has enough configurations already.
Therefore, I propose that the max/min are set to the zones that match them. This should give you reasonable comparable graphs but with better resolution. If lower zone is 0, use min value instead of 0, but crop max values.

Pair<Integer, Integer> MaxValues = hrZones.getHRValues(zoneCount);
Pair<Integer, Integer> MinValues = hrZones.getHRValues(1);
if(MaxValues != null && MinValues != null){
graphView2.getViewport().setMaxY(MaxValues.second*1.1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be set in complete()

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

Successfully merging this pull request may close these issues.

2 participants