Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.0.x] Add L6470 SPI daisy chain support #12895

Merged
merged 16 commits into from
Jan 24, 2019

Conversation

Bob-the-Kuhn
Copy link
Contributor

@Bob-the-Kuhn Bob-the-Kuhn commented Jan 13, 2019

This PR adds SPI daisy chain support for the L6470 stepper drivers. The following file contains an explanation of the hardware and software involved.
two_motors_synchronized.txt

Arduino-L6470 library revision 0.7.0 is required for this stepper driver. This library has not been released yet.

The L6470 drivers continue to operate in STEP_CLOCK mode. In this mode the direction and enable are done via SPI commands and the phase currents are changed in response to step pulses (generated in the usual way).

HARDWARE/SOFTWARE interaction
Powering up a stepper and setting the direction are done by the same command. Can't do one without the other.

All directions are set every time a new block is popped off the queue by the stepper ISR.

SPI transfers when setting the directions is minimized by using arrays and a SPI routine dedicated to this function. L6470 library calls are not used. For N L6470 drivers, this results in a N byte transfer. If library calls were used then N*N bytes would be sent.

Power up (reset) sequence:

  • Stepper objects are created before the setup() entry point is reached.
  • The L6470_chain array is populated during setup(). This array is used to tell where in the SPI stream the commands/data for an stepper is positioned.
  • The L6470 soft SPI pins are initialized.
  • The L6470 chips are initialized during setup(). They can be re-initialized using the L6470_init_to_defaults() function
  • The steppers are NOT powered up during this sequence.

Top level changes:

  • Change all occurrences of L6470 to ST_L6470 to resolve name conflict with the library.
  • Attempted to emulate some of the TMC functionality by adding a common code module (L6470_Marlin), a monitor function and custom gcodes.
  • The majority of the changes were in stepper_indirection and in stepper. These were to modify/update the setup of the stepper objects and support enabling/disabling the drivers. Code was added to minimize the time (number of SPI transmissions) when setting direction.
  • A fastio based soft SPI (HAL_spi_L6470.cpp) was added to support the SPI daisy chain transfers. Since it is fastio based it can be used by all HALs so it is in the HAL/shared folder.

The numbering of the gcodes will probably change. I didn’t find anything comparable (except for M906) so the assignment of numbers was arbitrary.

The example configurations have only had the L6470 to ST_L6470 name changes done. I’ll update them once this PR has had some feedback.


Marlin/Configuration_adv.h

  • Added daisy chain specific SPI pin assignments and chain positions
  • The 0.7.0 library supports SPI pins assignments for individual drivers. Pin assignments were added for these.
  • Added a “MAX_VOLTAGE” define for each driver. This is used to initialize a register called KVAL_HOLD. This register sets the max duty cycle for the stepper voltage PWMs which sets the power used by the stepper. This setting is critical in tuning the system to get maximum performance without over heating the drivers.

HAL_spi_L6470.cpp contains two SPIs and a pin init routine.

  • One routine inserts the data of interest in the correct position in the SPI transfer sequence and sends NOOPs to all the other drivers. This one is used to transfer data to/from an individual driver such as during setup. In a 6 driver system each transfer consists of one byte for each driver. It can take quite some time to change settings using this routine.
  • The other routine is meant to minimize the time needed to set the direction and enable the drivers when a new planner block is popped off the queue. It just transmits a buffer . The buffer contents are filled by Marlin (the library is not involved).

Marlin.cpp

  • Added running, if enabled, a L6470 monitor to the manage_inactivity() routine.
  • Modified manage_inactivity() routine to only run the stepper shutdown routine once when the shutdown timer expires. Previously it ran the shutdown routine every time it ran. When a L6470 driver was present it looked like 99% of the free time was spend doing SPI transfers with the old method.
  • Moved the stepper driver chip initialization/setup into the SETUP portion. In the old location the SPI transfers were being attempted before the peripherals were initialized so nothing happened.

drivers.h
Added the macro AXIS_IS_ST_L6470(A)

enum.h
Added L6470_driver_enum to provide unique names/indices for each of the 13 possible drivers.

macros.h
Defined macro UNUSED(x) to eliminate a compiler warning.

M114
Displays the absolute positions registers of the L6470 drivers during a M114 D. They should be within 1 of the counts for each axis.

G28
Set the absolute positions registers of the L6470 drivers to the counts for each axis after homing completes.

Added gcodes

  • M122 - Added L6470 version of M122 to report status of each driver.
  • M906 – set/display KVAL_HOLD register. Allows user to do run time tuning.
  • M916: L6470 tuning: Increase drive level until get thermal warning
  • M917: L6470 tuning: Find minimum current thresholds
  • M918: L6470 tuning: Increase speed until max or error

Conditionals_post.h
Macro USE_EXECUTE_COMMANDS_IMMEDIATE modified to be true when there is a L6470 driver in the system. The M916-M918 routines use this feature to inject movement commands into the queue.

SanityCheck.h
[AXIS]_IS_L6470 is now check modified to use ST_L6470

L6470_Marlin.h
Contains all the non-library definitions needed to support the L6470 software.

L6470_Marlin.cpp
Contains the common code and data structures.

stepper.cpp

routine set_directions() modified to generate and xmit a buffer of L6470 commands to set each driver to the correct direction. This also enables the driver. In a N driver system this results in sending N bytes when setting/changing direction rather the N squared bytes.

stepper_indirection.cpp

  • Does a hardware reset of all drivers if the reset pin exists. This is done just once before writing to any of the registers in any of the chips.
  • Added chain setup, individual driver SPI pin setup, KVAL_HOLD setting and clears out the absolute position register when initializing individual drivers..
  • Added axis level disable routines.

stepper_indirection.h
UIpdated driver enable macros.

Misc changes
added date stamp to AutoBuild script

TO DO:
Add MIXING_EXTRUDER section for L6470 drivers to stepper_indirection.h DONE - created DIR_WRITE macros such that MIXING_EXTRUDER section did not have to be modified.
MIXING_EXTRUDER doesn't matter to a L6470 system because all L6470 drivers always have direction set and enabled simultaneously WRONG - In some extruder systems the directions are not all the same for every extruder.

Update example configurations DONE
Update pinsDebug_list.h DONE
Test in mixed driver system

@Bob-the-Kuhn Bob-the-Kuhn added Needs: More Data We need more data in order to proceed PR: New Feature T: Development Makefiles, PlatformIO, Python scripts, etc. T: Design Concept Technical ideas about ways and methods. S: Don't Merge Work in progress or under discussion. Needs: Discussion Discussion is needed T: HAL & APIs Topic related to the HAL and internal APIs. labels Jan 13, 2019
@Bob-the-Kuhn
Copy link
Contributor Author

Bob-the-Kuhn commented Jan 13, 2019

Some big assumptions:

  • All X drivers are the same (either all L6470 or all something else)
  • All Y drivers are the same (either all L6470 or all something else)
  • All Z drivers are the same (either all L6470 or all something else)
  • All extruder drivers are the same (either all L6470 or all something else)!

@thinkyhead
Copy link
Member

I'm a maintainer of the Arduino-L6470 library. Should I do a formal release of 0.7.0?

@thinkyhead
Copy link
Member

Change all occurrences of L6470 to ST_L6470 to resolve name conflict with the library.

I modified the macros in driver.h so that the symbol L6470 is no longer declared, preventing the naming conflict with the L6470 class.

@thinkyhead
Copy link
Member

Note that this PR breaks mixing, dual-x duplication mode, and other special extrusion modes for the L6470. To fix this the L6470 macros to enable/disable stepper drivers should be extended for all the extrusion modes.

@Bob-the-Kuhn
Copy link
Contributor Author

I think I've gotten the L6470 extruder macros setup so that the standard mixing extruder logic works correctly in a L6470 system.

Bench testing shows that the new defines are working. Lots more testing to see if that's true in all cases.

I'm going to try to squash the commits.

@Bob-the-Kuhn
Copy link
Contributor Author

I'm a maintainer of the Arduino-L6470 library. Should I do a formal release of 0.7.0?

Not yet.

I need to do some more bench testing and then actually try a print before stating that it's ready for public consumption.

@Bob-the-Kuhn
Copy link
Contributor Author

I can't reproduce the Travis errors. Is Travis having problems?

@thinkyhead
Copy link
Member

thinkyhead commented Jan 18, 2019

I've squashed and pushed the commits, so you can just do this:

git fetch origin
git checkout L6470-PR
git reset --hard origin/L6470-PR

@thinkyhead
Copy link
Member

thinkyhead commented Jan 18, 2019

Is Travis having problems?

The issue is weird. I'm trying a small change to see if it finally compiles. There was something else blocking it before this most recent "TMCTMC2130Stepper WTF" compile error.

@Bob-the-Kuhn
Copy link
Contributor Author

Thanks for all your work.

To get it to compile for a L6470 driver I needed to fix a few syntax errors and had to revert the argument definitions in L6470_get_user_input.

I'm trying to keep the steppers powered down after a reset. Currently configuration_store.cpp calls reset_stepper_drivers() which calls set_directions() which powers up the L6470s . About 1/2 second later they get disabled.

To avoid that 1/2 second power up I've modified reset_stepper_drivers() to call set_directions() only if a TMC driver is in the system.

Why does configuration_store.cpp call reset_stepper_drivers() ? Maybe there's a better way to accomplish the goal.

@thinkyhead thinkyhead force-pushed the L6470-PR branch 2 times, most recently from 37c92df to 924a66c Compare January 23, 2019 21:14
@thinkyhead
Copy link
Member

thinkyhead commented Jan 23, 2019

Ok, so this is almost ready, but before it's merged the Arduino-L6470 0.7.0 library will need to be published. That library update might still need a little work, but we can deal with any L6470 issues over on that repo.

@Bob-the-Kuhn
Copy link
Contributor Author

I'll need to pull this down, do a git rebase as usual, then do a git diff and try to cherry-pick the changes

What??!!! How could you possibly think my amateur coding could be improved upon? 😉

@thinkyhead
Copy link
Member

Not much to improve, honestly! This is all pretty solid. The chain method is bound to be slow, so there's not much we can do about that.

The L6470 library has some interesting qualities, like requiring the client app (Marlin) to define L6470_SPI_init and L6470_Transfer on its behalf, otherwise weak-linking them as do-nothing functions. It probably does end up being smaller code than if (instead) we had to provide callback function pointers. But it's certainly an uncommon way to construct a library.

The way we would usually do it is to define the methods as pure-virtual, then require the client to make a sub-class L6470 that defines these methods. Unfortunately that causes the code size to increase and slow down, since a virtual table needs to be added.

Weird as it is, I guess it'll be ok for our purposes.

@thinkyhead thinkyhead merged commit 2f35747 into MarlinFirmware:bugfix-2.0.x Jan 24, 2019
@Bob-the-Kuhn
Copy link
Contributor Author

Thanks for all your work. Much appreciated.

As to the "weak link" method - I was expecting someone to come back and point me to a better method. Should I pursue making then a part of a class?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Discussion Discussion is needed Needs: More Data We need more data in order to proceed PR: New Feature T: Design Concept Technical ideas about ways and methods. T: Development Makefiles, PlatformIO, Python scripts, etc. T: HAL & APIs Topic related to the HAL and internal APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants