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.19k stars 19.22k forks source link

[BUG] Compilation broken since March 06 #17094

Closed Patag closed 4 years ago

Patag commented 4 years ago

@ellensp In file included from Marlin\src\HAL\HAL_LPC1768../../inc/MarlinConfig.h:38, from Marlin\src\HAL\HAL_LPC1768\HAL.cpp:25: Marlin\src\HAL\HAL_LPC1768../../inc/SanityCheck.h:2088:4: error: #error "An SPI driven TMC driver on Z2 requires Z2_CS_PIN."

error "An SPI driven TMC driver on Z2 requires Z2_CS_PIN."

Was compiling fine before, so no need of my config files

ellensp commented 4 years ago

no config files, cannot help you without them. We need the config files as who knows what weird option you may have enabled that lead to this. The above tells us you have some lpc1768 based controller, which one? there are currently 15 different motherboards options most with different pins.h files. Provide your config files.

tureka commented 4 years ago

I have the same problem. Stable branch is compiling without problems and bugfix shows this error. Freshly downloaded both and changed only settings for steppers and tmc software spi.

I compared config files from both branches to single sign. The only diffrence is: #define HAS_TRINAMIC_CONFIG in bugfix #define HAS_TRINAMIC in stable 2.0.4.4

This is shown only for Z2 axis with dual Z steppers option enabled. I tried to compile with TMC5160 and TMC2209.

oliver-eifler commented 4 years ago

Same error with BTT SKR Pro/tmc2130-spi/dual-Z and current bugfix-2.0.x ( https://github.com/MarlinFirmware/Marlin/tree/2a71e4f336e66d93983ca15714137085090a73b8) it compiles fine when redefining needed pins in configuration_adv.h

#define NUM_Z_STEPPER_DRIVERS 2   // (1-4) Z options change based on how many

#if NUM_Z_STEPPER_DRIVERS > 1
....
#endif
#undef E1_STEP_PIN        
#undef E1_DIR_PIN         
#undef E1_ENABLE_PIN      
#ifdef E1_CS_PIN
  #undef E1_CS_PIN        
#endif
#define Z2_STEP_PIN        PD15
#define Z2_DIR_PIN         PE7
#define Z2_ENABLE_PIN      PA3
#ifndef Z2_CS_PIN
  #define Z2_CS_PIN        PG15
#endif

current configuration.h/configuration_adv.h/platform.ini - not for a real printer yet ;) configs.zip

Patag commented 4 years ago

@ellensp your response is really boring... anyway conf.zip

GMagician commented 4 years ago

@patag I think you should have a little more respect of people who want to help you, considering that attaching configuration is required when posting an issue

Patag commented 4 years ago

@GMagician . You're right for sure. But please don't take me for a noob. By the way , did you test the last merge ?

oliver-eifler commented 4 years ago

defining E1_DRIVER_TYPE TMC2130 instead of Z2_DRIVER_TYPE TMC2130 compiles but its very unlogical as any other configuration is based on axis name and it has worked in the past same error/workaround for X_DUAL_STEPPER_DRIVERS, Y_DUAL_STEPPER_DRIVERS and NUM_Z_STEPPER_DRIVERS 2/3/4

oliver-eifler commented 4 years ago

I mean its unlogical that Z2_DRIVER_TYPE TMC2130 doesn't work but #if AXIS_IS_TMC(Z2) works and why #define Z2_DRIVER_TYPE is still an option in configuration.h that doesn't work without redefining pins neverthanless, from now on i'll redifine the required pins

tureka commented 4 years ago

Then I simply set #define Z2_DRIVER_TYPE TMC2130 in Configuration.h Then in Configuration_adv.h

#define NUM_Z_STEPPER_DRIVERS 2
.....
#if AXIS_IS_TMC(Z2)
    #define Z2_CURRENT      760
    #define Z2_CURRENT_HOME Z2_CURRENT
    #define Z2_MICROSTEPS    16
    #define Z2_RSENSE         0.11
    #define Z2_CHAIN_POS     -1
  #endif
.....
#define Z2_CS_PIN         P1_01 // assign pin to E1 CS pin on board.

Is it correct thinking???

Otherwise if #define E1_DRIVER_TYPE TMC2130 and all other settings for Z2 axis I am not able to set currents with M906 or M914 for StallGuard sensivity for this stepper.

ellensp commented 4 years ago

@tureka There does look to be an issue here... the gcodes expect Z2_DRIVER_TYPE while the config seems to expect E1_DRIVER_TYPE

thinkyhead commented 4 years ago

Originally, assigning driver types to axes was meant to apply based on the stepper driver positions on the board. So your E1 would be defined and you wouldn't need to define a Z2 unless the board actually had a space designated specifically for a Z2 plug. But some of the order of inclusion and dependency related most especially to Trinamic drivers kind of threw this off.

The automatic assignment of X2, Y2, Z2, etc. to the next unused En plug is in pins.h is only included after the two Configuration files. But we wanted to refer to these axes in the Configuration_adv.h file. So the compromise was made to assign driver types directly but leave the axis reassignment in place for pins.

So at least according to my understanding, Z2_DRIVER_TYPE should apply for the Z2 axis, and the E1_DRIVER_TYPE should only apply to a real E1 extruder.

thinkyhead commented 4 years ago

@tureka — You should be applying your settings on the "Z2" axis in in Configuration_adv.h because Marlin has no idea at that point that there is any axis reassignment in front of it, and the intermediate stuff that comes in-between Configuration.h and Configuration_adv.h — which includes all our Trinamic driver macros — also has no idea about our axis reassignment voodoo.

So, as described above, we have two paradigms. In the PINS paradigm you define based on the names silkscreened on the board. And in the DRIVERS paradigm we define based on the axis function within Marlin.

If you define your own Z2_CS_PIN then Marlin will respect it. Otherwise it will try to use the next open En_CS_PIN.

thinkyhead commented 4 years ago

I note that yes you must currently make sure your E1 driver type matches up with your Z2 otherwise Marlin nukes the E1_CS_PIN.

GMagician commented 4 years ago

You're right for sure. But please don't take me for a noob. By the way , did you test the last merge ?

@Patag not meant to take you as noob. I can't test this board 'cause I don't have it

thinkyhead commented 4 years ago

The patch I just merged will fix the general issue where the E1_CS_PIN was getting eaten.

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.