usbTrainer.py refactor?

[mattipee]

Hi,

I have, for the last hour or two, been having a play in and around usbTrainer.py and FortiusAnt.py.

My thoughts to begin with were to encapsulate a little

• don't pass devTrainer to Send() and Receive() functions, rather call Send() and Receive() on a trainer object
• don't return tuple from Receive() function, rather set the returned values as properties of the trainer object and use them as required
• get rid of the global trainer_type and LegacyProtocol variables and instead put them inside the trainer object.

Having broken simulation, and reached the point in the evening where I will stop, I though I'd post this issue for discussion.

Having two very distinct sets of behaviour (around LegacyProtocol for the iMagic), and now introducing the iVortex into the mix, it seems to me like a sensible direction to refactor further and separate concerns... each trainer type (or family) having it's own trainer class perhaps, with it's own send()/receive() implementations, yet sharing a common interface, and beginning to abstract a little to setLoad() or setGrade() functions, and a periodic doUpdate() or similar, as opposed to the manual Send/Receive pattern in FortiusAnt currently.

I've been bitten before refactoring someone else's code and there are several things to consider. I can be over keen. Even if there is appetite, timing can be important. That said, it feels like with work towards iVortex it could be both challenging and rewarding to refactor at this point. It would only really work with clear, helpful, incremental changes. Otherwise too much just becomes a fork that may never integrate, and that is unhelpful.

I've pushed to a "Refactor" branch in my fork. Feel free to have a look.

If you do have a look and get your head around my changes, worth perhaps me adding here some quick notes on my thoughts on next steps for context:

• trainer class gets split to four??... "fortius, magic, vortex, simulation", perhaps with a base for shared code, GetTrainer() decides which one to create.
• simulation therefore just becomes another device which you can peek/poke
• with that done, LegacyProtocol goes away, the various resistance/power/speed function implementations get split according to what kind of trainer, common code refactored
• perhaps then a setLoad()/setGrade() pair, and generally decoupling the main loop

The first step would be for me to restore simulation, discuss/fix my break to SpeedT, CadenceT etc, and consider raising a PR of something working, moving in an agreed direction.

Interested to hear your thoughts on this. Whether it's welcomed, if so thoughts on timing, direction, scope, scale, etc... if not, I'll be happy enough to just Ride On. :)

I am of course very grateful for the software as is - I stumbled across the trainer for not-too-much-money last year, and am delighted that it appears as capable as it is, even now. Thanks to you and those before you. A great lockdown activity.

[mattipee]

Restored SimulatedTrainer over morning coffee.

[WouterJD]

Well I agree with the thinking. Quick reaction.

• i-Vortex will be an ANT+ device and join the ANT-loop; my idea was to "disable"the usbTrainer
• returning tuples is from the original antifier code and I did not really fancy the style since it seems error-prone, but I left it since ... it just was as it is
• as you can see from coding-style, I'm still the call-a-library old-fashioned programmer and never made the C to C++ / object oriented programming step. I can learn there.
• so I'm not shy asking what is the difference between SendToTrainer(usbTrainer) or usbTrainer.SendToTrainer()
• adding Speed, Cadence... to usbTrainer; is that the way you should do it, or implement an own class with usbTrainer, Speed and Cadence attributes.

Since FortiusANT is my learn-python-on-the-job project I'm in for suggestions to improve.
Also, I want to keep understanding all the code - to meet up with previous.

So yes - suggestions for code-improvement more than welcome!

[mattipee]

Hi, great! :)

Refactoring to me is a multi-operation process, and I never seem to manage to have an end goal fully in mind when I start - it's iterative for me. So there are definitely still warts with what I have on my branch that I've introduced. I typically analyse the code, identify an issue area, abstract a concept, shuffle code, repeat. This started with the tuple being return, then with the fact that SendToTrainer() is a single function with multiple behaviours (stop, power, grade), then that it also handles two different protocols/calculations, etc...

Conceptually, and how I think it should be reflected in code - a trainer is a trainer, regardless of the backend (USB/ANT+). Variously, they can report speed, power, cadence, HR etc, and can be instructed to simulate grade, set point power, etc... this idea isn't where I started when beginning to shuffle code last night, but it's certainly a direction.

Returning the tuples was nasty, indeed. The next concept here I think is that all those values are basically a "last known state" - so storing them as attributes in the trainer class was simply the first step in refactoring. Grouping them together into a "state" object might be the next step. You should be able to query the trainer object for last known state, rather than require to wait until the next message is received... this opens up threading possibilities, decoupling GUI from USB protocol, etc.

In terms of style, the biggest difference between changing from SendToTrainer(usbTrainer) to usbTrainer.SendToTrainer() was the removal of the globals (trainer_type and LegacyProtocol). These control what SendToTrainer() do, so you either pass these things in as parameters (which is noisy), have them as globals once you're in the implementation (as was the case), or encapsulate them in the same object and access them once inside (as I propose). To motivate the benefit, imagine you were to allow simultaneous control of two trainers with different values for trainer_type... and I'm not saying you would... but encapsulation is key.

In my experience, and as I suggested in the first post, timing is key, and incremental adoption is also important - to avoid big-bang integration or simply forking the code and never integrating, but also to keep understanding the code.

Perhaps if you were to create a branch in your repository, into which I could submit PRs, instead of master? That way, it becomes "gated" - if things are moving in the right direction, you can accept a PR and it makes subsequent changes and discussions easier.

There's also a benefit to code commenting by line on the PR itself, rather than simple discussion on an issue.

Sun's out - I'll go real-world cycling today. :)

[WouterJD]

The last remarks touches an important point: getting the software work (starting from antifier) was quite a job. The proposals focus on getting the source more beautiful, a target I support, but will cost a lot of time and deliver small benefit. Would it be a commercial project that need to be future proof to support newer trainers that appear the story would be different.

Suggestions welcome anyway!

[mattipee]

getting the source more beautiful ... will cost a lot of time and deliver small benefit

I enjoy refactoring, and my time is freely given.

I would argue that in terms of maintainability of existing features, and in reducing the barriers to extending functionality or the workload therein, there is always benefit to be had in code clean up. I am not criticising any work to date - it's just fun (for me) to try to understand how best to shape and mould code to more elegantly express the problem and its solution.

Apart from that, a pet project through which to learn is perhaps a perfect candidate for beautification, where there is no commercial risk and opensource is a great platform for collaboration. I guess the intentions of embarking on such an effort would be to refine the domain-specific abstractions, minimise coupling, maximise cohesion, and generally exercise the programming muscles without necessarily introducing additional support load (features), simply to make maintenance easier, or if not, just for personal gain.

A bit philosophical, but I'd love to help. Even watching for old trainers on eBay.

[WouterJD]

Thanks for the offer!
I'm pretty occupied now to get the i-Vortex working.
Then going for next steps.

[mattipee]

I'm following your progress, well done! Not easy thing to do remotely.

[WouterJD]

Great assistance from @darkpotpot!

PS. did we introduce ourselves already, my name is Wouter and I'm from the Netherlands.

[mattipee]

@WouterJD
In #70 (comment), you suggested "let's work on the power-curve".

Did you mean #51 ?

If so, please.... would you entertain accepting some changes from me, in the direction of refactoring usbTrainer.py first? I am very keen on this, and I think it is the right time.

I deleted my old refactor branch, as I went too far too soon, I fear. Same thoughts remain in my head though. I am starting again, and am convinced that the direction in which I think it should go will help.

I think it needs careful planning though. My thoughts are:

• you create a new branch
• I create a new branch
• I commit incremental changes to my branch
• I create a PR to your branch
• you review, we discuss
• I modify where necessary
• you accept and merge
• I continue to commit to my branch
• we repeat PR by PR, whenever there is sufficient progress on conceptual improvements.

I think if it is to work at all, then it should be in stages (ie incremental) and it will take a short while to iterate through development/review/merge for these these stages... for example:

1. get rid of LegacyProtocol as a global and encapsulate most functions in TacxUsbTrainer class
2. get rid of tuple return value from ReceiveFromTrainer()
3. refactor functions using LegacyProtocol into two different implementations - subclassing TacxUsbTrainer
4. abstract SendToTrainer() to more meaningful SetGrade(), SetPower(), etc.
5. rearrange class heirarchy further to incorporate Vortex and Simluator

I really, honestly, believe this will make a difference - certainly steps 1 and 2, but you often "can't see the wood for the trees" and the sense behind further refactoring only becomes apparent when you get other things out of the way. It could be done in as little as a day or two, depending on your time available to review.

I think crucially, if it's to your new branch, not to master, then master is safe, and worst case the exercise is futile and you discard the branch. I don't think that will happen.

In a sense, I don't want to introduce the more complicated "virtual speed" notion part of my power curve work without first refactoring otherwise it becomes a bit spaghetti-like and difficult to follow.

What do you think? I'm ambitious, and hopeful...

It could be as simple as you create a branch, and watch me work as I create 3, 4 or 5 PRs into your branch.

[WouterJD]

Approach ok. Perhaps have a WebEx, discussing how to work with branches etc. I would have time Wednesday evening if you would think that's an idea.
1 and 2 agree. 3 likely also yes.

4 I doubt since Vortex is not a usbtrainer but an ant-device, but suggestions welcome.
Note that reworking usbTrainer.py implies work on FortiusAntBody.py and that testing gets more and more complicated because we do not have the trainers available for regressiontest.

[WouterJD]

Advantage of this is that when the Fortius works, the code is untouched when working on imagic

[WouterJD]

Note that l, there are two parameters:
1- interface buffer 24, 48 and 64 bytes (don't hold me to exact numbers here
2- power-to-resistance formula's

Today mapped on "legacy" but it seems all mixes are on the market.

[WouterJD]

Let's do refactoring first, power-curve as second. When done other ideas....

[mattipee]

@WouterJD Excellent. Yes, a live chat would be a good idea. Regression testing the biggest issue, but you have a community of users, I imagine a few might be willing to pull down a branch, run it and report back.

[mattipee]

@WouterJD curious - what development tools do you use?

I've been using Virtual Studio Code on this project and so far, quite happy. Good integration with git/github, python, etc.

[WouterJD]

You won't believe it: Notepad++, google/github.
Subjects for wednesday: what tools are good, should I use Github desktop?
Where do I download VSC? What benefits does it provide?

[mattipee]

I was using vim until I downloaded VSCode. https://code.visualstudio.com/

For example:

Left panel shows files, which ones have been changed, right panel shows an overview of the file (very zoomed out, if you like), very-very-bottom-left shows which branch you have checked out, main panel shows you by the small coloured bits beside the line numbers where changes have been made - click any one of them and you get a view of exactly what you changed (red old, green new). It has as linter (a tool that analyzes source code to flag programming errors, bugs), and just generally fits a lot onto the screen.

I've not used github desktop. But VSCode seems to be reasonable at what I need it for. I do a little command line too, but just because I'm used to that for some commands.

It's the first Python project I've really worked on. Other IDEs are available, but VSCode is fine for me for now. I'd give it a go if you have half an hour to have a play. It automatically prompts you to install the python linter etc.

[WouterJD]

I don't know whether you got the copy of my request to @totalreverse, hence this not.

I suggest to rename this issue into "Improve support of i-Magic, i-Flow, -Vortex trainer types" because that's what people look for and that's in the end the purpose of refactoring. Agree?

[WouterJD]

I'd give it a go if you have half an hour to have a play
Will do monday, just wrapping up issues now

[mattipee]

About renaming the issue - in my head, it's about encapsulating trainer characteristics and idiosyncrasies, abstracting operations, and separating concerns. It's not about improving support for trainer types, more about improving (hopefully) the maintainability of the code itself. The purpose is to end up with cleaner code, and not really and end-user concern. So a better title than this issue has now at least might be "Improve encapsulation and abstraction in usbTrainer.py and surrounding code". We'll try to keep any functional/behavioural changes out of the work associated with this - it may introduce regressions, but willing test subjects help mitigate this.

[WouterJD]

Just to run ahead on our meeting lateron next week

I would expect that in future will will say
devTrainer = clsTrainer.GetTrainer()

If we have different classes e.g. clsFortius and clsMagic, should the initial call be:
devTrainer = clsTrainer.GetTrainer()
and if so, how should the caller know which one to call?

Or is the class going to be:

class clsTrainer
    SendToTrainer = None
    def SendToFortius()
    def SendToMagic()

where __init__()
creates the SendToTrainer function with
self.SendToTrainer = SendToFortius()
or
self.SendToTrainer = SendToMagic()
depending on it's detection logic?

What do you think?

[WouterJD]

About renaming the issue -
OK - the art of writing nice code and I agree.

BUT
The maintainability purpose is exactly that.

We have two interfaces (48 and 64) and two engines (formula 1 and 2) so we need an idea how to support any mix of the two (in this case 4).

I call it a night for today - cu later!

[mattipee]

I would typically want to have the GetTrainer() function contain all the logic for determining which trainer is which - certainly detecting between the USB device IDs, but even the check for "if clv.Tacx_iVortex:" could go inside GetTrainer(). What trainer object you get is a function of a) command line parameters (-s for example), and b) what is actually connected. So let that function do that work - only one GetTrainer() function.

The trainer object returned should then be able to be used by calling code (almost?) identically regardless of which device it is. They all have concepts of Speed and Power, Cadence, HR only in some cases. Rather than SendToTrainer() which I don't like too much, I will suggest a move towards "SetPower()", "SetGrade()", "Stop()" operations etc that work regardless of what type of trainer object was returned by GetTrainer(). Further, SetGrade() for example does involve some calculations... however some are totally trainer-independent calculations (eg Grade2Power()), others are trainer dependent as they rely on conversion factors (Power2Resistance(), Wheel2Speed()). These kind of things become more apparent when you start to shuffle code around.

Mainly, my point is not to "couple" fine details of specific trainers to the main loop, and to conceptually separate interface from implementation.

[WouterJD]

Agree and curious what implementation you will suggest

[mattipee]

@WouterJD Me too... I'm working on it again in stages. Stage 1 is in my fork already, stage 2 I'm working on (got sidetracked with HRM/SCS and Button events) and should push later today - I think this has been the trickiest and hope it's not itself too big a step/jump. Stage 3 will be SendToTrainer work, etc...

[WouterJD]

Perhaps if you were to create a branch in your repository, into which I could submit PRs, instead of master?

Done: branch "#54 usbTrainer.py refactor" created

I guess this is the idea of it, so we can work in the same repository instead of you working in your own fork. Right?

[mattipee]

Excellent.

In my mind the idea is:

• I work in my own fork for the next few days, probably in several branches, one for each conceptual jump/stage
• intention is to then, one-by-one, create a PR from one of these branches to your new branch
• we review those changes, I explain the reasoning if not clear, and (with hopefully minimal changes) you accept the PR and merge the changes - I can then delete my branch, as you have adopted the changes, and we are equal.
• if any changes were made to my branch as a result of review before you agree the PR and merge, I'd need to pause to carry those forward into any subsequent "stage" branches I have already began work on, because I am jumping ahead of your acceptance and making several branches to separate my work into these stages, and in doing so making gross assumptions that I a) know what I'm doing, b) know where I'm headed and c) that you'll agree. ;)
• Once any such carrying-forward work is ready, I then create a PR from my next "stage" branch to your newly updated branch, and again you review, we discuss, and hopefully merge
• In the end, I hope you should end up with a usbTrainer.py refactor? #54 branch containing all my changes, and I no longer have any refactoring branches in my fork, until I want to continue making further changes for a subsequent PR.
• In terms of subsequent work, we would continue to work in our own forks, and keep our respective branches up-to-date with each other via github
• for a given branch that I work on, I would rebase my changes on top of the latest revision of your branch so that I have any changes you have made, and then PR my changes back to yours so that you can adopt mine.

The reason I think this overall approach is a good way to do things is to a) avoid one big massive PR that contains several conceptual changes at once, and b) to be able to focus on each conceptual change and discuss it before you accept it into your own fork.

Each PR will build on the previous. I have about 4 or 5 stages in mind I guess, perhaps the last couple are less clear as yet but should be as I get there. I hope to have all work I believe strongly in ready for Wednesday, so that the object of the exercise is to discuss my changes in stages, exercise some PRs and merges as we go, and then relax away from the detailed code review and either on the same call or another, think about a) where we've got to, b) where the risk lies, c) anything we've noted down along the way.

Hopefully the summary will be along the lines of:

• the simplifications to AntFortiusBody.py are clear to see
• structure of usbTrainer.py is improved, and code separation is beneficial
• we accept there may be some regressions but they'll be easy enough to fix, nothing is terminal
• and if we get good feedback from some willing volunteer testers, then we're probably good for this to PR to master, or will be after addressing some of our own observations/concerns.

I suggest that if we reach this point, and the exercise is complete, that you consider hard whether and when to proceed and merge to master - perhaps 2.7. The longer a significant branch like a refactor branch stays alive, the harder it is to keep it up to date. In principle you could have any number of small feature branches on the go at once... perhaps CYCPLUS, LCD, Vortex, I don't know what. Though each may be small, once merged to master, then the refactor branch needs to be updated wrt any changes now present on master. A significantly changed branch such as refactor is very possibly going to imply merge conflicts for even the smallest change. And keeping it up to date and ready to merge to master is crucial. Which is why you're absolutely right to raise regressions and your and my lack of ability to test everything, and why we will need to rely on the community of users to test a bleeding edge branch and then at some point soon, just go with it.

On a technical note rather than logistical, an interesting study will then will be to look at how implementing a non-linear-virtual-speed-relationship will look upon the new code, compared with how it might have looked pre-refactor.

[mattipee]

(wow, that was long)

[WouterJD]

I will read that in detail later.

Refer to:

• https://github.com/totalreverse/ttyT1941/wiki
• How to detect Headunit and Motorbrake Tacx Types totalreverse/ttyT1941#8

My findings:

• in usbTrainer I have given names to a 1902/1904/1932/1942 but they are all treated in two groups "Legacy" or "New protocol" and reading the two references above, this is probably the right approach.
• Giving names to these constants is probably causing confusion: we talk to two types of headunits,
• When TTS can work with the trainers, the interface must provide sufficient info. All we have is the number and the interface length.
• There are complaints from users regarding the Grade-functionality, but that's probably not related to the specific combination of headunit/trainer but to the poor powercurve at all (Grade2Resistance() configurable? #51)

My conclusion today:
Refactoring gives better code, the functionality does not need to be changed with the knowledge I have now.
We do not need a Fortius/Magic/Flow/Vortex etc class, but HeadUnitNew(1904/1932/1942), HeadUnitClassic(1902) [and perhaps Vortex=ANT *)] class because that represents the two [three] behaviours known untill now. (Note that the New interface has either 48 byte or 64 byte responses with identical content apart from the unused fille at the end).

*) Vortex is now treated like HeadUnitNew due to lack of Vortex user providing test-results of a power-curve test. "We" are badly distracted because of this CYCPLUS dongle stuff.

Next main thing is to improve the Grade2Resistance formula; #51

[WouterJD]

Agree with the long text; I hope we have the refactoring thing done asap so we can close and proceed with #51 which is as least as challenging because it touches the real purpose of the project: make old Tacxes work with Zwift.

[mattipee]

All sounding positive. Agree with your comments re: the conceptual/naming split re: trainer types.

[WouterJD]

:-)

[mattipee]

Further possible requirement to keep in mind while performing and evaluating refactor:
@martingeraghty @WouterJD
#65 (comment)

BT interface rather than ANT. FortiusBT as a separate program vs introduction of BT functionality in FortiusANT. I'd be in favour of the latter - supports the logic behind decoupling ANT from Trainer and from GUI threading, etc - thus making it much easier to introduce BT<-->Trainer as functional alternative to ANT<-->Trainer. Quite excited by this idea myself
.

[WouterJD]

Well let's not get off track.

The purpose of FortiusANT is to provide a bridge between USB and ANT. That's it. And that should work as good as possible.

There are two real issues:

• Improper working dongles, where other dongles are proven working. I would ignore it. They are faulty.
• Improve the powercurve.

Risk is that we're getting too distracted from the main objective.

[mattipee]

@WouterJD Agreed. Just making note - always good to consider "what if" and "does this refactor direction support/preclude X..." Not suggest it goes in now or that we detour, just good to validate architecture against things like this.

[mattipee]

A broader scope would be "FortiusANT is the best bet for people to integrate old TACX trainers with modern software" - and that could well be BT in some cases - eg a mobile device that has BT but not ANT. Anyway, it's not a bad idea imo, in time.

Edit: especially if someone else is able to put time in to develop it - least we can do is consider the potential for it during refactor and identify where the scalpel needs to cut, if you like.

[WouterJD]

Well.... waiting for somebody going to implement bluetooth
https://www.futurumshop.nl/xand-ant-dongle.phtml I don't know where they want to ship; I bought yesterday for my daugther and will be delivered today.

[martingeraghty]

@WouterJD you're right, lets not get off track. I was just voicing my interest to look into it myself in the future, not stating a development requirement.

[WouterJD]

Implemented in 3.0 (yes, iVortex also)