Python 3 Support, PM5 Bug Fix, and Updated Readme #8
Conversation
Python 2.7 hasn't been maintained in over 6,000 years. It's time.
This change applies the fix recommended by @mlyonupdesigns here: wemakewaves#5 Page 6 of the PM5 CSAFE specification indicates that the spec was revised on 04/06/23 by Mark Lyons, which is likely the change mentioned in the linked issue: Changed USB report ID 4 size from 62 to 500 bytes; added some a dditonal enumeration definitions; V0.26 See: https://www.concept2.com/files/pdf/us/monitors/PM5_CSAFECommunicationDefinition.pdf This change is said to be necessary for compatibility with PM5 ergs, which happens to be what I have on hand. I have confirmed that this commit enables the statshow.log command to function correctly with a PM5 Concept2 RowErg. Additional information: I am runing this on NixOS over USB with 3.13.
|
ooh yay one of my issues is fixed in this PR! Also i think https://github.com/droogmic/Py3Row also shares history with this project, so maybe there's changes in there that are worth bringing in? If you don't get any traction on this PR, I might also able to help keep an eye on a fork over on https://github.com/OpenRowingCommunity/, where I've been building (and also forking for the purposes of collecting) several cool open projects that relate to rowing |
|
@MoralCode I've been using this PR branch pretty actively for the last three weeks. I'm waffling over whether I want to make further improvements (eg restructure it to follow python3's module standards), or if I want to start over and write my own. I'm not confident that this repository is being monitored by it's maintainers anymore, so a fork might be a good idea in the mean time. If you are willing to maintain one, I'd be happy to contribute improvements to it. |
Forking is kinda a nuclear option. Maybe collaborating with @droogmic to bring this change into his existing fork could work. I've been thinking a lot recently about the burdens of maintainership, especially since I have merge access to a different project, but since i dont use the library, and dont have time to thoroughly test it (and dont want to break things with a YOLO merge), I havent been merging PRs. While my ultimate goal is to create a small community of well maintained open software in the rowing space, I feel such a community needs a core maintainer group that either deeply cares about the work, or is paid to care, or both, in order for such forks to not suffer the same fate as many other similar small open source projects. But that runs into two hard problems in FOSS: finding new maintainers who have as much passion as the creator, and getting funded. I'd love to support this kind of thing though, I just dont know if I can singlehandedly guarantee it will be sustainable. |
|
Hi both, thanks for the ping. I wasn't aware there was interest in this repo. My participation is low simply because I don't currently own a Concept 2... So it makes playing with this code much less fun. |
There was a problem hiding this comment.
actually, looking at the diff, it looks like there are no significant/useful changes made in this repo since you forked it into Py3Row
@Aeva Do you think you could port this PR over to that repo? Heres what it looks like it would take:
- (Un?)fortunately It looks like most of the changes (print statements and integer division) are already made on the Py3Row repo, so you dont have to include those commits
- the fix from Concept2 PM5 Firmware Change In April 2023 Has Broken USB Functionality #5 can be cherry picked with no merge conflicts
- the README fixes are still relevant (Py3Row still has some older/broken links) but the readme has changed so much that it may be easier to just remake your changes over there. The changes to the README from this repo seem generally unimportant as well so that makes things easy
| print "Stroke Pace = " + str(result['CSAFE_GETPACE_CMD'][0]) | ||
| print "Stroke Units = " + str(result['CSAFE_GETPACE_CMD'][1]) | ||
| print("Stroke Pace = " + str(result['CSAFE_GETPACE_CMD'][0])) | ||
| print("Stroke Units = " + str(result['CSAFE_GETPACE_CMD'][1])) |
There was a problem hiding this comment.
This and all the print() fixes are already fixed in Py3Row
| elif maxmessage <= 63: | ||
| message.insert(0, 0x04) | ||
| message += [0] * (63 - len(message)) | ||
| ## see https://github.com/wemakewaves/PyRow/issues/5 | ||
| #elif maxmessage <= 63: | ||
| # message.insert(0, 0x04) | ||
| # message += [0] * (63 - len(message)) |
There was a problem hiding this comment.
This change (i.e. just the commit this was in) can be straight up cherry picked onto the py3row codebase with no merge conflicts
Thanks for keeping the commits organized and separated btw!
| datapoints = results['CSAFE_PM_GET_FORCEPLOTDATA'][0] / 2 | ||
| datapoints = results['CSAFE_PM_GET_FORCEPLOTDATA'][0] // 2 |
There was a problem hiding this comment.
This has already been fixed in Py3Row so no need to port it over
Hello!
This pull request resolves issues #5 and #6, and several minor corrections to convert the code to run in Python 3.13.
I have confirmed that both
statshow.pyandstrokelog.pyboth function correctly on my Concept2 RowErg, which has a PM5 module. I am unable to test whether this is disruptive for older machines, however.I hope this is useful! Let me know if you have any questions or concerns. I am happy to make further revisions to these changes.