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 Behringer-CMD-PL-1 controller #11529

Closed
wants to merge 1 commit into from
Closed

Add Behringer-CMD-PL-1 controller #11529

wants to merge 1 commit into from

Conversation

yledoare
Copy link

@yledoare yledoare commented May 2, 2023

Hello,
this a pull request from 2.3 branch
I hope this fix the PR 11518
Regards

@ronso0
Copy link
Member

ronso0 commented May 2, 2023

Tthe mapping test fails:

The following tests FAILED:
	 93 - ControllerPresetValidationTest.MidiPresetsValid (Failed)

https://github.com/mixxxdj/mixxx/actions/runs/4859672642/jobs/8663843593?pr=11529

You need to install and run pre-commit as adviced multiple times there #11518 (comment)

@yledoare
Copy link
Author

yledoare commented May 4, 2023

I have this error on pre-commit

don't commit to branch...................................................Failed

  • hook id: no-commit-to-branch
  • exit code: 1

@yledoare
Copy link
Author

yledoare commented May 4, 2023

And when I want to make tests, I have a timeout

1/650 Test #1: AnalyzerWaveformTest.simpleAnalyze ..............................................***Timeout 1500.13 sec

Maybe because I need a defined DISPLAY ?

@ronso0
Copy link
Member

ronso0 commented May 4, 2023

I have this error on pre-commit

don't commit to branch...................................................Failed

* hook id: no-commit-to-branch

* exit code: 1

That is because you named your branch like the target. You can skip this hook with
SKIP=no-commit-to-branch git commit as explained in https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking

@ronso0
Copy link
Member

ronso0 commented May 4, 2023

But eslint also fails, see https://github.com/mixxxdj/mixxx/actions/runs/4859672639/jobs/8663843595?pr=11529#step:6:185
But you should also see that locally.

@Swiftb0y
Copy link
Member

Swiftb0y commented May 4, 2023

@yledoare May I ask what your intention is with this PR? Do you need help with mapping it or is this supposed to be a complete submission?

@yledoare
Copy link
Author

@ronso0 :

pre-commit makes no error with SKIP=no-commit-to-branch, thanks
eslint error should be fixed in last commit

@Swiftb0y

It is a complete submission

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'm sorry, I can't accept the PR in this state. Only 10% of the hardware is actually mapped and the implementation is sloppy and full of errors to the point where I doubt this mapping has ever even been tested. @yledoare if you want to make a helpful contribution, please take the time to get familiar with our documentation, XML and Javascript so you can create a usable and polished mapping.

Comment on lines +17 to +19
<key>play</key> i
<description>Play</description> i
<status>0x90</status> i
Copy link
Member

Choose a reason for hiding this comment

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

typos resulting in invalid XML

Comment on lines +7 to +8
<forums>https://mixxx.discourse.group/t/behringer-cmd-pl-1/15755/2</forums>
<wiki>https://github.com/mixxxdj/mixxx/wiki/Behringer%20CMD%20PL-1</wiki>
Copy link
Member

Choose a reason for hiding this comment

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

The forum post references a completely different mapping. The wiki page is basically empty.

Comment on lines +15 to +30
<control>
<group>[Channel1]</group>
<key>play</key> i
<description>Play</description> i
<status>0x90</status> i
<midino>0x23</midino>
<options> <switch/> </options>
</control>
<control>
<group>[Channel2]</group>
<key>play</key>
<description>Play</description>
<status>0x91</status>
<midino>0x23</midino>
<options> <switch/> </options>
</control>
Copy link
Member

Choose a reason for hiding this comment

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

why the mapping to two different channels?
these should be <normal/> and probably also need a midi note-off mapping.

};

CMDPL1.pitchSlider = function(channel, control, value, status, group) { // Lower the sensitivity of the pitch slider
const currentValue = engine.getValue(group, "rate");
Copy link
Member

Choose a reason for hiding this comment

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

you don't need the this, and please remove the unnecessary comments.

Comment on lines +25 to +47
let deckNumber = script.deckFromGroup(group);
//print("> value : "+deckNumber);

// A: For a control that centers on 0:
let newValue;
if (value < 64) {
newValue = value;
} else {
newValue = value - 128;
}

// B: For a control that centers on 0x40 (64):
newValue = value - 64;

// --- End choice

// In either case, register the movement
deckNumber = script.deckFromGroup(group);
if (engine.isScratching(deckNumber)) {
engine.scratchTick(deckNumber, newValue); // Scratch!
} else {
engine.setValue(group, "jog", newValue); // Pitch bend
}
Copy link
Member

Choose a reason for hiding this comment

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

you were only supposed to use either implementation, nevertheless, actually scratching won't work either because you never actually map the wheeltouch.

@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 Aug 15, 2023
@Swiftb0y
Copy link
Member

closing this PR as the author seems to be too negligent to follow the basic tutorial and too unresponsive to respond to review comments.

@Swiftb0y Swiftb0y closed this Aug 15, 2023
@o-bardiuk
Copy link

o-bardiuk commented Oct 28, 2023

It's been 3 years to include a mapping into the code. Cool

No wonder people give up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controller mappings incomplete stale Stale issues that haven't been updated for a long time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants