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

add DDJ-400 contoller #2403

Closed
wants to merge 37 commits into from
Closed

add DDJ-400 contoller #2403

wants to merge 37 commits into from

Conversation

nschloe
Copy link
Contributor

@nschloe nschloe commented Dec 16, 2019

After #2143, #2312, and #2385 this is another take on the Pioneer DDJ-400.

This PR implements most of the basic functionality of the controller:

Mixer Section (Faders, EQ, Filter, Gain, Cue)
Browsing and loading
Jogwheels, Scratching, Bending
Beat Sync
Beatjump mode
Hot cue Mode
Beat Loop Mode

I've successfully used this on three occasions and only found minor glitches. For example, when a loop is set and shift is pressed, the lights go out. I have not found a way to prevent this, and sending ON at shift state does not work. The functionality is still there though, so I'd consider this minor.

@nschloe nschloe mentioned this pull request Dec 16, 2019
@Be-ing Be-ing changed the base branch from master to 2.2 December 16, 2019 23:20
@Holzhaus
Copy link
Member

@nschloe You need to rebase, since there are lots of unrelated commits. Opening a new PR won't change that.

You can do it like this:

$ git remote add upstream https://github.com/mixxxdj/mixxx.git
$ git fetch upstream
$ git rebase -i upstream/2.2

Then, you need to remove all lines that are not from you. The lines are ordered by commit date, so your commits should be near the bottom of the list. When you're done, you'll need to force-push your commits:

$ git push -f origin pioneer-ddj400

IMHO it makes sense to reopen #2385 and continue the discussion there (and close this PR instead).

@nschloe
Copy link
Contributor Author

nschloe commented Dec 24, 2019

One issue that the controller still has is of cosmetic nature: When pressing the shift button, the loop lights will go out. This is not helped by sending a signal on the shift-state as suggested by @Be-ing, and as worked for the hotcue buttons.

Other than that, it'd be lovely if I got the lighting of the loop buttons to match the patterns it has in rekordbox (default on: both blinking when the loop is active), but perhaps all of that can be added later.

@nschloe
Copy link
Contributor Author

nschloe commented Jan 8, 2020

Alright, I've got the loop lights fixed now and I'm fairly happy with this now.

I had initially planned to make them blink as well (just like in the vendor software), but this turns out to be more complicated than I had anticipated. I'm fine with having static lights now.

I'd be happy to get a review on the code.

@Holzhaus Holzhaus added this to the 2.2.4 milestone Jan 9, 2020
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.

Awesome, thanks for contributing a mapping for the DDJ-400!
Apart from some minor issues, this looks mostly good IMHO.

In Rekordbox LOAD+Shift (DECK1) function is related tracks, since we have no equivalent function in MIXXX
and doubleclick would map to instant double we are mapping this to CloneFromDeck
-->
<description>LOAD +SHIFT (DECK1) - press - Clone Track from playing Deck</description>
Copy link
Member

Choose a reason for hiding this comment

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

According to the wiki page this should be "Sort library by criteria", not "Clone from deck".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These parts are commented out anyway; we'll have to wait for 2.3.0 before reactivating.

@Holzhaus
Copy link
Member

@nschloe I fixed some eslint issues for you in https://github.com/nschloe/mixxx/pull/1, please merge it ;-)

Pioneer DDJ-400: Merge 2.2 branch and fix some eslint issues
@nschloe
Copy link
Contributor Author

nschloe commented Jan 21, 2020

Thanks! I'll look at the timer thing soon.

@dj3730
Copy link
Contributor

dj3730 commented Jan 24, 2020

So I made a couple of small changes to the init and shutdown functions, in part as a test to ensure I was doing things properly, but also to add some new functionality - querying the controller for current control positions on startup, and also to shutdown all performance pad LEDs on shutdown. I've pushed a commit for these and filed a PR.

I have some other changes queued up to be added, but I'll be away from home for a few days and won't likely get to them until next week at the earliest.

One thing in particular I'm looking at is the blinking of the Sampler performance pads - right now, if a sampler is playing, the pad blinks, but if SHIFT is held then the pad stops blinking. Furthermore, once SHIFT+PAD PRESS is used to stop the sample from playing, the LED in the pad turns OFF instead of remaining ON to indicate that a sample is loaded into that sampler.

However, if you're going to be looking to restructure the startSampleFlicker routing based on @Holzhaus 's comment above, then I will hold off on this until after you've done so. Having a single timer driving all of the sample pad blinking would certainly be more efficient than having a separate timer for each (theoretically leading to potentially 16 timers running) however I can see that this would require significant restructuring of the existing code.

@nschloe
Copy link
Contributor Author

nschloe commented Feb 3, 2020

@dj3730 I think it is unuseful to try and make substantial changes to this PR. Support for the DDJ-400 has been cooking since June, ann imho it's more important to get this into a mergeable state. We can always add functionality later.

@dj3730
Copy link
Contributor

dj3730 commented Feb 3, 2020

@nschloe I can definitely see where you're coming from on that, and I do agree that the priority should be to get this into a releasable state, given how long it's been open. Once this PR is merged, I'll start up a new one for the new functionality.

That said, I did notice a small bug in the xml related to CUE Channel +SHIFT - I'll get a comment posted about that momentarily.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 16, 2020

Sorry for the delayed review. I did not see that @dj3730 made new changes because it was done in a separate PR to @nschloe's branch and not mentioned here. This is why I suggested @dj3730 make a new PR for mixxxdj/mixxx.

@leonfaeth
Copy link

Thanks for this mapping, works great!
Three questions: Is there a reason that the left and right arrow buttons in the fx section actively select the corresponding effect instead of changing the focus to the left or right, thus removing the possibility of choosing effects with the fx select button? (This was the case in PR #2143)
Additionally, is it possible to make the top right and bottom right button in pad fx 1 change the effects intensity by pressing and holding (or do they have another function mapped? Didn't see any changes happen after pressing them)?
I think it is sensible to speed up the quick search (shift + scratch) a little bit more, what do you think?

@leonfaeth
Copy link

@nschloe @Holzhaus @dj3730 @Be-ing
Can someone report on what the latest changes are/which PR is the "current" one? I'd love to see this merged, maybe I can contribute.

@Be-ing
Copy link
Contributor

Be-ing commented May 5, 2020

All the PRs for this controller have been quite confusing to follow. @leonFC I would be glad if you finished this. You can refer back to the unresolved review comments on this PR.

@uklotzde uklotzde modified the milestones: 2.2.4, 2.2.5, 2.2.x Jun 12, 2020
@Holzhaus
Copy link
Member

@nschloe @leonFC Any progress on this?

@nschloe
Copy link
Contributor Author

nschloe commented Jun 12, 2020

Not on my part.

@Holzhaus Holzhaus marked this pull request as draft June 21, 2020 22:02
@leonfaeth
Copy link

Sorry for the delay, I'm afraid I cannot currently contribute as much as what is necessary due to unexpected private events. Still playing around from time to time though. I'll report back when there is relevant progress.

@bvobart
Copy link

bvobart commented Sep 14, 2020

@nschloe @leonFC Have you been able to make any progress? How can I help?

I've recently gotten into DJ'ing, so I bought myself a DDJ-400 and started using Mixxx 2.2.4 along with an earlier version of this preset that I'd found on the forum (never actually used Rekordbox as it doesn't run on Linux). Since a week or so I've been using the preset from this PR and it has been working very well in most aspects (everything from the 'Working' section definitely works, keyshift mode works perfectly too). Awesome work, thank you both for creating this mapping!

However, I've noticed two usability issues. I'm both studying and working as a software engineer myself, so when I have some free time, I'm able to and glad to try to resolve those problems and help to get this PR merged.


The bug is with the tempo sliders: they work the wrong way around: moving the slider towards the + on the controller, visually moves the slider on the screen downwards too, but this actually decreases the BPM. Same vice versa: move the slider towards the - on the controller and the tempo increases. Thanks @Holzhaus for letting me know this can be changed in Mixxx's preferences!

As for the usability issues:

  • When no effect is selected, the FX Level/Depth knob controls FX1's Mix level. This is unintuitive, it often happens that I use the FX On/Off button to switch on an effect, use the knob to control it, then switch it off using the button, then turn the knob back to zero. When I want to do the same thing again afterwards (possibly with a different effect), the effect cannot be heard, because I 'accidentally' turned the Mix down from the default 100% to 0% in the mean time. Then I need to turn the knob back to 100, switch on the effect and quickly turn the knob back to 0, hoping that nobody listening will notice. It would be better if the FX Level/Depth knob changes the selected effect's level (whether active or not), and do nothing at all if no effect is currently selected (I think this is also Rekordbox's behaviour).
  • "LEFT selects EFFECT1, RIGHT selects EFFECT2, FX_SELECT selects EFFECT3" I guess I can get used to it, but this is unintuitive as well, not to mention it is not the same behaviour as Rekordbox. LEFT should select the effect slot left of the currently selected effect. RIGHT should similarly select the slot to the right of the currently selected slot. If no effect is currently selected, it will select the left-most or right-most effect on the board. FX_SELECT should open the effect selection box (if not already opened) of the selected effect (if one is selected, otherwise do nothing) and select the effect one below the currently chosen effect. Similarly, SHIFT + FX_SELECT should do the same, but select the effect one above the currently chosen effect. Since I have no prior experience in developing with Mixxx, I have no idea whether the API that Mixxx exposes to its controller mappings allows for such an implementation of the FX_SELECT button. Would be great if someone could point me in the right direction for this :)

@Holzhaus
Copy link
Member

The bug is with the tempo sliders: they work the wrong way around: moving the slider towards the + on the controller, visually moves the slider on the screen downwards too, but this actually decreases the BPM. Same vice versa: move the slider towards the - on the controller and the tempo increases.

You can configure this in the Mixxx preferences. In Mixxx 2.3, the default behavior will change, so that up is slower and down is faster.

@Holzhaus
Copy link
Member

Also, starting with Mixxx 2.3, all controller mappings are now supposed to be documented in the Manual. This is necessary so that other users know how to use this mapping. We also need it to review it properly and check if the mapping makes sense and if the code matches the documentation.

Please fork the mixxxdj/manual repository, check out the manual-2.3.x branch and add documentation to the source/hardware/controllers directory. Open the pull request and add the link to the PR description.

Here's an example how such documentation could look like: https://manual.mixxx.org/2.3/en/hardware/controllers/roland_dj_505.html (If you don't have schematic images and don't want to draw them yourself, just use the same numbering as in the controller's manual and document the buttons like that).

@Holzhaus
Copy link
Member

Holzhaus commented Dec 3, 2020

@bvobart Are you interested in picking this up?

@bvobart
Copy link

bvobart commented Dec 11, 2020

@Holzhaus I'm interested in picking this up, though I started my MSc thesis little less than a month ago and have an important intermediate deadline in the second half of January. I therefore doubt that I'll have time to work on this before then, but after that I should be able to free up some time to tackle this.

@jusko
Copy link
Contributor

jusko commented Dec 17, 2020

Hi all,

I've been using Mixxx very successfully with a DDJ-400 since early last year. My mappings are based on the original Warker ones that inspired this work by @nschloe and @dj3730 . At one point I truly wanted to get them into the project, but I slacked into backburner obscurity 😉

Though my mappings have diverged from these, they contain a few extremely useful additions (quantize toggle, phrase jumping and a more intuitive FX workflow). In all, it's not much work to add here if you like.

But first let me put myself forward to wrap this PR up over the festive season.

All the PRs for this controller have been quite confusing to follow. @leonFC I would be glad if you finished this. You can refer back to the unresolved review comments on this PR.

@Holzhaus, @Be-ing
Unless there are any objections, I will proceed to look into the unresolved comments. I'm not sure I understand all of them yet, but will figure it out and might hit you up on zulip if there are any details to discuss.

@jusko
Copy link
Contributor

jusko commented Dec 19, 2020

☝️ Addressing the discussion points in the above draft. Hope to get a fair bit done over the next week or so. Will ping back here when it's ready for review.

Please let me know if there are any other issues or work you'd like done on this. I've got some momentum and capacity next week.

@jusko
Copy link
Contributor

jusko commented Dec 22, 2020

Making some good progress at the moment. Just hitting some issues with the FX mappings which I'll bring to your attention:

1. BEAT FX section
1.1. <, > and v map to effect units 1, 2 and 3 respectively.
1.2. The On/Off button toggles the state of the effect units
1.3. The Level/Depth knob is mapped to the currently select effect unit's metaknob (if it is enabled) or the wet/dry mix (if the effect is disabled)

I think a small adjustment to 1.3. will make a big improvement. It's very strange to have the wet/dry mix disabled if an effect is enabled. This forces you to set a wet/dry level prior to enabling an effect, which doesn't sound good (and also stops you from mixing a chain of enabled effects in easily).

So unless there are objections, I'll invert the mapping to use the wet/dry mix when an effect is enabled, and use the metaknob when the effect is disabled. (I'll also add soft takeover to the knobs)

EDIT:
I see that it is possible to enable one or more effects, then get back to the wet/dry mix by pressing <, > or v twice. This takes some getting used to, but works quite well.

I've left the mapping as is then, but added:

  • much needed soft takeover to the wet/dry mix and metaknobs
  • the ability to cycle backwards and forwards through effects in the currently selected unit

@jusko
Copy link
Contributor

jusko commented Dec 22, 2020

2. PAD FX
The mapping here is rather messy and I'm not sure if we want to fix it, drop it or change it entirely.

In rekordbox, PAD FX1 simply plays different preset FX when you press pads 1-8 (with more effects when the pads are shifted). The same for PAD FX 2 (with even more effects).

The current mappings for PAD FX1 do the following:

2.1. Pads 1, 2 & 3 toggle effect units 1, 2 and 3 on & off.
2.2. Pads 5, 6 & 7 select the next effect for units 1, 2 & 3 respectively (or the previous effect in shift mode).
2.3. Pads 4 & 8 do nothing

As it stands, this can clobber the effects set up by our BEAT FX mapping (Deck 1 PAD FX uses the same effect unit).

Furthermore, the current mappings for PAD FX2 are invalid and don't work at all. 😕

So I'm guessing it's best to just remove the mappings rather than try to fix them?

Are preset FX something that's worth discussing? I.e., is there a way to assign an effect with a given set of parameters to a pad button?

EDIT:

As I tidy up the code a bit, I think it'd be good just to remove this. It's not very useful at the moment and BEAT FX give us a enough effects to work with for now.

@jusko
Copy link
Contributor

jusko commented Dec 22, 2020

Update

Things here are pretty much done. However, there's a last discussion to be had regarding all the secondary (shift) pad modes.

As mentioned above PAD FX1/2 aren't really that useful and are quite hackily implemented. I think they should be scrapped entirely.

Upon quite a bit more experimenting I'd have to say the same for both KEYBOARD and KEY SHIFT modes:

KEY SHIFT Mode (about 75% production ready):

  • Lights don't work as in rekordbox
  • Tries to emulate rekordbox by allowing you to increase/decrease the range of the pitch shift (and goes too far in this, it's not useful).
  • Has the issue of whether we implement chromatic, major or minor shift options

KEYBOARD Mode (about 50% production ready)

  • Lights don't work as in rekordbox
  • Playback doesn't work as intended
  • Hacky implementation

Does anyone feel these are worthwhile features to include in the first place? I'm happy to fix things up, but to be honest I feel that the original rekordbox features aren't very worthwhile in themselves. So why pollute Mixxx with them?

@Be-ing
Copy link
Contributor

Be-ing commented Dec 22, 2020

@jusko can you open a new PR to continue this discussion?

@Be-ing
Copy link
Contributor

Be-ing commented Dec 22, 2020

So I'm guessing it's best to just remove the mappings rather than try to fix them?

Sure.

Are preset FX something that's worth discussing? I.e., is there a way to assign an effect with a given set of parameters to a pad button?

This will be added in Mixxx 2.4: #2618

@Be-ing
Copy link
Contributor

Be-ing commented Dec 22, 2020

KEY SHIFT Mode (about 75% production ready)
KEYBOARD Mode (about 50% production ready)

I don't know what these are intended to do. I have no idea if the old wiki page is up to date. I think it's probably fine to remove them. Please open a PR for the manual with updated documentation.

@jusko
Copy link
Contributor

jusko commented Dec 22, 2020

@jusko can you open a new PR to continue this discussion?

Sure thing. Done over here: #3479

@Holzhaus Holzhaus closed this Dec 22, 2020
@uklotzde uklotzde modified the milestones: 2.2.5, 2.3.0 May 11, 2021
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