mapping files for Hercules DJControl Inpulse 300

[DJPhatso]

New submission for the mapping files for Hercules DJControl Inpulse 300, this time based on 2.2 (surely hope it is....)

[Holzhaus]

this time based on 2.2 (surely hope it is....)

You can check this with git log:

$ git log --oneline
c00c0ab3b7 (HEAD, DJPhatso/Hercules-Inpulse-300) mapping files for Hercules DJControl Inpulse 300
df57f34088 (upstream/master) Merge pull request #2452 from esbrandt/lp1857813
[...]

Unfortunately, the second latest commit is the HEAD of the master branch, so it's still based on 2.3. If you'd post an error message or the steps that you did we could help you find the issue.

I just checked out your branch ran the rebase command:

$ git checkout Hercules-Inpulse-300
$ git rebase --onto upstream/2.2 upstream/master
git rebase --onto upstream/2.2 upstream/master
First, rewinding head to replay your work on top of it...
Applying: mapping files for Hercules DJControl Inpulse 300

Now the branch is based on 2.2:

$ git log --oneline
ee3dcb2b1d (HEAD -> Hercules-Inpulse-300-rebase) mapping files for Hercules DJControl Inpulse 300
37470c000c (upstream/2.2) Merge pull request #2459 from uklotzde/denon-mc6000mk2-midi
[...]

If you don't want to rebase it yourself (i.e. run the git rebase comment I posted above), you can also use my rebased version instead (which is more complicated though). If you want to do the latter, you need to run:

$ git checkout Hercules-Inpulse-300
$ git remote add Holzhaus https://github.com/Holzhaus/mixxx.git
$ git fetch Holzhaus
$ git reset --hard Holzhaus/Hercules-Inpulse-300-rebase

In case you need help, please let me know. I'll hold off the rest of the review until you rebased.

Sorry for the hassle. If you don't care about this mapping being included 2.2.4 and just to get this over with, we can also ignore the rebase part and merge this into master instead. It will then go into Mixxx 2.3.0.

[DJPhatso]

Ok. ran the rebase from your version and that seems to have gone through...

[Holzhaus]

Great. You still need to push the changes to your repository:

$ git push - f origin Hercules-Inpulse-300

[DJPhatso]

Done.

[Holzhaus]

Since use eslint for checking JS code now, I pointed out two comments that should be removed, After you did that you can either run eslint --fix path/to/controller-script.js manually or just push the changes and have a look at the code style issues that are pointed out by the CodeFactor.io check below this PR.

[Holzhaus]

Please remove lines 50-67. We allow unused parameters if they are starting with an underscore.

Example:

// The function below uses value and control, but not group, so we prefix group with an underscore
//DJCi300.vuMeterUpdate = function (value, group, control) {
DJCi300.vuMeterUpdate = function (value, _group, control) {
    value = (value * 127) + 5;
    switch (control) {
        case "VuMeterL":
            midi.sendShortMsg(0xB0, 0x40, value);
            break;
        case "VuMeterR":
            midi.sendShortMsg(0xB0, 0x41, value);
            break;
    }
};

[Holzhaus]

Please remove the whole JSHint configuration block. This isn't necessary anymore, were using eslint now.

[DJPhatso]

Corrected some Code Factor and comment removed. The rest will have to wait until next week.

[Holzhaus]

Nice. BTW, I think you can fix most (if not all) of the remaining CodeFactor issues automatically by running:

$ eslint --fix path/to/controller-script.js

[DJPhatso]

Thanks for the continued help Jan.
I guess I need to install or enable something for esling to work?

Using the command you provided from inside the Mixxx folder gives me eslint: command not found

[Holzhaus]

yes, you need to install eslint: https://eslint.org/docs/user-guide/getting-started#installation-and-usage
Which OS are you on?

I did open a PR where I did run eslint --fix for you, but this has merge conflicts now.

[Holzhaus]

Thanks you for your work @DJPhatso. I think we're getting close. The mapping seems well thought out and apart from some minor issues with the code I think this ready to merge.

One thing I notices on the wiki page:

When in FX mode, each pad will send multiple and different Note and CC messages. As these could not all be used properly with Mixxx current effect framework still in development, the pads have not been assigned in the original mapping.

Can you elaborate? What exactly is the problem?

[DJPhatso]

Regarding the FX pad configuration:

Each pad sends specific MIDI command and timing to simulate specific effect actions. For example:

• activate delay/
• bring FX level up to half slowly
• move Dry-wet to 75%
• Etc

Included is an example assignment for one the pad:
PAD_FX_Example.xlsx

The main problem is that this was made to be used with very specific effects in mind, so when mapping for another software, these have to be taken into account. Given that Mixxx's FX framework is still in development, I figured I would wait before spending too much time on this part of the mapping.

[DJPhatso]

Left to be done:

• Merging of jogwheel functions (if necessary)
• Implement Soft takeover for Pitch (see question)

[Holzhaus]

There are still some unresolved conversations. If you fixed them please mark them as "resolved".

[Holzhaus]

@DJPhatso Is this ready to review or is there something left to do?

[DJPhatso]

Not yet. Hopefully I'll get a chance to work on the Soft-takeover for the pitch today, but my biggest hurdle is to do with the merging of the jogwheel functions; are those necessary or mostly code cosmetic ?

I ask because the same code is actually used in the DJControl Starlight #2061 (which has been accepted as official mapping), and honestly since Kerrick came up with all these improvements (my first draft was using the jog wheel code example from the wiki ), I wouldn't know where to start to make mergers without braking something.

[Holzhaus]

@DJPhatso I resolved the remaining issues for you: DJPhatso#5 Please have a look.

I understand that it's annoying that I ask you to fix code issues when a mapping with similar code already exists in Mixxx. However, it's important that we keep code as simple as possible to be able to maintain it later on. Right now, we have a lot of overcomplicated mappings that don't meet our coding standards, and it's becoming a burden when other users want to improve them.

If you verified that the PR above works and you merged it, we can integrate this mapping. I'll probably squash your commits because the commit messages are not really meaningful. 😉

[Holzhaus]

CI failures are unrelated, LGTM. Thank you very much for your contribution, @DJPhatso!

@Be-ing do you want to take a look? If not I'll merge this tomorrow and squash the commits.