cagnulein / qdomyos-zwift

Zwift bridge for smart treadmills and bike/cyclette
https://www.qzfitness.com/
GNU General Public License v3.0
391 stars 111 forks source link

[REQ] Decouple bluetoothdevice classes from UI (i.e. IOS lockscreen code) #1222

Open drmason789 opened 1 year ago

drmason789 commented 1 year ago

Ulterior motive: decouple device logic from UI to make it simpler to split main project into app and lib projects, and also to inject a test stub in place of the IOS lockscreen code to test that it's actually being called, on non IOS operating systems, and also, to avoid repeated code.

There is code repeated in various sublcasses of bluetoothdevice that puts some information on the lockscreen of various IOS devices. This code is disabled in non-IOS builds with compiler directives.

e.g.

https://github.com/cagnulein/qdomyos-zwift/blob/099bbf7d9c2c4f1dc038d43e2040e4a02f59cc24/src/yesoulbike.cpp#L170-L180

The classes that have this are: activiotreadmill bhfitnesselliptical chronobike computrainerbike concept2skierg cscbike domyosbike domyoselliptical domyostreadmill echelonconnectsport echelonrower echelonstride fakebike fakeelliptical faketreadmill fitplusbike fitshowtreadmill flywheelbike ftmsbike ftmsrower horizongr7bike horizontreadmill inspirebike keepbike kingsmithr1protreadmill kingsmithr2treadmill lifefitnesstreadmill m3ibike mcfbike mepanelbike nordictrackelliptical nordictrackifitadbbike nordictrackifitadbtreadmill npecablebike pafersbike proformbike proformelliptical proformellipticaltrainer proformrower proformtreadmill proformwifibike proformwifitreadmill renphobike schwinnic4bike shuaa5treadmill skandikawiribike smartrowrower snodebike solebike soleelliptical solef80treadmill stagesbike strydrunpowersensor tacxneo2 technogymmyruntreadmill truetreadmill trxappgateusbbike trxappgateusbtreadmill ultrasportbike wahookickrsnapbike yesoulbike

The classes that don't have it are: bowflext216treadmill bowflextreadmill domyosrower eliterizer elitesterzosmart eslinkertreadmill fitmetria_fanfit heartratebelt iconceptbike nautilusbike nautiluselliptical nautilustreadmill octaneelliptical octanetreadmill paferstreadmill smartspin2k spirittreadmill sportsplusbike sportstechbike technogymmyruntreadmillrfcomm toorxtreadmill

  1. Create an interface (in C++, fully abstract class) that has suitably named members for the lockscreen related functions, e.g. ILockscreenWorkaround
  2. Move all IOS-specific implementation into a single class that implements that interface
  3. Provide another implementation of the interface for use in non-IOS that doesn't do anything
  4. Replace the existing "lockscreen h" members with a member "ILockscreenWorkaround " in bluetoothdevice and remove all the headers required for lockscreen.
  5. Change the code that uses lockcreen directly to call the ILockscreenWorkaround object.

At some point in the future (a future PR), when the tests cover the testing of how each bluetoothdevice object responds to incoming data, plug an ILockscreenWorkaround implementation into this (dependency injection) to test on non-IOS operating systems, that the functions are being called as expected.

drmason789 commented 1 year ago

@cagnulein Is this something you would agree to me going ahead with? Should more of those devices have this functionality?

cagnulein commented 1 year ago

Is this something you would agree to me going ahead with?

yes absolutely, now I'm repeating these code lines only because I'm lazy :)

Should more of those devices have this functionality?

actually it's hard to say, i mean actaully every bluetooth and wifi device should have these lines. If they don't have it, it's a bug

drmason789 commented 1 year ago

That first commit ee03716 is a first step, but not complete according to the spec above. I've just brought in some changes in this direction that I'd already done to avoid repeated code.

drmason789 commented 1 year ago

@cagnulein The lockscreen class appears designed to be simply a collection of static functions.

Yet sometimes the code that uses it creates a new instance of lockscreen the "h" field in many classes, e.g. https://github.com/cagnulein/qdomyos-zwift/blob/ecc6306b7ddb09fa5b92db3bfd9b0cb20e711ad2/src/virtualtreadmill.cpp#L35-L46

Yet other times it uses a temporary instance:

https://github.com/cagnulein/qdomyos-zwift/blob/ecc6306b7ddb09fa5b92db3bfd9b0cb20e711ad2/src/activiotreadmill.cpp#L316-L325

Perhaps all the members of lockscreen could be made static.

cagnulein commented 1 year ago

i don't remember why i didn't do as a static member for all the methods, maybe at the very beginning it wasn't clear to me where i would have use this

but yes, static methods should be the way, but i need to check the code.

drmason789 commented 1 year ago

I can make the change, but I don't have the equipment to do an IOS build.

cagnulein commented 1 year ago

i can help you for sure with the tests. if you encounter some unclear code, let me know. hopefully they are just copy and paste things

drmason789 commented 1 year ago

Feature 1

https://github.com/cagnulein/qdomyos-zwift/blob/6235d00c556a911666ffbd02132ce3fe707332f7/src/chronobike.cpp#L200-L211

When the heart rate belt is disabled in the settings, and QZ is running in IOS, set the kcal and distance on the lock screen and get the heart rate from it.

This appears to be completely independent of the Peloton workaround the other features relate to.

Feature 2

https://github.com/cagnulein/qdomyos-zwift/blob/6235d00c556a911666ffbd02132ce3fe707332f7/src/chronobike.cpp#L267-L297

The first time the bluetoothdevice object receives an update from the device it represents, if QZ is running in IOS, detect if the Peloton workaround and the bike cadence sensor are enabled in the settings. If they are, the object's own instance of the lockscreen class is created. The existance of this object is later used in combination with another check of those settings, to determine if the Peloton workaround is active.

I believe the determination that the Peloton workaround is active can does not need to wait for this first update, but can be done when the bluetoothdevice object is instantiated, and afterwards, the fact that "h" is set will indicate the workaround is active, assuming the settings won't be changed while riding.

In addition, since the lockscreen class is basically a group of static methods and an instance has no state, this "lockscreen *h" could be replaced by a boolean if the methods in class lockscreen are all made static.

Feature 3

https://github.com/cagnulein/qdomyos-zwift/blob/6235d00c556a911666ffbd02132ce3fe707332f7/src/chronobike.cpp#L214-L224

If QZ is running in IOS, this code checks if the Peloton workaround is active and if so, updates the cadence and heart rate on the lockscreen and calls some functions for virtual bikes.

Feature 4

In the case of the virtualtreadmill, the Peloton workaround is in fact detected in the constructor:

https://github.com/cagnulein/qdomyos-zwift/blob/6235d00c556a911666ffbd02132ce3fe707332f7/src/virtualtreadmill.cpp#L35-L46

then used later. But this detection doesn't look for the bike cadence sensor setting.

https://github.com/cagnulein/qdomyos-zwift/blob/6235d00c556a911666ffbd02132ce3fe707332f7/src/virtualtreadmill.cpp#L335-L359

stale[bot] commented 1 year ago

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[bot] commented 1 year ago

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[bot] commented 1 year ago

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.