Skip to content

Behringer CMD STUDIO 4a mapping.#869

Merged
rryan merged 9 commits intomixxxdj:1.12from
GandalfMixxx:BehringerCMDStudio4a
Jan 13, 2017
Merged

Behringer CMD STUDIO 4a mapping.#869
rryan merged 9 commits intomixxxdj:1.12from
GandalfMixxx:BehringerCMDStudio4a

Conversation

@GandalfMixxx
Copy link
Copy Markdown
Contributor

This is my new mapping for the Behringer CMD STUDIO 4a.

Forum: http://www.mixxx.org/forums/viewtopic.php?f=7&t=7868
Wiki: http://www.mixxx.org/wiki/doku.php/behringer_cmd_studio_4a

I think it's ready to be included into a Mixxx release, but please let me know if anything needs to be changed.

Regards,
Craig.

new file:   res/controllers/Behringer CMDStudio4a.midi.xml
new file:   res/controllers/Behringer-CMDStudio4a-scripts.js
Added JSHint header as described in the Wiki.
Update to the mapping to include reverse and reverse-slip plaback,
plus general clean-up of files to comply with coding standards.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you mean it doesn't work? What are you trying to accomplish? Generally, the shutdown function should turn off all LEDs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do restore the LEDs to their default illumination in initLEDs(), but in init() I have to connect the VU meters to the VuMeterL and VuMeterR parameters with connectControl(). I noticed that to disconnect a control you have to pass an additional true param to the call and assumed that since the script had connected the control in init(), it was the scripts responsibility to clean up and disconnect the control in shutdown(). However, when you try to do this an error is thrown, so I guess this isn't the case. I thus commented out the disconnect (but put a reminder in the code in case I forgot why later, or if anyone else not familiar with Mixxx was wondering about this).

I didn't see anywhere in the Wiki that said you don't have to do this clean-up, and the assumption is (generally) that if you allocate a resource you should deallocate that resource when you are finished with it.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 22, 2016

Thanks for submitting this :) I'll take a look through it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please put spaces around assignment operators throughout the script for readability. For example:
BehringerCMDStudio4a.delState[channel] = !BehringerCMDStudio4a.delState[channel];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, fair point, I do it most of the time, but have missed a few, bad habit! :-) Fixed now.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 22, 2016

I think it would make more sense to move the gain control to the FX knobs closest to the mixer.

EDIT: Nevermind. With the numbered buttons below it would be a bit awkward that way if one knob was above button 1 but the other was above 4.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would make more sense to declare these variables as arrays. For example:
BehringerCMDStudio4a.pitchInc = [false,false,false,false];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, fixed that. The objects (defined somewhat differently) were a left-over from an early (stupid) version of the code and I forgot about them when I moved to numeric indexes.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 22, 2016

Do the main output and headphone volume knobs control the controller's sound card? If so, mention it on the wiki so users don't wonder why it isn't changing the software gains in Mixxx.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 22, 2016

Could you add controls for fixed beat loops (referred to on some controllers as "autoloops")? I suggest making DELETE + Loop on/off toggle a 4 beat loop and DELETE + the arrow buttons halve/double the loop size. Do you have any ideas for ways to add in controls for moving loops?

EDIT: Thinking about it more, I think that should be the default way of using the loop controls and the manual setting of the in & out points should be the functions activated by holding DELETE.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Likewise, this could be simplified by combining both the press and release functions into one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like you've done this for every button handled by JS. Could you use one function per button throughout?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See earlier comment.
I've only done this where the function is different for each (press or release). I didn't know about the toggleControl() function at the time, so I could replace the cuePush() and cueRelease() functions with a cue() "super-function" using toggleControl() but thinking about it now, what if the behaviour get's out of sync for some reason (e.g. if the GUI is touched perhaps?, or because of some other odd interaction??). If that happens the Push/Release functions would get swapped, (with no simple way for the user to restore normal behaviour). Perhaps this is unlikely, but I believe in "defensive programming" :-)

Perhaps you could get round this by keeping the explicit setValue() calls and wrapping the super function in an if condition to detect whether we are processing a push or a release, but then we are back to adding an if clause to the "super-function" to do this (i.e. creating a more complex function) when we already have the ability to do this test in the XML.

If single functions for both press and release is the Mixxx "house-style" then I will change this (let me know), but (IMHO) it seems simpler the way it is (although again, if shift/mode functionality could be declared directly in the XML without additional scripting, life would be much easier" :-))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do think that is really unlikely and not really the job of a mapping to deal with at the cost of largely redundant code in the mapping. The most likely scenario I could think of in which this might possibly be an issue would be someone holding the cue button on one controller or the keyboard and accidentally hitting it on this controller before releasing it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Single functions for press and release are the conventional way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, for this button. script.toggleControl() wouldn't be the best way to do it. Just use (value === 127) ? 1 : 0 as the third parameter of engine.setValue()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I've replaced all the push/release functions with single functions using (value === 127) ? 1 : 0
Code density up, but code readability down I think. :-(

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 22, 2016

Overall the mapping and wiki look pretty good, so thanks for the easy review. :) I edited the wiki a bit. Most changes to the wiki were just small rewordings.

@sblaisot
Copy link
Copy Markdown
Member

Please add the new files to windows uninstaller
http://www.mixxx.org/wiki/doku.php/contributing_mappings#windows_installer_update

@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

Many thanks for the helpful comments and suggestions. I'll get to work on the changes this evening.

@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

Be-ing commented:

Do the main output and headphone volume knobs control the controller's sound card? If so,
mention it on the wiki so users don't wonder why it isn't changing the software gains in Mixxx.

The main and headphone volume knobs don't control the sound card, they do control the software gains in Mixxx ([Master].gain and [Master].headMx) they are set-up in the XML (and do change the software gains in Mixxx properly).

@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

sblaisot commented:

Please add the new files to windows uninstaller
http://www.mixxx.org/wiki/doku.php/contributing_mappings#windows_installer_update

Opps, sorry, completely missed that, fixed now!

@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

Be-ing commented:

Overall the mapping and wiki look pretty good, so thanks for the easy review. :) I edited the wiki a
bit. Most changes to the wiki were just small rewordings.

Many thanks, much appreciated.

@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

Be-ing commented:

I think it would make more sense to move the gain control to the FX knobs closest to the mixer.
EDIT: Nevermind. With the numbered buttons below it would be a bit awkward that way if one
knob was above button 1 but the other was above 4.

Yep, the deck layouts aren't very consistent. The controls are mirrored left/right, but the numeric identifiers (the hot-cue and FX-control/assign buttons) aren't mirrored (for obvious reasons) so the layout ends up a bit of a mishmash between the two options.

A number of minor (non functional) changes as a reult of the pull request,
plus a few other (non functional) improvements, e.g. Loop start/end release
XML definitions, (so as not to corrupt the GUI state).
@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

Be-ing commented:

Could you add controls for fixed beat loops (referred to on some controllers as "autoloops")? I
suggest making DELETE + Loop on/off toggle a 4 beat loop and DELETE + the arrow buttons
halve/double the loop size. Do you have any ideas for ways to add in controls for moving loops?
EDIT: Thinking about it more, I think that should be the default way of using the loop controls and
the manual setting of the in & out points should be the functions activated by holding DELETE.

Mmm..., Yep, auto-loops are missing from the deck layout and I'm not happy about that. I have some ideas about how to squeeze this into the deck, but I need to work out what will work best. Let me try some options out and I'll get back to you.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extra indentation here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, tabs crept into the file, fixed now.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 23, 2016

Okay, leave a comment when you push a new commit.

Further (hoppefully non functional) changes to comply with the pull request review.
Rationalised button push/release functions into single button functions.
Minor typos corrected.
Auto-loop functionality is still missing.
Added auto-loop functionality.
Added FX change functionality.
DEL button now has to be held to delete a hot cue.
Further code clean-up for pull-request.
@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

New version of the mapping committed. Now includes auto-loop functionality as requested.

@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

Wiki has been updated to reflect the functionality changes in this new version (I've also updated the forum thread).

Minor typos corrected.
Changed the slip "re-sync" timer after the discussion in
     https://bugs.launchpad.net/mixxx/+bug/1538200
Consolodated the pitch inc/dec functions.
Further minor clean-up.
@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Apr 5, 2016

The main and headphone volume knobs don't control the sound card, they do control the software gains in Mixxx ([Master].gain and [Master].headMx) they are set-up in the XML (and do change the software gains in Mixxx properly).

Do volume controls show up in your OS mixer program (alsamixer on GNU/Linux, speaker system tray icon on Windows and Mac) when you select the CMD 4a's sound card? If so, these should be used rather than the software gains in Mixxx.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Apr 5, 2016

I thought of a few possibilities for the loop button behavior. What do you think about these?

  • Exit the loop on button up for short loops <= 1 beat, which is the top row of buttons, but not for longer loops in the second row of buttons. I just tried this in the Hercules P32 mapping and I think it's intuitive to use.
  • Exit the loop on button up only when slip_enabled is 1.
  • Make releasing loops on button up an option that can be easily changed by users by editing the value of a boolean at the top of the script. Or make one of the behaviors above an option.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Apr 5, 2016

Sorry for putting this on hold for so long. I got a bit overwhelmed when 5 mappings were submitted in about a week.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Apr 28, 2016

@GandalfMixxx , thoughts about the above questions?

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Nov 26, 2016

@GandalfMixxx : ping. What do you think about the other ways of mapping the loop buttons I suggested above?

@rryan
Copy link
Copy Markdown
Member

rryan commented Jan 11, 2017

It would be great to have this in Mixxx 2.1 -- @GandalfMixxx @Be-ing what's left to do here?

@GandalfMixxx
Copy link
Copy Markdown
Contributor Author

When I last worked on this about a year ago it was ready for inclusion as far as I was concerned. I'd addressed all the comments/suggestions from Be-ing and also created what I considered to be pretty good documentation on the Wiki (I'd also posted everything to the Mixxx forum for others to try out). Then I didn't get any further replies for months and still no merge so I moved on to other things.

Three months later Be-ing pops up with more suggestions for alterations that didn't make any real sense to me and I really didn't have the energy to come back to this only to get more objections to a merge. I figured that the mapping is out there and easily downloadable for those that actually have the controller (from the forum), and the documentation is on the Wiki too. If it's not good enough for a merge then fair enough.

More months pass, then in November another post from Be-ing, again not talking about a merge but instead asking me to go over his suggestions from over 6 months ago??!! Sorry, I made every effort to give back to the community already and got nowhere. My time is too valuable to mess about like this.

So here we are a year later... All I can say is that a year ago the mapping was working well enough for me. It was (as far as I was concerned) feature complete, and while it may not be perfect, or suit everyone, I think it was good enough for inclusion back then (even if only on the basis of a first version that could be extended later). I can't honestly say if it will work as well with the latest version of Mixxx as I've not installed it, but I see that there have been some downloads from the forum, some generally positive feedback, and some useful modifications from people that actually own (or owned) the controller (that I might have taken the time to roll into the mapping here if my experience with this contribution hadn't been so negative).

I'm happy for you to include it in a version of Mixxx "as is" if you are now happy with it or for you to reject the PR if you think it's still not up to scratch. I may come back to work on this mapping in the future but it probably won't be for a while yet as I'm busy with other things right now.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 12, 2017

It would be good to update the mapping for the new effects UI, but I'd be okay with merging it as is. I think there's room for improvement with how the loop buttons are mapped, but it's not bad how it is.

The underlying functionality for the new effects UI has been merged, but so far the skins have not been updated for it. If you'd like to see where it's going, PR #1063 is for updating the Deere skin. PR #1054 contains a JavaScript library that will make it easy to map the full capabilities of the new effects UI to this controller.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 12, 2017

I apologize for not responding for a long time when you first submitted this. Coincidentally when you submitted this, 4 other mappings were submitted within a week so I got overwhelmed.

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 12, 2017

Thank you for your contribution. Hopefully the user who posted a modified version on the forum will want to take up the work of updating it for the new effects UI.

@rryan
Copy link
Copy Markdown
Member

rryan commented Jan 13, 2017

Sounds good -- let's merge this then. @GandalfMixxx thanks for your contribution and sorry for the delay.

@rryan rryan merged commit cb4a910 into mixxxdj:1.12 Jan 13, 2017
@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Jan 13, 2017

This was targeted at the 1.12 branch. @rryan can you merge it into master?

@rryan
Copy link
Copy Markdown
Member

rryan commented Jan 13, 2017

This was targeted at the 1.12 branch. @rryan can you merge it into master?

Good catch, done.
f3905ba

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants