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 TMC2240 #270

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

thinkyhead
Copy link
Contributor

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
Copy link

discip commented Jun 5, 2023

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

@thinkyhead
Copy link
Contributor Author

thinkyhead commented Jun 6, 2023

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
Copy link

discip commented Jun 6, 2023

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

thanks in advance

@thinkyhead
Copy link
Contributor Author

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
Copy link

discip commented Jun 12, 2023

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
Copy link
Contributor Author

@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 thinkyhead force-pushed the TMC2240_by_z1996xm branch from 9e8cec7 to efdef25 Compare June 19, 2023 06:25
@thinkyhead
Copy link
Contributor Author

thinkyhead commented Jun 19, 2023

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 z1996xm/Marlin@146262d 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 teemuatlut self-assigned this Jun 19, 2023
@teemuatlut
Copy link
Owner

teemuatlut commented Jun 19, 2023

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
Copy link

discip commented Jul 18, 2023

@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
Copy link
Contributor Author

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
Copy link

discip commented Aug 7, 2023

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

@discip
Copy link

discip commented Oct 7, 2023

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

Are you still planning to implement this? 👉🏻👈🏻

@thinkyhead
Copy link
Contributor Author

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

@Lurchiii
Copy link

Hopefully in 2024 ;)

@thinkyhead
Copy link
Contributor Author

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.

@thinkyhead thinkyhead force-pushed the TMC2240_by_z1996xm branch 8 times, most recently from f002206 to 8176388 Compare January 23, 2024 17:48
@discip
Copy link

discip commented Jan 23, 2024

@thinkyhead

Thank you very much! 👍🏻

I'm so excited to be finally able to implement the 2240 into my setup. 😀

@thinkyhead
Copy link
Contributor Author

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
Copy link

discip commented Jan 23, 2024

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
Copy link

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.

@thinkyhead thinkyhead force-pushed the TMC2240_by_z1996xm branch from fe7e42a to 8f76701 Compare July 29, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants