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 a control surface MCU basic interface #7649

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

altrouge
Copy link

Adds compatibility with:

  • Play button
  • Stop button
  • Loop button
  • Record button (if piano roll opened)

@altrouge
Copy link
Author

altrouge commented Jan 15, 2025

It adds the option in the settings/save in the config manager

mcu_control_surface

Partial implementation of #6129

I am not quite familiar with the code, so I may have done wrong/strange things, hopefully not too much !

Adds compatibility with:
- Play button
- Stop button
- Loop button
- Record button
- Jog wheel to select instrument
@altrouge altrouge force-pushed the add_surface_control branch from 835b6d7 to 3ace4be Compare January 23, 2025 16:34
};
} // namespace lmms

#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif // LMMS_CONTROL_SURFACE_H

};
} // namespace lmms

#endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#endif
#endif // LMMS_CONTROL_SURFACE_MCU_H

@@ -512,6 +516,7 @@ private slots:
TimePos m_exportSongEnd;
TimePos m_exportEffectiveLength;

std::shared_ptr<ControlSurfaceMCU> m_mcu_controller = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::shared_ptr<ControlSurfaceMCU> m_mcu_controller = nullptr;
std::shared_ptr<ControlSurfaceMCU> m_mcuController;

Style, and removing = nullptr since the default constructor sets it to nullptr.


ControlSurfaceMCU::ControlSurfaceMCU(const QString& device)
: MidiEventProcessor()
, m_midiPort(QString::fromStdString("mcu_controller"), Engine::audioEngine()->midiClient(), this)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
, m_midiPort(QString::fromStdString("mcu_controller"), Engine::audioEngine()->midiClient(), this)
, m_midiPort("mcu_controller", Engine::audioEngine()->midiClient(), this)

Comment on lines +705 to +706
int current = m_assignableMidiDevices->findText(ConfigManager::inst()->value("midi", "midiautoassign"));
if (current >= 0) { m_assignableMidiDevices->setCurrentIndex(current); }
Copy link
Member

Choose a reason for hiding this comment

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

Keeping this in its own block when it is no longer part of the else branch makes it look a bit confusing.

If you want to restrict the scope of current, I would use C++17's if statement with an initializer:

if (int current = m_assignableMidiDevices->findText(ConfigManager::inst()->value("midi", "midiautoassign")); current >= 0)
{
	m_assignableMidiDevices->setCurrentIndex(current);
}

Comment on lines +722 to +723
int current = m_MCUDawMidiDevices->findText(ConfigManager::inst()->value("midi", "midimcudaw"));
if (current >= 0) { m_MCUDawMidiDevices->setCurrentIndex(current); }
Copy link
Member

Choose a reason for hiding this comment

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

Same here


void ControlSurface::play()
{
Engine::getSong()->playSong();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Engine::getSong()->playSong();
if (auto song = Engine::getSong())
{
song->playSong();
}

I believe there are cases where song can be null.

And there are some other instances in this file where song is assumed to be non-null that should be corrected.


void ControlSurface::stop()
{
auto piano_roll = gui::getGUI()->pianoRoll();
Copy link
Member

Choose a reason for hiding this comment

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

gui::getGUI() returns nullptr when in headless mode

void Song::setControlSurfaceMCU()
{
m_mcu_controller.reset();
const QString& device = ConfigManager::inst()->value("midi", "midimcudaw");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const QString& device = ConfigManager::inst()->value("midi", "midimcudaw");
const QString device = ConfigManager::inst()->value("midi", "midimcudaw");

Avoids reference to a temporary

@messmerd messmerd mentioned this pull request Mar 3, 2025
1 task
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.

2 participants