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

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" <<endl;
+	//cout << "MackiePort::MackiePort" <<endl;
 }
 
 MackiePort::~MackiePort()
 {
+	//cout << "~MackiePort" << endl;
 	close();
+	//cout << "~MackiePort finished" << endl;
 }
 
 int MackiePort::strips() const
@@ -81,7 +83,7 @@ int MackiePort::strips() const
 // should really be in MackiePort
 void MackiePort::open()
 {
-	cout << "MackiePort::open " << *this << endl;
+	//cout << "MackiePort::open " << *this << endl;
 	_sysex = port().input()->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