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

Added tests for bike power functions #2174

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

Conversation

drmason789
Copy link
Contributor

@drmason789 drmason789 commented Mar 1, 2024

To finish this PR, some thought will be needed about what it should test.

I have defined the erginterface abstract class, which acts as an interface for the powerFromResistance and resistanceFromPower functionality offered by the bike class. Currently the bikeergfunctions class implements this for bikes, by setting the bike's current cadence, using the corresponding function to get the resistance or power, then restoring the bike's original cadence.

The interface supports optionally defined (min, max) x (cadence, resistance) representing the domain the bike knows about so the tests adapt to the boundaries provided, where provided.

What is tested?

  • if there is a (minimum,maximum) (cadence,resistance) defined, if the (cadence, resistance) goes (below,above) the (minimum,maximum) for a constant (resistance, cadence), does the power stop changing?
  • if the bike::powerFromResistanceRequest function returns a power p for a specific cadence c and range of resistances rmin..rmax, does the bike::resistanceFromPowerRequest function return the rmin when called on p and c?

What is not tested?

  • effects of watt gain and offset, resistance gain and offset
  • wattsFromResistance function

@drmason789
Copy link
Contributor Author

drmason789 commented Mar 1, 2024

@cagnulein is this something you want to continue with?

  • I think it would be helpful to refactor the power/resistance functions in the bike class so that are not dependent on bike state then put these functions into an implementation of erginterface (or whatever you'd like to rename it to), then make bike have a field referring to an erginterface object.
  • The power tables you have been implementing could be passed as an array into an implementation of erginterface that generically takes care of storage and searching.

@drmason789
Copy link
Contributor Author

drmason789 commented Mar 1, 2024

  • I believe I can make many tests pass simply by returning 0 resistance or 0W for a negative cadence.
  • In all the cases I've looked at, the resistance from power doesn't invert the power from resistance. I wonder if you don't intend it to.

@cagnulein
Copy link
Owner

@drmason789 Actually I was looking to finalize the dynamic erg table for the bike where I don't have a static power table first. Do you agree?

Of course the dynamic power table will be applied only to the bike with automatic resistance and without the static power table.

@cagnulein
Copy link
Owner

@drmason789 i mean this one #2175
if you want to review my code go on! (I didn't test myself yet)

@cagnulein cagnulein added this to the 2.16 milestone Mar 2, 2024
@drmason789
Copy link
Contributor Author

Actually I was looking to finalize the dynamic erg table for the bike where I don't have a static power table first. Do you agree?

Yes.

@drmason789
Copy link
Contributor Author

@drmason789 i mean this one #2175
if you want to review my code go on! (I didn't test myself yet)

Definately. I took a quick look and will have some comments about this later.

…esistance for bikes. Move some power-related functionality up the class heirarchy
- Disabled test for going below minimum cadence as there's no public way of setting a negative cadence.
- Boundary tests mostly if not all passing.
- Tests that resistanceFromPowerRequest inverts powerFromResistanceRequest still failing
- adjusted power conversion test to force no watt gain
- adjusted bike::resistanceFromPowerRequest to make it easier to see what it's comparing and to cache calculations
- many power conversion tests now passing, but some strange values remain in the failing tests
} else if (requestResistance < min_resistance) {
requestResistance = min_resistance;
if(!this->resistanceLimits().contains(requestResistance)) {
requestResistance = resistanceLimits().clip(requestResistance);
} else if (requestResistance == 0) {
requestResistance = 1;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cagnulein What's going on with these few bikes that have this form of clipping of the resistance request?

i.e. here the min_resistance constant is -20 and the max_resistance constant is 100, and you clip the requestedResistance to that range, except if it's 0 then you make it 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also in the proformwifi and proformtelnet bikes.

Copy link
Owner

Choose a reason for hiding this comment

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

min_resistance can't never be less than 0 in QZ. I mean I never encountered a bike with a negative resistance. So the current master can't handle this. That's why for inclination, instead, i'm using a -100 value as a "no action" value (because inclination can go lower than 0 (but not to -100 of course)).

did I answer to your question?

Copy link
Contributor Author

@drmason789 drmason789 Mar 27, 2024

Choose a reason for hiding this comment

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

I'm aware of the use of -1 and -100 to specify "no action" for resistance and inclination. But I don't know the purpose of the -20..100 range you're using here.

I'll use 1..100 for this exercise unless you specify something else.

Copy link
Contributor Author

@drmason789 drmason789 Mar 27, 2024

Choose a reason for hiding this comment

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

BTW, you might have noticed in some test code I've switched to C++17 and used the std::optional template. This enables you to specify something like

std::optional<resistance_t> resistanceRequest;

and use resistanceRequest.has_value() and resistanceRequest.reset() instead of having a "no action constant". If you are agreeable, I will do this for the resistance, inclination and power requests, in a different PR.

Copy link
Owner

Choose a reason for hiding this comment

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

sorry for the delay @drmason789

the -20 in this case was copied by another project that I found so actually I didn't care about it. It could be that this bike can have also negative resistance (so it will make sense to use -100 for the resistance too). I didn't remember this.

and use resistanceRequest.has_value() and resistanceRequest.reset() instead of having a "no action constant". If you are agreeable, I will do this for the resistance, inclination and power requests, in a different PR.

yes sure! thanks!

- fixed test settings / watt gain
- adjusted some wattsFromResistance functions to avoid test timeouts
- temporarily use subset of resistance values for power conversion test to avoid timeouts
Comment on lines -105 to -106
if (requestResistance > 23) {
requestResistance = 23;
Copy link
Contributor Author

@drmason789 drmason789 Mar 27, 2024

Choose a reason for hiding this comment

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

Unusual, the maxResistance function from the header returns 24.

Copy link
Owner

Choose a reason for hiding this comment

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

@drmason789 because the resistance levels are 24 but the bike wants it starting from 0. we can replace that 23 with max_resistance - 1

@drmason789
Copy link
Contributor Author

@cagnulein What is the intended difference between the functions wattsFromResistance and powerFromResistanceRequest?

@drmason789
Copy link
Contributor Author

@cagnulein Remember the resistance_t change? Would you be agreeable to me adding cadence_t, heartRate_t, power_t, pelotonResistance_t, inclination_t alongside it and propagating throughout the codebase, in a different PR?

@cagnulein
Copy link
Owner

@cagnulein What is the intended difference between the functions wattsFromResistance and powerFromResistanceRequest?

@drmason789 it was intented to use for renpho bikes but at the end powerFromResistanceRequest is a dead code

@cagnulein
Copy link
Owner

@cagnulein Remember the resistance_t change? Would you be agreeable to me adding cadence_t, heartRate_t, power_t, pelotonResistance_t, inclination_t alongside it and propagating throughout the codebase, in a different PR?

@drmason789 yes sure!

Copy link

stale bot commented Apr 18, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Apr 18, 2024
@cagnulein
Copy link
Owner

@drmason789 i added erg tables for all the bikes with the new ergtable module. Maybe it could impact also this

@stale stale bot removed the wontfix This will not be worked on label Apr 24, 2024
Copy link

stale bot commented May 10, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label May 10, 2024
@stale stale bot closed this May 17, 2024
@cagnulein cagnulein reopened this May 17, 2024
@stale stale bot removed the wontfix This will not be worked on label May 17, 2024
@cagnulein cagnulein modified the milestones: 2.16, 2.18 Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants