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

Add powerSTEP01/L6480 support #12

Merged
merged 16 commits into from
Feb 26, 2019
Merged

Add powerSTEP01/L6480 support #12

merged 16 commits into from
Feb 26, 2019

Conversation

Bob-the-Kuhn
Copy link

The chain array is not specific to a stepper so it shouldn't be inside the L6470 class.

The PR moves the chain array out of the class and calls it L6470_chain.

STRANGE - I can't pull the latest dev branch code into my local copy. This PR shows a lot of false changes because of this. Hopefully Scott can fix this.

Using different header files (and supporting CPP files) allows the main code base to include support for different chips without making changes to this library.
@Bob-the-Kuhn
Copy link
Author

Bob-the-Kuhn commented Jan 30, 2019

Just added code to support the powerSTEP01/L6480 family.

This may need to be split out into different libraries but … the files are almost identical which pushes toward one library.

There is one header file (and supporting CPP) for the L6470 family and another set for the powerSTEP01/L6480 family. The only difference between the two header files are a few defines. The only difference between the two CPP files is which header file is pulled in.


The changes that were made are:

  • Added additional definitions and #if statements to select the appropriate definitions.
  • Changed the Overcurrent Detection threshold and Stall threshold commands to allow for variable numbers of bits and variable "mA per count" constants.
  • Created a header/CPP file set for each family. The only differences between the two sets are several defines.

@Bob-the-Kuhn Bob-the-Kuhn changed the title move chain array out of the class and call it L6470_chain move chain array out of the class and call it L6470_chain, add powerSTEP01/L6480 support Jan 30, 2019
@thinkyhead
Copy link
Collaborator

I've rebased this onto the current dev code. You should grab that if you don't have it:

git fetch upstream
git checkout dev
git reset --hard upstream/dev
git push -f origin

And to get this:

git fetch origin
git checkout SPI-chain-changes
git reset --hard origin/SPI-chain-changes

@thinkyhead thinkyhead changed the title move chain array out of the class and call it L6470_chain, add powerSTEP01/L6480 support L6470::chain => L6470_chain, add powerSTEP01/L6480 support Feb 2, 2019
@thinkyhead
Copy link
Collaborator

thinkyhead commented Feb 2, 2019

  • I don't think this is the proper way to implement a library supporting multiple stepper types. Won't all .cpp files be compiled and produce naming conflicts?

  • It would be most friendly to allow L6470, L6480, and powerSTEP01 steppers to be in use at the same time, and as part of the interface design there ought to be a single parent class for sub-classes L6470, L6480, and powerSTEP01 that all share common code.

  • I'm not keen on using the L64XX macro throughout the L6470 library as a method to provide the class name. If the library is always going to export a single class name anyway then there should be no reason to use this macro.

Co-Authored-By: Bob Kuhn <bob-the-kuhn@users.noreply.github.com>
@thinkyhead thinkyhead changed the title L6470::chain => L6470_chain, add powerSTEP01/L6480 support Add powerSTEP01/L6480 support Feb 2, 2019
@Bob-the-Kuhn
Copy link
Author

Bob-the-Kuhn commented Feb 2, 2019

single parent class for sub-classes L6470, L6480, and powerSTEP01 that all share common code

I'll find out what that means and implement it.

L64XX_chain => L64XX::chain

Doesn't that mean that each stepper has its own unique version of chain? What the SPI transmit routine needs is a single array that contains all of the chain info for all of the steppers.


I'll also replace the weak function definitions per an earlier comment.

Are there other areas that need to be changed to provide a proper/standard method?

@thinkyhead
Copy link
Collaborator

thinkyhead commented Feb 4, 2019

Doesn't that mean that each stepper has its own unique version of chain?

When you declare a class member as static then there is only one shared instance, no matter how many instances of the object you create. See https://en.cppreference.com/w/cpp/language/static

@Bob-the-Kuhn
Copy link
Author

I've added subclasses for the L6470, L6480 and powerSTEP01.

The L6470 and powerSTEP01 are functional. I don't have a L6480 to test with.

I still need to test in a mixed system..


Scott - I can't find your message on how to setup the library to use external SPI transfer routines.

In this commit all I did was remove the weak section but … that means the user that wants to use the internal SPI routines will get a link error because the code assumes the external SPI routines will be defined.


I'll also need to re-visit the examples.

…ged names of L6470 slew rate definitions to match data sheet
@Bob-the-Kuhn
Copy link
Author

I've successfully tested this in a mixed system.

I've also changed the library version.

OPEN ITEMS:

  • How to setup the library to link with or without the presence of external SPI transfer routines.
  • Revisit the examples once the above has been finalized.

@thinkyhead
Copy link
Collaborator

thinkyhead commented Feb 26, 2019

The library has to be able to compile without any external references, unless those external references are also in libraries, in which case just add an include for the needed library header and state the dependency in the library manifest.

WEAK references should be avoided, unless there's no alternative.

@thinkyhead
Copy link
Collaborator

Instead of weak linking functions and requiring certain functions to be defined by clients of the library, it would be better to define typedefs for the needed function signatures and store pointers to those functions inside the class.

L6470.h:

typedef uint8_t (*spi_transferFunc_t)(const uint8_t data, const int16_t ss_pin);
typedef uint8_t (*spi_chainFunc_t)(const uint8_t data, const int16_t ss_pin, const uint8_t chain_index);
. . .
  static spi_transferFunc_t transferFunc;
  static spi_chainFunc_t chainFunc;

L6470.cpp:

uint8_t dummyTransferFunc(const uint8_t data, const int16_t ss_pin) { return 0; }
uint8_t dummyChainFunc(const uint8_t data, const int16_t ss_pin, const uint8_t chain_index) { return 0; }
spi_transferFunc_t L64XX::transferFunc = dummyTransferFunc;
spi_chainFunc_t L64XX::chainFunc = dummyChainFunc;

Calling these is the same as for regular functions / methods:

transferFunc(data, ss_pin);
chainFunc(data, ss_pin, index);

@thinkyhead thinkyhead merged commit c906cc6 into ameyer:dev Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants