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

Improve SPI TFT and XPT2046 support (HAL rework) #24911

Closed
wants to merge 100 commits into from

Conversation

quiret
Copy link
Contributor

@quiret quiret commented Oct 20, 2022

  • improved SPI TFT and XPT2046 support (HAL handling rework based onlibrary sources + (electrical) testing)

Description

brought to you by Martin Turski, company owner of EirDev ([email protected]) this adds support for the Makerbase MKS TS35-R V2.0 screen in combination with the MKS Robin E3D V1.1 board. proper testing has shown that this hardware combination works very well with the Marlin firmware. previously the bus negotiations of various Marlin components was conflicting so much that this use-case was a head-ache!

Adds proper baudrate determination between hardware specifications and board clock frequencies.

  • new configuration option: TOUCH_BAUDRATE
  • new configuration option: TFT_BAUDRATE_READ
  • new configuration option: TFT_BAUDRATE_WRITE

https://green-candy.osdn.jp/external/promo/mks_robin_e3d_v1_1_with_ts35r_v2_0.png

Simply connect the screen across the EXP1/EXP2 connectors, flash the properly configured firmware and give it a go!

Interupt-driven SPI DMA has not been tested. Also I have not tested every configuration combination. I would appreciate some verification by the Marlin FW community.

Requirements

MKS Robin E3D V1.1
MKS TS35-R V2.0

Benefits

  • No shared SPI bus problems between TFT, touch and SD
  • Dynamic baudrate calculation
  • HAL SPI stabilization
  • More hardware support without headaches

Configurations

config.zip

Related Issues

#24867

Have fun!

…library sources + (electrical) testing)

brought to you by Martin Turski, company owner of EirDev ([email protected])
this adds support for the Makerbase MKS TS35-R V2.0 screen in combination with the MKS Robin E3D V1.1 board. proper testing has shown that this hardware combination works very well with the Marlin firmware. previously the bus negotiations of various Marlin components was conflicting so much that this use-case was a head-ache!

Adds proper baudrate determination between hardware specifications and board clock frequencies.
- new configuration option: TOUCH_BAUDRATE
- new configuration option: TFT_BAUDRATE_READ
- new configuration option: TFT_BAUDRATE_WRITE

Buy MKS Makerbase TS35-R V2.0 screen: https://de.aliexpress.com/item/1005003634231986.html
Buy MKS Makerbase Robin E3D V1.1 board: https://de.aliexpress.com/item/4000781744682.html

https://green-candy.osdn.jp/external/promo/mks_robin_e3d_v1_1_with_ts35r_v2_0.png

Simply connect the screen across the EXP1/EXP2 connectors, flash the properly configured firmware and give it a go!

Interupt-driven SPI DMA has not been tested. Also I have not tested every configuration combination. I would appreciate some verification by the Marlin FW community.
@quiret
Copy link
Contributor Author

quiret commented Oct 20, 2022

I have also made a change to support TMC driver components on the same SPI bus as TFT, SD, Touch, etc. NO PROBLEM! It requires this pull though because SPI is really messed up in the Marlin release.

quiret@9096c4b

You need the following fixed TMCStepper source code to compile the latest code:

TMCStepper_dep.zip

I had no idea that TMCStepper was not part of Marlin, so stuff to be fixed there unfortunately... Head-ache.

Please support this pull request to improve the TMCStepper library: teemuatlut/TMCStepper#255

You can find the most up-to-date version of my TMCStepper here: https://github.com/quiret/TMCStepper

…MC drivers on the same SPI bus as TFT, Touch, SD, etc

brought to you by Martin Turski, company owner of EirDev ([email protected]). In this commit the SPI support has been improved in all of Marlin - across ALL the boards! - so that the big problem of connecting peripherals on the same SPI bus is tackled. I have tested the commit on my MKS Makerbase Robin E3D V1.1 board and all SPI components have successfully initialized, are successfully communicating and it works quite nicely. I invite you to test your own board using this commit.

I tested the TMC2130 drivers in SPI mode and there is no issue when using them. Also connected as a MKS Makerbase TS35-R V2.0 screen.

Have fun!
quiret added a commit to quiret/TMCStepper that referenced this pull request Oct 20, 2022
…th TMC by properly initializing and then terminating the SPI usage

in the way that this library was previously written it was not easy to share the SPI bus between components because the SPI protocol was not properly terminated/cleaned-up across SPI sessions, causing conflicts for different SPI bus components. This library has been tested with Marlin 2.1.x bugfix on this repo: github.com/quiret/Marlin/

There is a Marlin pull request that would really appreciate this improvement: MarlinFirmware/Marlin#24911

Please consider releasing another version of this library with the included fix so that the Marlin FW can be drastically improved! clobbering the global SPI object is not a good idea, especially on single-SPI boards...

Martin Turski, company owner of EirDev ([email protected])
@thinkyhead thinkyhead added C: LCD & Controllers T: HAL & APIs Topic related to the HAL and internal APIs. A: STM32 labels Oct 30, 2022
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 10 times, most recently from cbc8876 to b10e1fc Compare October 30, 2022 02:13
- added YouTube videos about the Arduino pin layout and the AVR alternate pin functions to the comments
quiret and others added 2 commits February 15, 2023 19:00
- fixed a bug in ESP32 HAL SPI where interrupt code cannot be assumed to be entirely in IRAM due to access to peripheral memory, possibility of having a compiler-internal function inside the generated code-graph
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 3 times, most recently from ae0e415 to 45f2139 Compare February 21, 2023 00:26
@thinkyhead thinkyhead removed their request for review February 21, 2023 01:26
@thinkyhead
Copy link
Member

My review so far just extends to adjusting formatting and peeking at the new code. I could go through various boards and printers and flash Marlin onto them to see if they boot, but I only have a small batch, and that seems like a lot of work without a huge return. i.e., I wouldn't be able to exercise all the features or catch issues that only arise after an hour of printing. So, my thought is, after the release of Marlin 2.1.3 we should merge this for Foist Testing. That's the testing methodology where we unceremoniously foist the changes upon the world, see what happens, and fix issues as they are found. Not very different from how we test most other changes.

My main concern with any very large overhaul that touches many files is that the build size will explode, or various other things that depend on quirks, accurate timing, or side-effects will explode. This PR touches 457 files, which is enormous by any standard. So if it can be split up into more PRs that have fewer changes each, that would help. For example, it would make it easier to revert related changes without touching others. If we merge this whole PR in one go, we may have a harder time seeing the effects of any one set of changes.

In general, we aim to keep Marlin lightweight and do as little as possible, so there is not a lot of argument validation and other checks, except where we are validating things like G-code input. That is more a concern for AVR than other targets, and also more a concern related to profiling. i.e., If certain code is not being called too often then the tiny bit of extra overhead is not a big deal. Where performance might be impacted, we can always use assert which only gets included in a debug build.

@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 2 times, most recently from e90c213 to 4b9bb85 Compare March 7, 2023 05:17
@quiret
Copy link
Contributor Author

quiret commented Mar 23, 2023

My review so far just extends to adjusting formatting and peeking at the new code. I could go through various boards and printers and flash Marlin onto them to see if they boot, but I only have a small batch, and that seems like a lot of work without a huge return. i.e., I wouldn't be able to exercise all the features or catch issues that only arise after an hour of printing. So, my thought is, after the release of Marlin 2.1.3 we should merge this for Foist Testing. That's the testing methodology where we unceremoniously foist the changes upon the world, see what happens, and fix issues as they are found. Not very different from how we test most other changes.

My main concern with any very large overhaul that touches many files is that the build size will explode, or various other things that depend on quirks, accurate timing, or side-effects will explode. This PR touches 457 files, which is enormous by any standard. So if it can be split up into more PRs that have fewer changes each, that would help. For example, it would make it easier to revert related changes without touching others. If we merge this whole PR in one go, we may have a harder time seeing the effects of any one set of changes.

In general, we aim to keep Marlin lightweight and do as little as possible, so there is not a lot of argument validation and other checks, except where we are validating things like G-code input. That is more a concern for AVR than other targets, and also more a concern related to profiling. i.e., If certain code is not being called too often then the tiny bit of extra overhead is not a big deal. Where performance might be impacted, we can always use assert which only gets included in a debug build.

@thinkyhead The improvements I have attempted to bring are in line with your vision. Any noise, like the unexpected compiler code-gen that increased firmware size for the AVR internals, can be avoided by adjusting the build configurations. And the increase in execution time should not be too big of a deal, especially because of the configurability using preprocessor macros.

I am very sorry that I have to stop my activities on this PR. Marlin firmware will always be dear in my heart and the fork I have made should be an example of my passion for this project. Maybe we can meet again in the future when certain dust has settled and then Marlin can be made greater than before, again with my contributions. I wish the community a great future especially with the recent events regarding online AI scavenging the whole internet. Hold strong and thoughen up! ;)

@quiret quiret closed this Mar 23, 2023
@thinkyhead
Copy link
Member

when certain dust has settled

If you're referring to my personal focus in recent weeks on fixing up the Stepper class / Input Shaping and making preparations for the 2.1.3 release, those dusty tasks should be settled pretty soon, hopefully clearing the way for more comprehensive refactors.

I can make no promises, as my first priority each day is to review and fix up the latest PRs, and then try to attack the backlog, and some weeks are more loaded with new PRs than others. Naturally, the larger and more complex a PR is, the more time it requires for review, and the more that I want to get it tested before merging. If it's a toss-up between spending a whole day on one large PR or trying to get through a bunch, I tend to go through a set of easier ones first.

Of course, I also have aspirations to bring the Documentation site up to date, continue to improve the Auto Build Marlin extension, and spend some time reviewing extant bug reports, but there just aren't enough hours in the day. That said, every PR will get its time in the sun. Refactors have had to take a back-seat to certain features demanded by the community, such as Input Shaping, time-based motion planning, support for the latest boards, fixing motion-related anomalies, etc. Some of my own PRs have been sitting unfinished for months and years, so I must beg the patience of all contributors. As the weather has been improving, so has my mood, and with it my productivity. (We'll see how things go over the coming weeks, as I need spend some of that precious time to find a new apartment.)

One thing that can help get PRs closer to being merged is to get them tested. That is one of the aims of the @ testers role and "testing" channel on Discord, to help contributors find and recruit testers. Testers with the appropriate hardware can certify, at least, that a PR is working and they can provide feedback on their experiences, help locate hidden bugs, and bring more confidence that a PR is safe to merge. Of course, this is an all-volunteer project that everyone works on for fun in their spare time, and it may not always be easy to find help, so I must always beg the patience of contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: STM32 C: LCD & Controllers T: HAL & APIs Topic related to the HAL and internal APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants