Cadence timing tweaks and working around Garmin.

[jeremydk]

Noticed that Zwift, TR and my Garmin all displayed around 5rpm too high even when using the bot simulator. While the requested intervals are correct, the actual scheduler has some variability, and I added a really simple control mechanism to tune closer to the correct RPM.

Additionally, testing on a Garmin 530 showed that, while watts were displaying properly, cadence would experience drops to "---" (i.e. null). While the BLE spec allows frames to not contain crank information, the Garmin device was unhappy with that. Since it uses time deltas between timestamps, we can resend the last crank information with the same timestamp and count and allow the display to maintain the last known cadence.

[rmaster78]

This is great! I did notice that rpm number being reported into Zwift from Gymnasticon seemed a bit high when compared to the TACX T2 trainer that I used to compare the power numbers, but figured this was due to the Flywheel bike just having smooth "spin" compared to the bike I was using.

I am not a SW engineer so I am not sure how to get this implemented into the existing program I have. Do I have to wait until the original programmer integrates this changes into the new release and install through "npm" command? or is there way for me to get this implmented on my bike now?

[jeremydk]

Going to test more on actual hardware (rather than bike=bot) before looking to fully merge -- wanted to open up for comments early.

[jeremydk]

Testing on an actual bike (versus just the bot) showed incongruities when cadence would update. Given that a human tends to do that, identified another course of action. Going to test run with this.

[jeremydk]

Working well on my end after a couple of rides -- Considering this ready for review.

[ptx2]

Hi, please forgive me for the slow reply, I've been away.

Thank you for finding and submitting this. It turns out the main issue with over-reporting the cadence is actually very simple and comes down to me making a typo when refactoring this for release ;) The scale to convert milliseconds to 1/1024ths was inverted. Fixing that got Zwift reporting pretty much exactly the cadence reported by the bike. However there was still a little flapping (e.g. 100 rpm would flap between 99 and 100 in Zwift) which is indeed caused by execution timing as you pointed out. I was able to change those calculations so are not dependent on execution timing and now Zwift is reporting exactly 100 rpm. I opened a PR with those two changes here #15

Regarding the Garmin issue, would it work to change this line so that it also sends the crank data?

gymnasticon/src/app/app.js

Line 123 in e0d7bd1

this.server.updateMeasurement({ power });

e.g. this.server.updateMeasurement({ power, crank: this.crank });

(It's possible it needs to happen on line 114 also.)

I think if we make the change there instead of in the underlying code, the app remains in control of the decision of whether to include crank data. (e.g. perhaps it is useful to send a packet without crank data for some other device).

[jeremydk]

Not a problem! I think your proposed fix is cleaner, and I’m happy to go down that route. For the Garmin, I can pull trunk on my raspberry pi and test against actual hardware. I think optionally sending crank info is still the right call per the BLE spec itself, and whenever possible I’d rather hold to that correctness. I’ll validate this weekend and get back to you.

…

On Sat, Sep 19, 2020 at 6:00 PM ptx2 ***@***.***> wrote: Hi, please forgive me for the slow reply, I've been away. Thank you for finding and submitting this. It turns out the main issue with over-reporting the cadence is actually very simple and comes down to me making a typo when refactoring this for release ;) The scale to convert milliseconds to 1/1024ths was inverted. Fixing that got Zwift reporting pretty much exactly the cadence reported by the bike. However there was still a little flapping (e.g. 100 rpm would flap between 99 and 100 in Zwift) which is indeed caused by execution timing as you pointed out. I was able to change those calculations so are not dependent on execution timing and now Zwift is reporting exactly 100 rpm. I opened a PR with those two changes here #15 <#15> Regarding the Garmin issue, would it work to change this line so that it also sends the crank data? https://github.com/ptx2/gymnasticon/blob/e0d7bd1126b86b858939150f512115b24ef5de2a/src/app/app.js#L123 e.g. this.server.updateMeasurement({ power, crank: this.crank }); (It's possible it needs to happen on line 114 also.) I think if we make the change there instead of in the underlying code, the app remains in control of the decision of whether to include crank data. (e.g. perhaps it is useful to send a packet without crank data for some other device). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADAM6MZ72RIH4JEZ6GVE64TSGVH2TANCNFSM4Q63XJAA> .

[jeremydk]

I’ve dropped to origin/1.0.5 and merged in the Peloton changes. For my Garmin, I also needed to remove the descriptors from the macOS branch. I ran a quick test without the proposed changes, and can confirm I see the intermittent dropouts of cadence as it falls below 60rpm or so. I’ll patch before a ride later today and pull some data.

…

On Sat, Sep 19, 2020 at 6:10 PM Jeremy Klein ***@***.***> wrote: Not a problem! I think your proposed fix is cleaner, and I’m happy to go down that route. For the Garmin, I can pull trunk on my raspberry pi and test against actual hardware. I think optionally sending crank info is still the right call per the BLE spec itself, and whenever possible I’d rather hold to that correctness. I’ll validate this weekend and get back to you. On Sat, Sep 19, 2020 at 6:00 PM ptx2 ***@***.***> wrote: > > > Hi, please forgive me for the slow reply, I've been away. > > > Thank you for finding and submitting this. It turns out the main issue > with over-reporting the cadence is actually very simple and comes down to > me making a typo when refactoring this for release ;) The scale to convert > milliseconds to 1/1024ths was inverted. Fixing that got Zwift reporting > pretty much exactly the cadence reported by the bike. However there was > still a little flapping (e.g. 100 rpm would flap between 99 and 100 in > Zwift) which is indeed caused by execution timing as you pointed out. I was > able to change those calculations so are not dependent on execution timing > and now Zwift is reporting exactly 100 rpm. I opened a PR with those two > changes here #15 <#15> > > > Regarding the Garmin issue, would it work to change this line so that it > also sends the crank data? > > > > https://github.com/ptx2/gymnasticon/blob/e0d7bd1126b86b858939150f512115b24ef5de2a/src/app/app.js#L123 > > > e.g. this.server.updateMeasurement({ power, crank: this.crank }); > > > (It's possible it needs to happen on line 114 also.) > > > I think if we make the change there instead of in the underlying code, > the app remains in control of the decision of whether to include crank > data. (e.g. perhaps it is useful to send a packet without crank data for > some other device). > > > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#13 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADAM6MZ72RIH4JEZ6GVE64TSGVH2TANCNFSM4Q63XJAA> > . > > >

[jeremydk]

Looks like the proposed change is working nicely. I'm happy to discard this PR. I've got some further modifications to the Peloton code once #12 is moved out of draft state.

Thanks for the work here, really appreciate it.