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

Traktor S2 MK1: Add calibration and refactor #11237

Merged
merged 16 commits into from
Sep 13, 2023

Conversation

leifhelm
Copy link
Contributor

@leifhelm leifhelm commented Feb 1, 2023

As discussed in #3905 with @JoergAtGithub this PR add the capability to read the feature report of the controller and apply the calibration stored in it.
I cleaned up the messy code and replaced it with nice OOP code.

I guess this has to wait until #3905 gets into 2.4.

@leifhelm leifhelm marked this pull request as draft February 1, 2023 17:30
@JoergAtGithub
Copy link
Member

On a first look, the code looks very clean.

@JoergAtGithub
Copy link
Member

You might be interested in initializing Mixxx with the knob positions at startup, using getInputReport (which is another new HID feature in 2.4). Example: e221885

@JoergAtGithub
Copy link
Member

We have issue #10667 where this mapping was derived from.
Both mappings use the CO "load_preset" which is no longer available in 2.4. Could you try to change this to use the "loaded_chain_preset".

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jun 24, 2023
@ronso0 ronso0 removed the stale Stale issues that haven't been updated for a long time. label Jul 2, 2023
@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 2, 2023

I’m back. The mapping of knobs seems to have changed. Only goes non-linearly halfway. 🤔

@ronso0
Copy link
Member

ronso0 commented Jul 2, 2023

The browse knob behaves now exactly like in Traktor Pro.

What does that mean? Mixxx is not Traktor.

@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 2, 2023

Turning Browse: go up or down in track list.
Shift + Turning Browse: go up or down in search box
Click Browse: Mini/Maximize Library
Shift + Click Browse: Open Folder (GoToItem)
I have not found a description on how the browse knob should behave.

I guess the desired behavior would be (based on Traktor S4 Mk2):
Click Browse: GoToItem
Turning Browse: go up or down where ever the focus is.
Shift + Turning Browse: Move the focus around.

So if you would like to select a track in the track list, you have to do the following:

  1. Check if the focus is on the track list.
  2. If not move the focus to the track list.
  3. Turn the knob to select the track.

MoveFocus also shifts focus to the text search box, a bit odd when using the controller.

@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 2, 2023

script.absoluteNonLin seems to be broken. Mapping to 0…2 and then squaring looks more linear to me.

@ronso0
Copy link
Member

ronso0 commented Jul 2, 2023

go up or down in track list.
Shift + Turning Browse: go up or down in search box
Click Browse: Mini/Maximize Library
Shift + Click Browse: Open Folder (GoToItem)
I have not found a description on how the browse knob should behave.

And with this implementation there's no way to move focus to the library sidebar, right?

I guess the desired behavior would be (based on Traktor S4 Mk2):
Click Browse: GoToItem
Turning Browse: go up or down where ever the focus is.
Shift + Turning Browse: Move the focus around.

Yes, one possibility.

So if you would like to select a track in the track list, you have to do the following:

  1. Check if the focus is on the track list.
  2. If not move the focus to the track list.
  3. Turn the knob to select the track.

Yep. But if you don't click on any beatsize spinboxes the focus remains on Tracks, even if you toggle the maximized library.

MoveFocus also shifts focus to the text search box, a bit odd when using the controller.

Why is that odd? You can then cycle through the list of saved search queries. If you never do that and want to prevent accidental query change, you can disable the Up/Down functionality in Preferences > Library.

@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 3, 2023

In my implementation pressing shift moves focus to the sidebar (Tracks, Auto-DJ, …). Releasing shift moves focus back to the Track list. Have a look at shift(field)

@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 3, 2023

There are dedicated controls for beatsize adjustment. When selecting the beatsize with the mouse, [Library], MoveVertical moves the focus always to the track-list.

The current system is optimized for people who navigate around their library with the side bar and the track list, maybe frequently switching between them as they navigate a folder structure. And do not use the Seach… input box.

Do you have a better idea of what to do when pressing Browse in track-list?

I think I messed up the terminology in my previous comments.

@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 3, 2023

I seems that different equalizers need different parameter scaling. How should this be handled?

@ronso0
Copy link
Member

ronso0 commented Jul 3, 2023

The current system is optimized for people who navigate around their library with the side bar and the track list, maybe frequently switching between them as they navigate a folder structure. And do not use the Seach… input box.

[Library], MoveVertical (shift: ScrollVertical?) and MoveFocusForward play very well together with the focus highlight in skins. With [Library], GoToItem (using [Library], focus_widget internally) you can jump from the searchbox or from the sidebar to the tracks table.
It seems you optimized the navigation for your use case (and for 2.3 and earlier this implementation is okay), but this way you exclude other users from using new 2.4 features with this mapping (search box' saved queries).

Btw what I expect the Browse mapping to do is pretty much what @JoergAtGithub proposed in your previous PR #3905 (comment)

When selecting the beatsize with the mouse, [Library], MoveVertical moves the focus always to the track-list.

Yes, correct. Since the spinboxes can only be focused by mouse click it is assumed the beat size is change by mouse or keyboard. With [Library], MoveVertical you can easily return to the tracks table in case your hands are already back on the controller (no need to reach for the mouse again). Same happens with [Library], GoToItem, though there are many simple controllers which may not have a button to map it, so MoveVertical is helping out (even though it may change the spinboxes, but as you said, if a users needs to do that regularly they'll map the dedicated controls).

@leifhelm leifhelm marked this pull request as ready for review July 4, 2023 19:35
@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 7, 2023

@ronso0 are you happy with the browse knob behavior now?

@JoergAtGithub
Copy link
Member

Could you please explain step by step, how you tested that the calibration values are used in the same way as in Traktor Pro.

@ronso0
Copy link
Member

ronso0 commented Jul 8, 2023

@ronso0 are you happy with the browse knob behavior now?

Yeah, I think this can work well.
What about this Playlist residue though?
https://github.com/mixxxdj/mixxx/pull/11237/files#diff-1e471385f72a5137810cf36856f4456991bc962bcef3ae54a772080a96abb873R1135

@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 9, 2023

I captured the USB Traffic from Traktor Pro with Wireshark and USBPcap. When doing the calibration sequence I would only turn/move one knob/slider. Traktor Pro seems to have a check in place that writes default values if you do not move a knob/slider. This can be used to check which values have changed. As the calibration sequence demands left, right, center for knobs, and three values which are the exact same as in the last received input packet, I figured that the corresponding values are the ADC readings of the left, right, center position. Sliders only have the two end positions in the calibration routine.
Similar methodology was applied with the jog wheel touch calibration. There exists a restore factory settings button that writes the default values, or the factory jog wheel calibration (read earlier).

I hope this explains that enough. As decompilation was not used, I can not for example tell how the calibration values are used in Traktor Pro.

What about this Playlist residue though?

Good catch. I changed it to [Library].

@JoergAtGithub
Copy link
Member

This describes the capture of the calibration procedure, but did you also captured the readout when Traktor Pro starts up?

Did you tried to intentionally calibrate intentionally "wrong", e.g. left and right cross-fader positions close to the center, to get extra sharp cuts? If you do, does it cut at the same fader position with Traktor Pro and Mixxx?

@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 10, 2023

When Traktor Pro 2 starts up the feature reports d0-d4 are read, followed by input report 01 and 02. This is the only USBHID traffic apart from fetching the HID report descriptor and a SET_IDLE request.

@leifhelm
Copy link
Contributor Author

leifhelm commented Jul 10, 2023

I calibrated the crossfader to about 5 mm from the edge. When slowly moving the slider from the outside inwards the crossfader in the software starts moving at the same point for Traktor and Mixxx.

Comment on lines 1042 to 1045
this.decks[0].registerInputs(InputReport0x01, InputReport0x02, {
gainEncoderPress: [0x0E, 0x01],
shift: [0x0D, 0x80],
sync: [0x0D, 0x40],
Copy link
Member

Choose a reason for hiding this comment

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

This registerInputs structure is very difficult to understand. For me it's not clear anymore, in which InputReport an input data item is located. Please rethink how to make this part of the code easier readable for others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like this?

@leifhelm leifhelm requested a review from JoergAtGithub August 29, 2023 08:24
Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

Code structure is much better readable now - but I found some small points. Please have a look!

res/controllers/Traktor-Kontrol-S2-MK1-hid-scripts.js Outdated Show resolved Hide resolved
return;
}
// was enabled, and button has been let go. maybe latch it.
if (now - this.syncEnabledTime > 300) {
Copy link
Member

Choose a reason for hiding this comment

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

this.syncEnabledTime is not a numeric type, it's defined as this.syncEnabledTime = {};

Copy link
Contributor Author

@leifhelm leifhelm Sep 13, 2023

Choose a reason for hiding this comment

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

Would NaN be better? isNaN(now - NaN) === true and NaN > 300 === false.

res/controllers/Traktor-Kontrol-S2-MK1-hid-scripts.js Outdated Show resolved Hide resolved
Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

LGTM! Waiting for CI.

@JoergAtGithub JoergAtGithub merged commit 42798e9 into mixxxdj:2.4 Sep 13, 2023
@JoergAtGithub
Copy link
Member

Thank you!

@JoergAtGithub JoergAtGithub added the changelog This PR should be included in the changelog label Sep 13, 2023
@JoergAtGithub
Copy link
Member

There is a report in our forum, that this mapping is not working on Linux: https://mixxx.discourse.group/t/ni-traktor-s2-mk1-linux-only-half-of-the-mapping-works/29468

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog controller mappings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants