Closed mojca closed 2 years ago
josephernest
commented
2 years ago @mojca @johnroper100 I just created this python3 branch: https://github.com/josephernest/SamplerBox/tree/python3
Can you modify your PR to merge into this new python3 branch?
josephernest
commented
2 years ago Also can you split this PR into maybe 3 or 4 smaller ones?
I like to merge from Github UI these days (rather than command line) but the web interface doesn't seem to have git cherry-pick to choose which commit we take, and which commit we don't take.
mojca
commented
2 years ago Also can you split this PR into maybe 3 or 4 smaller ones?
Yes and no.
Yes, I gladly split them once you tell me what precisely you want, or what you want first.
These commits depend on each other to quite some extent, so if I split the code to four smaller PRs, I'll have to fix all the other three PRs as soon as you merge the first one, then again the remaining two PRs once you merge the second one etc.
I like to merge from Github UI these days (rather than command line) but the web interface doesn't seem to have
git cherry-pickto choose which commit we take, and which commit we don't take.
Do you only like the UI or do you also really really want to stick with merge commits as well? (Personally I don't like the super complex history that these merge commits create even for trivial one-character fixes. It quickly becomes impossible to follow. But that's a personal taste, not general truth.)
Linear history is still fully compatible with nice user interface when using the "right" settings.
josephernest
commented
2 years ago Oops I completely did a mess with a wrong PR with a merge in "rebase" mode between python3 and master: https://github.com/josephernest/SamplerBox/network, I don't know how to revert PR 52 and 53. Maybe you can wait until this is solved before doing new PR. It should be solved in a few minutes
josephernest
commented
2 years ago Summary of our chat, and of the parts which need to be checked before merging:
Make samplerbox.py work with rtmidi-python
Install python-rtmidi instead of rtmidi-python=> We need to double check the new one works on:
PC/Win Python 2.7
PC/Win Python 3
RPi Python 2.7
RPi Python 3
If anybody wants to do these 4 tests, it would be interesting. Is it available for the 4 platforms via pip without any recompilation, or does it need to have gcc (or MSVC++ on Win) installed to be able to compile it? (I remember one of these modules were not shipped in binary wheel in pip a few years ago). When this is checked, we can merge the commit related to this.
(also discussed here: https://github.com/josephernest/SamplerBox/pull/43#issuecomment-1208765608)
Support easy configuration of serial port
Logging from serial portAs discussed on chat, this is a custom system you designed for your device (using COM port on Windows, etc.), and this is probably better in a fork. In the main repo, we support 2 cases: 1) USB MIDI keyboard/interface/controller (with a class compliant USB MIDI) 2) RPI GPIO <--- the standard official MIDI circuit with optocoupler and resistors (see schematics) <--- 31250 baud <---- MIDI OUT of MIDI device, e.g. Roland or Yamaha synth (just an example)
Replace xrange with range for python3 compatibility
Prefix binary strings for python 3 compatibility
Integer division (not sure)Yes good
Some replacements of percentange sign (not sure whether they are correct)We need to double check this, I don't remember exactly why these characters were needed (with slash/antislash escaping), but it probably was, so we need to check.
About 38400 vs 31250 baud etc. (see https://github.com/josephernest/SamplerBox/issues/49) we need to test with the MIDI IN circuit (here https://www.samplerbox.org/files/samplerbox_schematics.jpg we just use the MIDI IN circuit from the official MIDI specifications) + a standard synth Roland Yamaha, etc.
josephernest
commented
2 years ago Which error do you get in Python 3 if you keep the original:
pattern = pattern.replace(r"\%midinote", r"(?P<midinote>\d+)").replace(r"\%velocity", r"(?P<velocity>\d+)")\
.replace(r"\%notename", r"(?P<notename>[A-Ga-g]#?[0-9])").replace(r"\*", r".*?").strip() # .*? => non greedywithout removing the \ in \%.
Could you past the error, and an example of definition file with generates the error?
Thanks @mojca.
mojca
commented
2 years ago Make samplerbox.py work with rtmidi-python Install python-rtmidi instead of rtmidi-python=> We need to double check the new one works on: PC/Win Python 2.7 PC/Win Python 3 RPi Python 2.7 RPi Python 3 If anybody wants to do these 4 tests, it would be interesting.
I tested three out of four (I didn't install python 2.7 on Windows), but it would of course be helpful if someone checked this independently. Now I seem to be at least a bit confused because the website doesn't mention support for 2.7.
Is it available for the 4 platforms via
pipwithout any recompilation, or does it need to havegcc(orMSVC++on Win) installed to be able to compile it? (I remember one of these modules were not shipped in binary wheel inpipa few years ago). When this is checked, we can merge the commit related to this.
It's only available in binary form for python 3.6 to 3.9 for Windows. So I'm mostly missing python 3.10 on that list, not so much python 2.7. (Nobody says that Windows users need to use Python 2.7 when they have a newer version available and working. I can request publishing wheels for 3.10.)
But doesn't one need the compiler for samplerbox_audio as well?
Support easy configuration of serial port Logging from serial portAs discussed on chat, this is a custom system you designed for your device (using COM port on Windows, etc.), and this is probably better in a fork.
I removed this part. This code could in fact be much easier (I added some error handling because I wanted user-friendly errors when a port was missing etc.), but I'll revisit it once I add support for easier configuration that won't require messing with the source code. Let's forget about it for now.
Integer division (not sure)
Is this one also correct? It wasn't technically needed, I just thought it might be better to use explicit integer results. If yes, I'll just modify the commit message.
Some replacements of percentange sign (not sure whether they are correct)We need to double check this, I don't remember exactly why these characters were needed (with slash/antislash escaping), but it probably was, so we need to check.
Yes, we definitely need to check.
Which error do you get in Python 3 if you keep the original:
pattern = pattern.replace(r"\%midinote", r"(?P<midinote>\d+)").replace(r"\%velocity", r"(?P<velocity>\d+)")\ .replace(r"\%notename", r"(?P<notename>[A-Ga-g]#?[0-9])").replace(r"\*", r".*?").strip() # .*? => non greedywithout removing the
\in\%.Could you past the error, and an example of definition file with generates the error?
The note names were simply not recognised. I don't remember whether there was in fact any error as I needed to add some additional debugging myself to see what was actually happening / why the sound was missing. We are using simple C4.wav, D4.wav with the definition %notename.wav.
Here's a minimal example to demonstrate the problem:
import re
pattern1 = '%notename.wav'
pattern2 = re.escape(pattern1.strip())
pattern3a = pattern2.replace(r'\%notename', r'foo')
pattern3b = pattern2.replace(r'%notename', r'foo')
print(pattern3a)
print(pattern3b)which outputs
%notename\.wav
foo\.wavin python 3.10 under Windows and
foo\.wav
\foo\.wavin python 2.7 under Linux (I have those two at hand, I can test more later).
@vgroenhuis
josephernest
commented
2 years ago @mojca There was a change at Python 3.7 about the % escaping: https://docs.python.org/3/library/re.html#re.escape
Changed in version 3.7: Only characters that can have special meaning in a regular expression are escaped. As a result, '!', '"', '%', "'", ',', '/', ':', ';', '<', '=', '>', '@', and "`" are no longer escaped.
So, after thinking about it, I'll do a Python 3 patch, and it won't be retro-compatible to Python 2, maybe it was needed after all. (I don't want to have many cases in the code "if Python2 then", "if Python3 then"...)
About rtmidi_python (https://github.com/superquadratic/rtmidi-python/), I've tested on Linux and Windows, and pip3 install rtmidi-python works in both cases, see more information here: https://github.com/josephernest/SamplerBox/pull/43#issuecomment-1209160064
I'll close this PR for now, because it is too complex for me with git to cherry pick one commit or the other, I am unfortunately not experienced enough with git to do this correctly and safely.
I'll push a new commit with the Python 3 modifications (and a few others), of course I'll mention your commits!
This pull requests includes various changes that I couldn't easily split into individual pull requests as they would conflict with each other, but I would be more than happy to isolate some changes out and remove others.
The changes currently include
python-rtmidi(covered by #43)What's still missing:
What I'm puzzled about:
see hack in /boot/cmline.txt: 38400 is 31250 baud for MIDI!", but there's no/boot/cmline.txtanywhere and the baud rate was in fact 38400, not 31250 as claimed. Since we control the midi source, we just switched the baud rate to 115200 on both sides. When sending MIDI messages at 31250 we had tons of strange issues, with midi messaged being misinterpreted at times (while working at other times).