Skip to content

Traktor Kontrol S4 Mk3: add 4 effect units#14895

Draft
ronso0 wants to merge 2 commits intomixxxdj:2.6from
ronso0:s4-4effect-units
Draft

Traktor Kontrol S4 Mk3: add 4 effect units#14895
ronso0 wants to merge 2 commits intomixxxdj:2.6from
ronso0:s4-4effect-units

Conversation

@ronso0
Copy link
Member

@ronso0 ronso0 commented Jun 3, 2025

Uses Mixxx' standard effects mapping, supports effect focus mode.
https://manual.mixxx.org/2.5/en/chapters/effects#controller-effects-mapping

This is an attempt to merge the midi-components effect unit into the S4 one.
Bonus: the previous effect focus state is restored

I've been using a minimal, hacky version of this since.. a week after I got the S4 ; )
and finally I managed to backport this and clean it up!

Features / Extras

In addition to the standard fx mapping

  • Shift + ⯇/⯈ assign buttons do assign unit 3 / 4
  • focus state from last session is restored (it's already stored in mixxx.cfg but wasn't used)
  • ...

Breaking changes, due to switch to Mixxx standard mapping:

  • new control order, L->R:
    Meta knobs 1 | 2 | 3 | Mix knob
    Effect toggles 1 | 2 | 3 | Focus button
    (before it was X | 1 | 2 | 3)
  • there is no Main assign switch anymore, rightmost button is now the focus button
    (we may find a way to integrate that, but this had no prio for me)
  • rightmost button does not toggle all effects anymore

TODO

Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of surface comment, haven't tested it yet!

Comment on lines -1468 to -1479
shift: function() {
this.group = this.unit.group;
this.outKey = "group_[Master]_enable";
this.outConnect();
this.outTrigger();
},
unshift: function() {
this.outDisconnect();
this.outKey = undefined;
this.group = undefined;
this.output(false);
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature, which allows to enable post fader FX seem to be entirely lost. Could we move it so it can still be used somewhere? (e.g shift+long press)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that was the intention.

Then it would be good to make it a quick toggle / powerwindow button, ie. act on press.
And the other function (switch effect unit?) could be the longpress function as it's not time-sensitive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sounds good!

Comment on lines +1506 to +1510
// onShortPress: function() {
// // TODO dummy impl to avoid "Unknown control [EffectRack1_EffectUnit1], "
// // TODO seems this slips through in Button -> defaultInput()
// // console.warn(`focusButton shortPress`);
// },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Member Author

@ronso0 ronso0 Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I saw that warning being logged.
But I'll test again.

Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for coming back so late to this! I am planning to test this in the coming days! Feel free to prepare the manual update already :)

Comment on lines +1826 to +1827
console.warn("ERROR! new S4Mk3EffectUnit() called without specifying any unit numbers!");
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about throwing an exception? Otherwise, this will log the failure but will carry on the execution in a "broken" state.

Comment on lines +1829 to +1832
if (numbers.length > 2) {
console.warn("ERROR! new S4Mk3EffectUnit() called with more than 2 unit numbers!");
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - exception instead?

Suggested change
if (numbers.length > 2) {
console.warn("ERROR! new S4Mk3EffectUnit() called with more than 2 unit numbers!");
return;
}
if (numbers.length !== 2) {
throw Error("S4Mk3EffectUnit() constructed with more or less than 2 unit numbers!");
}

Comment on lines +1833 to +1842
let numCount = 0;
for (const num of numbers) {
if (typeof num === "number") {
numCount++;
}
}
if (numCount < numbers.length) {
console.warn("ERROR! new S4Mk3EffectUnit() called with array that contains other types than numbers!");
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let numCount = 0;
for (const num of numbers) {
if (typeof num === "number") {
numCount++;
}
}
if (numCount < numbers.length) {
console.warn("ERROR! new S4Mk3EffectUnit() called with array that contains other types than numbers!");
return;
}
if (!numbers.every(num => typeof num === "number")) {
throw Error("S4Mk3EffectUnit() constructed with array that contains other types than numbers!");
}

Comment on lines -1468 to -1479
shift: function() {
this.group = this.unit.group;
this.outKey = "group_[Master]_enable";
this.outConnect();
this.outTrigger();
},
unshift: function() {
this.outDisconnect();
this.outKey = undefined;
this.group = undefined;
this.output(false);
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sounds good!

// empty 'key' is not considered in Button/PowerWindowButton
onShortPress: function() {},
onShortRelease: function() {
// console.warn("focusButton shortRelease");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment cruft

Suggested change
// console.warn("focusButton shortRelease");

There is a bunch of them in the file, and they seem to have been added for a one off debugging, so I don't think it make sense to keep them, are you happy to remove them?

@ronso0
Copy link
Member Author

ronso0 commented Jan 5, 2026

Sorry for coming back so late to this! I am planning to test this in the coming days! Feel free to prepare the manual update already :)

Thanks for taking the time to review.
I didn't use this for long, just my personal implementation, so I think I might need to take another look at this and double-check the UX.

The manual is here mixxxdj/manual#777

@acolombier
Copy link
Member

Tested this mapping and unfortunatelly, I was unable to get the effect 3 and 4 to work as expected.

@acolombier
Copy link
Member

I think I might need to take another look at this and double-check the UX

Are you happy to make that PR a draft till you get a chance to look into this?

@ronso0
Copy link
Member Author

ronso0 commented Jan 10, 2026

Sure.

@ronso0 ronso0 marked this pull request as draft January 10, 2026 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants