MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.3k stars 19.25k forks source link

Cannot compile TMC2208/SoftwareSerial #9221

Closed brisma closed 6 years ago

brisma commented 6 years ago

Hi all, after have enabled the support for UART of TMC2208 I cannot compile due to this error:

libraries/SoftwareSerial/SoftwareSerial.cpp.o (symbol from plugin): In function `SoftwareSerial::read()':
(.text+0x0): multiple definition of `__vector_9'
sketch/Marlin_main.cpp.o (symbol from plugin):(.text+0x0): first defined here
/Applications/Arduino.app/Contents/Java/hardware/tools/avr/bin/../lib/gcc/avr/4.9.2/../../../../avr/bin/ld: Disabling relaxation: it will not work with multiple definitions
libraries/SoftwareSerial/SoftwareSerial.cpp.o (symbol from plugin): In function `SoftwareSerial::read()':
(.text+0x0): multiple definition of `__vector_11'
sketch/Marlin_main.cpp.o (symbol from plugin):(.text+0x0): first defined here
libraries/SoftwareSerial/SoftwareSerial.cpp.o (symbol from plugin): In function `SoftwareSerial::read()':
(.text+0x0): multiple definition of `__vector_10'
sketch/Marlin_main.cpp.o (symbol from plugin):(.text+0x0): first defined here
collect2: error: ld returned 1 exit status
exit status 1

Another strange thing is that I only choose one TMC2208 stepper, but is mandatory to have TMC2130 library installed.

I tried on Win10 64bit and MacOS... same error :(

Bob-the-Kuhn commented 6 years ago

Please ZIP & attach the following files:

What flavor of Marlin are you using?

What are you using to compile it?

brisma commented 6 years ago

Hi Bob, I'm using the stock pins_MKS_13.h, I did only some changes on Configuration.h and Configuration_adv.h (the zip in attachment).

I'm compiling from last commit of bugfix-1.1.x branch (cbdbeb3e6) and I'm using Arduino 1.8.5 with TMC2130Stepper, TMC2208Stepper and u8glib libraries on MacOS (10.13.2), but with the same configuration on Windows 10 is the same.

I already tried to revert to the last stable release (1.1.8) but with no results, same error.

conf.zip

brisma commented 6 years ago

Uhmm I found something interesting, the problem is this define in the Configuration.h:

#define ENDSTOP_INTERRUPTS_FEATURE

If commented everything will compile correcty, but if defined will generate the error above...

Bob-the-Kuhn commented 6 years ago

If you hadn't included the #define ENDSTOP_INTERRUPTS_FEATURE I'd never have found the cause.

The ENDSTOP_INTERRUPTS_FEATURE and the TMC2208 both use pin change interrupts.

The TMC2208 uses them to implement a software UART.

Bob-the-Kuhn commented 6 years ago

Clarification - the TMC2208 uses the software UART if the pins_YOUR_BOARD.h file doesn't point it at a hardware UART.

Bob-the-Kuhn commented 6 years ago

PR #9237 adds a sanity check so the user knows what the problem is and how to fix it.

pmarkiewicz commented 5 years ago

I have to reopen topic. On branch bugfix-2.0.x if I enable both TMC2130 and ENDSTOP_INTERRUPTS_FEATURE I got same link error related to SoftwareSerial

MOTHERBOARD BOARD_RAMPS_14_SF MEGA 2560

thinkyhead commented 5 years ago

So… should it be this?

/**
 * TMC2208 software UART and ENDSTOP_INTERRUPTS both use pin change interrupts (PCI)
 */
#if (HAS_DRIVER(TMC2130) || HAS_DRIVER(TMC2208)) && ENABLED(ENDSTOP_INTERRUPTS_FEATURE) && !( \
       defined(X_HARDWARE_SERIAL ) \
    || defined(X2_HARDWARE_SERIAL) \
    || defined(Y_HARDWARE_SERIAL ) \
    || defined(Y2_HARDWARE_SERIAL) \
    || defined(Z_HARDWARE_SERIAL ) \
    || defined(Z2_HARDWARE_SERIAL) \
    || defined(Z3_HARDWARE_SERIAL) \
    || defined(E0_HARDWARE_SERIAL) \
    || defined(E1_HARDWARE_SERIAL) \
    || defined(E2_HARDWARE_SERIAL) \
    || defined(E3_HARDWARE_SERIAL) \
    || defined(E4_HARDWARE_SERIAL) )
  #error "Select hardware UART for TMC2130/2208 to use both TMC2130/2208 and ENDSTOP_INTERRUPTS_FEATURE."
#endif
pmarkiewicz commented 5 years ago

Well, I'm not sure as TMC2130 doesn't use UART, they use SPI.

thinkyhead commented 5 years ago

Good point!

So, if you quit the IDE and restart it, same error as in the original post?

ghost commented 5 years ago

So, if you quit the IDE and restart it, same error as in the original post?

@thinkyhead yup, it's not some wierd compiler and temp files issue, I have exactly the same compiler error (anet a8 default config / ramps board / tmc2130 in spi mode).
Disabling #define ENDSTOP_INTERRUPTS_FEATURE makes the build finish and upload, printer works.

thinkyhead commented 5 years ago

Curious. So perhaps the TMCStepper library is including SoftwareSerial.h for TMC2130 when it should only be including it for TMC2208…?

ghost commented 5 years ago

bf2-config.zip

I rebased my config to the latest bugfix-2.0.x branch, including the latest tmc commit just to make sure the error didn't go away, endstops interrupts is enabled, which consistently reproduces the error.
It's slightly different, as the multiple definitions are not in marlin_main.cpp but in endstops.cpp :

libraries\SoftwareSerial\SoftwareSerial.cpp.o (symbol from plugin): In function `SoftwareSerial::read()':

(.text+0x0): multiple definition of `__vector_10'

sketch\src\module\endstops.cpp.o (symbol from plugin):(.text+0x0): first defined here

libraries\SoftwareSerial\SoftwareSerial.cpp.o (symbol from plugin): In function `SoftwareSerial::read()':

(.text+0x0): multiple definition of `__vector_11'

sketch\src\module\endstops.cpp.o (symbol from plugin):(.text+0x0): first defined here

Not sure if that deserves a new issue, as it differs from OP.

ghost commented 5 years ago

The vector conflict seems to be related to hardware/arduino/avr/libraries/SoftwareSerial/src/SoftwareSerial.cpp :
line 227 and below > #if defined(PCINT0_vect)

It conflicts with : https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/HAL/HAL_AVR/endstop_interrupts.h#L87

Commenting the whole vectors definitions in endstop_interrupts.h gives a succesful build (but probably cribbled).
I didn't look at the commit introducing the change, but if it worked previously, doesn't that mean we could probably ifdef out the softwareserial stuff ?

EDIT: Found the culprit : https://github.com/teemuatlut/TMCStepper/commit/d9616323b09f196105c02c0ef8d2dbfcfb8a2f53

Latest bugfix-2.0.x builds if :
#define SW_CAPABLE_PLATFORM defined(__AVR__) || defined(TARGET_LPC1768) is commented, which probably mean ifdef'ing "not TMC2130 and ENDSTOP_INTERRUPTS_FEATURE" fixes the conflict in proper way.
I didn't upload the firmware tho, so who knows :>

kAdonis commented 5 years ago

there was a discussion before in #12078

ghost commented 5 years ago

Yes but truglodite couldn't build after commenting part of the SoftwareSerial code in the new TMCStepper lib.
If it worked before, maybe it can be fixed :)

thinkyhead commented 5 years ago

For now just disable ENDSTOP_INTERRUPTS_FEATURE.

@teemuatlut — Would it be possible to have a flag set ahead of including TMCStepper telling it not to use software serial, so when using ENDSTOP_INTERRUPTS_FEATURE users could force hardware serial to be used? Are there situations where software serial would definitely be required by TMCStepper?

Or, should we just add sanity checks to Marlin telling users to disable ENDSTOP_INTERRUPTS_FEATURE in all cases where the software serial library might be included by TMCStepper?

teemuatlut commented 5 years ago

TMCStepper will include SoftwareSerial.h on all platforms that have the library. Regardless of course of possible Marlin configuration options.

By my understanding, even if we set a preprocessor flag before including the TMCStepper.h header preventing SoftwareSerial.h header from getting included, it will still be compiled in when the compiler goes through the internal sourse files of TMCStepper. So when compiling TMCStepper.cpp, it of course has no knowledge of this conditional flag and will include SW Serial. And then we will again get the linker error. Before merging the TMC libraries this wasn't a problem as users with TMC2130 did not compile the TMC2208 source files that depend on this SW Serial library.

I haven't looked into it at all but I would think that there have been people in the past who have used the SoftwareSerial library and pin interrupts together.

For now until something better is found out, our solution can be to have a sanity check where if a TMC stepper is enabled on AVR and ENDSTOP_INTERRUPTS_FEATURE is also enabled, we give an error message that's more understandable than what the compiler/linker itself will give the user.

teemuatlut commented 5 years ago

Another option then is to revert back the creation of the Serial instance back to Marlin and pass the reference to the stepper object. Then Marlin again is in control of the communication object creation and can decide between the HW and SW classes and if either is even needed, like when only TMC2130 is in use. But that's also a regression as we will again have issues where those who wish to actually use SW Serial don't have a working serial receive (a limitation by the SW Serial library).

However, as that issue was solved by just having access to SoftwareSerial::listen(), which is not present in the parent class Stream, perhaps that particular method can be baked into TMC2208Stepper class itself.

Burárum.............

thinkyhead commented 5 years ago

a sanity check where if a TMC stepper is enabled on AVR and ENDSTOP_INTERRUPTS_FEATURE is also enabled

I will add that now, and we can keep it in place until we know how well the other ideas work out.

thinkyhead commented 5 years ago

Right now we have this…

#if HAS_DRIVER(TMC2208) && ENABLED(ENDSTOP_INTERRUPTS_FEATURE) && !( \
       defined(X_HARDWARE_SERIAL ) \
    || defined(X2_HARDWARE_SERIAL) \
    || defined(Y_HARDWARE_SERIAL ) \
    || defined(Y2_HARDWARE_SERIAL) \
    || defined(Z_HARDWARE_SERIAL ) \
    || defined(Z2_HARDWARE_SERIAL) \
    || defined(Z3_HARDWARE_SERIAL) \
    || defined(E0_HARDWARE_SERIAL) \
    || defined(E1_HARDWARE_SERIAL) \
    || defined(E2_HARDWARE_SERIAL) \
    || defined(E3_HARDWARE_SERIAL) \
    || defined(E4_HARDWARE_SERIAL) )
  #error "Select hardware UART for TMC2208 to use both TMC2208 and ENDSTOP_INTERRUPTS_FEATURE."
#endif

Does that still apply? It seems like this would be better:

/**
 * TMC2208 software UART and ENDSTOP_INTERRUPTS both use pin change interrupts (PCI)
 */
#define IS_SOFT_2208(A) (AXIS_DRIVER_TYPE_##A(TMC2208) && !defined(A##_HARDWARE_SERIAL))
#if HAS_DRIVER(TMC2208) && ENABLED(ENDSTOP_INTERRUPTS_FEATURE) && ( \
       IS_SOFT_2208(X)  || IS_SOFT_2208(X2) \
    || IS_SOFT_2208(Y)  || IS_SOFT_2208(Y2) \
    || IS_SOFT_2208(Z)  || IS_SOFT_2208(Z2) || IS_SOFT_2208(Z3) \
    || IS_SOFT_2208(E0) || IS_SOFT_2208(E1) \
    || IS_SOFT_2208(E2) || IS_SOFT_2208(E3) \
    || IS_SOFT_2208(E4) || IS_SOFT_2208(E5) )
  #error "Select hardware UART for TMC2208 to use both TMC2208 and ENDSTOP_INTERRUPTS_FEATURE."
#endif

For the 2130 and 2660 it sounds like we just need very simple tests…

/**
 * TMC drivers cause SoftwareSerial.h to be included, leading to a compile error.
 */
#if ENABLED(ENDSTOP_INTERRUPTS_FEATURE)
  #if HAS_DRIVER(TMC2130)
    #error "TMC2130 is currently incompatible with ENDSTOP_INTERRUPTS_FEATURE."
  #elif HAS_DRIVER(TMC2660)
    #error "TMC2660 is currently incompatible with ENDSTOP_INTERRUPTS_FEATURE."
  #endif
#endif
teemuatlut commented 5 years ago

Actually I think it's much simpler than that. If you have a TMC driver on AVR, don't use endstop interrupts.

#if defined(__AVR__) && HAS_TRINAMIC && ENABLED(ENDSTOP_INTERRUPTS_FEATURE)
  #error
#endif

No matter which (TMC) driver you have and no matter what the rest of your (AVR specific) configuration is, you can't use endstops interrupts due to the way the AVR SoftwareSerial is implemented.

Fourth time: AVR

thinkyhead commented 5 years ago

Looks easy enough. And AVR, naturally. The code above is from the AVR-only sanity-check file.

github-actions[bot] commented 4 years ago

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.