Verwendung von CCriticalSection

Hi Leute,

ich hätte da eine Frage bezüglich CriticalSetions.
Zum Ablauf:
ein Workerthread liest von der seriellen Schnittstelle ein Wort (2Bytes hintereinander) in ein inBuf ein.
Die Methode synchronize() stellt fest, ob gerade ein neuer Datenblock mit Header usw beginnt.
decodeInput holt schliesslich die Nutzdaten, die für den Hauptthread relevant sind.

Meine Frage lautet nun:

Wie synchronisiere ich den Zugriff auf die globale Variablenstruktur "dbgMonData"? Muß ich da im Hauptthread auch alle Stellen, wo ich auf dieselbe Variable zugreifen will, in ein CriticalSection verpacken?

hier der Codeabschnitt im Workerthread:

UINT ThreadProc (LPVOID pParam)
{
	CPlotterDlg* pDlg = (CPlotterDlg*) pParam;

	while(true)
	{
		if (pDlg->synchronize() == true)
		{
			pDlg->decodeInput();
		}
		// schlafen, damit der Thread nicht 100% Rechenleistung beansprucht
		Sleep(1);
	}

	return 0;
}

// liest 2 Bytes aus dem UART-Buffer und erstellt ein Wort daraus (16 Bit)
bool CPlotterDlg::readRS232Word(WORD *data)
{
	ReadFile(hComm, byteInBuf, 2, &dwBytesRead, NULL);
	*data = (((WORD)byteInBuf[0]) << 8) + (WORD)byteInBuf[1];
	return true;
}

bool CPlotterDlg::synchronize()
{
	m_CriticalSection.Lock();

	readRS232Word(&dbgMonData.syncWord);
	if (dbgMonData.syncWord == SYNC_WORD)
		return true;
	else
	{
		dbgMonData.syncWord = NULL;
		return false;
	}

	m_CriticalSection.Unlock();
}

void CPlotterDlg::decodeInput()
{
	m_CriticalSection.Lock();

	readRS232Word(&dbgMonData.header);
	readRS232Word(&dbgMonData.length);

	switch (dbgMonData.header)
	{
		case TEAK_TSPP_HDR:
			readRS232Word((WORD *)&dbgMonData.tsppData.DT);
		//	readRS232Word((WORD *)&dbgMonData.tsppData.DT_sum);
			break;
	}

	m_CriticalSection.Unlock();
}

Danke im voraus

condor

RHBaum

prinzipell:
Solltest du alles schuetzen, worauf du lesend und schreibend parallel drauf zugreifen koenntest .

bool CPlotterDlg::synchronize()
{
    m_CriticalSection.Lock();

    readRS232Word(&dbgMonData.syncWord);
    if (dbgMonData.syncWord == SYNC_WORD)
        return true;
    else
    {
        dbgMonData.syncWord = NULL;
        return false;
    }

    m_CriticalSection.Unlock();
}

Das wird dir das genick brechen !!!
den lock solltest schon aufheben, bovor die schleife verlaesst . in deinem Code bleibt das object bei austritt gelockt.

Ne elegante Moeglickeit sich diesen Aerger zu sparen ist nen fuer den Lock ne Klasse zu schreiben, die das Locken fuer dich komplett uebernimmt, im konstruktur und destruktor, so dass bei verlassen des Gueltigkeitsbreeiches automatisch entlockst .... Am besten als Tamplate !

template<class TCS>
class ObjectLock
{
public:
    ObjectLock(TCS & rxCS):
    m_CS(rxCS)
    {
        m_CS.Lock();
    };
    ~ObjectLock
    {
        m_CS.Unlock();
    };
private:
    TCS & m_CS;
}

Nun brauchst das Template nor noch typisieren, und dem deine CCriticalSection zu uebergeben, und schon entlockt sich die Methode selber ....

irgendwo den Typ definieren ...
typedef ObjectLock<CCriticalSection> ObjectLockT;

bool CPlotterDlg::synchronize()
{
    // m_CriticalSection.Lock(); 
    // nu das Object locken lassem 
    ObjectLockT myLock(m_CriticalSection);

    readRS232Word(&dbgMonData.syncWord);
    if (dbgMonData.syncWord == SYNC_WORD)
        // hier haette es geknallt, weil rausgehst ohne zu entlocken 
        return true;
    else
    {
        dbgMonData.syncWord = NULL;
        // hier haette es geknallt, weil rausgehst ohne zu entlocken 
        return false;
    }

    // m_CriticalSection.Unlock();
    // Braucht man nu ned mehr ... 
}

Dadurch dass dein Objectlock nen Template ist, kannst die sache auch objektorientiert besser designen, in dem deiner zu lockenden Klasse die CCriticalsection als member gibts, der Klasse ne eigene Lock und Unlock member funktion speniderst (klasse sollte eh immer die selbe Critical section verwenden) dann kannst die eigene Klasse als Template vorlage verwenden ... und den Lock mit this an die eigene klasse delegieren ...

// die h. Datei ... 
class CPlotterDlg // was hier sonst noch so kommt ... 
{
    // ... was sonst noch so brauchst .... 
public:
    void Lock(){mcs.Lock();};
    void Unlock(){mcs.Unock();};
private:
    ObjectLock<CPlotterDlg> ObjectLockT;
    CCriticalSection mcs;
};
// nu kannst in jeder Methode ziemlich einfach locken: 
bool CPlotterDlg::synchronize()
{
    ObjectLockT myLock(this);
    // und weiter im Text ... das teil entlockt sich automatisch dann ... 
}

Deadlocks sind nett zu debuggen .... glaub mir

Ciao ...

Danke ist eine gute Idee mit der eigenen Lock-Klasse, ich werde das mal durchsehen, habe nebenher noch ein paar andere Stellen endeckt, die mit geringfügiger "Optimierung" den Lock überflüssig machen.

nochmals thx

Oh man jetzt erst sehe ich was ich da für einen Mist bei synchronize() gebaut habe,
na ja ist das erste Mal, dass ich mit Synchronisierung arbeite. Danke @RHBaum für die hervorragende Klassenlösung! Jetzt weiß ich was ich zu tun habe.

Mfg

condor

RHBaum

Die Klassenloesung ist aber ned das nonplus ultra ....

Um so komplizierter dein Aufbau wird, um so weniger kannst sowas nutzen ...

Achten solltest du auch auf folgendes ....

rufe niemals Schnittstellenmothoden(Com+) im gelockten zustand auf.
aquivalent zu normalen C++: rufe niemals funktionen eines Objektes im gelockten zusatend auf, das nicht unter deiner 100%igen kontrolle ist. (Du musst sicher sein, dass dieses Object dich nicht auch wieder aufruft mittels eine Methode die dann Lockt )

Breche das Locking so weit wie meoglich runter. EIne Klasse die keine Members hat, braucht ned locken. eine Klasse die nur Threadsichere members hat, braucht auch ned locken ...
Geht nicht immer, aber wenn, dann schreib Objecte die Dir komplett die Daten halten, mach diese Threadsicher (lock unlock). Damit trennst du logic von den daten, und brauchst nicht bei dem Logic Handling locken ... erspart dir manchmal viel aerger.

Nutze zum Debuggen spezielle CCritical section objecte ... die Dir das Locken raustracen. Das hilft enorm um deadlocks aufzuspueren ! Templates helfen da ungemein ... durch die kannst du schnell die Critical Sections ersetzen ...

noch was zu nem anderem thema ...

// schlafen, damit der Thread nicht 100% Rechenleistung beansprucht
        Sleep(1);

Aehm schon der Kommentar deutet auf schlechtes Design hin
Bei Multithreading musst viel sorgsamer mit der rechenleistung umgehen ... und ja keine unnoetigen schleifen !!!
Nutze Events ! bei threads ist das das Mittel der Wahl !
Stichworte ! SetEvent ResetEvent WaitForSingleObject WaitForMultiObject .....

Ciao ...

Das mit dem Sleep(1) habe ich aus einem Buch, ist also nich auf meinem Mist gewachsen, obwohl ich natürlich einsehe, dass Events da besser sind...

mfg
condor