Marlin: Bug: TMC2208 UART Communication uses wrong pins for SoftwareSerial

[melvinisken]

I'm currently trying to get my TMC2208 running completely with Marlin. Since I got no correct reply from the M122 command, I digged deeper into the internals and found some bugs I guess. I use four TMC2208 (X,Y,Z,E0) on an MKS Gen 1.4 board. As far as I know, this only affects reading, not writing.
Two main Bugs I am facing now:

1. Wrong Pins used. I could not receive the status from all drivers. I found that according to the description of the Arduino SoftwareSerial library, not all pins can be used as Rx (https://www.arduino.cc/en/Reference/SoftwareSerial):

Not all pins on the Mega and Mega 2560 support change interrupts, so only the following can be used for RX: 10, 11, 12, 13, 14, 15, 50, 51, 52, 53, A8 (62), A9 (63), A10 (64), A11 (65), A12 (66), A13 (67), A14 (68), A15 (69).

But currently also other pins are defined in pins_RAMPS.h. I tried to make it clearer by sketching the situation.

a) depicts the current configuration in pins_RAMPS.h (pin combinations vertically)
b) shows the pins that are capable of beeing Rx pins

I suggest to reorder the pins as shown in c) (pins combined horizontically, the X connector needs to be flipped in contrast to the other connectors)

This would lead to pin configuration in d)

1. Second bug:
   Again, according to SoftwareSerial documentation:

If using multiple software serial ports, only one can receive data at a time.

If I use the M122 command to get debug information, only information of the last driver is received. For example, if I have 4 TMCs connected (and the pin order changed like mentioned above) namely X,Y,Z,E0, only the status of E0 is reported, X,Y,Z report no results. If I remove the E0 from the configuration, Z shows results and X,Y not. And so on.
I have no solution so far, but I guess if might work with adding some wait time between status requests to each driver..

Here some sample output. The only change between both versions is to remove #define E0_IS_TMC2208. No wiring etc. is changed.

With E0_is_tmc:

X   Y   Z   E0
Enabled     false   false   false   true
Set current 800 800 800 800
RMS current 1436    1436    1436    795
MAX current 2025    2025    2025    1121
Run current 25/31   25/31   25/31   25/31
Hold current    12/31   12/31   12/31   12/31
CS actual       0/31    0/31    0/31    12/31
PWM scale   0   0   0   0
vsense      0=.325  0=.325  0=.325  1=.18
stealthChop false   false   false   true
msteps      256 256 256 16
tstep       0   0   0   1048575
pwm
threshold       0   0   0   0
[mm/s]      -   -   -   -
OT prewarn  false   false   false   false
OT prewarn has
been triggered  false   false   false   false
off time        0   0   0   5
blank time  16  16  16  24
hysterisis
-end        -3  -3  -3  2
-start      1   1   1   3
Stallguard thrs             
DRVSTATUS   X   Y   Z   E0
stst                    X
olb                 
ola                 
s2gb                    
s2ga                    
otpw                    
ot                  
157C                    
150C                    
143C                    
120C                    
s2vsa                   
s2vsb                   
Driver registers:   X = 0x00:00:00:00
    Y = 0x00:00:00:00
    Z = 0x00:00:00:00
    E0 = 0xC0:0C:00:00

Removing E0_is_tmc define and not touching any hardware:

X   Y   Z
Enabled     false   false   true
Set current 800 800 800
RMS current 1436    1436    795
MAX current 2025    2025    1121
Run current 25/31   25/31   25/31
Hold current    12/31   12/31   12/31
CS actual       0/31    0/31    12/31
PWM scale   0   0   0
vsense      0=.325  0=.325  1=.18
stealthChop false   false   true
msteps      256 256 16
tstep       0   0   1048575
pwm
threshold       0   0   0
[mm/s]      -   -   -
OT prewarn  false   false   false
OT prewarn has
been triggered  false   false   false
off time        0   0   5
blank time  16  16  24
hysterisis
-end        -3  -3  2
-start      1   1   3
Stallguard thrs         
DRVSTATUS   X   Y   Z
stst                X
olb             
ola             
s2gb                
s2ga                
otpw                
ot              
157C                
150C                
143C                
120C                
s2vsa               
s2vsb               
Driver registers:   X = 0x00:00:00:00
    Y = 0x00:00:00:00
    Z = 0xC0:0C:00:00

[brisma]

I can confirm to have exactly the same problem with my configuration (MKS Gen 1.4 + 5xTMC2208).

I didn't know about the SoftwareSerial was able to use only some pins for RX connection, I'll try to arrange the wires order to match the proposed schema.

In any case I think there are two problem:

• we need a little wait to receive the response from the driver before switching to the next one (maybe 100ms should be enought?)
• reading the SoftwareSerial documentation, only the last initialized serial is in listening mode. So, before sending the command to the driver, we need to invocate the .listen() method of the SoftwareSerial when changing from one driver to another.

[Grogyan]

Keep in mind that Pin 66 is already assigned for MAX31855 CS use #9406

[melvinisken]

I would like to help in correcting the code but at the moment I am a bit lost due to all these define macros that I can’t find the place where the actual read is done..

[brisma]

Any news about this issue?

[melvinisken]

Not from my side. Unfortunately I had no time to dig deeper into the code. It would be great if someone with knowledge about the TMC-serial-communication-code could have a look. It should not take much time to implement your proposed changes (hopefully).

[melvinisken]

Today I tried to have a closer look at the code and I think it's not that easy to implement the required changes (mentioned by @brisma ) unfortunately.
I wanted to integrate the listen() method to activate the respective pins before the read is done.

As far as I understood up to now, the SoftwareSerial.listen() method need to be called within the TMC2208-library, because during initialization (stepper_indirection.h) the SoftwareSerial object is created and handed over to the constructor of the TMC2208Stepper object. Within tmc_util.cpp where the debug-read is done, only the stepper objects are available (not the original SoftwareSerial objects). So there is no way to use the original SoftwareSerial object there. But, within the TMC2208Stepper object, the SoftwareSerial reference is passed as Stream type (I understood it's the parent class of Serial etc.) to the constructor which does not support the listen() method. So within the TMCStepper object, it's also not possible to use the listen() method.
I hope, you can follow at least a little :-)
There is a commented constructor within the library,
//TMC2208Stepper::TMC2208Stepper(HardwareSerial& SR) : TMC_SERIAL(SR) {}
which seems to go into the direction I'm thinking currently, but I failed to use this one or change it to something including SoftwareSerial. It could also be just some leftover of something that never worked..
Maybe @teemuatlut you can say something here, since I guess your knowledge in this case is far above mine :-)

[teemuatlut]

I may need to rethink how the library uses serial objects...

[brisma]

Mmmm, I don't think modify the library is the correct approach, the problem is how (and when) the serial communication is used in Marlin.
The core function of the library is only to communicate with the driver through a stream of byte, nothing else.

I think a better approach can be this:

• from stepper_indirection.h we can extern the serial communication of each driver (under "extern TMC2208Stepper stepperX;" for example)
• inside tmc_util.cpp, before the reading of the driver status, we can check if the communication is not ??_HARDWARE_SERIAL, just to be sure that is SoftwareSerial and, in this if, we can call the .listen method of the externed serial from stepper_indirection.h

What do you thinks about this approach?

[teemuatlut]

The core function of the library is only to communicate with the driver through a stream of byte, nothing else.

That's the core function of serial.transfer(). The purpose of the driver library is provide a hassle free way of configuring and communicating with that specific driver chip.
Once you start to go through all the places where Marlin communicates with the driver, you'll realize that adding checks and listen calls for each one is not a practical solution.

[teemuatlut]

I converted the library to a templated class (dev branch) and while I haven't tested it yet, it should allow me to use the software serial object directly, and thus the listen method.
The problem right now is that it only compiles for softwareserial...

[melvinisken]

It works! I had to change some code but now M122 provides data for all four TMCs:

Send: M122
Recv:       X   Y   Z   E0
Recv: Enabled       true    false   true    true
Recv: Set current   800 800 800 800
Recv: RMS current   795 795 795 795
Recv: MAX current   1121    1121    1121    1121
Recv: Run current   25/31   25/31   25/31   25/31
Recv: Hold current  12/31   12/31   12/31   12/31
Recv: CS actual     12/31   12/31   12/31   12/31
Recv: PWM scale 0   0   0   0
Recv: vsense        1=.18   1=.18   1=.18   1=.18
Recv: stealthChop   true    true    true    true
Recv: msteps        16  16  16  16
Recv: tstep     1048575 1048575 1048575 1048575
Recv: pwm
Recv: threshold     0   0   0   0
Recv: [mm/s]        -   -   -   -
Recv: OT prewarn    false   false   false   false
Recv: OT prewarn has
Recv: been triggered    false   false   false   false
Recv: off time      5   5   5   5
Recv: blank time    24  24  24  24
Recv: hysterisis
Recv: -end      2   2   2   2
Recv: -start        3   3   3   3
Recv: Stallguard thrs
Recv: DRVSTATUS X   Y   Z   E0
Recv: stst      X   X   X   X
Recv: olb
Recv: ola
Recv: s2gb
Recv: s2ga
Recv: otpw
Recv: ot
Recv: 157C
Recv: 150C
Recv: 143C
Recv: 120C
Recv: s2vsa
Recv: s2vsb
Recv: Driver registers: X = 0xC0:0C:00:00
Recv:   Y = 0xC0:0C:00:00
Recv:   Z = 0xC0:0C:00:00
Recv:   E0 = 0xC0:0C:00:00
Recv: 
Recv: 
Recv: ok

I will make a list of the changes I did and post them here (hopefully tonight already).

Edit: Sorry for the crippled output, it seems copying from OctoPrint isn't a good idea :-)

[teemuatlut]

Awesome! Too bad it doesn't compile with HW serial =)

[melvinisken]

Here are the files I needed to change. In stepper_indirection.h/.cpp and tmc_util.cpp:
search & replace "TMC2208Stepper" by "TMC2208Stepper\

Check out the TMC2208Stepper library dev branch and edit
TMC2208Stepper.cpp: add "tmc_serial->listen();" to sendDatagram-function (line 144)

template<class SERIAL_TYPE> bool TMC2208Stepper<SERIAL_TYPE>::sendDatagram(uint8_t addr, uint32_t *data, uint8_t len) {
    uint8_t datagram[] = {TMC2208_SYNC, TMC2208_SLAVE_ADDR, addr, 0x00};
    datagram[len] = calcCRC(datagram, len);
    tmc_serial->listen();
    while (tmc_serial->available() > 0) tmc_serial->read(); // Flush

    for(int i=0; i<=len; i++) tmc_serial->write(datagram[i]);

    tmc_serial->flush(); // Wait for TX to finish
    for(int byte=0; byte<4; byte++) tmc_serial->read(); // Flush bytes written
    delay(replyDelay);

    uint64_t out = 0x00000000UL;
    while(tmc_serial->available() > 0) {
        uint8_t res = tmc_serial->read();
        out <<= 8;
        out |= res&0xFF;
    }

    uint8_t out_datagram[] = {(uint8_t)(out>>56), (uint8_t)(out>>48), (uint8_t)(out>>40), (uint8_t)(out>>32), (uint8_t)(out>>24), (uint8_t)(out>>16), (uint8_t)(out>>8), (uint8_t)(out>>0)};
    if (calcCRC(out_datagram, 7) == (uint8_t)(out&0xFF)) {
        *data = out>>8;
        return 0;
    } else {
        return 1;
    }
}

TMC2208Stepper.h: remove or comment last line; result:
//template class TMC2208Stepper<HardwareSerial>;

Think those were all the changes.
TMCSoftSerial.zip

[teemuatlut]

Which just about outlines my commit from yesterday: https://github.com/teemuatlut/Marlin/commit/28d3afbd752318833082b1ed1d6eeabb98be7178

The change to TMC2208Stepper.h can't be made because then Hardware serial doesn't compile.

[thinkyhead]

Any further action needed on this? If not, please close. Thanks!

[teemuatlut]

The SW/HW Serial compatibility hasn't yet made its way into upstream but will be fixed after v1.1.9.
The default pins have been fixed.

[melvinisken]

So shall I close now?

[teemuatlut]

Go ahead. A proper fix needs some work and will hopefully be ready for v2.0.0.

[thinkyhead]

will be fixed after v1.1.9

Read: with 2.0. But note, we will most likely do an "AVR-certified" release of 2.0 early, starting with release candidates.

[lienbacher]

What is the status with these changes? I have been struggling to get both drivers connect to uart with the master branch of the TMC2208 library, just realized the changes regarding were not in the code, so I changed the library to the dev branch, did the changes, but it does not compile with the Marlin Bugfix branch ... I am now stuck with the missing push() function in the dev branch of the library.

[teemuatlut]

As said above, there will be a fix after the release of 1.1.9 and I'll be targeting the 2.0.x branch.
This only affects SW read capabilities from the drivers.

Don't use the dev branch. It was just an idea for a solution that will not be going forward.

[lienbacher]

Gotcha, thanks! Just to make sure, is it correct this only affects reading the drivers and writing works?

[teemuatlut]

As far as I understand, that is the case yes.

[melvinisken]

I would agree, writing always worked for me.
@teemuatlut or @thinkyhead : In the meantime I started to play around with 2.0.x-bugfix and noticed that the default pins are not changed in pins_RAMPS.h (so this was only fixed for 1.1.8+). By that time I only used 1.1.x and did not create a pull request for 2.x. Just to let you know..

[thinkyhead]

the default pins are not changed in pins_RAMPS.h

Sorry, which pins? A compare of pins_RAMPS.h between bugfix-1.1.x and bugfix-2.0.x shows all pins as matching.

[melvinisken]

@thinkyhead Sorry, that was my fault. I don't know how, but I seem to have overwritten the new pins_RAMPS.h with an old version. I thought I had checked out the most recent version. I don't know how I did it. Sorry for wasting your time :-)

[swapsCAPS]

I am trying to set current using UART. Write only should suffice I guess.
But It does not appear to do anything, multimeter shows same values and it does not matter if I set it to 1mA, it still moves fine.
Setting Spreadcycle mode instead of Stealthchop also does nothing, as far as I can tell.
Running Marlin 1.1.8. On an MKS 1.4
I've soldered the jumper at the bottom of the 2208 and am using a Y cable with a 1K resistor to Tx.
Also using the recommended pin out in the OP.
It's a bit frustrating that I couldn't get it to work. Any pointers would be greatly appreciated : )

M122 ouput:

SENDING:M122
        X   Y   Z   E0
Enabled     false   false   false   false
Set current 200 800 800 800
RMS current 331 1436    1436    1436
MAX current 467 2025    2025    2025
Run current 5/31    25/31   25/31   25/31
Hold current    2/31    12/31   12/31   12/31
CS actual       0/31    0/31    0/31    0/31
PWM scale   0   0   0   0
vsense      0=.325  0=.325  0=.325  0=.325
stealthChop false   false   false   false
msteps      256 256 256 256
tstep       0   0   0   0
pwm
threshold       0   0   0   0
[mm/s]      -   -   -   -
OT prewarn  false   false   false   false
OT prewarn has
been triggered  false   false   false   false
off time        0   0   0   0
blank time  16  16  16  16
hysterisis
-end        -3  -3  -3  -3
-start      1   1   1   1
Stallguard thrs
DRVSTATUS   X   Y   Z   E0
stst
olb
ola
s2gb
s2ga
otpw
ot
157C
150C
143C
120C
s2vsa
s2vsb
Driver registers:
    X = 0x00:00:00:00
    Y = 0x00:00:00:00
    Z = 0x00:00:00:00
    E0 = 0x00:00:00:00

[teemuatlut]

We have a brief troubleshooting section for TMC drivers.
Start there and let us know if you still have a problem.

[swapsCAPS]

Sweet! It write works with the 1.1.x bugfix branch!
I had to disable sd card support though as it would not compile otherwise.

Compile error:
pasting "/* SPI Master In Slave Out pin*/" and "_DDR" does not give a valid preprocessing token

And I am having the same issue as the OP with only the last value being read out.
But write works at least, as I can adjust the current and cause motors to stall when too low.
Thanks for the link! Made my day : ) :tada: :+1:

[thinkyhead]

I had to disable sd card support though as it would not compile otherwise.

The compile error you see is due to a bug in GCC. Remove end-of-line comments from any pins that invoke this bug in GCC as a workaround, or try building with PlatformIO, which uses a newer version of GCC with fewer bugs.

[Nellor]

Maybe I missed something.... Im running the latest Marlin 2.x version with the latest 0.2.1 stepper library.
Am I right that the problem "read only status from last driver" is still existent in that combination?
I used the correct pins but still get only the status from the last driver. If I remove the last one I get the status from the next one and so on...
Unfortunately the mod from @melvinisken from 14 Feb. at the files is not straight forward anymore as they look somehow different now.

[thinkyhead]

@Nellor — Paste your output from M122 demonstrating what's wrong, plus what would be preferred. We'll check with @teemuatlut to see if it's a limitation of driver status, or whether this is something we can fix.

[Nellor]

@thinkyhead this is what I get:

If I remove Z from the config I get the status for Y and if I remove Y too I get the status for x.

[thinkyhead]

Which of those lines are correct, and which are incorrect?

[Nellor]

Z is correct - rest not. It's the same issue like first post.

Second bug:
Again, according to SoftwareSerial documentation:
If using multiple software serial ports, only one can receive data at a time.

Should it be already fixed in latest Marlin/TMC2208 lib or is it normal that I still get this result?

[teemuatlut]

It's a limitation of AVR SoftwareSerial library. This will be fixed when we move to using TMCStepper and update the constructors along with it. Essentially the SoftwareSerial instance gets moved inside the stepper instance.
If you absolutely want to fix this right now, here's what the new constructor call looks like:

- #define _TMC2208_DEFINE_SOFTWARE(ST) SoftwareSerial ST##_HARDWARE_SERIAL = SoftwareSerial(ST##_SERIAL_RX_PIN, ST##_SERIAL_TX_PIN); \
-                                      TMC2208Stepper stepper##ST(&ST##_HARDWARE_SERIAL, ST##_SERIAL_RX_PIN > -1)
+ #define _TMC2208_DEFINE_SOFTWARE(ST) TMC2208Stepper stepper##ST(ST##_SERIAL_RX_PIN, ST##_SERIAL_TX_PIN, R_SENSE, ST##_SERIAL_RX_PIN > -1)

The serial begin calls also need to move to use stepper#.beginSerial.

-      X_HARDWARE_SERIAL.begin(115200);
+      stepperX.beginSerial(115200);

https://github.com/teemuatlut/Marlin/commit/4cd88327753a1eba118b3d194ed2b49d02d7dfab#diff-0d12add47cba33ebf7c4dba77c7d401fL379

@thinkyhead Do we still need to have parity between 1.1.x and 2.0.x?

[thinkyhead]

Do we still need to have parity between 1.1.x and 2.0.x?

For the time being, if a bug is found and known to exist in both, then both should be patched. Mainly to get the TMC support more complete.

[Nellor]

Thank you @teemuatlut for that information - that's what I asked for.

[teemuatlut]

For the time being, if a bug is found and known to exist in both, then both should be patched. Mainly to get the TMC support more complete.

Would you then agree to merge in a simple fix for 1.1.x and a more substantial shift to a new library for 2.0.x?
This will break parity between the two but it should fix the issues. And possibly introduce new ones on 2.0.x side...

[thinkyhead]

Parity isn't important, so feel free to apply a band-aid to bugfix-1.1.x while we move 2.0.x into the true future.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.