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

Rename TMC_SW_(MISO|MOSI|SCK) to TMC_SPI_(MISO|MOSI|SCK) #25362

Closed

Conversation

quiret
Copy link
Contributor

@quiret quiret commented Feb 9, 2023

Description

As part of HAL SPI refactoring I realized that TMC_SW_* is a misleading prefix for pin mapping of the TMC SPI stepper drivers. After all, the Marlin firmware is capable of driving those stepper drivers using hardware SPI aswell. At least it can be done in the future. With this PR I want to prepare the Marlin firmware for that future, where all SPI peripherals work together nicely.

Benefits

Better naming convention of Marlin pin mappings.

Related Issues

#24911 (depends on this PR)

@ellensp
Copy link
Contributor

ellensp commented Feb 10, 2023

Nearly all of those pin you have renamed are not capable of hardware spi.. only sw, thus the original name

@quiret
Copy link
Contributor Author

quiret commented Feb 10, 2023

Nearly all of those pin you have renamed are not capable of hardware spi.. only sw, thus the original name

In the near future I plan to change that. STM32 chips have great pin functionality. Also LPC and ESP32. Also exclusion of a platform from HW support should not imply SW only in general thus removing either implication is good.

@ellensp
Copy link
Contributor

ellensp commented Feb 10, 2023

You cannot change the pin functionality, it ether support this at hardware level or it does not.

@quiret
Copy link
Contributor Author

quiret commented Feb 10, 2023

You cannot change the pin functionality, it ether support this at hardware level or it does not.

Would you be happy if I add detection of HW capability at compile-time at the HAL level, then set TMC_USE_SW_SPI conditionally?

This is (easily) possible on the LPC, ESP32, SAM3X, SAMD21, SAMD51 and AVR. The big odd-one out is the STM32. There I have to implement a runtime check and switch between SW and HW SPI depending on requested pins. This is doable thus I stand by my proposal to rename the preprocessor defs.

@ellensp
Copy link
Contributor

ellensp commented Feb 10, 2023

Your very premises for this PR is not accurate. "TMC_SW_* is a misleading prefix"

All implementations of SPI for talking to TMC use a software implementation. (as far I I know)

There is zero need for using hardware SPI, it is only used for configuration. (ie low speed, low volumes of data)

Most of the controllers cannot actually do hardware SPI on the pin required.

If you where to use hardware, you would use SPI2 SPI3 etc and not individual pins. Similar to how we have software serial (which needs a tx and rx pin set) vs hardware serial, where you just tell it the UART being used.

@quiret
Copy link
Contributor Author

quiret commented Feb 10, 2023

All implementations of SPI for talking to TMC use a software implementation. (as far I I know)

There are several boards, for example the MKS Robin E3D V1.1, which fully supports HW SPI for the TMC stepper drivers. In my testing it has worked very well. Thus I want to share the greatness with everyone else.

There is zero need for using hardware SPI, it is only used for configuration. (ie low speed, low volumes of data)

A valid point but I don't see any contradiction in pushing for improvement.

Most of the controllers cannot actually do hardware SPI on the pin required.

I still see a necessity in the generalization because it would improve the applicability of the Marlin firmware. Having a bottleneck in the firmware JUST FOR THE HECK of it... I don't like that. I am sure that it is a reasonable thought that most users of Marlin share.

If you where to use hardware, you would use SPI2 SPI3 etc and not individual pins. Similar to how we have software serial (which needs a tx and rx pin set) vs hardware serial, where you just tell it the UART being used.

You are conceptually leaving out pin configurations that do not map to hardware. Using an intelligent muxing system between HW/SW that would be covered. I just want Marlin to support every single SPI configurable board out there and starting out with the TMC support in its configuration I think that is a good stepping stone.

I fail to see the inaccuracy in my vision. You are welcome to elaborate your point further.

@quiret
Copy link
Contributor Author

quiret commented Mar 23, 2023

I am sorry but I have decided to halt my activities on this PR for an indetermined amount of time. You are free to decide whether to continue it yourself. Thank you to the Marlin firmware community for providing this interesting playing ground for testing my ideas and vision. I hope that Marlin will continue to go towards the best 3D printing solution! Hopefully my ideas have inspired you on how to continue this software!

@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 5 times, most recently from 27df113 to 8d31429 Compare March 25, 2023 04:25
@thisiskeithb
Copy link
Member

This PR was committed directly: 769ad27

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.

4 participants