Skip to content

[2.0.x] Migrate Marlin to a new TMC library#11943

Merged
thinkyhead merged 5 commits intoMarlinFirmware:bugfix-2.0.xfrom
teemuatlut:bf2_new_library
Oct 3, 2018
Merged

[2.0.x] Migrate Marlin to a new TMC library#11943
thinkyhead merged 5 commits intoMarlinFirmware:bugfix-2.0.xfrom
teemuatlut:bf2_new_library

Conversation

@teemuatlut
Copy link
Member

Replaces #11792 and 11919

I'll rebase and sort the added commits once they've been reviewed.


I'm starting the process to migrate to a new TMC library that aims to support more driver models.
It might be a bit early but I'd like to get a review going to make things go smoothly.

Here's a general plan for the rest of the year. Ticked boxes mark the changes introduced in this PR.

  • Move to new universal TMCStepper library
  • Implement inherited Marlin TMC classes
  • Support TMC2660
  • Support TMC5160
  • Add sanity checking for driver errors
  • Greatly reduce communication attempts for monitoring
  • Improve TMC error verbosity with better error messages
  • Add communication checking on boot
  • Add the ability to change chopper timings (spreadCycle)
  • Tweak default chopper settings for quieter spreadCycle operation
  • Support Trinamic coolStep technology
  • Improve M122 with fetching raw register data
  • Add TMC section to the LCD
  • Support TMC5130
  • Support TMC2224
  • Add SOFTWARE_ENABLE for all smart drivers (Request from Panucatt)

Regarding this PR.

A more universal library allows me to support more chips without creating a completely new library for each and every one of them. The maintenance would quickly become too great when the number of supported models is more than a couple.
The current aim is to support TMC2130, TMC2208, TMC2224, TMC5130, TMC5160 and TMC2660.
The library itself should be in pretty good condition but as always, it is mostly untested at this point.

Implementing a child class within Marlin allows us to attach certain functions to the stepper objects so they don't have to be passed around as parameters, or be tracked as indexes to an array. This PR primarily uses this for printing out the axis labels and otpw counter values.

Basically this PR forms the base for many of the new features, functions and fixes.
I ask that the PRs don't get squashed together as I've tried to organize the commits thematically and it'll make it easier to debug in case of any issues.

@thinkyhead
Copy link
Member

The Marlin AxisEnum doesn't contain items for X2, Z3, etc.

True. That enum was designed only for direct addressing of the directional axes — indexing into the current_position and destination arrays for G-code commands like G1. A new and separate enum should be used if you want to address individual stepper drivers.

@teemuatlut teemuatlut force-pushed the bf2_new_library branch 4 times, most recently from a44f6f7 to 374e12d Compare September 30, 2018 19:12
@thinkyhead
Copy link
Member

thinkyhead commented Sep 30, 2018

Did you see teemuatlut#25 ? It's just a merge of bugfix-2.0.x onto this PR. I submitted it because merging is always a bit challenging. It was in good shape yesterday, but now it's conflicting with the added commit. I could recreate/update that PR, or I can close it and leave it to you to rebase this PR.

I'm available to help if you want to get this rebased, squashed, and passing Travis CI quickly.

@teemuatlut
Copy link
Member Author

I can keep doing the rebases, but it also needed some additional changes than just a rebase. Right now there's also some build issue with LPC that I don't know how to explain as it builds just fine locally.
Maybe the LPC toolchain handles macro expansion a bit differently than the other platforms we're testing.

@thinkyhead
Copy link
Member

thinkyhead commented Oct 1, 2018

Enable TMC_USE_SW_SPI and try compiling for LPC176x to start debugging the error.

@teemuatlut
Copy link
Member Author

All good now. I'll clean this up today and go trough it once more.

@teemuatlut
Copy link
Member Author

@thinkyhead Let me know what you think about using structs in TMC EEPROM functionality. It makes things a bit cleaner and we can then remove the TMC_AxisEnum.
There are also some SRAM reductions when HYBRID_THRESHOLD is disabled but I can't explain it.

@thinkyhead
Copy link
Member

I like the use of structs. The one thing we can do to improve it is to use C++11-style initializers. I’ll try that out and see if it makes the code any neater or smaller.

@thinkyhead
Copy link
Member

We can look closer to see why HYBRID_THRESHOLD uses SRAM. If there aren’t added globals, then maybe there are some non-PROGMEM strings someplace.

@teemuatlut
Copy link
Member Author

teemuatlut commented Oct 1, 2018

No it wasn't anything like that.
It was a difference between
tmc_hybrid_threshold_t tmc_hybrid_threshold{ 100, 100, 3, 100, 100, 3, 3, 30, 30, 30, 30, 30, 30 };
and
tmc_hybrid_threshold_t tmc_hybrid_threshold{ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
This was 100% reproducible for me when I was making the changes but I can't see the difference anymore. I know there should be no difference between the two as far as memory consumption goes, which is why it was so puzzling for me.
However, since it doesn't seem to be the case anymore, I can move the default values to the initializer.

EDIT: To clarify, this was a memory consumption difference in my new implementation that I was seeing. I think I also saw a reduction in sram use from what we had before when switching to structs, but I can't explain that either. But perhaps this behavior is not reproducible anymore either. I'll take a closer look at memory use tomorrow but there shouldn't be any added globals or statics.

@thinkyhead
Copy link
Member

I've submitted PR teemuatlut#26 with some minor tweaks.

@teemuatlut
Copy link
Member Author

I looked through it and I'll merge later today. Do you mind if I split the EEPROM changes to TMC related and others?

@thinkyhead
Copy link
Member

Reorganize your commits in whatever way works best for you.
I'm mostly a spectator until you consider this PR complete.

@teemuatlut
Copy link
Member Author

The basics at least seem to working alright. I'd consider this to be a pretty big change for the TMC support so I wouldn't be surprised if there are issues but lets see how it goes.

@thinkyhead
Copy link
Member

Should this be merged for testing, or do you want to fill out more of the checklist in the OP?

@teemuatlut
Copy link
Member Author

I'll save the rest for future PRs.

@thinkyhead thinkyhead merged commit c3229e1 into MarlinFirmware:bugfix-2.0.x Oct 3, 2018
thinkyhead added a commit that referenced this pull request Oct 3, 2018
@teemuatlut teemuatlut mentioned this pull request Oct 3, 2018
16 tasks
@teemuatlut teemuatlut deleted the bf2_new_library branch October 7, 2018 07:42
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

Comments