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

Kontrol S4 Mk3 ES6 mapping #4056

Closed
wants to merge 1 commit into from
Closed

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Jul 4, 2021

This mapping is not compatible with Mixxx 2.3. It requires the main branch to work at all and #2618 for full functionality. I recommend making use of Git worktree to use both branches simultaneously.

This is complete though the jog wheel handling could be improved.

Turn the left encoder to beatjump. Press and turn the left encoder to adjust the beatjump size. Turn the left encoder with shift can be used to nudge the track.
Turn the right encoder to adjust the loop size. Press the right encoder to (de)activate a loop. Press the right encoder with shift to reactive an existing loop if looping is disabled, or if looping is enabled, jump to the beginning of the loop and stop playback. Turn the right encoder with shift to beatjump by 1 regardless of the configured beatjump size.

Shift + HOTCUES and record pad mode buttons can be used for manually setting the loop in and out points.

The button with the play icon adjacent to the library encoders (top right of the decks) loads the selected track to the preview deck and plays it. While the preview deck is playing, the library encoder beatjumps forward/backward 16 beats in the preview deck. The star button acts like pressing shift + tab and the playlist icon acts like pressing tab. The view button maximizes the library.

The buttons beside the gain knobs store the current gain of the deck as the track's default gain (#4031).

The default pad mode is mapped with the top row controlling intro & outro cues and the bottom row controlling hotcues 1 - 4. Press the HOTCUES pad mode button for hotcues 5 - 12 and the button next to it with the recording symbol for hotcues 13 - 20. Press the button for the active pad mode to switch back to the default pad mode. Note that the record pad mode button's LED is only red and does not support other colors, so it will not always match the deck color.
The pads can be switched to control samplers by pressing the SAMPLES button and a second page of samplers is available by pressing the mute icon. Press an unlit pad to load the selected track in the library to that sampler. While a sampler is loaded, it will be dimly lit if it is paused and brightly lit if it is playing. Pressing a loaded sampler plays from the cue point. Pressing a sampler pad with shift pauses it if it is playing and ejects it if it is paused. The script assigns the samplers to the crossfader by default, which can be disabled by setting samplerCrossfaderAssign at the top of the script to false.
The STEMS pad mode button toggles keylock.

Effect chain preset selection (FX SELECT buttons) requires #2618 to work. To load presets, press FX SELECT 1-4 buttons. Each selector button is lit in the color representing the preset it will load. Pressing and releasing a selector button will load that preset to all decks unless one of the QuickEffect enable buttons is pressed before the selector button is released. In that case, the selected preset will only be loaded to the selected deck. 16 presets can be selected with combinations of selector buttons, which is very powerful. While holding a selector button, the other buttons will change colors indicating more presets that can be selected by pressing a second selector button before releasing the first. For example, to load preset 5, hold button 1 then press button 2.

The effects mapping is currently basic with the knobs controlling the metaknobs and the buttons controlling the enable buttons. The leftmost knob of each effect unit is the mix knob. There is not yet any effect focusing or control of individual parameters.

The deck colors are set to match my Chroma Caps. Feel free to change them to your liking by editing the deckColors array at the top of the script.
image

@Be-ing Be-ing marked this pull request as draft July 4, 2021 03:15
@uklotzde
Copy link
Contributor

uklotzde commented Jul 4, 2021

Btw, I also purchased an S4 MK3 some time ago but didn't find the time to test it yet 🙈 So count me in ;)

@Be-ing Be-ing requested a review from Swiftb0y July 5, 2021 02:21
@Be-ing Be-ing force-pushed the kontrol_s4_mk3 branch 3 times, most recently from bec512b to 9235022 Compare July 5, 2021 02:24
res/controllers/Traktor-Kontrol-S4-MK3.js Outdated Show resolved Hide resolved
res/controllers/Traktor-Kontrol-S4-MK3.js Outdated Show resolved Hide resolved
res/controllers/Traktor-Kontrol-S4-MK3.js Show resolved Hide resolved
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 30, 2021

I have expanded the foundation of the Components library for HID and used it to map the play, cue, and deck toggle buttons of the left deck, now including LED output.

@Swiftb0y I know we had discussed wanting to decouple the Components library from I/O but this turned out not to be practical without ugly hacks. For HID we need to compare each field in an input packet to its state in the prior packet and only execute Components' input callbacks if their state has changed, similar to the common-hid-packet-parser.js library. Otherwise, each Component would need to track this state.

Without this, logic errors will occur excessively executing callbacks. For example, I ran into a logic error when holding the cue button for previewing, pressing the play button, then releasing the cue button. What should happen in this case is that the play Control gets set to 1, then cue_default gets set to 0 when the cue button is released. Without comparing input data to the old state, the input callback for the play button was inappropriately executed when releasing the cue button because they are in the same input packet, so immediately after cue_default was set to 0, play was toggled to 0 and the track stopped.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 30, 2021

 warning [0x5580786fc000] ControllerScriptHandlerBase: "Uncaught exception: file:///home/runner/work/mixxx/mixxx/res/controllers/Traktor-Kontrol-S4-MK3.js:173: TypeError: Cannot call method 'trigger' of undefined\nBacktrace: outTrigger@file:///home/runner/work/mixxx/mixxx/res/controllers/Traktor-Kontrol-S4-MK3.js:173\nconstructor@file:///home/runner/work/mixxx/mixxx/res/controllers/Traktor-Kontrol-S4-MK3.js:377\n%entry@file:///home/runner/work/mixxx/mixxx/res/controllers/Traktor-Kontrol-S4-MK3.js:472"
/home/runner/work/mixxx/mixxx/src/test/controller_mapping_validation_test.cpp:122: Failure
Value of: testLoadMapping(mapping)
  Actual: false
Expected: true
Error while validating /home/runner/work/mixxx/mixxx/res/controllers/Traktor Kontrol S4 MK3.hid.xml

This test failure is an artifact of the test setup. There is no problem when running Mixxx.

@Swiftb0y
Copy link
Member

For HID we need to compare each field in an input packet to its state in the prior packet and only execute Components' input callbacks if their state has changed, similar to the common-hid-packet-parser.js library. Otherwise, each Component would need to track this state.

Yes, that was my understanding as well. We already have a MidiMapper (as a module currently unused because its only exposed to the ModuleScriptingEngine) and we need a complementary HIDMapper as well.

I have never taken a look at our common-hid-packet-parser.js library myself so I can't really say much about it except that I heard others say its terrible...

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 30, 2021

I mapped the QuickEffect enable buttons and FX SELECT buttons which can be tested together with #2618. The QuickEffect enable buttons' colors represent the currently loaded preset. Press them toggle the enable switch of the QuickEffect chain. Long press and release to toggle the state again when releasing the button ("power window" behavior).

To load presets, press FX SELECT 1-4 buttons. Each selector button is lit in the color representing the preset it will load. Pressing and releasing a selector button will load that preset to all decks unless one of the QuickEffect enable buttons is pressed before the selector button is released. In that case, the selected preset will only be loaded to the selected deck. 16 presets can be selected with combinations of selector buttons, which is very powerful. While holding a selector button, the other buttons will change colors indicating more presets that can be selected by pressing a second selector button before releasing the first. For example, to load preset 5, hold button 1 then press button 2.

I was initially planning on including the FILTER button as a 5th preset selector button which would allow selecting up 25 different presets. However, the LEDs only support 16 different colors. Also, by design, there is no API to load a hardcoded effect. 16 presets is a lot to choose from, so I'm not really upset about this... but now I don't know what to do with this button. Maybe map it to the headphone split switch because it's right by the headphone mix and volume knobs? 🤷

@Be-ing Be-ing force-pushed the kontrol_s4_mk3 branch 6 times, most recently from 7606bde to 0698ec8 Compare August 31, 2021 02:36
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 31, 2021

Both decks and the mixer are now mostly mapped. The top row of the pad grid is mapped to intro & outro cues. The bottom row is hotcues 1 - 4 (with colors supported :) ).

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 31, 2021

This is a basically usable state now. It is already more useful than my Xone K2. I briefly attempted to get the jog wheels working but I have not figured out how to interpret the signals from the controller for them. As a quick, temporary hack I mapped the turntable and jog buttons to seek backward/forward (faster with shift) until the jog wheels are working. I took a look at the S3 mapping and the signals for the jog wheels seem similar but I don't think they're identical. The S3 has a timer indicated 3 bytes. The S4 Mk3 seems to have 4 bytes that continually vary for each wheel. @uklotzde could you test this and try to make sense of the jog wheel signals in input report 3?

@Be-ing Be-ing force-pushed the kontrol_s4_mk3 branch 2 times, most recently from e916bfe to 3b6c856 Compare September 11, 2021 00:11
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

I only bothered to look at the components code for now. I wouldn't even bother to fix most of the issues I pointed out for now since there are more broad issues I'd like to discuss first. For example, I still don't believe that coupling components to the actual protocol is a good idea, though this version of components is already a much better starting point than its midi counterpart for further decoupling efforts.

Comment on lines +67 to +83
constructor(reportId) {
this.reportId = reportId;
Copy link
Member

Choose a reason for hiding this comment

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

I dislike the assumption that the packet structure changes based on the reportID. For example, the pioneer protocol abuses the reportID just indicating which deck (or generally unit) on the controller receives the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Pioneer is breaking the HID standard with that, so I'm not really concerned about that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... as I said, they don't care about that because they're actually not using anything actually related to the HID standard. They might as well not be using HID since their entire driver is just implemented as part of the DJ software.

Copy link
Member

Choose a reason for hiding this comment

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

But at the same time, they still have standard packets and parsing them using this library could be quite useful, so thats why I dislike the dependency on that. In the end, we write the same user-space device drivers as Pioneer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be up to the scripts for Pioneer devices that abuse the HID protocol to figure out how to identify packets some other way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's really possible.

Copy link
Member

Choose a reason for hiding this comment

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

I still believe it is, but I have yet to explore my ideas with some concrete experiments.

My rough idea is to create another abstraction that sits between IO and Components and irons out the differences in the protocol. This would also do the parsing. Components would then only used for modeling the actual controller and tracking the state of the individual components.

We already do this ironing a bit in the part that compares values between packets and then only call input handlers when the value actually changed. We just need to abstract this for ComponentsJS so the components don't have to care about how midi or HID works.

Copy link
Member

Choose a reason for hiding this comment

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

I'd gladly discuss this on Zulip some time. Though I'm aware that an actual PoC would make more sense, I don't think I could find the time for that currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would need to see a proof of concept to understand how this would work.

Copy link
Member

Choose a reason for hiding this comment

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

I'll try to come up with something.

if (bitOffset === undefined) {
bitOffset = 0;
}
if (typeof bitOffset !== "number" || bitOffset < 0 || !Number.isInteger(bitOffset)) {
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
if (typeof bitOffset !== "number" || bitOffset < 0 || !Number.isInteger(bitOffset)) {
if (!Number.isInteger(bitOffset) || bitOffset < 0) {

works just as good I think.

Comment on lines +88 to +105
if (bitLength === undefined) {
bitLength = 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

JS supports default values, use that instead. (pretty sure that applies to ES7 as well)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters

Comment on lines +110 to +127
this.fields = this.fields.filter((element) => {
return element !== field;
});
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
this.fields = this.fields.filter((element) => {
return element !== field;
});
this.fields = this.fields.filter((element) => element !== field);

also, capturing a reference to the field here could cause some memory leaks in the long term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Capturing a reference to the field is required for this to work at all.

Copy link
Member

Choose a reason for hiding this comment

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

I know, thats why I'm saying we should consider finding an alternative solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of a better way. Do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Please remind me tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, the field should get destroyed as soon as its removed from the fields array and the handler to the callback goes out-of-scope. So its probably not a memory-leak potential.

return;
}

for (let field of this.fields) {
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
for (let field of this.fields) {
for (const field of this.fields) {

Copy link
Member

Choose a reason for hiding this comment

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

This is applicable everywhere generally. This would be enforced by the prefer-const eslint rule. We should probably update our ESlint rules for main anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not always applicable, but I'll switch to it where possible.

Copy link
Member

Choose a reason for hiding this comment

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

Well at last when iterating over a container, I have not spotted any location in this code where a for (let [...] of/in [...] would be justified.

Comment on lines +464 to +494
output(value) {
if (value) {
this.send(this.color + this.brightnessOn);
} else {
this.send(0);
}
}
outConnect() {
if (undefined !== this.group) {
this.outConnections[0] = engine.makeConnection(this.group, this.outKey, this.output.bind(this));
this.outConnections[1] = engine.makeConnection(this.group, this.colorKey, (colorCode) => {
this.color = this.colorMap.getValueForNearestColor(colorCode);
this.output(engine.getValue(this.group, this.outKey));
});
}
}
Copy link
Member

Choose a reason for hiding this comment

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

this makes a lot of assumptions that need to be worked over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

Well thats largely related to my previous comment that this only supports half the usecase that the ColoredHotcueButton was actually made for.

Copy link
Member

Choose a reason for hiding this comment

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

but a quick example that comes to mind is the this.color + this.brightnessOn part because thats device specific.
Also this.send(0); should rather be this.send(this.off);

Comment on lines +491 to +507
input(pressed) {
if (!this.shifted) {
Copy link
Member

Choose a reason for hiding this comment

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

this is very different to how we usually handle shift with other components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is intentional. The old way with MIDI Components of reassigning the input method IMO ended up being rather awkward and clumsy in many cases. I do not recall who it was, but I do remember a mapping developer giving feedback on the MIDI Components library and mentioning that approach being overcomplicated. With this system it would be even worse because the input callback would need to be deregistered from the HIDInputPacket and the new one would need to be registered every time shift is pressed.

Copy link
Member

Choose a reason for hiding this comment

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

Mhmm. I'll think about that. I find the overwriting input method a bit more elegant, but it also forces the JIT to deoptimize code so it might not be that good idea after all...

Copy link
Contributor

Choose a reason for hiding this comment

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

My apologies for jumping in here, I just read your discussion and would like to add my 2 cents.

Both of you already had a look into the custom library that I contributed with Behringer devices in mind. I chose a different approach for the shift logic there and want to share my thoughts; maybe some details may be applied to other mappings, too.

I decided against the idea of handling the different behaviors on shifted/unshifted state inside a JS Component. Instead, the two behaviors are split into two components which are registered/unregistered according to the state of the shift button, so they are unaware of shifting.

In my implementation, you'll find a shiftable Component called "layer manager" which holds 2 component containers (= layers) named Default and Shift. JS Components may be added to either layer. This layer manager is the only component that knows about shifting. When it is shifted or unshifted, all affected components on the corresponding layer are registered / unregistered. Components that are not shiftable stay untouched.

The benefit of this approach is a good separation of concerns for the cost of more frequent component registrations. In my mappings, the number of shiftable components is small (measured against the total component count), and thus the number of registration changes is low. This approach works fine for me: the components look uncluttered and I didn't notice any effect on the performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While that is another valid approach, that's not what I want to do. That would require considerably more boilerplate and complexity. Most Components have some alternative functionality with shift so that complexity would add up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I consider the layered approach as the more versatile and cleaner architecture. But it may depend on the use case. Probably both in parallel are needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The layered approach also better resembles how Shift is actually implemented, i.e. it alters the feedback of all controls and not only a single one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it does depend on the use case and it is good to support both approaches.

Copy link
Member

Choose a reason for hiding this comment

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

The layered approach described seems valid. The actual physical control surface is still a single physical component, so using multiple components seems a bit wrong. We should think about adjusting terminology or maybe introduce a new concept for this.
Using another container class seems like an interesting concept actually. If that container class is responsible about connecting components based on whether they're shifted or not, it could actually have customizable behavior depending on the controller. For example for taking advantage of the controllers firmware shift feature.

Comment on lines +518 to +535
this.send(this.color + this.brightnessOn);
} else {
this.send(this.color + this.brightnessOff);
Copy link
Member

Choose a reason for hiding this comment

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

I didn't even know the samplerbutton came with any color feature until reading this method...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is hardware specific.

}
}
outConnect() {
if (undefined !== this.group) {
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
if (undefined !== this.group) {
if (this.group !== undefined) {

res/controllers/Traktor-Kontrol-S4-MK3.js Outdated Show resolved Hide resolved
@wastez
Copy link

wastez commented Feb 9, 2022

Wow this is really Great.
I buyed a s4 mk3 and i'm so unhappy with it because the lock of 8% +- pitch in tt mode. (Which idiot Design some stupid thing? Yes i know they want a 1210 feeling but that is no reason to take the one Bad thing from them)
I buyed it because the tt Mode. (I'm comming from turntables)
Is there a working Version out there for mac?
Otherwise i will sell this fucking Controller after one day.
I would like it but this pitch is a no go.
Could you please help me?????

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 9, 2022

I haven't tested this myself on macOS but AFAIK there's no reason it shouldn't work. The arbitrary +/- 8% limitation in Traktor also really irritated me, among other strange behavior of the controller mapping with Traktor. You can set the tempo fader range in Mixxx's Deck preferences.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 9, 2022

I buyed it because the tt Mode. (I'm comming from turntables)

I kinda got the motorized wheels working but there are lot of rough edges with that. I'd be grateful if someone picked this up to improve that!

@wastez
Copy link

wastez commented Feb 9, 2022

So just compile your Version for mac should work?
I'm so angry about this controller , i cant describe my frustration.
Got it today and now i'm think about to sell it immedatly.
Would be really cool of your solution could help me.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 9, 2022

You don't need to compile Mixxx. You can use a development build and download the mapping files from this branch:
https://raw.githubusercontent.com/Be-ing/mixxx/kontrol_s4_mk3/res/controllers/Traktor-Kontrol-S4-MK3.js
https://raw.githubusercontent.com/Be-ing/mixxx/kontrol_s4_mk3/res/controllers/Traktor%20Kontrol%20S4%20MK3.hid.xml

and put them in ~/Library/Containers/org.mixxx.mixxx/Data/Library/Application Support/Mixxx/controllers

@rah2501 rah2501 mentioned this pull request Oct 3, 2022
@ex0nuss
Copy link

ex0nuss commented Nov 16, 2022

Hi there,
sorry for reopening this pull again. I am currently looking for an Upgrade to my DDJ-400 and the S4 MK3 looks pretty neat. With this Upgrade I also wanna switch to Mixx :)
What is the state of this mapping and does it work with the main branch?

Cherrs!

@daschuer
Copy link
Member

I think this is in a "working" state and it looks like the prerequisites are merged to main. However this PR needs some work and testing to be merge-able to the main branch. Do you have interest to continue this work? You need the controller and some basic Java script skills. Your help is welcome.

@Serphentas
Copy link

Coming in late but I also acquired the controller and have skills and time to spare. I'll check out what's been done to date and expand on it. Any pointers, feel free to drop them here.

Many thanks for what's been done until now !

@JoergAtGithub
Copy link
Member

Have a look at #11284 which requires #11326 to work.

@Serphentas
Copy link

I've compiled Mixxx from the 2.4-beta tag successfully, but upon enabling the mapping the console shows:

warning [Controller] Failed. Trying to open with make, model & serial no: 6092 5920 "3A47C8C5"
warning [Controller] Unable to open specific HID device "Traktor Kontrol S4 MK3 C8C5_3" Trying now with just make and model. (This may only open the first of multiple identical devices.)
warning [Controller] Unable to open HID device "Traktor Kontrol S4 MK3 C8C5_3"

Here is lsusb with the Controller setting view:

image

For reference, loading any mapping works in Mixxx 2.3.5 (Arch).

@JoergAtGithub
Copy link
Member

This PR is no longer maintained. Please try the latest version of the mapping in #11284
Regarding the problem - you might change your udev file on Linux, because 2.4 use the hidraw kernel module instead of the libusb userspace driver. Full installation of 2.4 should fix this.

@Serphentas
Copy link

You're right, sorry about that. I've used an appropriate udev rule and no read error anymore. Thanks !

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.

9 participants