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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ctlra integration to Mixxx #1308

Open
wants to merge 27 commits into
base: master
from
Open

Conversation

@harryhaaren
Copy link

harryhaaren commented Jul 9, 2017

Hi!

This PR adds Ctlra support to Mixxx, post-discussion on the Mixxx-devel mailing list. It intended to be as small and easy to consume as possible, providing a quick overview of the actual state of the patchset. An accompanying video is available here: https://www.youtube.com/watch?v=sNC4r7-TB0Y

The basic structure of the code is simple;

  • ControllerManager class has a new item, the CtlraEnumerator
    -- This class owns the main Ctlra library instance, and handles discovery and hotplug
  • The CtlraReader is a reader thread to poll the Ctlra enabled devices (same idea as HID)
  • A CtlraController instance represents one physical device attached to Mixxx
    -- Each instance has its own virtual event_func and feedback_func
    -- The virtual functions allow derived classes handle events in specific ways (JS engine, TCC, etc)
    -- The CtlraController can be derived from to provide high-performance support a specific controller

Finally, to demonstrate that this all actually works, I have created 2 hard-coded bindings;

  • Event input SLIDER with ID 0 affects the ControlProxy "Master" "crossfader" (see here)
  • Mixxx "cue_indicator" mapped to Ctlra Light ID 0 (see here)

I am aware that the coding style is not Mixxx's preferred - and that there are some other issues to be resolved too. Please review the concept here instead of the exact detail so no "typo" comments OK?? 馃槂

To compile this branch please read this commit message carefully - it details the steps required to successfully configure and link against Ctlra : openAVproductions@29862e2

Next steps should be to discuss and POC an implement a mapping system from Ctlra Events (see CtlraController::event_func) to the Mixxx engine, in a way that is both powerful and consumable for users.

Thanks for reading and if you have ideas please do get involved - more voices will ensure that the solution is workable for both users, developers and everybody else!

Cheers, -Harry

[1] openAVproductions/openAV-Ctlra#13

@harryhaaren
Copy link
Author

harryhaaren commented Jul 9, 2017

Just a side-note that the CI builds won't work, given it links (awkwardly) against a library that the CI won't have installed.

@sblaisot
Copy link
Member

sblaisot commented Jul 9, 2017

you can update .travis.yml / appveyor.yml to instruct the CI pipeline how to prepare for the link to work.

@harryhaaren
Copy link
Author

harryhaaren commented Jul 9, 2017

@sblaisot, thanks for the tip. AppVeyor (aka Windows) is certainly not expected to work, given Ctlra isn't currently x-platform. Travis on Ubuntu should be possible, however I'm not experience with Travis, and compiling things from Git may not be the easiest. I'll attempt it later.

Right now, my opinion is that there is more value in reviewing the concept, discussing mapping from Ctlra events to Mixxx, and how to enable user-workflows with controllers best.

@Be-ing
Copy link
Member

Be-ing commented Jul 10, 2017

Thanks for submitting this! As you may have noticed, we have quite a backlog of PRs in progress. I'd like to finish up some of those before looking at this in detail.

def configure(self, build, conf):
if not build.platform_is_linux:
return
ctlra_env = str(os.environ["CTLRA_PATH"])

This comment has been minimized.

Copy link
@daschuer

daschuer Jul 10, 2017

Member

You schould consider to use pkg-config instead.

This comment has been minimized.

Copy link
@harryhaaren

harryhaaren Jul 10, 2017

Author

Yes absolutely. I don't actually have experience writing pkg-config files, and while its meant to be easy, I didn't find an easy to follow tutorial / example with CMake how to do it properly. I'll try / keep digging though, and ultimately when Ctlra is upgraded then this can follow suit.

Copy link
Member

daschuer left a comment

A good start. For now, there is no instant benefit compared to the pure hid version.
It looks like a step back compared to the midi version where point an click mapping is available.
Do you have any ideas how to interface to that?

I think an issue are your different control types.
Can we unify them to be more like midi?

case CTLRA_EVENT_BUTTON:
name = ctlra_info_get_name(&info, CTLRA_EVENT_BUTTON,
e->button.id);
printf("[%s] button %s (%d)\n",

This comment has been minimized.

Copy link
@daschuer

daschuer Jul 10, 2017

Member

Use our Logging facilities here.

if(e->slider.id == 0) {
// TODO: optimize to avoid repeated
// lookups of ControlProxy
ControlProxy("[Master]", "crossfader")

This comment has been minimized.

Copy link
@daschuer

daschuer Jul 10, 2017

Member

This is all done in our controller system.
You schould look into the Hid implementation and route the call in the same way to js.

This comment has been minimized.

Copy link
@Be-ing

Be-ing Jul 10, 2017

Member

IMO that would defeat the point of Ctlra. Each event should be handled by its own JS callback function registered by the script, not all dumped into the same callback with a giant switch/case statement.

This comment has been minimized.

Copy link
@Be-ing

Be-ing Jul 10, 2017

Member

Potentially the hash of Ctlra events to callback functions could be maintained in JS, but I'm not sure if that would have any advantage over doing it in C++.

This comment has been minimized.

Copy link
@harryhaaren

harryhaaren Jul 10, 2017

Author

I see value in providing per-event callback functions in JS, but indeed the implementation of calling a JS function from C/C++ can be "inspired" from the HID version. I'll look into this when I have some time.

This comment has been minimized.

Copy link
@Be-ing

Be-ing Jul 10, 2017

Member

I think it would make sense to keep the hash of Ctlra events to input callbacks on the C++ side to have a more consistent API with how output callbacks are managed.

// this class is initialized by caller.
m_preset.setDeviceId(info->serial);
char buf[64];
snprintf(buf, sizeof(buf), "%s %s\n", info->vendor, info->device);

This comment has been minimized.

Copy link
@daschuer

daschuer Jul 10, 2017

Member

Use qt strings here .

@Be-ing
Copy link
Member

Be-ing commented Jul 10, 2017

I think an issue are your different control types.
Can we unify them to be more like midi?

I think the only potential issue with the control types are how jog wheels would be handled. @Pegasus-RPG do you have any thoughts about that? @harryhaaren, look at the code for the current S4 mapping.

@harryhaaren
Copy link
Author

harryhaaren commented Jul 10, 2017

Re Jog Wheels;
They are currently implemented with a delta offset, and the timestamp value from the USB HID packet isn't used (timestamping is on my TODO list, but requires some thought). Indeed the "device supplied" timestamp (when the Jog wheel moved) can be used for jog wheel events, but timestamping will also be required for other Ctlra events too. Not sure how to implement this yet - hence its not done. Filed an issue to track timestamping in Ctlra: openAVproductions/openAV-Ctlra#25 For details on how the Ctlra driver decodes S2 jog wheels, please see here.

Re Advantages of Ctlra;

  1. Ctlra supports hotplug, which means that Mixxx can stay running and controllers be added/removed on the fly. That's a pretty big win in my book. Also if your controller gets unplugged during a DJ set, you just plug it back in and keep DJ-ing, instead of restarting Mixxx. Admittedly the Mixxx UI doesn't update itself yet - but Ctlra handles the hotplug, creates the CtlraController and then actually polls the device - so it will function.
  2. Device support, currently some devices that are not usable on Linux are enabled by Ctlra. These happen to be the NI devices that I have access to. Devices implemented in future will be made available in Mixxx without performing the HID packet parsing etc - just mapping events.
  3. Yes a "Ctlra Mapping Mode" just like MIDI map mode can be implemented. The question is about value - ultimately multiplexing these controllers provides more powerful workflows, and a "single-static-mapping" like a MIDI Map doesn't achieve that. It is easy though - so I'm not opposed to a "Move-Fader-Then-Move-UI-item" workflow like the MIDI Wizard has. This is possible due to the generic event abstraction that Ctlra emits by parsing the (eg: USB HID) input.
  4. Once the "Dummy device" is implemented, we can provide a "virtual" Ctlra instance, allowing anybody to create a mapping, even if they don't have access to the hardware. This enables techie users to script something, and that same script to work on the HW controller of a not-so-techie user. These kind of workflows should enable the community to work together to create powerful mappings. (This feature will take a few months, but is worth noting here because it will enable integration of the more-techie and less-techie folks in the community - something I see as a very strong point)
  5. Finally, based on the capability to derive from the CtlraController class, high-performance implementations for specific controller devices can be implemented, enabling the use of eg: screens, or other capabilities that the JS engine wouldn't handle well, or currently doesn't support.

Apologies for the essay - hope it makes clear what my goals are with the Ctlra integration! -Harry

harryhaaren added a commit to openAVproductions/openAV-Ctlra that referenced this pull request Jul 10, 2017
Added support to have TravisCI automatically build Ctlra, as a compile
time check only. This work was partly initiated due to the Mixxx
integration requiring TravisCI to work and build Mixxx with Ctlra:
mixxxdj/mixxx#1308

Note that the include_directories command in CMakeLists.txt has been
reverted to the older CMake variety. It shouldn't have any negative
side effects.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
@harryhaaren
Copy link
Author

harryhaaren commented Jul 10, 2017

The above commits should build Mixxx with Ctlra enabled on Linux using Travis. OsX and Windows do not currently enable Ctlra. I see more value in working out a mapping strategy for controls and feedback, than supporting more platforms right now.

@Be-ing
Copy link
Member

Be-ing commented Dec 28, 2017

Hi @harryhaaren,
I'm excited by the possibilities of Ctlra, but I am doubtful this would be ready for our 2.2 release which we plan to feature freeze around June 2018 considering all the other cool new features we have planned for 2.2. That said, I do want to start looking at what you have so far soonish and clarify how this should fit into Mixxx to give you a clearer direction to continue development and come up with a tentative time table for merging it.

It has come to our attention that the QtScript API we use for the current controller scripting system has been deprecated, so we might need to work on a new scripting system with Qt5's newer QJSEngine API. It would be a shame if we put in a bunch of work to integrate Ctlra into Mixxx then had to redo large parts of it later, so we should clarify what to do about the deprecation of the QtScript API before working to integrate Ctlra with Mixxx's JS scripting system.

Personally, I'm not comfortable contributing to the Ctlra library itself, which may be of commercial value to for-profit companies, unless it is changed to a copyleft license like the Mozilla Public License and I can be certain that free software will benefit if it is adopted by proprietary software companies. We can revisit that discussion on the issue in Ctlra's tracker.

@Be-ing Be-ing added this to the stalled milestone Apr 18, 2018
@IF-Adin
Copy link

IF-Adin commented Jan 6, 2019

@Be-ing So, can we put this back on the radar, please? Mixxx 2.2 is out, can't it be integrated into the next version? I have two f1's laying around, just gathering dust, would be nice to finally be able to use them with mixxx. I tested Ctlra, they work fine with it.

@daschuer
Copy link
Member

daschuer commented Jan 6, 2019

@IF-Adin thank you for putting attention on this. Can you clarify why the F1 controller can't be used in Midi mode?

To continue here, it would be a first good step to adopt here some hid controller code and pass the ctrla events to the JS script. This should be a straight forward task. The next step would be to map the ctlra events to Mixxx COs using JS.
Die you have interests to adopt some of this work?

@IF-Adin
Copy link

IF-Adin commented Jan 6, 2019

@daschuer As much as i would like to adopt this work, i am a frontend developer, i can't do this kind of stuff. As far as i know @harryhaaren is maintaining ctlra.

The reason the controller can not be used in midi mode is that the only way to put it into midi mode is to use the official driver, which is only available for windows and mac. This means that if you connect it to a linux machine, it will detect it, but there is no way to switch on the midi mode. @harryhaaren has released some demo code, though and i can confirm that it works, it correctly identifies all buttons and seems to be quite low latency, at least from what i can see.

If someone would create a branch for this, i could help by testing it. I also have an kontrol s4 mk 1 and an kontrol s2 mk 1 availible for testing, though they are not implemented into ctlra yet.

@daschuer
Copy link
Member

daschuer commented Jan 6, 2019

"i am a frontend developer" what does this mean?
It is just that this PR has stalled for over a year and no one has interests to change that. So I am afraid we need to wait for a second contributor with "backend" knowledge and a personal interest to merge this here.
Or ... you take the adventure to dive into the backend. I am happy to help.

@Be-ing
Copy link
Member

Be-ing commented Jan 6, 2019

We have recently had lots of discussion about integrating Ctlra with Mixxx. Unfortunately it is still not clear what the role of Ctlra and Mappa are and how they could be integrated into Mixxx. If they will be integrated with Mixxx, it does not seem that will happen very soon.

The current focus of development on the controller system is enabling support for ES7 (#1795), then removing the requirement of the redundant XML mappings for MIDI controllers.

Regarding your F1s, those are class compliant HID devices. @xerus2000 is working on a new HID mapping for them. Unfortunately the Kontrol S4 Mk1 and Kontrol S2 Mk1 are trickier because they use their own nonstandard USB protocol. It might be possible to add support for them with the existing "USB bulk" controller system in Mixxx.

@IF-Adin
Copy link

IF-Adin commented Jan 6, 2019

@daschuer As much as i would like to, this honestly isn't my area of expertise and my job leaves me with very little time to learn this.

@Be-ing Thank you so much for your answer. If you guys could make that would i'd be ever so grateful. If there is any testing you need done with the s4 or s2 please let me know. I figure that at the very least i could somehow supply you with the data you require to figure them out.

Huge respect for the work you guys do here.

@Be-ing
Copy link
Member

Be-ing commented Jan 6, 2019

Adding support for a controller via the USB bulk controller system requires whitelisting them here with the USB vendor ID, product ID, and the endpoint addresses for the input and output endpoints. These can be found with lsusb -v. If you'd like to try to get that working, please make a new topic in the #controller mapping stream on Zulip so we don't drag the discussion on this PR off topic.

@harryhaaren
Copy link
Author

harryhaaren commented Jan 9, 2019

Hey folks, just to set the record straight, there was no loss in interest in this PR, it took a different direction with the Mappa component, refer to code here: https://github.com/openAVproductions/openAV-Ctlra/tree/mapping_v1, and I have a Mixxx branch that i'll try push in the next weeks to show how that Ctlra/Mappa code integrates with Mixxx.

In terms of devices, the NI F1 doesn't have a "MIDI Mode", its a USB HID device only. The Win/OsX drivers do software emulation of "midi mode" by using a virtual MIDI device. For this reason, on Linux (no official driver) these devices (as well as most other modern NI gear) will not function at all on linux without Ctlra.

Yes, Ctlra was built with low latency + high performance as requirements.

S4 Mk1 and S2 Mk1 are not standard USB Bulk devices IIRC, so I don't think the existing code will help. There is a linux-events subsystem that supports some of the NI Mk1 hardware, that might show how the USB comms work, I haven't investigated much as I don't currently have access to any Mk1 hardware.

In terms of Ctlra integration into Mixxx, its pretty clear to me that there are about 3 different approaches, each with pros/cons:
0) Ctlra + Hard-coded-support

  1. Ctlra + TCC scripting
  2. Ctlra + Mappa (dynamic, powerful, multi-layered, user-supplied config file mappings)

We (as the Mixxx open-source community as a whole) have discussed this a lot on Zulip chat of Mixxx, under the controller-mappings topic, and although there were some novel inputs, there was no actual progress (IMO) in terms of selecting a single one of these options in terms of likelyhood of it actually getting it accepted upstream.

@daschuer
Copy link
Member

daschuer commented Jan 9, 2019

Thank you for summarize.
Both option can be reasonable in the long term.

I think we have to split this project into do-able and review-able steps. The first issue is that we need to establish ctlra Mixxx.
One of the main issue because this has stalled is that there is no pressure, to integrate that in terms that a contributor is offering some spare time. This is an issue for all controller mappings by the way. Only a few controller owner benefit from it. If there is js speaking guy among them it is done, else not.

Here draft steps (for improvement)

  • Pipe the ctlra events to our scripting domain
  • Define a data structure to expose the non CO player states to the scripting domain and ctrla
  • Extend our current XML mapping with Mappa.
  • Extend our current scripting system by TCC

What do you think?

@IF-Adin
Copy link

IF-Adin commented Jan 9, 2019

@daschuer JS is something i can do. I use it for mostly frontend applications, but i still understand it large. I could try to help.

As for pressure, that is true, but i do think there are a lot of old NI devices out there, they sold a lot and it's not like they really that worse nowadays then they used to be. I wouldn't say that it's a small group of people.

@harryhaaren The main reason i bumped this was to get the discussion going again, so that at least a small degree of it is visible to the public again. I understand this is not the best place to discuss.

@daschuer
Copy link
Member

daschuer commented Jan 9, 2019

@daschuer JS is something i can do. I use it for mostly frontend applications, but i still understand it large. I could try to help.

Great, so we "just" need one who is able to adopt the path into the JS domain from controllers/hid/hidcontroller.cpp

Any volunteers?

@harryhaaren
Copy link
Author

harryhaaren commented Jan 9, 2019

@daschuer First steps first - I'll review this code to align it with latest Ctlra progressions / best-practice (not sure much changed, but to be sure) and then rebase it against Mixxx-git-master. Once I have some hard-coded events being handled correctly, then I'll push the code up and we can look at the JS side. (For context, note there is discussion going on here too https://mixxx.zulipchat.com/#narrow/stream/113295-controller-mapping )

harryhaaren added 2 commits Jul 6, 2017
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit adds the enumerator class for Ctlra devices.
The main ctlra context is owned by the Enumerator class,
as this is where the device hotplug callbacks reside.
When a new device is hotplugged, it can be added/removed
from the list of controllers in the UI panel easily, as
that is the class that owns the context.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
harryhaaren and others added 24 commits Jul 8, 2017
This commit introduces the ctlra context, initializes it, and
accepts any devices that Ctlra supports using the accept_dev
callback.

The struct mixxx_accept_dev_t is used to hide ctlra typedefs
from the CtlraEnumerator.h file, instead only requiring the .cpp
file to include the Ctlra header file.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit adds a QThread derived class to poll the Ctlra
Controller instances. If no devices are detected, the thread
just sleeps without consuming CPU time.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This ensures that the ctlra libusb context doesn't interfere
with libusb based BULK devices already supported in Mixxx,
or the HIDAPI libusb instance.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit refactors the CtlraReader to operate correctly,
and adds an event printing function to show what events are
handled by Mixxx's Ctlra instance.

All devices are accepted, and all events are routed to the
same handling function, which obviously doesn't scale to
having multiple controllers use different mappings. That will
be solved when the CtlraController class is introduced.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
A ControlProxy is used to lookup the crossfader and set it
to react to generic slider event 0. This mapping needs to be
made controler-device (and selected mapping) specific, but the
general idea is shown well here.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit introduces the CtlraController class, which is derived
from the Mixxx Controller class. This allows it to be inserted into
the GUI menu, and treated just like other (MIDI/HID) controllers.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit will iterate all devices at startup using Mixxx's
queryDevices() function, and for each Ctlra detected controller,
it will show up in the Options->Preferences->Controllers list.

Currently the controller name and info is all hard-coded, passing
in the correct metadata and making it dynamic is next steps.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit adds the ctlra_dev_info_t struct to the CtlraController
class, allowing it to display useful metadata of the device that
it represents. Opening the Controller tab of preferences shows a list
of Ctlra enabled devices with metadata just like any other controllers.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit moves event handling into the CtlraController class,
enabling easy lookup of the Controller properties, like which event
handling script to use, or what to do with the events.

Deriving a class from CtlraController would enable various specializations
to exist. For example, if a JS Mixxx integration is required, it could
derive a JSCtlraController, or if TCC was desired, it would derive the
TCCCtlraController.

If a specific devices *requires* C/C++ code for performance reasons, it
could derive a C/C++ class to handle events or provide a very-high
performance backend for a specific device. Thinking here is that HD
screens should be rendered using C/C++ code, and hence this code will
require access to the device itself to blit the pixels over the cable.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Making event_func virutal enables derived classes to override the
event_func, and hence interpret events as needed. If required, the
parent-class's event_func can still be invoked of course - enabling
the default behaviour again.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit introduces the feedback_func into the CtlraController
class, enabling it to write LED and other feedback to the device.
Simiarly to how the input events are mapped, the Mixxx state can
be mapped to the functions available in Ctlra to set the device
feedback.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This commit adds the Ctlra library to the Mixxx depends.py file,
and as such allows the build system to be aware of the library to link.

Given that Ctlra doesn't have a .pc file (yet), the library path to link
with is retrieved from an environment variable.

In order to build Mixxx with Ctlra support, do the following steps:
- git checkout https://github.com/openAVproductions/openAV-Ctlra ctlra
- cd ctlra
- mkdir build && cd build
- cmake ..
- make -j4
- export CTLRA_PATH=`pwd`

Then using the *SAME* terminal, cd <mixxx>, checkout the ctlra branch,
and type "scons ctlra=1". Other flags (-j, optimize etc) can be used as
normal.

Any questions on compiling, please contact me.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
This branch will be used to force-push new updates to have TravisCI try
to build Ctlra from Git and Compile Mixxx against it.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
In March '18 the signature to accept a device changed
for usability, replacing raw function-pointer writing
with some API calls. This changes brings Ctlra code
from > 1 year ago up to date with Ctlra from git master.

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
@harryhaaren harryhaaren force-pushed the openAVproductions:ctlra branch from 63d659a to 67d37db Jan 9, 2019
@harryhaaren
Copy link
Author

harryhaaren commented Jan 9, 2019

OK the above force-push brings the Ctlra code to Mixxx-git-master, and the final commit updates the code to Ctlra-git-master. There is no TCC currently, so the code itself will print Ctlra USB debug messages at runtime, but otherwise there's no way to know that its working. I'll see what the CI comes back with.

This avoids a possible segfault on shutdown due to
the reader thread reading from libusb FDs while ctlra_exit()
has previously called libusb_exit().

Signed-off-by: Harry van Haaren <harryhaaren@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can鈥檛 perform that action at this time.