TMC2130 DIAG tweaks v2 (#301)

* Revert "Reset SG when writing to GCONF or COOLCONF (#300)"

This reverts commit bc56fba.

* Test for "FINISHED" in TMC2130

This ensures no extra output goes untested

* TMC2130: handle DIAG correctly when entering STST

TMC2130 clears DIAG when the overload condition is resolved, not when
moving off an endstop. This means that when the motor is not moving DIAG
should be cleared.

Transition to standstill implies this, so handle this situation by
adding a couple of helper functions to set/clear DIAG consistently
and use them when StandStill is detected.

Add tests to check this condition.

Also cancel the StandStill timer when the object is destroyed to avoid
callbacks on uninitialized instances, causing crashes during quit.

* TMC2130: add some extra commentary to the test sequence

* TMC2130: clear DIAG also when changing state

As a followup from 997ba3d, disabling
motors should have the same end result by clearing the DIAG flag.

We do this by triggering standstill internally, which is not 100%
accurate if we take into account freewheeling and/or passive braking,
but good enough to allow full debugging of PowerPanic on the Prusa FW.

parts/components/TMC2130.cpp

@@ -154,10 +154,7 @@ void TMC2130::ProcessCommand()
 		switch (m_cmdProc.bitsIn.address)
 		{
 			case 0x00: // GCONF
-			case 0x6D: // COOLCONF
-				// Reset and adjust DIAG out
-				m_regs.defs.DRV_STATUS.stallGuard = false;
-				RaiseDiag(m_regs.defs.DRV_STATUS.stallGuard);
+				RaiseDiag(m_regs.defs.DRV_STATUS.stallGuard); // Adjust DIAG out, it mayhave  been reconfigured.
 				break;
 			case 0x6C: // Chopconf
 				m_uiStepIncrement = std::pow(2,m_regs.defs.CHOPCONF.mres);
@@ -203,6 +200,26 @@ void TMC2130::RaiseDiag(uint8_t value)
 	}
 }
 
+void TMC2130::ClearDiag()
+{
+	if (m_regs.defs.DRV_STATUS.stallGuard)
+	{
+		m_regs.defs.DRV_STATUS.SG_RESULT = 250;
+		m_regs.defs.DRV_STATUS.stallGuard = false;
+		RaiseDiag(0);
+	}
+}
+
+void TMC2130::SetDiag()
+{
+	if (!m_regs.defs.DRV_STATUS.stallGuard)
+	{
+		m_regs.defs.DRV_STATUS.SG_RESULT = 0;
+		m_regs.defs.DRV_STATUS.stallGuard = true;
+		RaiseDiag(1);
+	}
+}
+
 // Called when CSEL changes.
 void TMC2130::OnCSELIn(struct avr_irq_t *, uint32_t value)
 {
@@ -224,6 +241,7 @@ void TMC2130::OnDirIn(struct avr_irq_t * , uint32_t value)
 avr_cycle_count_t TMC2130::OnStandStillTimeout(avr_t *, avr_cycle_count_t)
 {
     m_regs.defs.DRV_STATUS.stst = true;
+	ClearDiag();
     return 0;
 }
 
@@ -274,16 +292,9 @@ void TMC2130::OnStepIn(struct avr_irq_t * irq, uint32_t value)
     TRACE(printf("cur pos: %f (%u)\n",m_fCurPos,m_iCurStep));
 	bStall |= m_bStall;
     if (bStall)
-    {
-        RaiseDiag(1);
-        m_regs.defs.DRV_STATUS.SG_RESULT = 0;
-    }
-    else if (!bStall && m_regs.defs.DRV_STATUS.stallGuard)
-    {
-          RaiseDiag(0);
-          m_regs.defs.DRV_STATUS.SG_RESULT = 250;
-    }
-    m_regs.defs.DRV_STATUS.stallGuard = bStall;
+		SetDiag();
+    else
+		ClearDiag();
     m_regs.defs.DRV_STATUS.stst = false;
     // 2^20 comes from the datasheet.
     RegisterTimer(m_fcnStandstill,1U<<20U,this);
@@ -294,6 +305,13 @@ void TMC2130::OnEnableIn(struct avr_irq_t *, uint32_t value)
 {
 	TRACE(printf("TMC2130 %c: EN changed to %02x\n",m_cAxis.load(),value));
     m_bEnable = value==0; // active low, i.e motors off when high.
+
+	if(!m_bEnable)
+	{
+		// transition immediately to standstill
+		CancelTimer(m_fcnStandstill,this);
+		OnStandStillTimeout(m_pAVR, 0);
+	}
 }
 
 // needed because cppcheck doesn't seem to do bitfield unions correctly.
@@ -316,6 +334,11 @@ TMC2130::TMC2130(char cAxis):Scriptable(std::string("") + cAxis),m_cAxis(cAxis)
 	RegisterActionAndMenu("Reset","Clears the diag flag immediately",ActResetDiag);
 }
 
+TMC2130::~TMC2130()
+{
+	CancelTimer(m_fcnStandstill,this);
+}
+
 Scriptable::LineStatus TMC2130::ProcessAction (unsigned int iAct, const std::vector<std::string> &)
 {
 	switch (iAct)

parts/components/TMC2130.h

@@ -63,6 +63,7 @@ class TMC2130: public SPIPeripheral, public Scriptable
 
         // Default constructor.
         explicit TMC2130(char cAxis = ' ');
+		~TMC2130();
 
         // Sets the configuration to the provided values. (inversion, positions, etc)
         void SetConfig(TMC2130_cfg_t cfg);
@@ -108,7 +109,10 @@ class TMC2130: public SPIPeripheral, public Scriptable
         void ProcessCommand();
         void CreateReply();
 
+		// Stallguard DIAG handling
         void RaiseDiag(uint8_t value);
+		void SetDiag();
+		void ClearDiag();
 
         bool m_bDir  = false;
         std::atomic_bool m_bEnable {true}, m_bConfigured {false};

scripts/tests/test_TMC2130.c

@@ -156,16 +156,16 @@ int main()
 	PORTA |= 1u<<2;
 	printf("DIAGDEDGE %02x\n",PINA&0x02);
 
+	// ensure DIAG is cleared by moving off the endstop
 	PORTA|=(1U<<3); // rev dir
 	PORTA&=~(1U<<2); // step.
+	PORTA&=~(1U<<3); // fwd again
+	if (PINA&0x2)
+			printf("DIAG NOT CLEARED\n");
 
-	PORTA&=~(1U<<3);
-
-	// DISABLE
+	// check that DIAG is not raised when disabled
 	PORTA|=(1U<<4);
 	printf("DISABLED\n");
-	if (PINA&0x2)
-			printf("DIAG NOT CLEARED\n");
 
 	for(int i=0; i<10; i++)
 	{
@@ -177,18 +177,26 @@ int main()
 	step();
 	printf("DIAGDISABLE %02x\n",PINA&0x02);
 
-	// Keep existing configuration (DIAG0_stall && pushpull),
-	// but ensure DIAG is cleared when GCONF is written to
+	TMCTX(0x80,1U<<7U); // DIAG0_stall, Act low
+	printf("DIAG %02x\n",PINA&0x02);
+
+	// transition to standstill should clear DIAG
+	_delay_ms(70); // 2^20clk ~ 65ms
 	printf("DIAG %02x\n",PINA&0x02);
-	TMCTX(0x80,1U<<7U | 1U << 12);
+
+	// disabling should clear standstill as well
+	step(); // trigger DIAG again
+	if (PINA&0x2) // ensure it's active
+		printf("DIAG ERR\n");
+	PORTA|=(1U<<4);
+	PORTA&=~(1U<<4);
 	printf("DIAG %02x\n",PINA&0x02);
 
+	printf("FINISHED\n");
 
 	cli();
 
 
-	printf("FINISHED\n");
-
 	// this quits the simulator, since interupts are off
 	// this is a "feature" that allows running tests cases and exit
 	sleep_cpu();

scripts/tests/test_TMC2130.txt

@@ -22,6 +22,8 @@ Board::Pause()
 GLHelper::SnapRect(tests/snaps/TMC04,0,164,500,80)
 Board::Resume()
 Serial0::NextLineMustBe(DIAGDISABLE 02)
-Serial0::NextLineMustBe(DIAG 02)
 Serial0::NextLineMustBe(DIAG 00)
+Serial0::NextLineMustBe(DIAG 02)
+Serial0::NextLineMustBe(DIAG 02)
+Serial0::NextLineMustBe(FINISHED)
 Board::Quit()