54 usb trainer - release 3.0 beta #84
Conversation
| # factor 289.75 gives a slightly higher speed | ||
| # factor 301 would be good (8-11-2019) | ||
| #------------------------------------------------------------------------------- | ||
| def Wheel2Speed(self): |
There was a problem hiding this comment.
I tend to dislike functions that have a side-effect (setting a member variable) where unnecessary. I saw where this function was called and immediately went "hold on... what's this doing?" Obviously I knew, but I would take WheelSpeed as an argument and return a value - and store to SpeedKmh at the calling site. It's a simple function, but it's very "coupled" the way it's written here. Splitting hairs - just an opinion.
There was a problem hiding this comment.
agree on the principle
But these functions are not generally callable but intend to calculate member-variables into member-variables and are a function to separate code into pieces.
It avoids passing lists of variables and returning a value to be put into a variable for the sole purpose of the principle.
There was a problem hiding this comment.
One could ask if it is even meaningful to have WheelSpeed as a member variable? The units we get in the response from the trainer are essentially of no use to the external interface of the class. And if the scale factor is a constant, then this function can become a class @staticmethod, not an instance method - preferring to avoid any reference to "self" if none is needed.
There was a problem hiding this comment.
We do not disagree on the basics, I think.
I read "self.function()" as function does an operation on the object self and hence modifying the attributes of self is not a side effect but the intended operation.
And re WheelSpeed; I have put the relevant attributes together in clsTacxTrainer where this one is arguable especially since not needed for iVortex. Having all relevant parameters together has it's advantage. I have decided to keep WheelSpeed to stop converting up-and-down but have WheelSpeed, SpeedKmh and VirtualSpeedKmh simultaneously available.
PS. VirtualSpeed must be based upon CurrentPower, not TargetPower and I will update that. The overall idea works, the formula's are not yet 100%.
cu!
There was a problem hiding this comment.
Such a simple function to debate. ;)
Re: VirtualSpeed, I think we are in slightly different directions with our respective ideas. I'm not quite understanding how speed can be worked out from grade and power though... are you assuming a steady state? I could be going much faster but putting in less power because I'm slowing down due to fatigue - I think you'd need to account for inertia in this case, too? Which implies some knowledge of current speed and therefore the calculation is calculating a projected speed? I'm getting too wrapped up.
My thoughts have been simply to have a deterministic (but potentially non-linear) relationship between wheel speed and (virtual) speed, and nothing more. I believe that concept alone solves all remaining issues around low-speed-high-load, running out of gears at high speed, accurate simulation, etc.
I'm not thinking too hard on it now - will let things settle and revisit some other time.
There was a problem hiding this comment.
All good.
You might save yourself effort to scrap VfietsNL now and look at simply using your powercurvefactor against self.SpeedScale, leaving the buttons doing what they're doing.
Calculations are:
- TargetPower as a function of TargetGrade and VirtualSpeed
- TargetResistance as a function of TargetPower and ActualSpeed
- CurrentPower as a function of CurrentResistance and ActualSpeed
- VirtualSpeed as a function of ActualSpeed and your factor.
Ironic how this discussion is on Wheel2Speed().
There was a problem hiding this comment.
I must say, you are fast.
Spot on, I like this solution very much. Thank you so much. I could just discard some hours of working in another direction, trying to find the Zwift/Rouvy formula, which I do not need since I know the virtual gear I'm in.
There was a problem hiding this comment.
@WouterJD I am happy you like the solution.
It was difficult to express in words, or find a way to describe it, or a real-word concept to relate it to, but the key is that for the reverse calculation, you are right - you already know the "virtual gear". I stumbled upon the idea and then the effort was in evaluating it. Sorry for writing so many words but I think it's a really great point to have reached in the software!
Feedback on it...
- 0.3 is not sufficient for low-gearing... on a 20% at 90kg+ I find myself needing more. Suggest 0.1 (for 10x) or 0.2 (for 5x) as lower limit? Upper limit as 3 is probably ok, haven't really tried.
- Personally, I don't want to touch the buttons much - I may encode a non-linear relationship into the speedscale factor so I get, hands-off, extra low, extra high, and linear middle as I had played with previously, but that said, not wanting to touch buttons too much but accepting it is the interface, I imagine the use of it will be in slight panic in-game and in response to an "oops... need more gear here quick..." then increasing the 1.1 to 1.5 might be better... meaning only 3-5 presses for hard uphills, 2-3 presses for screaming downs.
- since I am running without GUI - currently, during development I guess, it would be useful to see the factor, or the equivalent wheel diameter, or whatever metric it makes sense to display. If the factor, then 2 decimal places perhaps, though if you decide on a 1.5 step value instead of 1.1, then perhaps just showing discrete "levels"... eg -1, -2, -3, +1, +2, +3 might make more sense. I don't know.
- Basically - GREAT! Maths is simpler, true/known relationship to wheel speed, no guesswork, low-end-high-load is worked around, top end has more gears when needed, and tuning/reporting could do with a tweak
Personally I'm thinking of trying 1.5 multiplier with a 5 step limit down (factor 0.132) and a 4 step limit up (factor 5.06).
Then, given we know that the low end suffers, there might be no harm in choosing a non-linear relationship in some upcoming release... I don't know. Need some kms/% under our belts and some feedback from others.
I'm excited. Well done, Thanks, Etc.
There was a problem hiding this comment.
Feedback on my ride last night...
- having not-too-many-but-reasonably-large jumps for "button down" makes sense... eg 1, 0.7, 0.45, 0.3, 0.2 or similar.
- having fewer (smaller) jumps for "button up" makes sense - 1.5x per-button-push is too large, presumably because air resistance has such a MASSIVE impact as you increase speed... so 1, 1.25, 1.5, 1.75 probably enough.
However... even a modest grade on Zwift (with trainer difficulty 100%), riding Yorkshire with the odd 6% or 7% grade climb at 95-kg or so, I was struggling at low-end.
This definitely suggests to me that the default 1:1 power factor at low speed is wrong... we should probably default in our implementation to automatically become non-linear at lower speeds without any user-interaction... ie as you slow down, the factor drops <1 without user having to push any buttons... we shouldn't HAVE to push buttons for modest grades, we should probably at least try to mask the limitation out of the box, if not all the way down to 20%, then certainly for 7%. I don't know if there's a "VirtualSpeed" flag we can set in the FE broadcast to indicate that we're doing tricksy things with the power/speed... I guess that's how i-Vortex uses its VirtualSpeed flag.
There was a problem hiding this comment.
And regarding the buttondown factor; a shimano cassette has a 10% step (10-11-12....36-40) doing big steps will trigger the inverse critique.
I get Grade, calculate power from it.
If you are on a slope and you wanna ride away, start in a low gear.
What does "I was struggling at low-end" mean?
What required power was displayed when doing 7%
|
I guess if Grade2Power() does negative, this will be converted to a negative resistance and Fortius motor will speed up, at which point we redo calculation next time round loop, speed has increased and Grade2Power() eventually reaches steady state, where air+rolling resistance balance with gravity. The speed should be whatever the motor speed is, unless doing something tricksy with speed curves. Did you commit VirtualSpeedKmh implementation yet? Did I miss it? |
|
Hmmm... for some reason the branch is not updated. |
|
Should be better now |
|
Good work, Wouter. You've not been afraid to make changes!! Refactoring and refinement is, in my opinion, one of the most enjoyable parts of coding. I'll give it a test shortly. |
pythoncode/usbTrainer.py
Outdated
| logfile.Write ("Grade2Power; TargetGrade=%4.1f%%, Speed=%4.1f, Weight=%3.0f, TargetPower=%3.0fW" % \ | ||
| (self.TargetGrade, self.SpeedKmh, self.UserAndBikeWeight, self.TargetPower) ) | ||
|
|
||
| def __PfietsNL(self): |
There was a problem hiding this comment.
I would argue this should not be a member function of a trainer class. It is a function that implements a physical calculation, nothing specific to a trainer - not a function on a trainer.
It is parameterised by several things - current speed (a property of a trainer), user and bike weight (not a property of a trainer), target grade (not a property of a trainer), and all the various ANT page tuning parameters (of which weight and grade are two, drag coefficients etc the others). I get that having it as a member function means some of those variables are available on self., but that's convenience at the expense of complexity.
My aim would be to simplify the trainer class hierarchy to a bare minimum - to only have in there what is needed for a common interface and what is needed to implement trainer-specifics (message types, trainer-specific unit conversions etc).
To me, "code coupling" (modification variables inside functions, distributed logic, etc...) weaves complexity into the code - it takes longer to read and understand, change or re-use. In contrast, decoupling reduces complexity and creates clarity, where updates to state are explicit, functions are pure(r), and nothing important is hidden away. The logic that actually does the interesting work should be separated from the machinery that implements it, and then be plainly stated and as close together as possible.
As an aside: it's why calling SetGrade() from inside the ANT+ handling routine irks me - why I turned it around so that rather than the code saying "the act of receiving an ANT page immediately changes trainer state", this become three much cleaner, decoupled steps, namely "the act of receiving an ANT page updates ANT page state", "TargetGrade is a function of ANT page state" and then "change trainer state".
It is down to style. I'm not saying "do what I did", just describing my thought process. It's a subtle change of approach but has profound impact to maintainability in my experience. My mantra - "decouple, decouple, decouple". My other mantra is "const-correctness" but as we discussed, Python doesn't have const.
There was a problem hiding this comment.
I agree on approach, but it's different from what I did.
clsTacxTrainer is not only the USB-interface but the entire trainer, where required power or grade can be set (SetPower(), SetGrade()). Other design decisions could have been made, but this is how it's done.
Also clsTacxTrainer does not know about ANT and/or FE-C (sorry for devAntDongle which could be removed) and could be used in TTS if Tacx would like (conceptually, of course they don't).
Trainer-specifics have been put in child-classes and this is a great advantage over the previous implementation; clsTacxLegacyUsbTrainer and clsTacxVortexTrainer have not been tested (although I believe code is 98% ok) and when updated later on it's most likely that the now tested code needs not be touched and hence there is no regression.
There were two big issues in FortiusANT:
- too many locations of data, this is resolved
- Grade2Power() did not work; see all issues now open
I am going to focus on the Grade2Power() and virtual speed now, in the meantime improving code where required. Hope you can join in on that
pythoncode/FortiusAntBody.py
Outdated
| # now all the ANT traffic is here. | ||
| #--------------------------------------------------------------- | ||
| if clv.Tacx_iVortex and VTX_VortexID: | ||
| info = ant.msgPage16_TacxVortexSetPower (ant.channel_VTX_s, VTX_VortexID, self.TargetPower) |
There was a problem hiding this comment.
Will you refactor this into Trainer.SetPower() so that you can implement SetGrade() in terms of it? It seemed to refactor ok in that way.
There was a problem hiding this comment.
Conceptually, SetPower()/SetGrade() sets the data received from CTP into clsTacxTrainer.
Refresh() calls SendToTrainer() and should do the job and the code could be moved there.
See comment, I left it here.
Thanks for pointing me to this code; self.TargetPower must be changed into TacxTrainer.TargetPower.
There was a problem hiding this comment.
And yes, I agree. The iVortex trainer class does not work without this external assistence.
But now the ANT-loop is kept in one place.
It's all about choices
Well it was a challenge for sure to take the whole thing apart and create clsTacxTrainer from bottom up. And yes it's fun, especially seeing the code became easier each cycle of the process. Program On! |
|
Not sure if this helps, lots of disconnection/connections, however had successful ride on zwift for 31 mins... By the way, I've also bought 2x Suunto dongles which work perfectly, so will be sending back the Cycplus ones, however can hang onto them for another week if helps any debugging? FortiusANT.2020-05-12 18-31-28.log |
pythoncode/usbTrainer.py
Outdated
| return Proll + Pair + Pslope + Pbike | ||
| return int(Proll + Pair + Pslope + Pbike) | ||
|
|
||
| def __VfietsNL(self): |
There was a problem hiding this comment.
19:36:22,091: Target=-1.7%(43W) Speed=33.8kmh hr= 0 Current= 64W Cad= 77 r= 546 29
Traceback (most recent call last):
File "FortiusAnt.py", line 520, in
Console.Autostart()
File "FortiusAnt.py", line 161, in Autostart
Tacx2Dongle(self)
File "FortiusAnt.py", line 110, in Tacx2Dongle
rtn = FortiusAntBody.Tacx2Dongle(self)
File "/home/pi/WouterJD/FortiusANT/pythoncode/FortiusAntBody.py", line 448, in Tacx2Dongle
rtn = Tacx2DongleSub(self, Restart)
File "/home/pi/WouterJD/FortiusANT/pythoncode/FortiusAntBody.py", line 684, in Tacx2DongleSub
TacxTrainer.Refresh() # = Receive + Calc + Send
File "/home/pi/WouterJD/FortiusANT/pythoncode/usbTrainer.py", line 728, in Refresh
super().Refresh(TacxMode)
File "/home/pi/WouterJD/FortiusANT/pythoncode/usbTrainer.py", line 445, in Refresh
self.VirtualSpeedKmh = self.__VfietsNL()
File "/home/pi/WouterJD/FortiusANT/pythoncode/usbTrainer.py", line 597, in __VfietsNL
v = 3.6 * ( (P - 37) / (c * m * g + i/100 * m * g) )
ZeroDivisionError: float division by zero
There was a problem hiding this comment.
solved. Sorry crappy code.
pythoncode/FortiusAntBody.py
Outdated
| if debug.on(debug.Function): logfile.Write('Tacx2Dongle; start main loop') | ||
| try: | ||
| while self.RunningSwitch == True: | ||
| while self.RunningSwitch == True and not AntDongle.DongleReconnected: |
There was a problem hiding this comment.
Traceback (most recent call last):
File "FortiusAnt.py", line 520, in
Console.Autostart()
File "FortiusAnt.py", line 161, in Autostart
Tacx2Dongle(self)
File "FortiusAnt.py", line 110, in Tacx2Dongle
rtn = FortiusAntBody.Tacx2Dongle(self)
File "/home/pi/WouterJD/FortiusANT/pythoncode/FortiusAntBody.py", line 448, in Tacx2Dongle
rtn = Tacx2DongleSub(self, Restart)
File "/home/pi/WouterJD/FortiusANT/pythoncode/FortiusAntBody.py", line 669, in Tacx2DongleSub
while self.RunningSwitch == True and not AntDongle.DongleReconnected:
AttributeError: 'NoneType' object has no attribute 'DongleReconnected'
There was a problem hiding this comment.
Yep. Already found it and solved
pythoncode/usbTrainer.py
Outdated
| # UserAndBikeWeight is not used! | ||
| elif self.TargetMode == mode_Grade: | ||
| Target = self.TargetResistance | ||
| Weight = 0x0a # weight=0x0a is a good fly-wheel value |
There was a problem hiding this comment.
Disagree 0x0a is a good value
There was a problem hiding this comment.
Suggestion: don't what is should not be, without stating what it should :-)
There was a problem hiding this comment.
Flywheel weight should be UserAndBikeWeight for simulation trainer mode (ie mode_Grade). I really want to know why "an 100kg flywheels gives undesired behaviour" because for me, it gives much (much) better behaviour and a 10kg flywheel gives undesired behaviour. Test is.... ride downhill and stop pedalling, do you carry your speed? Ride on flat and hit a sudden 4% downhill, does resistance drop immediately, or do you gather speed? Ride on flat and hit a sudden 4% uphill (Rouvy is great at sudden changes!), does resistance hit a wall or do you slowly lose momentum.
There was a problem hiding this comment.
Cool. Will implement and test.
Interesting info for @totalreverse for the interface description.
Thanks
There was a problem hiding this comment.
I was riding my trainer on Rouvy at the time I made the change, restarted FortiusAnt and wrote that comment, while still pedalling. ;)
There was a problem hiding this comment.
I see that @totalreverse changed field-description jan 4th. Makes sense.
antifier has been struggling with this as well.
There was a problem hiding this comment.
Ride on flat and hit a sudden 4% uphill (Rouvy is great at sudden changes!),
Fully agree.
There was a problem hiding this comment.
Zwift can send fractional kg, so ensure int() when populating the message to the trainer.
|
New version uploaded. Tested with exception of iMagic (legacy) and iVortex trainer. Feedback appreciated. |
done |
Agree on that. Note that the current formula not only depends on the mentioned parameters, but also RollingResistance, WindResistance, WindSpeed, DraftingFactor In the logfile I see (matthew thanks): But that does not explain the 2867Watts Indeed, if there is no cadence, the resistance drops to 1500. This is a design decision and should be good; but cadence is required. I removed the condition |
|
OK it's been a bit of searching. Should work now. My appologies for the inconvenience. |
Could you please redo the test with 0.02 and send the logfile. |
|
The huge number of watts was due to up button having been pressed several times for a virtual speed of 90+ kmh. The grade2power debug showed actual speed by mistake. Hence it's not clear in the log. The huge wattage didn't matter because spiral of death code was limiting to 1500 r. But why is spiral of death code active in grade mode anyway? |
It's active because it's part of Power2Resistance() and not limitted to a mode. Since condition is now |
But in grade mode, messing with resistance values directly defeats the virtual gear and you lose the "known quantity" of the factor applied to speedscale. Far better for grade mode to say if speed low, start lowering the speedscale factor automatically. IE nonlinearity!! I hope this explains my previous comment about being suspicious of any direct change to resistance values. Nonlinearity requires tuning to implement nicely so off topic here but the reasoning behind it, rather than resistance value modification I believe to be sound. In my opinion, resistance, actual speed and power have a defined relationship which should not be touched. If you want resistance to change, then you should conceive of the change earlier in the chain of calculations... Refactor the change. So for erg mode, the challenge is different. I've seen an approach where user backpedals... Cadence with no power. This turns off or resets erg mode to allow user to pedal back up to cadence, and then algorithm for fixed power continues. Whatever, I think there might be a better way than to touch resistance values directly for the erg mode spiral problem, just as nonlinear speedscale is a better way for grade mode. |
|
You could, for example, in grade mode, say if speed less than 15 and resistance greater than 6000 (which is the "floor" you have worked out through experimentation?) then divide speedscalefactor by 1.1. IE change virtual "gear" automatically That aaproach itself doesn't account for changing gear back up again, which would need to be a button press or more code or rather a different approach, but it would be an automatic way, in grade mode, to avoid motor stuttering at low speed, without touching resistance value, rather going in with a numerical modification earlier in the calculation, using a concept we already have introduced, precisely to lower the resistance. |
Most of what I said above should be read alongside the fact that for a suitably large gradient, it will definitely activate in grade mode. |
Welll... It may be that the last night solution was a quick fix where the implementation was faulty. All that's needed is what the formula says: avoid cycle of death. No luxury and/or maths, just throttle down. Any practical suggestion appreciated. |
Am still thinking on this one. Allowing low wheel speeds isn't really the issue, it's whether you modify resistance directly or not. I think the biggest benefit of never modifying resistance manually is that you can still trust your virtual gear. Indeed, we've managed to allow high efforts at low virtual speeds specifically by avoiding low wheel speeds, but we do that workaround on speedscale and the notion of a virtual gear, and not by modifying the resistance value directly. I think this has been an excellent improvement, and hoping for a degree of non-linearity soon to make it easier for me as a rider to ride all grades without button pushes. I'll play with the idea of auto-scaling speedscale later. It's not far off conceptually what I was thinking with my original non-linear speed curve anyway, just expressed on a subtly different variable. For cycle of death, the problem is, I've only ever really done two ERG mode rides and while I noticed a runaway increasing cadence on my last, was not really familiar with the spiral of death as something to fix in software until now. Obviously it's a thing, and understandable when you think about how it's implemented. I think detecting the rider "backing off" completely is possibly a good idea. Something like "if CurrentPower < 10W for 2 seconds: then (TargetPower*=0.75 for 5 seconds)". I don't know how that would feel, but the idea that I stop pedalling (or back pedal) for 2,3 or 4 revolutions, then set myself back to the cadence I want to be at and wait for the short kick back up to 100% effort feels not too rough from a rider's point of view. That said, I'm not a seasoned ERG ride. And I've only really skim read https://www.trainerroad.com/forum/t/erg-resistance-cutout-to-prevent-spiral-of-death-at-low-cadence-feature-request/20059/12 |
|
@WouterJD @mattipee I checked my cadence sensor and it seemed fine but I re-seated it anyway. I have been using a version from Elie regularly with no real problems. I took another clone of this branch at 10:00 this morning and tested it after a 1hr ride using Elie's version. It was much better and responded as desired (not sure if my cadence sensor had anything to do with it or the pushes you have made since yesterday). |
Weird... looks like your cadence sensor wasn't working and then kicked in a few minutes in, <10seconds before you stopped riding. Given the resistance limiting code that you were hitting was low_cadence triggered and is now low_speed+high_resistance triggered, you're no longer hitting that, even with cadence sensor being iffy. Same with Elie's branch. So you're saying it's riding fine? Well? Did you press a button once? Did you feel a difference? |
|
Yup, riding fine and I felt a resistance change with the button press. The
cadence is displaying accurately on zwift. I'm running a headless r pi so
didn't check the stdout. I could ssh in to it tomorrow and take a look on
my laptop. If that helps.
…On Fri, 15 May 2020, 15:09 mattipee, ***@***.***> wrote:
@WouterJD <https://github.com/WouterJD> @mattipee
<https://github.com/mattipee> I checked my cadence sensor and it seemed
fine but I re-seated it anyway. I have been using a version from Elie
regularly with no real problems. I took another clone of this branch at
10:00 this morning and tested it after a 1hr ride using Elie's version. It
was much better and responded as desired (not sure if my cadence sensor had
anything to do with it or the pushes you have made since yesterday).
FortiusANT.2020-05-15_11-50-29.log
<https://github.com/WouterJD/FortiusANT/files/4634673/FortiusANT.2020-05-15_11-50-29.log>
Weird... looks like your cadence sensor wasn't working and then kicked in
a few minutes in, <10seconds before you stopped riding.
Given the resistance limiting code that you were hitting was low_cadence
triggered and is now low_speed+high_resistance triggered, you're no longer
hitting that, even with cadence sensor being iffy. Same with Elie's branch.
So you're saying it's riding fine? Well? Did you press a button once? Did
you feel a difference?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AORJG3UNTTM7BAT3BC4GPILRRVEK5ANCNFSM4M6G36LA>
.
|
|
sounds good. Log not needed for me. Please tell what output you would like to have, seems ride without ui "in the blind"; though Zwift is your user interface |
| # Refresh() | ||
| #--------------------------------------------------------------------------- | ||
| def Refresh(self, QuarterSecond, TacxMode): | ||
| super().Refresh(QuarterSecond, TacxMode) |
There was a problem hiding this comment.
https://en.wikipedia.org/wiki/Call_super
There are several places still where I firmly believe that more refactoring will yield cleaner, less coupled code. It's often the case that one change enables the next, or that one smell masks another, and personally, I never stop looking at code and asking how might this be better expressed. There's a few things that still stand out to me.
That's not to diminish any of the work you've done, and I applaud your efforts on this software and the community of users it has enabled to get riding with modern software, hopefully more to come. And for you being new to python and OO, etc.
I accept some items are down to style and we've discussed them already, though I still believe in "decouple, decouple, decouple". I'm not implying that I think you should refactor anything else now. I am not intending to raise any more of these thoughts uninvited.
The user of call-super just made me chuckle and I thought I'd just post a wee kind of refactoring-review-summary type of thing as we approach the time to merge this PR. A combination of a well-done to you and some enthusiastic support from me.
There was a problem hiding this comment.
A lot of text (too much?) to express a concern.
The implementation is: do the standard class-thing AND do also the ivortex-key conversion.
I think it's coded as python suggests.
Alternative is to create _ReceiveFromTrainer() for ivortex, semantically probably a more elegant implementation
There was a problem hiding this comment.
A lot of text (too much?) to express a concern.
The implementation is: do the standard class-thing AND do also the ivortex-key conversion.
I think it's coded as python suggests.
Alternative is to create _ReceiveFromTrainer() for ivortex, semantically probably a more elegant implementation
There was a problem hiding this comment.
As it is, the specific code calls back to the common code, and must do so in each child class for correct behaviour. It's functionally correct, a natural way to think of it, but implemented this way round has the name of "call-super" and there is a cleaner way.
To refactor and avoid use of call-super is to have the parent/common function not be overridden and for it to call a differently named and indeed overridden function on the child class - ie the common code calls out to the specific code. I used "Impl" as a suffix for such functions in my branch.
As the extra text suggested, I think you're close to wanting to release so while I enjoy refactor and review, and would like to see/do more, I mention it and move on.
There was a problem hiding this comment.
...requiring the parent to be changed (add additional call) which basically is against inheritance.
One could standardize that each call has a ..before() and after() but that would complicated things as well.
I adhere to the idea of clean code. Question here is: what would the correct code be, given that the existing classes are frozen and only clsVortex may be changed?
There was a problem hiding this comment.
And yes - I want to release 3.0; not excluding further devs. People are waiting for it!
There was a problem hiding this comment.
Leave as is. Functionally, it doesn't matter. Also maintenance impact is minimal.
|
My ideas are still only half-baked. I hope I don't confuse by typing this too early. In the end, I'm trying to rationalise any and all impact on the code due to the fundamental problem with the Tacx Fortius at high load and low wheel speed. We have managed to get as far as having a factor which we multiply with SpeedScale with which we can implement known-ratio virtual gearing - this is independent of any physical gearing and mathematically equivalent to simulating different wheel sizes. With this, we can push button into a lower gear, with which we can climb steep slopes at slow speed. For each button push, we have a different gear - each gear implements a different linear relationship between wheel speed and virtual speed, where each gear effectively changes the slope/gradient, but it's still a linear relationship for each. It is not a huge step from there to begin to alter the relationship automatically as a function of wheel speed itself - to introduce non-linearity into the relationship and thereby achieve a hands-off approach to lower gearing or steep slopes at low speed. It's not implemented yet and there would be work in tuning or suggesting defaults, etc. I think there's value, that's why I keep talking about it. But there might be some interplay between how that works most elegantly and what the requirement is for ERG mode. The reason I'm writing this post is because something is bugging me about all this and it may help to write things down and share. I think there's a solution in here somewhere and I'm trying to zero in on it. In playing around with the spiral of death code a couple of nights ago, and more specifically, disabling it, it made me think. Disabled in ERG mode, you can't get started on a 250W ERG ride... the wheel stutters. Similar to riding a steep grade at low speed... Grade2Power() alone results in a target power that you can't ride... the wheel stutters. Your code allows you to get past that point. The code in question is above. For low speeds (<15kmh) and high loads (6000+), you implement the following: In principle, I dislike the approach of modifying resistance value directly because you lose the "known" aspect of any virtual gear SpeedScale factor. In ERG mode that's likely not an issue, but it still sits in my mind as something to reformulate, because I want to try to unify the solution for Grade and ERG. But your approach does successfully ensure the inability to push X watts at low speed Y, and thereby avoid wheel stutter... specifically, inability to push 200W under 15kmh, 100W under 11khm, 50W under 7.5khm, etc... more or less. That's my reading/calculating, I hope I have not misunderstood/miscalculated. In grade mode, we prefer virtual gears to this <15kmh-modify-resistance approach because we know the speed relationship, but we now NEED the user to change gear with buttons, otherwise without your additional <15kmh it will stutter, but with it, we lose accurate simulation of power and virtualspeed. I'm deliberately ignoring spiral of death just now. So recap - the fundamental issue is "power X at speed Y" where X is high and Y is low. Your approach above disallows "power X at speed Y", doing so by modifying resistance directly. In Grade mode, we use virtual gearing to allow us to redefine speed Y as speed Z as a different way to avoid "power X at speed Y", employing "power X at speed Z" instead. Given SpeedScale and PowerResistanceFactor are constants, the relationship between resistance, speed and power is known. You could therefore conceivably express your "<15kmh, resistance 1500-6000" code in terms of power in watts... ie there is some relationship which says "at speed Y, the maximum power is X". Which is effectively what you have. The result is that in ERG mode, you don't achieve target power until wheel speed is high enough. Eg... set to 200W and you do feel increasing resistance as you increase speed but never manage to hit a steady 200W until wheel speed is 15kmh or above. In reducing the calculated target resistance value, you are effectively reducing the power value requested in SetPower() to something "manageable by hardware" according to the current wheel speed. We don't know what hardware limitation curve looks like but you do have a set of values that avoid stuttering. Ideally SetPower() would have a constant effect (ie regardless of speed, set resistance to equal X watts). But we can't do that at low speed, so we make it not-constant - variable target power - specifically, lower power than the requested X for low wheel speeds. But while it's expressed in code just now as a function on speed which results in a change of resistance value, it can also be expressed as a function of TargetPower and ActualSpeed which results in a modified TargetPower. A small conceptual change, but still mathematically equivalent, which took a long time to type. Important point this one... without fundamentally changing the approach, it's possible to express what's there already as something like: But here's where sore legs and a tired head find it difficult to test or think about the next bit. I'm fairly convinced there's a single implementation that solves both problems elegantly, with a minor conceptual twist relating to TargetPower and SpeedScale factoring. But I'm afraid it's not quite crystalised yet. |
|
a. I would suggest to raise a separate issue; resolution is not part of this release |
|
All that said, leave the resistance-fiddle in for both ERG and grade and you're good to release. The pursuit of elegance in expression of the problem's solution is my bit of fun, and it's therefore my task to make it work and convince you it's better. I type so much in case you wish to think along with me, but I acknowledge your differing priorities. |
We post at the same time, with basically the same thoughts. |
Please verify following changes.
Note that: usbTrainer.py and FortiusAntBody.py have undergone major modifications.
And also: CYCPLUS update is merged.
Changes:
Special attention:
Regarding Grade2Power: what to do with negative power? And if power is negative, what is the VirtualSpeed in that case?