Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complete getsysex.sh rewrite using cUrl to maximize compatibility #444

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

donluca
Copy link
Contributor

@donluca donluca commented Feb 28, 2023

I have completely rewritten the bash script with several improvements including:

  • Using cUrl which has been included in most of the modern Unix distros (macOS included)
  • Check for internet availability
  • Check if the target website is online
  • Make it easier to add links for people who want additional voices
  • Extra voices downloaded by default, all standard cartridges available for the original DX7

@donluca donluca changed the title Complete rewrite using cUrl to maximize compatibility Complete getsysex.sh rewrite using cUrl to maximize compatibility Feb 28, 2023
Forgot to add "exit" if a connection cannot be established due to secure connection issues.
@donluca donluca mentioned this pull request Feb 28, 2023
There was a bunch of nonsense which I've cleaned up.
Added a 0 to the padding to match the description on the wiki where it states that the file name should be 000000_something.syx (six zero padding).
@probonopd
Copy link
Owner

probonopd commented Apr 2, 2023

Personally I'd like to see each and every URL and file path. So that someone who uses a different tool for downloading has no issues reproducing the same result. It must be visible at first glance which patch from which URL ends up in which file.

So please:

  • Use one curl command per file
  • Make visible the path from where the file is downloaded
  • Make explicitly visible (hardcoded) the target path

I want to avoid that the same banks end up at different numbers on different MiniDexed installations just because someone added yet another URL without considering that this changes all the numbers.

Pseudo-code:

CURL=curl -...

HOST1=https://blackboxes.host/foo/bar

"${CURL}" "${$HOST1}"/some.syx > sysex/voice/000000_rom1a.syx
"${CURL}" "${$HOST1}"/some.syx > sysex/voice/000001_rom1b.syx

@donluca
Copy link
Contributor Author

donluca commented Apr 2, 2023

I respectfully disagree with your approach, but I think I can make some subtle changes to actually accomodate some of your requests.

The reason I disagree is that I'd like non-tech savvy users to be able to easily add patches to the script without learning Bash or any kind of scripting language.

The way the script is currently coded is that the "standard" (ie: the one included already) sysex files get downloaded and numbered automatically (and always in the same order, for consistency, as you requested) and then the user, at the end of the list, can add his own links for extra patches he wishes to download in addition to the ones already included.

I'll address every concern you've expressed:

It must be visible at first glance which patch from which URL ends up in which file.

I can declare a variable at the beginning which holds the path where the files get saved, so people will know where those files end up.
The format of the files (000000_something.syx) is already outlined in the Wiki and, I believe, I've documented it inside the script itself.
Besides, it's not that convoluted: it's just the name of the sysex file downloaded (which is clearly visible in the link) with numbering at the start (padded with 6 zeros).

Use one curl command per file

This is redundant and greatly hurts the visibility of the links and the readability for those not familiar with command line commands and Bash scripting.
A list of links is way more "friendly" and easy to understand and use.

Make visible the path from where the file is downloaded

The list of links has the complete URL so it is already visible.

Make explicitly visible (hardcoded) the target path

As I said before, I can do this by declaring a variable with a good, descriptive name at the beginning of the script, so the user knows where the files are being saved.

I want to avoid that the same banks end up at different numbers on different MiniDexed installations just because someone added yet another URL without considering that this changes all the numbers.

This can never happen because I've explicitly said in the script to add extra links at the end of the list.
This means that the "included" banks are always getting the same number in the same order and will be the same across all the MiniDexed installations, just like you requested.
If the user disregards the instructions and acts on his own, (for example he starts adding links at the start of the list or in the middle of it) I can't do anything about that, but it's on him for not reading a single line of instructions.

I'd be curious if you like more the approach I took in a previous commit.

@probonopd
Copy link
Owner

probonopd commented Apr 2, 2023

I'd be curious if you like more the approach I took in a previous commit.

Yes, significantly: At least one can immediately see which URL gets saved to which file.

@donluca
Copy link
Contributor Author

donluca commented Apr 2, 2023

Ok, I'll revert to that commit then and make a small modification where I'll declare a variable at the beginning specifying the folder into which those files will be saved into for better visibility, as you requested.

I still personally prefer the other approach where the link list has just the link as I find it more user-friendly and easier for users to add new links, but eventually we can come back to this at a later time, right now it's more important to implement a new getsysex script which works across different operating systems.

I'll make the changes tomorrow.

Reverted code to an older approach and made the target directory more visible
@donluca
Copy link
Contributor Author

donluca commented Apr 2, 2023

I had a bit of spare time so I just did it and tested it on macOS and it works correctly.

@probonopd
Copy link
Owner

In the (near?) future, we might just download the zip with the curated collection instead, and unzip that.
This script would become a two-liner.

@donluca
Copy link
Contributor Author

donluca commented Apr 8, 2023

I'm personally against this, as I don't believe that a giant library with 2000+ banks is a "sane default" for a project such as Minidexed.

The standard DX7 voices and its Voice Cartridges, IMHO, represent an excellent starting point which has the bonus of making the Minidexed boot almost instantaneous.
Those cartridges cover a pretty big range of instruments and noises which, when you start to layer them and play with performance, would give a vast choice of sounds to the user and will probably keep him busy/entertained for a long time.
After all, if Minidexed is trying to mimic the behavior of a real DX7, then it would make sense to make available what a "normal" buyer of a DX7 would have had at his disposal back then.

We'll put a link somewhere in Minidexed to the curated library so that people looking for more patches will be able to download it, along with a bit of documentation, especially about longer boot times.

@Banana71
Copy link

Banana71 commented Apr 8, 2023

At the latest when saving individual patches works (I'm looking forward to it), my library will shrink to a minimum. Then I would load, create or change the sounds I need with the help of Dexed on the PC and transfer them to the miniDexed via MIDIDump and then save them in a bank and process them in a performance.

Nevertheless, I see a benefit if the miniDexed is able to load large libraries.

@diyelectromusic
Copy link
Contributor

Presumably this change is still in progress?

I'm only asking as we really should update the existing script to number voice files starting from 000001 following the recent changes! But if this is close, maybe this update should just take that into account and replace it anyway?

@donluca
Copy link
Contributor Author

donluca commented Apr 17, 2023

With all the discussion and changes about skipping EMPTY/INIT VOICE/etc... where are we now with the USER BANK idea?
Should we still make one and put it at the end?

EDIT: and yes, this is still in progress as I'm near to wrapping up the library thing.

@diyelectromusic
Copy link
Contributor

With all the discussion and changes about skipping EMPTY/INIT VOICE/etc... where are we now with the USER BANK idea?

To be totally honest, I've completely lost track!

I think the idea was still to have a user editable blank bank with a different voice name wasn't it? Then it could just "be available" for people to copy in I guess... or there was some talk about have this as the first or last one in the "standard set"...?

@donluca
Copy link
Contributor Author

donluca commented Apr 17, 2023

I hate to say this... but I'm afraid that I've completely lost track as well.

I think that the last thing we decided was to change the code (which has already been merged from #466 ) so that while browsing voices, the UI would automatically skip voices named EMPTY so that "blank" voices which followed Yamaha's standard name of INIT VOICE would still be visible.

So... unless I'm mistaken, we should be adding a blank USER BANK at the end of the download filled with INIT VOICE which is what Dexed does when creating a new bank from scratch.

About changing the numbering to start form 000001... no problem.
And, actually, now that you mention it, I should do this for the library as well...

So it's confirmed that voice numbering will start from 000001 onwards and not from 000000?

Also, I've noticed that in the code it checks for [NNNN]N_name, so only 5 numbers are required?
Honestly, we've seen the downside of having a billion banks, so I think that, for readability reasons, we could shorten the numbering down to four, like 0001_name.syx as I don't think anyone will ever reach more than 9999 banks (which would also take forever to load).

Since you're already working on the numbering, it might be worth to think about this as well.

@diyelectromusic
Copy link
Contributor

diyelectromusic commented Apr 18, 2023

I think that the last thing we decided was to change the code (which has already been merged from #466 ) so that while browsing voices, the UI would automatically skip voices named EMPTY so that "blank" voices which followed Yamaha's standard name of INIT VOICE would still be visible.

I'm not sure I follow the logic of that. If someone is at the point of having banks that have been changed then they are probably editing out blank voices anyway. I thought this was to avoid having to scroll through lots of empty voices in banks obtained from the Internet...

I'll have to re-read the discussions... (unless someone can accurately summarise where things have got to?)

Update: Ah ok - yes, I remember! #466 (comment)
So we're preserving the original DX7 behaviour of INIT_VOICE then...

Kevin

@donluca
Copy link
Contributor Author

donluca commented Apr 18, 2023

Yes, that seemed to be it.

What do you think about shortening the numbering of the file names from 6 numbers to 4, as I don't think anyone will ever go over 9999 banks?

As soon as you PR the change to have the banks starting at 000001 (or 0001) I'll make the changes to both this script and the library to accomodate this change.

@diyelectromusic
Copy link
Contributor

I think it should remain five digits as technically they can have numbers up to 16384 (even if there is just one bank). The PR for 1-numbered voices is already merged - see #473 .

Kevin

@donluca
Copy link
Contributor Author

donluca commented Apr 18, 2023

As far as I can tell, I've seen all files with 6 digits numbering, so we're going to reduce those to 5 digits, and start from 00001, correct?

@diyelectromusic
Copy link
Contributor

The code just does this: sscanf (sBankName, "%u", &nBank) != 1) so is completely ambivalent to the number of digits and doesn't even need to include leading zeros either.

It is just a convention in the documentation and scripts.

Kevin

@donluca
Copy link
Contributor Author

donluca commented Apr 18, 2023

I've added the blank user bank and updated the getsysex.sh script accordingly.

@probonopd
Copy link
Owner

Please keep it /bin/sh. Needs to be tested to work on POSIX shells, not just bash. E.g., I am using FreeBSD which has no bash by default.

@probonopd
Copy link
Owner

probonopd commented Apr 18, 2023

I've added the blank user bank

What is it for? Is it useful at the moment (without being able to save there)?
And it should be downloaded from the https://github.com/probonopd/MiniDexed/ repository, not from your fork.

@donluca
Copy link
Contributor Author

donluca commented Apr 18, 2023

Please keep it /bin/sh. Needs to be tested to work on POSIX shells, not just bash. E.g., I am using FreeBSD which has no bash by default.

Done.

I've added the blank user bank

What is it for? Is it useful at the moment (without being able to save there)?

To be consistent with what the Wiki says. We discussed it some time ago and it was one of the points you made on not touching the Yamaha INIT VOICE and rather use a custom empty bank named EMPTY in the curated library.

The only change is that instead of NO NAME it is called userBank.

This way the user won't have to scroll through, possibly, lots of banks to find an INIT VOICE.

@probonopd
Copy link
Owner

Any specific reason we can't name the bank NO NAME and the file 000xx_NO_NAME.syx? xx should be known and not changing, so that it is the same number for everyone.

@donluca
Copy link
Contributor Author

donluca commented Apr 18, 2023

NO NAME is non-descriptive and potentially misleading for a user looking forward creating his own voices, especially because there is no way to rename banks (or edit/save voices, for that matter) so that he can change it to something like "Luca's Bank" or "Peter's Bank", etc.

As for the "positioning": you can't beat "last" or "first".
They are known and will always be there, whatever you do with the banks.
Putting it last is ideal as you can access it just by starting MiniDexed and then going back one bank which will wrap around and put you right on the User Bank.

Leaving it at a fixed number is potentially a mess, because if the user will add other banks then the User Bank will end up in the middle and it will be harder to find... unless the user himself picks it and put it last himself, extra work which is completely unnecessary.

This way the user can use the script to either download the standard banks we provide in it or add his own freely, without worrying about where the User Bank will end up being, because it will always end up last.

@probonopd
Copy link
Owner

Maybe we should make it 16383_...syx? If I am not mistaken, This is the highest allowable bank number (128*128-1).

@diyelectromusic
Copy link
Contributor

Maybe we should make it 16383_...syx? If I am not mistaken, This is the highest allowable bank number (128*128-1).

(16364 now :))

@probonopd
Copy link
Owner

I am still not sure whether that is the right thing to do.
Why would the file have a different number than the MIDI message.
In the GUI, we count from 1. But in all technical implementations, one counts from 0.

@diyelectromusic
Copy link
Contributor

Why would the file have a different number than the MIDI message.

Because noone ever sees the MIDI message! As I've now said several times, the MIDI spec itself says "As with program numbers, banks begin counting from 1. Thus the actual bank number will be (MIDI value + 1)." It says itself "the actual bank number" and surely we'd want to the person using MiniDexed to be seeing and using "the actual bank number" as this is what they will be seeing in any MIDI tool and the MiniDexed UI.

So if we ask the user what bank number ROM_1A is they would (rightly) say "oh, its bank 1". But the MIDI message (which the user will NEVER see) rightly sends 0 according to the spec ("bank number - 1") but the gui will still correctly show "Bank 1: Rom 1A".

If a separate patch management programme on a PC is being used it will show the user "Bank 1" and they will be thinking "ah yes, bank 1 is the file 00001_Rom_1A I put on MiniDexed that shows up as 1: Rom 1A in the menu" and so on.

But in all technical implementations, one counts from 0.

Exactly. But users don't see the underlying technical implementation - the MIDI spec tells us as technical implementors to not expose 0-indexed bank numbers to humans (same as patch numbers in Program Change messages). All "user visible" interpretations of the bank number are supposed to be consistently treated as 1-indexed numbered. The fact that "underneath" MIDI uses 0-indexing is irrelevant - no-one sees the "on wire protocol". It is actually written in the MIDI Spec that this is how it should work!

As an analogy, every attachment to email is sent as base64-encoded data "on the wire" - but who actually cares about or sees that? The email programmes and users show the image or file or whatever - the base64 encoding is exactly that - an encoding, not meant for human reading.

The MIDI 0-indexed encoding is the same. People are meant to see the 1-indexed Bank Numbers, not the 0-indexed "on the wire encoding". As I say, on this the MIDI spec is pretty clear (for once)!

If we use 0-indexed files, it is going against the MIDI spec as it is exposing our internal implementation details to users; and it will cause inconsistency between user experiences of "I want this bank to be number X" when they set up MiniDexed and what every tool they will ever use from that point onwards (including the MiniDexed menu) will tell them is bank number X.

Seriously, I don't quite know how else to say it ;)

Kevin

@donluca
Copy link
Contributor Author

donluca commented Apr 23, 2023

Maybe we should make it 16383_...syx? If I am not mistaken, This is the highest allowable bank number (128*128-1).

This is actually a very good idea and quite "definitive" (in the literal sense of the term: that's the highest bank number MiniDexed supports).

@diyelectromusic what happens when there is a gap in the numbering on Minidexed? Say, we go from bank 00030_something.syx and there is nothing until 00050_somethingElse.syx?

@probonopd
Copy link
Owner

probonopd commented Apr 23, 2023

So @kevin you seem quite determined that starting the files with 1 is the correct thing to do. I trust your experience here.

@diyelectromusic
Copy link
Contributor

So @kevin you seem quite determined that starting the files with 1 is the correct thing to do. I trust your experience here.

Haha! To be honest, I don't care as such (I know you wouldn't guess from my comments!), but the MIDI spec is quite explicit in what it says on this one (for once) - there really does seem no doubt ;)

Kevin

@diyelectromusic
Copy link
Contributor

@diyelectromusic what happens when there is a gap in the numbering on Minidexed? Say, we go from bank 00030_something.syx and there is nothing until 00050_somethingElse.syx?

It doesn't care (anymore). It only cycles through loaded banks even if there are large gaps between the bank numbers.

Kevin

@donluca
Copy link
Contributor Author

donluca commented Apr 23, 2023

Great, then UserBank is going to be the last one numerically as well, so there won't ever be any issues, like @probonopd suggested, I'll make the change tomorrow and I believe this can be merged.

@donluca
Copy link
Contributor Author

donluca commented Apr 28, 2023

It is done, sorry for the wait, I have a lot of work to do, hopefully I'll be able to wrap up all my contributions shortly.

@probonopd
Copy link
Owner

probonopd commented May 1, 2023

Can we move https://github.com/probonopd/MiniDexed/raw/main/userBank.syx to https://github.com/probonopd/MiniDexed/raw/main/sysex/voice/userBank.syx please?

So that in this repository, userBank.syx will not be in the root directory but in sysex/voice. Similar to the performances.

@donluca
Copy link
Contributor Author

donluca commented May 1, 2023

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants