teemuatlut / TMCStepper

MIT License
501 stars 196 forks source link

Add TMC2240 #270

Open thinkyhead opened 1 year ago

thinkyhead commented 1 year ago

TMC2240 support was quietly added by @z1996xm for BTT over at https://github.com/z1996xm/TMC2240-Lib for the benefit of the BIQU B1-SE-Plus so here's a PR to get it into the upstream repo for all to enjoy!

It's not clear whether it was implemented 100% according to the standard, as it doesn't inherit from TMCStepper, but then again neither does TMC2660Stepper, so it's probably fine.

Meanwhile, there is also a branch by @bigtreetech for TMC2240 put together by @alanma at https://github.com/bigtreetech/Marlin/tree/tmc2240 which (along with B1-SE-Plus-TMC2240) I'm cleaning up so TMC2240 can be added to upstream Marlin.

The CHOPPER_TIMING used in that branch is { 3, -1, 6 } (i.e., chopconf.toff = 3; .hend = 2; .hstrt = 5;) so maybe that deserves a new CHOPPER_* define in tmc_util.h depending on whether it's for some common stepper type. I'll need to examine the B1-SE-Plus-TMC2240 branch to see what it's all about.

Thanks and have a great day!

discip commented 1 year ago

@teemuatlut @thinkyhead What do I need to do to make this work with my local marlin fork?

thinkyhead commented 1 year ago

There are TMC2240-related changes that I need to bring over from https://github.com/z1996xm/B1-SE-Plus-TMC2240/tree/main/B1-SE-Plus and I might have already started a Marlin branch with TMC2240 additions that's sitting in my work pile frmmthe day I made this PR. I can check on that and I'll push a PR with the necessary pieces to the Marlin project soon. In the meantime you should be able to build that B1-SE-Plus branch using the TMCStepper branch that forms the basis of this PR.

discip commented 1 year ago

First of all thank you for answering that quickly. 😊 πŸ‘πŸ»

I can check on that and I'll push a PR with the necessary pieces to the Marlin project soon.

That would be great!

In the meantime you should be able to build that B1-SE-Plus branch using the TMCStepper branch that forms the basis of this PR.

I did eventually manage to compile, but I am not sure if all is setup correctly, since the repo provided by the seller is not 100% identical to the one you referenced.

The version that was provided to me appears to be a tad newer, but I'm not sure if that's the case.

Since I don't want to ruin my brand new SKR 3 EZ 723 nor the TMC2240 stepper drivers, I need help to get it right.

Do I have to do any thing about the CHOPPER-TIMING you mentioned initially?

Here is the link to the provided repo:

link [Marlin-TMC2240-SPI-SKR3.rar](https://we.tl/t-vBdeYJk6Qs)

thanks in advance

thinkyhead commented 1 year ago

I went ahead and created the PR MarlinFirmware/Marlin#25974 to add support for this new stepper driver to Marlin. It looks like it has the same capabilities as the TMC2209 but we'll need to confirm that further. That PR may be updated temporarily to point to my working copy of TMCStepper which forms the basis of this PR so that it can try to run some CI tests, and then once this is merged –and before that PR is merged– it can be reverted to point to the regular TMCStepper. Hopefully I didn't make too many errors, and we'll get this all merged soon!

discip commented 1 year ago

Thank you very much! πŸ˜ƒπŸ‘

This is definitely highly appreciated. Unfortunately I am currently on vacation and not near my hardware to test. Hopefully your PRs will be merged in until then.

once more: many thanks

thinkyhead commented 1 year ago

@teemuatlut β€” Apparently TMC2240 drivers can operate in SPI or UART mode, but BTT plans to ship their drivers with SPI as the default (with a solder-pad to switch to UART), so I will need to add SPI support to this. I will follow the existing drivers as the example, assuming there is nothing special about 2240 compared to other SPI-based drivers.

Is it possible to have a driver support both SPI and UART in the same class, or must they use just one of these interfaces? If necessary, we can add one class for SPI and one for UART and then in Marlin add use driver type enums: TMC2240_SPI and TMC2240_UART.

thinkyhead commented 1 year ago

I've incorporated changes from https://github.com/z1996xm/tmc2240_lib/tree/master here and will test a build with them over the next day or two. The way that @z1996xm implemented __TMC_SPI_DEFINE over at https://github.com/z1996xm/Marlin/commit/146262d6447a130bfe21832f864cfa4769308698 doesn't allow for mixing 2240 drivers with others, so I'll need to tweak that macro to fix that issue. Otherwise, I think I got all the required pieces.

It looks like the 2240 may be more closely aligned with the 2130/5130/2160/5160 than the 2208/9 as it seems to use the get_pwm_scale(TMC2130Stepper ...) and get_driver_data(TMC2130Stepper ...) calls, but in the TMCStepper additions it doesn't seem to inherit from TMC2130Stepper so I'll need to look closer at that. Maybe its triggering of the HAS_TMCx1x0 flag is actually inappropriate and I can remove it from the conditions that enable that flag. We'll see!

More on this later….

teemuatlut commented 1 year ago

I think the best way for me is to accept this as is when you say it's ready and rework the changes into v1.0 whenever I manage to get back into it again. I don't have the drivers to test nor am I really printing anything anymore so I'll have to trust others to do the testing and making sure the changes work. And if it doesn't, there's always the next release.

discip commented 1 year ago

@thinkyhead Is there any progress yet? I've implemented your changes into my fork, but I still can't get it to compile successfully.

thinkyhead commented 1 year ago

I've been on a mental vacation and lately catching up on other Marlin stuff. I will get back to this issue pretty soon.

discip commented 1 year ago

Greetings, definitely well deserved! I'm waiting patiently. 😊

discip commented 12 months ago

@thinkyhead Hopefully you do well. I really do not want to get on your nerves, but ...

Are you still planning to implement this? πŸ‘‰πŸ»πŸ‘ˆπŸ»

thinkyhead commented 10 months ago

It will definitely be completed and merged before the next (2.2.0) release.

Lurchiii commented 9 months ago

Hopefully in 2024 ;)

thinkyhead commented 9 months ago

Juggling a lot of tasks ahead of the next release, but this is near the top of the list. Things got changed over at the tmc2240 branch leading to the conundrum of whether to implement UART or SPI or both, and if both then finding a good example of that. Anyway, I'll aim for whatever BTT is favoring first and if it needs to be extended then it can get extended later.

discip commented 8 months ago

@thinkyhead

Thank you very much! πŸ‘πŸ»

I'm so excited to be finally able to implement the 2240 into my setup. πŸ˜€

thinkyhead commented 8 months ago

Thank you very much! πŸ‘πŸ»

Happy to help! I only wish I wasn't stuck on the fine details. I've reached out to BTT for some help with this, so maybe we'll finally get it done after all this time.

discip commented 8 months ago

I've reached out to BTT for some help with this, so maybe we'll finally get it done after all this time.

From my experience I can say that they are very cooperative, even the sellers from Aliex... are very responsive, courteous & obliging. I have had to contact several of them and in most cases they have been able to help me, except for this particular issue. πŸ˜…

Hopefully they won't let us down in this case.

SimonBuxx commented 2 months ago

Hey @thinkyhead, we are currently trying to get some TMC2240's running for an additive manufacturing research project. While trying to setup your fork, we ran into the problem that the functions void global_scaler(uint8_t) and uint8_t global_scaler(void) that you've added are used and declared but there is no definition for them. The GLOBAL_SCALER functions are defined but global_scaler seems to be something different. Could you maybe help us out? Any help will be greatly appreciated.