OSC Server/Client Integration #2078

iffyloop · 2019-04-08T03:50:20Z

This PR adds OSC server support to Mixxx in a way that nicely coexists with Jakob Braun's OSC client implementation. It's basically a more robust implementation of #2064 where I've tried to take into consideration many of the changes requested there. Thanks to everyone for your patience and constructive feedback. Sorry this has taken so long.

Conflicts: build/depends.py src/mixer/playerinfo.cpp src/mixxx.cpp src/mixxx.h src/preferences/dialog/dlgpreferences.cpp

…/wiki/doku.php/osc-client

…o osc_support

daschuer

Thank you for the good first step. I have left some comments

daschuer · 2019-04-08T06:10:12Z

src/mixxx.cpp

@@ -561,6 +563,8 @@ void MixxxMainWindow::initialize(QApplication* pApp, const CmdlineArgs& args) {

    m_pOscClientManager = new OscClientManager(pConfig, m_pEngine);

+    m_pOscServer = new OscServer(pConfig);


we prefere using std::uniwue_ptr and make_unique in new code.

@daschuer That makes sense and I understand that unique_ptr is safer. However, may I ask why we would use unique_ptr since the rest of the codebase seems to use new/delete (e.g. line 181 where m_pSettingsManager = new SettingsManager)? Also, would it be preferable to just store an actual OscClientManager instance rather than a pointer to it in the class? I've always been a little confused why all of the manager members of MixxxMainWindow were stored as pointers.

Also, sorry I've been gone for so long

daschuer · 2019-04-08T06:11:47Z

src/oscserver/defs_oscserver.h

+#ifndef __OSCSERVER_DEFS_H__
+#define __OSCSERVER_DEFS_H__
+
+#define OSC_SERVER_PREF_KEY "[OSCserver]"


Pure camel case would be [OscServer]

daschuer · 2019-04-08T06:15:20Z

src/oscserver/oscserver.cpp

+        return;
+    }
+
+    QString port = pConfig->getValueString(ConfigKey(OSC_SERVER_PREF_KEY, "Port"));


should we fall back to a default port if the preferences value is empty? Or does this introduce issues with already used ports?

daschuer · 2019-04-08T06:16:17Z

src/oscserver/oscserver.cpp

+    QString port = pConfig->getValueString(ConfigKey(OSC_SERVER_PREF_KEY, "Port"));
+    m_st = lo_server_thread_new(port.toLatin1().data(), OscServer::oscErrorHandler);
+    if (!m_st) {
+        return;


Do we have any user feedback? Ab exclamation mark on the osc preferences page or such?

@daschuer Do you think a message box might be preferable to an exclamation mark? I think that might more directly capture the user's attention (e.g., if they click the "OK" button, then the preferences page closes before they see the exclamation mark).

daschuer · 2019-04-08T06:20:28Z

src/oscserver/oscserver.cpp

+    }
+
+    lo_server_thread_add_method(m_st, nullptr, nullptr, OscServer::oscMsgHandler, nullptr);
+    lo_server_thread_start(m_st);


Please handle the return value.

daschuer · 2019-04-08T06:25:34Z

src/oscserver/oscserver.cpp

+    QRegularExpressionMatch pathMatch = pathRegEx.match(QString::fromLatin1(path));
+
+    if (!pathMatch.hasMatch()) {
+        qWarning() << "ERROR: Invalid OSC path! Proper format: /<group>/<control>";


You can omit "ERROR:"
How about show the received string? This can be useful if only on if many us broken.

daschuer · 2019-04-08T06:26:24Z

src/oscserver/oscserver.cpp

+    ConfigKey key = ConfigKey(pathMatch.captured(1), pathMatch.captured(2));
+
+    if (key.isNull() || key.isEmpty()) {
+        qWarning() << "ERROR: Invalid group/key pair specified in OSC path!";


The same here.

daschuer · 2019-04-08T06:28:46Z

src/oscserver/oscserver.cpp

+            (argType == LO_SYMBOL)) {
+            if (OscServer::m_st && (reinterpret_cast<char*>(argv[i])[0] == '?')) {
+                lo_address msgTo = lo_message_get_source(static_cast<lo_message>(data));
+                lo_send(msgTo, path, "d", ControlObject::get(key));


Handle return value.

@daschuer What would you like to happen when lo_send returns a failure code? Right now I'm thinking of just printing an error message "Failed to reply to OSC get parameter message from: " followed by the URL of the target address.

daschuer · 2019-04-08T06:32:54Z

src/oscserver/oscserver.h

+private:
+    static void quitServer();
+
+    static void oscErrorHandler(int err, const char* msg, const char* path);


If possible move the message handler into an anonymous namspace in the cpp file.
All other functions should be non static.

daschuer · 2019-04-08T09:22:58Z

src/preferences/dialog/dlgprefoscdlg.ui

-     </property>
-     <property name="openExternalLinks">
-      <bool>true</bool>
+      <string>(You must restart Mixxx for your changes here to take effect).</string>


A restart OSC Server Button would be helpful.

I'll hopefully redesign this so that the OSC server can automatically restart whenever the user clicks "Apply" or "OK"

It is Ok for me if you keep this for a second PR.

Ok. I'll wait then so we can get the other fixes applied sooner.

iffyloop · 2019-04-08T16:36:01Z

Thanks so much for the comments. I'll work on some fixes when I'm at my computer again. I can respond to your question about falling back to a default port - the reason I simply let the empty port cause an error and quit the server is that I thought specifying an empty port would let the user disable the OSC server (since probably not all users want Mixxx connected to the internet available for remote control) -------- Original Message --------

…

On Apr 8, 2019, 10:32 AM, Daniel Schürmann wrote: @daschuer requested changes on this pull request. Thank you for the good first step. I have left some comments --------------------------------------------------------------- In [src/mixxx.cpp](#2078 (comment)): > @@ -561,6 +563,8 @@ void MixxxMainWindow::initialize(QApplication* pApp, const CmdlineArgs& args) { m_pOscClientManager = new OscClientManager(pConfig, m_pEngine); + m_pOscServer = new OscServer(pConfig); we prefere using std::uniwue_ptr and make_unique in new code. --------------------------------------------------------------- In [src/oscserver/defs_oscserver.h](#2078 (comment)): > @@ -0,0 +1,6 @@ +#ifndef __OSCSERVER_DEFS_H__ +#define __OSCSERVER_DEFS_H__ + +#define OSC_SERVER_PREF_KEY "[OSCserver]" Pure camel case would be [OscServer] --------------------------------------------------------------- In [src/oscserver/oscserver.cpp](#2078 (comment)): > @@ -0,0 +1,114 @@ +#include "oscserver.h" + +lo_server_thread OscServer::m_st = nullptr; + +OscServer::OscServer(UserSettingsPointer& pConfig) { + if (m_st) { + return; + } + + QString port = pConfig->getValueString(ConfigKey(OSC_SERVER_PREF_KEY, "Port")); should we fall back to a default port if the preferences value is empty? Or does this introduce issues with already used ports? --------------------------------------------------------------- In [src/oscserver/oscserver.cpp](#2078 (comment)): > @@ -0,0 +1,114 @@ +#include "oscserver.h" + +lo_server_thread OscServer::m_st = nullptr; + +OscServer::OscServer(UserSettingsPointer& pConfig) { + if (m_st) { + return; + } + + QString port = pConfig->getValueString(ConfigKey(OSC_SERVER_PREF_KEY, "Port")); + m_st = lo_server_thread_new(port.toLatin1().data(), OscServer::oscErrorHandler); + if (!m_st) { + return; Do we have any user feedback? Ab exclamation mark on the osc preferences page or such? --------------------------------------------------------------- In [src/oscserver/oscserver.cpp](#2078 (comment)): > + +lo_server_thread OscServer::m_st = nullptr; + +OscServer::OscServer(UserSettingsPointer& pConfig) { + if (m_st) { + return; + } + + QString port = pConfig->getValueString(ConfigKey(OSC_SERVER_PREF_KEY, "Port")); + m_st = lo_server_thread_new(port.toLatin1().data(), OscServer::oscErrorHandler); + if (!m_st) { + return; + } + + lo_server_thread_add_method(m_st, nullptr, nullptr, OscServer::oscMsgHandler, nullptr); + lo_server_thread_start(m_st); Please handle the return value. --------------------------------------------------------------- In [src/oscserver/oscserver.cpp](#2078 (comment)): > +OscServer::~OscServer() { + quitServer(); +} + +void OscServer::quitServer() { + if (!m_st) { + return; + } + + lo_server_thread_free(m_st); + m_st = nullptr; +} + +void OscServer::oscErrorHandler(int err, const char* msg, const char* path) { + qWarning() << QString("ERROR: %1 in OSC path %2 (code %3)").arg(QString::fromLatin1(msg)).arg(QString::fromLatin1(path)).arg(err); + quitServer(); Is this always fatal? --------------------------------------------------------------- In [src/oscserver/oscserver.cpp](#2078 (comment)): > + m_st = nullptr; +} + +void OscServer::oscErrorHandler(int err, const char* msg, const char* path) { + qWarning() << QString("ERROR: %1 in OSC path %2 (code %3)").arg(QString::fromLatin1(msg)).arg(QString::fromLatin1(path)).arg(err); + quitServer(); +} + +int OscServer::oscMsgHandler(const char* path, const char* types, lo_arg** argv, int argc, void* data, void* userData) { + Q_UNUSED(userData); + + QRegularExpression pathRegEx("\\/(.+)\\/(.+)"); + QRegularExpressionMatch pathMatch = pathRegEx.match(QString::fromLatin1(path)); + + if (!pathMatch.hasMatch()) { + qWarning() << "ERROR: Invalid OSC path! Proper format: /<group>/<control>"; You can omit "ERROR:" How about show the received string? This can be useful if only on if many us broken. --------------------------------------------------------------- In [src/oscserver/oscserver.cpp](#2078 (comment)): > + +int OscServer::oscMsgHandler(const char* path, const char* types, lo_arg** argv, int argc, void* data, void* userData) { + Q_UNUSED(userData); + + QRegularExpression pathRegEx("\\/(.+)\\/(.+)"); + QRegularExpressionMatch pathMatch = pathRegEx.match(QString::fromLatin1(path)); + + if (!pathMatch.hasMatch()) { + qWarning() << "ERROR: Invalid OSC path! Proper format: /<group>/<control>"; + return 1; + } + + ConfigKey key = ConfigKey(pathMatch.captured(1), pathMatch.captured(2)); + + if (key.isNull() || key.isEmpty()) { + qWarning() << "ERROR: Invalid group/key pair specified in OSC path!"; The same here. --------------------------------------------------------------- In [src/oscserver/oscserver.cpp](#2078 (comment)): > + for (int i = 0; i < argc; ++i) { + lo_type argType = static_cast<lo_type>(types[i]); + + if ((argType == LO_BLOB) || + (argType == LO_TIMETAG) || + (argType == LO_MIDI) || + (argType == LO_NIL) || + (argType == LO_INFINITUM)) { + continue; + } + + if ((argType == LO_STRING) || + (argType == LO_SYMBOL)) { + if (OscServer::m_st && (reinterpret_cast<char*>(argv[i])[0] == '?')) { + lo_address msgTo = lo_message_get_source(static_cast<lo_message>(data)); + lo_send(msgTo, path, "d", ControlObject::get(key)); Handle return value. --------------------------------------------------------------- In [src/oscserver/oscserver.h](#2078 (comment)): > +#include <QRegularExpression> + +#include "lo/lo.h" +#include "preferences/usersettings.h" +#include "control/controlobject.h" +#include "oscserver/defs_oscserver.h" + +class OscServer { +public: + OscServer(UserSettingsPointer& pConfig); + ~OscServer(); + +private: + static void quitServer(); + + static void oscErrorHandler(int err, const char* msg, const char* path); If possible move the message handler into an anonymous namspace in the cpp file. All other functions should be non static. --------------------------------------------------------------- In [src/preferences/dialog/dlgprefoscdlg.ui](#2078 (comment)): > <property name="text"> - <string><a href="http://www.mixxx.org/wiki/doku.php/osc-client">Show documentation</a></string> - </property> - <property name="openExternalLinks"> - <bool>true</bool> + <string>(You must restart Mixxx for your changes here to take effect).</string> A restart OSC Server Button would be helpful. — You are receiving this because you authored the thread. Reply to this email directly, [view it on GitHub](#2078 (review)), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AhnMpEms7epCBAnZAdqdWsleLDP03dOsks5ve2EUgaJpZM4chPfI).

uklotzde · 2019-04-08T19:40:55Z

Shouldn't OSC support be provided as an optional feature? I would recommend this.

It should it least be disabled by default without starting any threads or opening ports.

iffyloop · 2019-04-08T19:57:33Z

Do you mean it should be optional in the build system? It's already optional in the app - if you specify a blank or invalid port then the server doesn't start. -------- Original Message --------

…

On Apr 8, 2019, 2:40 PM, Uwe Klotz wrote: Shouldn't OSC support be provided as an optional feature? I would recommend this. It should it least be disabled by default without starting any threads or opening ports. — You are receiving this because you authored the thread. Reply to this email directly, [view it on GitHub](#2078 (comment)), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AhnMpBxW45nmoF-zQaqsUWac2Z3LpQdIks5ve5tKgaJpZM4chPfI).

daschuer · 2019-04-08T21:22:18Z

I would prefer to use a separate "enabled" key to enable the OSC sever. Having it disabled by default sounds reasonable. It may scare user if Mixxx pops up in a firewall.

Has OSC reserved ports, I have not found anything.
TouchOSC uses 8000 and 9000. Unfortunately this is also used for ShoutCast. However it should not conflict because it is an outgoing port. So how about default to that if the saved port setting is invalid?

iffyloop · 2019-04-09T18:52:00Z

That sounds like a good idea @daschuer. Thanks for bringing ShoutCast to my attention. I don't believe OSC has any reserved ports, but it makes sense to default to TouchOSC port settings if the user enters an invalid port.

@daschuer

Corrected implementation according to @daschuer's comments: #2078 (review)

iffyloop · 2019-05-02T23:39:07Z

@daschuer Just finished making requested changes. Thanks for your suggestions and please let me know your thoughts on these revisions.

daschuer · 2019-05-05T22:33:47Z

build/depends.py

        if not build.platform_is_windows:
-            build.env.Append(CXXFLAGS='-std=c++11')
+            build.env.Append(CXXFLAGS='-std=c++14')


Ups, sorry I should have say that we have our own implementation:
src/util/memory.h that works with c++11

However maybe it is time to enable c++14, but that should be done in a separate PR.

That's fine. Thanks for mentioning it! I will switch to src/util/memory.h if possible

daschuer · 2019-05-05T22:41:17Z

src/engine/sidechain/engineoscclient.cpp

+        float speed = 1 + float(rate.get()) * float(rateRange.get());
+        if(rev.get())
+            speed *= -1;
+        lo_send(serverAdress, "/mixxx/deck/speed" ,"if", deckNr, speed);


I think we need to consolidate the OSC adressspace somewhere maybe a wiki page:
https://www.mixxx.org/wiki/doku.php/osc-client
https://www.mixxx.org/wiki/doku.php/osc_backend

Is this two parameter call TouchOSC compatible?

I don't have TouchOSC (I have an Android device but no Google Play) so I can't test. From what I've read in the docs about custom parameters I think it should work though. If we have a volunteer to test that would be great.

daschuer

I have added some comments. Thank you for all the work.

daschuer · 2019-05-05T22:42:29Z

src/engine/sidechain/engineoscclient.h

+    lo_address serverAdress;
+    UserSettingsPointer m_pConfig;
+    QList<ControlProxy*> connectedControls;
+    ControlProxy prefUpdate;


We use m_ prefix for all private member variables.

daschuer · 2019-05-05T22:43:58Z

src/engine/sidechain/engineoscclient.h

+    QTime time;
+    lo_address serverAdress;
+    UserSettingsPointer m_pConfig;
+    QList<ControlProxy*> connectedControls;


Is this uses somewhere? Can we remove it?

I believe all of these variables are in use in engineoscclient.cpp

daschuer · 2019-05-05T22:47:20Z

src/mixxx.cpp

@@ -557,6 +561,10 @@ void MixxxMainWindow::initialize(QApplication* pApp, const CmdlineArgs& args) {
            SIGNAL(currentPlayingDeckChanged(int)),
            this, SLOT(slotChangedPlayingDeck(int)));

+    m_pOscClientManager = new OscClientManager(pConfig, m_pEngine);


Also make_unique?

daschuer · 2019-05-05T22:49:43Z

src/oscclient/oscclientmanager.cpp

+void OscClientManager::maybeSendState()
+{
+
+}


The formatting of this file is not correct. Please have a look here:
https://www.mixxx.org/wiki/doku.php/coding_guidelines

I notice there are several formatting issues with the OSC-related source files. Do I need to fix these manually or is it appropriate to simply run clang-format on all the .cpp/.h files in this pull request?

daschuer · 2019-05-05T22:50:29Z

src/oscserver/oscserver.cpp

+
+namespace {
+    void oscErrorHandler(int err, const char* msg, const char* path) {
+        qWarning() << QString("%1. OSC path: %2. Error Code: %3.").arg(QString::fromLatin1(msg)).arg(QString::fromLatin1(path)).arg(err);


please break long lines at ~80.

daschuer · 2019-05-05T22:54:42Z

src/oscserver/oscserver.cpp

+
+        if (!pathMatch.hasMatch()) {
+            qWarning() << "Invalid OSC path: " << QString::fromLatin1(path);
+            qWarning() << "Proper OSC path format: /<group>/<control>";


Here, we put the group into the root of the address field while we send out data with the Mixxx prefix.
Will we ever have a problem with this?

I thought the Mixxx prefix would make sense for outgoing data because, if we're interacting with a client that is compatible with multiple OSC apps, the prefix clarifies to the client that they are receiving data from Mixxx rather than another application that may use paths like /Channel1/play (because those are pretty generic names). Incoming data, on the other hand, was explicitly addressed to Mixxx, so the prefix is unnecessary because, e.g., /Channel1/play, can only have one possible meaning in that context. Please let me know if that makes sense or if I'm overthinking it.

Yes the mixxx prefix for the outgoing data makes sense. I think from this point of view the incoming messages should have also a mixxx prefix.
In this case a transmitting button and a receiving LED can be on the same address.

Sounds good. I will fix that.

iffyloop · 2019-05-22T05:00:40Z

@daschuer Requested changes have been made. Please let me know if anything else is necessary.

stjohn909 · 2019-12-16T04:58:41Z

This is exactly the feature I need for a current experiment. Will this make it into the release soon?

Be-ing · 2019-12-16T05:04:00Z

@stjohn909: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/OSC.20Support
No, sorry. But you're welcome to use this branch for your own use before then.

github-actions · 2020-10-19T00:22:29Z

This PR is marked as stale because it has been open 90 days with no activity.

jakobbraun and others added 12 commits November 2, 2016 00:53

added basic osc support

ca40aec

Conflicts: build/depends.py src/mixer/playerinfo.cpp src/mixxx.cpp src/mixxx.h src/preferences/dialog/dlgpreferences.cpp

use connectValueChanged

179910a

added documentation

b6adaf2

added icon for osc-client

c91cf7b

added link to wiki for osc-client

b1a4f9b

removed osc client doc as it is in the wiki now: http://www.mixxx.org…

0a2a5d7

…/wiki/doku.php/osc-client

updated icon

ac46133

osc client: added speed option

1b74417

Merge remote-tracking branch 'upstream/master' into osc_support

898ba76

Merge branch 'osc_support' of https://github.com/jakobbraun/mixxx int…

c2bf921

…o osc_support

Added liblo-dev to the ubuntu ci

acc684c

Add OSC server support

70de71f

iffyloop mentioned this pull request Apr 8, 2019

[WIP] OSC support via liblo #2064

Closed

daschuer requested changes Apr 8, 2019

View reviewed changes

Fix requested changes

6b7e02f

Corrected implementation according to @daschuer's comments: #2078 (review)

daschuer reviewed May 5, 2019

View reviewed changes

daschuer requested changes May 5, 2019

View reviewed changes

iffyloop added 2 commits May 21, 2019 23:32

Fix coding style, add incoming /mixxx prefix

583eae2

Apply git-clang-format changes

c50cc9d

foltik added a commit to foltik/mixxx that referenced this pull request Jun 5, 2020

Merge PR mixxxdj#2078

e1a4a4f

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 19, 2020

Be-ing changed the base branch from master to main October 23, 2020 23:27

ronso0 marked this pull request as draft February 14, 2021 21:55

mixxxbot mentioned this pull request Aug 23, 2022

UDP message to the localhost port, for additional mixxx PLUGINS #9664

Open

iffyloop closed this by deleting the head repository Sep 12, 2022

fwcd mentioned this pull request Nov 6, 2023

(WIP) Axka changes #12250

Draft

		@@ -561,6 +563,8 @@ void MixxxMainWindow::initialize(QApplication* pApp, const CmdlineArgs& args) {

		m_pOscClientManager = new OscClientManager(pConfig, m_pEngine);

		m_pOscServer = new OscServer(pConfig);

OSC Server/Client Integration #2078

OSC Server/Client Integration #2078

Conversation

iffyloop commented Apr 8, 2019

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyloop commented Apr 8, 2019 via email

uklotzde commented Apr 8, 2019

iffyloop commented Apr 8, 2019 via email

daschuer commented Apr 8, 2019

iffyloop commented Apr 9, 2019

iffyloop commented May 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyloop commented May 22, 2019

stjohn909 commented Dec 16, 2019

Be-ing commented Dec 16, 2019

github-actions bot commented Oct 19, 2020