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

Mappings for Pioneer DDJ SB3 Controller #3821

Merged
merged 34 commits into from
Jan 10, 2022
Merged

Conversation

jevilalta
Copy link
Contributor

Hello!

Here is the mapping for the DDJ SB3 controller. I think I've got the prior issues resolved. I have not finished updating the manual yet.

@jevilalta
Copy link
Contributor Author

I've submitted the manual changes: mixxxdj/manual#379

@Swiftb0y Swiftb0y self-assigned this May 5, 2021
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks, some comments. @Swiftb0y Wanna take another look?

res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
controlsToFunctions.slipEnabled = "PioneerDDJSB3.slipLed";

for (i = 1; i <= 8; i++) {
controlsToFunctions["hotcue_" + i + "_enabled"] = "PioneerDDJSB3.hotCueLeds";
Copy link
Member

Choose a reason for hiding this comment

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

It's pretty inefficient to update all hotcue LEDS when changing just a single hotcue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewed this and this particular section is just setting up the mapping of the function to the control, it is not updating / changing the hotcues. This function runs once as part of the init routine.

hotCueLeds function itself only updates one hotcue when called.

Copy link
Member

Choose a reason for hiding this comment

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

@Swiftb0y
Copy link
Member

Swiftb0y commented May 6, 2021

Yes, Started my review yesterday, but I'm not sure when it'll be finished.

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.

Thanks for submitting your mapping. I'm not yet finished reviewing but these are my remarks on the first 1150 lines.

res/controllers/Pioneer-DDJ-SB3.midi.xml Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3.midi.xml Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
Comment on lines 750 to 770
PioneerDDJSB3.deckSwitchTable = {
"[Channel1]": "[Channel1]",
"[Channel2]": "[Channel2]",
"[Channel3]": "[Channel3]",
"[Channel4]": "[Channel4]"

};

PioneerDDJSB3.deckShiftSwitchTable = {
"[Channel1]": "[Channel3]",
"[Channel2]": "[Channel4]",
"[Channel3]": "[Channel1]",
"[Channel4]": "[Channel2]"
};

PioneerDDJSB3.deckIndexTable = {
"[Channel1]": 0,
"[Channel2]": 1,
"[Channel3]": 2,
"[Channel4]": 3
};
Copy link
Member

Choose a reason for hiding this comment

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

Whats the purpose of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to support the 4 deck configuration. The controller only has 2 physical decks but when you select the deck 3 and 4, other "virtual" decks are enabled. These lookup tables are used to handle the shifted decks.

That said, the deckIndexTable isn't needed so I've removed that.

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 understand, its just a weird way to implement it.

res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
res/controllers/Pioneer-DDJ-SB3-scripts.js Show resolved Hide resolved
@uklotzde
Copy link
Contributor

Thanks for signing the contributor agreement ✔️

It would be cool if we could merge this PR before releasing 2.3.0. Do you have time to resolve the review comments?

@jevilalta
Copy link
Contributor Author

Thanks for signing the contributor agreement ✔️

It would be cool if we could merge this PR before releasing 2.3.0. Do you have time to resolve the review comments?

I will make the changes this weekend. I am finishing up the manual changes. I will submit that today and move on to this.

@uklotzde
Copy link
Contributor

A minor code style issue left to fix.

@Holzhaus The manual PR mixxxdj/manual#379 is almost ready? Only a link check failure.

@uklotzde
Copy link
Contributor

...and some unresolved comments by @Swiftb0y.

@uklotzde uklotzde added this to the 2.3.2 milestone Sep 7, 2021
@jevilalta
Copy link
Contributor Author

Any progress on this? Rescheduling to 2.3.2.

I haven't made any progress. I'm still limited on time, I'm happy to transfer the repo over if anyone is interested.

Seems I would need to rebase onto the 2.3.2 branch then, in order to continue working on this and change the target branch of the PR? Can you confirm?

@Be-ing
Copy link
Contributor

Be-ing commented Sep 8, 2021

We don't make separate branches for minor releases; we only make new Git tags. We will keep using the 2.3 branch. So there is no need to rebase.

@jevilalta
Copy link
Contributor Author

Hi, I am done with the changes requested. Let me know if I missed anything.

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.

Thanks, last comment, LGTM otherwise.

res/controllers/Pioneer-DDJ-SB3-scripts.js Outdated Show resolved Hide resolved
@Swiftb0y Swiftb0y added the changelog This PR should be included in the changelog label Dec 1, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Dec 18, 2021

I trust @Swiftb0y and @Holzhaus to have sufficiently reviewed the code here. The only glaring issue is the lack of access to hotcues 5-8. When that is fixed and the pending review comments on the documentation are resolved, let's merge this.

@jevilalta
Copy link
Contributor Author

jevilalta commented Dec 26, 2021

I have pushed the update which enables hotcues 5-8. I will move on to fixing the documentation.

@jevilalta
Copy link
Contributor Author

i think that's the last of the requested changes, but lmk if anything else needs to be changed

@Swiftb0y
Copy link
Member

Swiftb0y commented Jan 9, 2022

pre-commit is complaining about trailing whitespace in your XML. It's not really an issue, but I'd prefer to merge without precommit complaining.

@Holzhaus Holzhaus requested a review from Swiftb0y January 9, 2022 23:03
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.

Thank you very much for mapping this controller. It'll likely be released in Mixxx 2.3.2

@Swiftb0y Swiftb0y merged commit f76fe15 into mixxxdj:2.3 Jan 10, 2022
@jevilalta jevilalta deleted the Pioneer-DDJ-SB3 branch January 10, 2022 19:32
@travstone
Copy link

Am I doing something wrong, or does this mapping still not work for decks 3/4 when you're using an external soundcard?

I read somewhere else that one might be able to launch mixxx with the DDJ-SB3 internal soundcard selected (which does seem to work) and then switch back to the external card, but I almost immediately got an error:

Uncaught exception at line 308 in file C:/Program Files/Mixxx/controllers/Pioneer-DDJ-SB3-scripts.js.

Exception:
  TypeError: Result of expression 'PioneerDDJSB3.headphoneLevel' [undefined] is not an object.

Backtrace:
  <anonymous>(channel = 0, control = 63, value = 0, status = 144, group = '[Channel1]') at C:/Program Files/Mixxx/controllers/Pioneer-DDJ-SB3-scripts.js:308
  <anonymous>(arg1 = 0, arg2 = 63, arg3 = 0, arg4 = 144, arg5 = '[Channel1]') at 1
  <global>() at -1

I got this error once before when I tried to use the shift button to change the decks (prior to trying the trick of changing the soundcard), but I also got it again after. In that second case, I had hit the shift button to use the platter to skip forward quickly. After that the deck button stopped being illuminated and I was no longer able to select decks 3/4.

Not sure this is enough info to be helpful, but I'll try to replicate the steps more carefully later.

Also, just want to know if there's something else I should be doing to make the deck 3/4 switching work correctly. Thanks for all your work on this!

@Swiftb0y
Copy link
Member

Swiftb0y commented Feb 3, 2022

@travstone
Thank you for making us aware of this issue. Looking at the code, the problem should occur everytime the shift button is released, so I'm wondering why this has not been noticed sooner.
If you just remove line 308 (this one: PioneerDDJSB3.headphoneLevel.unshift();) it should work. Could you try that and see if it fixes the issue?
Thanks

@travstone
Copy link

@Swiftb0y confirmed; when I comment that line I no longer get an error for either of the cases, all good there.

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.

7 participants