From bfd6bdf392a0fffac8a2f7a0d9634e9d6ec14600 Mon Sep 17 00:00:00 2001 From: John Anderson Date: Sun, 11 Mar 2007 15:05:15 +0000 Subject: [PATCH] to fix the abort on shutdown bug, make sure SurfacePort destructor doesn't kill the mutex until readers & writers have finished. git-svn-id: svn://localhost/ardour2/trunk@1572 d708f5d6-7413-0410-9779-e7cbd77b26cf --- libs/surfaces/mackie/TODO | 7 ++-- .../mackie/mackie_control_protocol.cc | 17 ++++----- .../mackie/mackie_control_protocol_poll.cc | 8 ++--- libs/surfaces/mackie/mackie_port.cc | 35 +++++++++++-------- libs/surfaces/mackie/surface_port.cc | 26 +++++++++++++- libs/surfaces/mackie/surface_port.h | 4 ++- 6 files changed, 61 insertions(+), 36 deletions(-) diff --git a/libs/surfaces/mackie/TODO b/libs/surfaces/mackie/TODO index 92593e3345..af4bfeadef 100644 --- a/libs/surfaces/mackie/TODO +++ b/libs/surfaces/mackie/TODO @@ -2,21 +2,18 @@ where ENSURE_CORRECT_THREAD is a macro that is modelled on ENSURE_GUI_THREAD if the handler is not called in the "correct thread", it will use a pseudo-RT-safe-enough technique to get the correct thread to recall "handler" later on, and return. -* On shutdown, sometimes: GThread-ERROR **: file gthread-posix.c: line 160 (): error 'Device or resource busy' during 'pthread_mutex_destroy ((pthread_mutex_t *) mutex)' -aborting... -Aborted - * jog with transport rolling doesn't work properly. My use of ScrollTimeline also doesn't work. +* make sure rew button can go past the previous if pressed twice, relatively quickly. * finish button mapping. * concurrency for bank switching? And make sure "old" events aren't sent to "new" faders * TODOs in code * removal of a route results in a strip that isn't dead, but doesn't have any effect on the session * use i18n. see string_compose -* remove couts * docs in manual, including button assignment diagram Later ----- +* remove commented couts * Queueing of writes? * Generic surface code to common location * bulk remote id changes cause too many surface updates diff --git a/libs/surfaces/mackie/mackie_control_protocol.cc b/libs/surfaces/mackie/mackie_control_protocol.cc index 4ff69d1acc..7824f1cb81 100644 --- a/libs/surfaces/mackie/mackie_control_protocol.cc +++ b/libs/surfaces/mackie/mackie_control_protocol.cc @@ -98,14 +98,14 @@ MackieControlProtocol::MackieControlProtocol (Session& session) , pfd( 0 ) , nfds( 0 ) { - cout << "MackieControlProtocol::MackieControlProtocol" << endl; + //cout << "MackieControlProtocol::MackieControlProtocol" << endl; // will start reading from ports, as soon as there are some pthread_create_and_store (X_("mackie monitor"), &thread, 0, _monitor_work, this); } MackieControlProtocol::~MackieControlProtocol() { - cout << "~MackieControlProtocol::MackieControlProtocol" << endl; + //cout << "~MackieControlProtocol::MackieControlProtocol" << endl; try { close(); @@ -118,7 +118,7 @@ MackieControlProtocol::~MackieControlProtocol() { cout << "~MackieControlProtocol caught unknown" << endl; } - cout << "finished ~MackieControlProtocol::MackieControlProtocol" << endl; + //cout << "finished ~MackieControlProtocol::MackieControlProtocol" << endl; } Mackie::Surface & MackieControlProtocol::surface() @@ -364,7 +364,6 @@ int MackieControlProtocol::set_active (bool yn) if ( yn ) { // TODO what happens if this fails half way? - cout << "set_active true" << endl; // create MackiePorts { @@ -389,7 +388,7 @@ int MackieControlProtocol::set_active (bool yn) } // wait until all ports are active - // TODO a more sophisticate approach would + // TODO a more sophisticated approach would // allow things to start up with only an MCU, even if // extenders were specified but not responding. for( MackiePorts::iterator it = _ports.begin(); it != _ports.end(); ++it ) @@ -407,19 +406,17 @@ int MackieControlProtocol::set_active (bool yn) // send current control positions to surface // must come after _active = true otherwise it won't run - cout << "update_surface in set_active" << endl; update_surface(); } else { - cout << "set_active false" << endl; close(); _active = false; } } catch( exception & e ) { - cout << "set_active to false because " << e.what() << endl; + cout << "set_active to false because exception caught: " << e.what() << endl; _active = false; throw; } @@ -729,7 +726,7 @@ void* MackieControlProtocol::_monitor_work (void* arg) XMLNode & MackieControlProtocol::get_state() { - cout << "MackieControlProtocol::get_state" << endl; + //cout << "MackieControlProtocol::get_state" << endl; // add name of protocol XMLNode* node = new XMLNode( X_("Protocol") ); @@ -745,7 +742,7 @@ XMLNode & MackieControlProtocol::get_state() int MackieControlProtocol::set_state( const XMLNode & node ) { - cout << "MackieControlProtocol::set_state: active " << _active << endl; + //cout << "MackieControlProtocol::set_state: active " << _active << endl; int retval = 0; // fetch current bank diff --git a/libs/surfaces/mackie/mackie_control_protocol_poll.cc b/libs/surfaces/mackie/mackie_control_protocol_poll.cc index 1c5c67ba55..05681c0c25 100644 --- a/libs/surfaces/mackie/mackie_control_protocol_poll.cc +++ b/libs/surfaces/mackie/mackie_control_protocol_poll.cc @@ -88,7 +88,7 @@ void MackieControlProtocol::update_ports() for( MackiePorts::iterator it = _ports.begin(); it != _ports.end(); ++it ) { - cout << "adding port " << (*it)->port().name() << " to pollfd" << endl; + //cout << "adding port " << (*it)->port().name() << " to pollfd" << endl; pfd[nfds].fd = (*it)->port().selectable(); pfd[nfds].events = POLLIN|POLLHUP|POLLERR; ++nfds; @@ -127,7 +127,7 @@ bool MackieControlProtocol::poll_ports() if ( nfds < 1 ) { lock.release(); - cout << "poll_ports no ports" << endl; + //cout << "poll_ports no ports" << endl; usleep( no_ports_sleep * 1000 ); return false; } @@ -180,13 +180,13 @@ void MackieControlProtocol::handle_port_active( SurfacePort * port ) // TODO but this is also done in set_active, and // in fact update_surface won't execute unless // _active == true - cout << "update_surface in handle_port_active" << endl; + //cout << "update_surface in handle_port_active" << endl; update_surface(); } void MackieControlProtocol::handle_port_init( Mackie::SurfacePort * sport ) { - cout << "MackieControlProtocol::handle_port_init" << endl; + //cout << "MackieControlProtocol::handle_port_init" << endl; _ports_changed = true; update_ports(); } diff --git a/libs/surfaces/mackie/mackie_port.cc b/libs/surfaces/mackie/mackie_port.cc index 23371c4d1f..9bcee638fb 100644 --- a/libs/surfaces/mackie/mackie_port.cc +++ b/libs/surfaces/mackie/mackie_port.cc @@ -49,12 +49,14 @@ MackiePort::MackiePort( MackieControlProtocol & mcp, MIDI::Port & port, int numb , _emulation( none ) , _initialising( true ) { - cout << "MackiePort::MackiePort" <sysex.connect( ( mem_fun (*this, &MackiePort::handle_midi_sysex) ) ); // make sure the device is connected @@ -90,11 +92,14 @@ void MackiePort::open() void MackiePort::close() { + //cout << "MackiePort::close" << endl; + // disconnect signals _any.disconnect(); _sysex.disconnect(); - // or emit a "closing" signal + // TODO emit a "closing" signal? + //cout << "MackiePort::close finished" << endl; } const MidiByteArray & MackiePort::sysex_hdr() const @@ -181,7 +186,7 @@ MidiByteArray calculate_challenge_response( MidiByteArray::iterator begin, MidiB MidiByteArray MackiePort::host_connection_query( MidiByteArray & bytes ) { // handle host connection query - cout << "host connection query: " << bytes << endl; + //cout << "host connection query: " << bytes << endl; if ( bytes.size() != 18 ) { @@ -202,7 +207,7 @@ MidiByteArray MackiePort::host_connection_query( MidiByteArray & bytes ) // not used right now MidiByteArray MackiePort::host_connection_confirmation( const MidiByteArray & bytes ) { - cout << "host_connection_confirmation: " << bytes << endl; + //cout << "host_connection_confirmation: " << bytes << endl; // decode host connection confirmation if ( bytes.size() != 14 ) @@ -219,10 +224,10 @@ MidiByteArray MackiePort::host_connection_confirmation( const MidiByteArray & by void MackiePort::probe_emulation( const MidiByteArray & bytes ) { - cout << "MackiePort::probe_emulation: " << bytes.size() << ", " << bytes << endl; + //cout << "MackiePort::probe_emulation: " << bytes.size() << ", " << bytes << endl; string version_string; for ( int i = 6; i < 11; ++i ) version_string.append( 1, (char)bytes[i] ); - cout << "version_string: " << version_string << endl; + //cout << "version_string: " << version_string << endl; // TODO investigate using serial number. Also, possibly size of bytes might // give an indication. Also, apparently MCU sends non-documented messages @@ -238,11 +243,11 @@ void MackiePort::probe_emulation( const MidiByteArray & bytes ) void MackiePort::init() { - cout << "MackiePort::init" << endl; + //cout << "MackiePort::init" << endl; init_mutex.lock(); _initialising = true; - cout << "MackiePort::lock acquired" << endl; + //cout << "MackiePort::lock acquired" << endl; // emit pre-init signal init_event(); @@ -258,7 +263,7 @@ void MackiePort::init() void MackiePort::finalise_init( bool yn ) { - cout << "MackiePort::finalise_init" << endl; + //cout << "MackiePort::finalise_init" << endl; bool emulation_ok = false; // probing doesn't work very well, so just use a config variable @@ -303,18 +308,18 @@ bool MackiePort::wait_for_init() Glib::Mutex::Lock lock( init_mutex ); while ( _initialising ) { - cout << "MackiePort::wait_for_active waiting" << endl; + //cout << "MackiePort::wait_for_active waiting" << endl; init_cond.wait( init_mutex ); - cout << "MackiePort::wait_for_active released" << endl; + //cout << "MackiePort::wait_for_active released" << endl; } - cout << "MackiePort::wait_for_active returning" << endl; + //cout << "MackiePort::wait_for_active returning" << endl; return SurfacePort::active(); } void MackiePort::handle_midi_sysex (MIDI::Parser & parser, MIDI::byte * raw_bytes, size_t count ) { MidiByteArray bytes( count, raw_bytes ); - cout << "handle_midi_sysex: " << bytes << endl; + //cout << "handle_midi_sysex: " << bytes << endl; switch( bytes[5] ) { case 0x01: @@ -389,6 +394,6 @@ void MackiePort::handle_midi_any (MIDI::Parser & parser, MIDI::byte * raw_bytes, } catch( MackieControlException & e ) { - cout << bytes << ' ' << e.what() << endl; + //cout << bytes << ' ' << e.what() << endl; } } diff --git a/libs/surfaces/mackie/surface_port.cc b/libs/surfaces/mackie/surface_port.cc index 8767e692b1..08ed1a75a7 100644 --- a/libs/surfaces/mackie/surface_port.cc +++ b/libs/surfaces/mackie/surface_port.cc @@ -39,20 +39,37 @@ SurfacePort::SurfacePort( MIDI::Port & port, int number ) { } +SurfacePort::~SurfacePort() +{ + //cout << "~SurfacePort::SurfacePort()" << endl; + // make sure another thread isn't reading or writing as we close the port + Glib::RecMutex::Lock lock( _rwlock ); + _active = false; + //cout << "~SurfacePort::SurfacePort() finished" << endl; +} + MidiByteArray SurfacePort::read() { const int max_buf_size = 512; MIDI::byte buf[max_buf_size]; MidiByteArray retval; + // check active. Mainly so that the destructor + // doesn't destroy the mutex while it's still locked + if ( !active() ) return retval; + // return nothing read if the lock isn't acquired Glib::RecMutex::Lock lock( _rwlock, Glib::TRY_LOCK ); + if ( !lock.locked() ) { - cout << "SurfacePort::read not locked" << endl; + //cout << "SurfacePort::read not locked" << endl; return retval; } + // check active again - destructor sequence + if ( !active() ) return retval; + // read port and copy to return value int nread = port().read( buf, sizeof (buf) ); @@ -84,7 +101,14 @@ void SurfacePort::write( const MidiByteArray & mba ) { //if ( mba[0] == 0xf0 ) cout << "SurfacePort::write: " << mba << endl; //cout << "SurfacePort::write: " << mba << endl; + + // check active before and after lock - to make sure + // that the destructor doesn't destroy the mutex while + // it's still in use + if ( !active() ) return; Glib::RecMutex::Lock lock( _rwlock ); + if ( !active() ) return; + int count = port().write( mba.bytes().get(), mba.size() ); if ( count != (int)mba.size() ) { diff --git a/libs/surfaces/mackie/surface_port.h b/libs/surfaces/mackie/surface_port.h index c060e52d93..87419f1bcd 100644 --- a/libs/surfaces/mackie/surface_port.h +++ b/libs/surfaces/mackie/surface_port.h @@ -38,10 +38,12 @@ class SurfacePort : public sigc::trackable { public: SurfacePort( MIDI::Port & port, int number ); - virtual ~SurfacePort() {} + virtual ~SurfacePort(); // when this is successful, active() should return true virtual void open() = 0; + + // subclasses should call this before doing their own close virtual void close() = 0; /// read bytes from the port. They'll either end up in the