Redundant temp sensor fixups 2

[slowbro]

Description

Fix some more bugs that were pointed out in issues & the original PR, as well as make a few cleanup passes on macros & temperature.cpp.

Requirements

Most of this revolves around MAX Thermocouple boards (specifically MAX31855 and MAX31865).

Benefits

Fixes bugs/issues:

• [BUG] 2.0.8+ breaks MAX31865 hardware SPI on BTT GTR 1.0 #22188
• (issue raised in PR) max6675 and max31855 .begin() should not have an argument like max31865
• (issue raised in PR) fix improper SERIAL_ECHOPGM
• Revisting [BUG & Fix] LPC1768 and LPC1769 will not compile when trying to use Adafruit MAX31865 board & PT100 sensor #20132

Adds/improves:

• New configuration for MAX31865 to set 2_WIRE, 3_WIRE, or 4_WIRE.
• MAXTC_CS_PIN and MAXTC_CS2_PIN genericizations
• MAX TC MISO/MOSI/SCK/etc de-confusing (it's all referencing MAX6675 still)

Configurations

Enable a MAX Thermocouple (-2, -3, or -5).

Testing

MAX6675

Motherboard	Board Qty	Probe	Result
SKR V2 (stm32f4)	1x	K-Type	✔️ 2021-07-03
SKR V2 (stm32f4)	2x	K-Type	❔ Pending, second board on order
BTT SKR V1.4 (lpc1768)	1x	K-Type	✔️ 2021-07-03
BTT SKR V1.4 (lpc1768)	2x	K-Type	❔ Pending, second board on order
RAMPS v1.2 (mega2560)	1x	K-Type	❔ Pending
RAMPS v1.2 (mega2560)	2x	K-Type	❔ Pending, second board on order

MAX31855

Motherboard	Board Qty	Probe	Result
GTR V1 (stm32f4)	1x (onboard)	Copper Shunt	✔️ 2021-06-25 [outdated]
SKR V2 (stm32f4)	1x	K-Type	✔️ 2021-07-03
SKR V2 (stm32f4)	2x	K-Type	✔️ 2021-07-03
BTT SKR V1.4 (lpc1768)	1x	K-Type	✔️ 2021-07-03
BTT SKR V1.4 (lpc1768)	2x	K-Type	✔️ 2021-07-03
RAMPS v1.2 (mega2560)	1x	K-Type	❔ Pending
RAMPS v1.2 (mega2560)	2x	K-Type	❔ Pending

MAX31865

Pt100

Motherboard	Board Qty	Probe	Result
BTT SKR V2 (stm32f4)	1x	Pt100 2-wire	✔️ 2021-07-03
BTT SKR V2 (stm32f4)	2x	Pt100	❔ Pending, second board on order
BTT SKR V1.4 (lpc1768)	1x	Pt100	❔ Pending
BTT SKR V1.4 (lpc1768)	2x	Pt100	❔ Pending, second board on order
RAMPS v1.2 (mega2560)	1x	Pt100	❔ Pending
RAMPS v1.2 (mega2560)	2x	Pt100	❔ Pending, second board on order

Pt1000

Motherboard	Board Qty	Probe	Result
SKR V2 (stm32f4)	1x	Pt1000	❔ Pending, board on order
SKR V2 (stm32f4)	2x	Pt1000	❗ Only have Pt100 board
BTT SKR V1.4 (lpc1768)	1x	Pt1000	❔ Pending, board on order
BTT SKR V1.4 (lpc1768)	2x	Pt1000	❗ Only have Pt100 board
RAMPS v1.2 (mega2560)	1x	Pt1000	❔ Pending, board on order
RAMPS v1.2 (mega2560)	2x	Pt1000	❗ Only have Pt100 board

Related Issues

Fixes #22188
Resolves (issue raised in PR)
??? #20132

[slowbro]

@thinkyhead regarding LPC176x + MAX318x5, how do you feel about bringing @GadgetAngel's modified Adafruit library into src/libs/ and managing it locally? It would allow us to standardize the calls between the libraries and make switches between hardware/software SPI more straightforward - and would fully resolve the confusing situation with SKR boards and MAX thermocouples.

[GadgetAngel]

@thinkyhead regarding LPC176x + MAX318x5, how do you feel about bringing @GadgetAngel's modified Adafruit library into src/libs/ and managing it locally? It would allow us to standardize the calls between the libraries and make switches between hardware/software SPI more straightforward - and would fully resolve the confusing situation with SKR boards and MAX thermocouples.

When this was done, the order of the parameters of the Library calls was standardized.

Marlin at the time did not use library calls for the MAX6675 board or the MAX31855 board.

Each motherboard did their own manipulation of the appropriate signals for the MAX6675 and MAX31855 boards. All that was left in place.

I noticed that some motherboards would not respond with this direct manipulation of the signals and requested that Marlin add the ability to allow the user to use software library calls instead of the direct manipulation of the signals via Marlin.

So we decided to allow a stub library call be be placed in Marlin and by using the correct Compile time directives in platformio.ini file the user could have Marlin call their own software user libraries.

The Marlin Dev team decided not to include the modified Adafruit libraries into src/libs/ because anyone can create their own software libraries and maintain them for themselves. My mods were only to allow the Marlin users the option to have the ability to use a user software library call for the MAX6675 or the MAX31855 boards if they chose to do so.

[slowbro]

Understood.

However, I would argue that if we are supporting a feature (i.e. MAX31865), we should ensure that it works out of the box for all platforms. As it stands, MAX31865 is broken out of the box for LPC176x, with no clearly-defined workarounds or solutions - at least, not without having to troll through google/issues to find the secret sauce.

We can leave the MAX6675/MAX31855 libraries out of Marlin, since they are not required for functioning. We can do raw SPI reads on those without issue. But from how the code is now, you are required to use a library for MAX31865 and the default shipped library (Adafruit stock) is not sufficient. We can pare the Adafuit library WAY down and do the bare minimum to get usable data (i.e. apply calibration calculations, etc)... or we can integrate it in & refactor it to be more Marlin-y, keeping all the niceties (like simultaneous fault detection and reading).

The other other issue I have seen called out with the Adafruit libs is that it uses non-printing-safe delays which can cause stuttering/pausing. If we bring the library in (which again, is required for MAX31865!), we can fix those issues as well.

[slowbro]

Ready for review for reals this time :)

Tested (compilation) with a bunch of combinations of MAX TC (single & double -5, -3, -2; REDUNDANT MAX TC), on the default RAMPS and also LPC1768. Tested non-MAX temp combos, tested HW/SW SPI switching. Tested LIB_USR_MAX318x5. Tested GTR V1.0 for the related issue; builds ok, pending user testing (if they are willing).

I have hardware for testing, will try to get that set up and run some actual thermocouple tests.

Edit; Compiled ab1b48e vs bugfix-2.0.x for the config supplied in 22188; result was +24 bytes RAM and +328 bytes Flash.

[slowbro]

I've finished testing with the three MAX types (6675, 31855, 31865) on my test setup - an SKR V2 (smt32f407vg). It turns out I don't have two 6675s (swore I did..); I don't have a second of the others either. I'm going to try a setup with "two" sensors using the same board w/ two CS pins to make sure double-max-tc works.

I could keep working on this forever because I'm having fun with it, but I'm getting the feeling I should stop and let things settle a bit. There's more optimization that can be done for sure, but I'm going to at least let it get through a review cycle first in case any of the changes get rejected.

There will probably be one more commit coming w/ any fixes necessary for multi-max-tc, and then I swear I will chmod -x /usr/bin/git.

[slowbro]

Curious - is the 'private spi' lib required? It seems redundant; https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/libs/private_spi.h

Marlin/Marlin/src/libs/private_spi.h

Lines 27 to 34 in d32feb7

	template<uint8_t MisoPin, uint8_t MosiPin, uint8_t SckPin>
	class SPIclass {
	static SoftSPI<MisoPin, MosiPin, SckPin> softSPI;
	public:
	FORCE_INLINE static void init() { softSPI.begin(); }
	FORCE_INLINE static void send(uint8_t data) { softSPI.send(data); }
	FORCE_INLINE static uint8_t receive() { return softSPI.receive(); }
	};

and in temperature.cpp https://github.com/slowbro/Marlin/blob/adb78c90aa22b989e41acfb3b450bc04276d298d/Marlin/src/module/temperature.cpp#L197-L202

  #if !HAS_MAXTC_LIBRARIES && HAS_MAXTC_SW_SPI
    // Initialize SoftSPI for non-lib Software SPI; Libraries take care of it themselves.
    template<uint8_t MisoPin, uint8_t MosiPin, uint8_t SckPin>
      SoftSPI<MisoPin, MosiPin, SckPin> SPIclass<MisoPin, MosiPin, SckPin>::softSPI;
    SPIclass<TEMP_0_MISO_PIN, SD_MOSI_PIN, TEMP_0_SCK_PIN> max_tc_spi;
  #endif

Could this just be SoftSPI<TEMP_0_MISO_PIN, SD_MOSI_PIN, TEMP_0_SCK_PIN> max_tc_sp?

[slowbro]

Let me know if there is anything you'd like changed, fixed, or tested @thinkyhead. I can also break this into different PRs if you'd like to focus on the larger changes on their own.

[thinkyhead]

how do you feel about bringing @GadgetAngel's modified Adafruit library into src/libs/ and managing it locally?

The Marlin Dev team decided not to include the modified Adafruit libraries into src/libs/ because anyone can create their own software libraries and maintain them for themselves.

It's always best if we can partner with the library's maintainer to include the improvements we need in their next release, but it is understood that not all library maintainers are able to prioritize the PRs we would send to them, and even then they might not do very frequent releases, so we can't escape the need to maintain a custom Marlin-compatible branch somewhere, at least temporarily. The best way to do that is to just fork it.

That said, the custom library doesn't necessarily have to be forked under the MarlinFirmware organization (like U8glib-HAL), as long as we can be assured that the archive won't go away for a while. (Even GitHub Actions operates under that brittle assumption.) In any case, when we fork a library, we add the minor problem of needing to maintain it so that it doesn't fall too far behind the latest official release (assuming newer == better). However, if it's a library that needs to grow alongside Marlin for a while, then keeping our own fork can help keep those tasks organized while we submit PRs from it.

Note that where needed, we can cause Marlin configuration options to affect third-party libraries by passing global defines with -D build flags. That's just one way we can install hooks into third-party libraries without having to maintain our own fork. The Arduino-L6470 library is another interesting example of a library that was designed in tandem with Marlin with a hook system. It's a little bit baroque but it seems to be working. Of course, the best library is one that "just works."

[slowbro]

It's always best if we can partner with the library's maintainer to include the improvements we need in their next release, but it is understood that not all library maintainers are able to prioritize the PRs we would send to them, and even then they might not do very frequent releases, so we can't escape the need to maintain a custom Marlin-compatible branch somewhere, at least temporarily. The best way to do that is to just fork it.

Indeed. In the case of the Adafruit MAX libraries, the changes we'd want are not likely something they would incorporate, since their codebases are aimed at simplicity & verbosity, for learning purposes. They use non-HAL pin setups/writes/reads for software SPI, delay()s, including WProgram.h, etc. That said, I haven't actually tried asking them :)

In reality, the MAX libraries themselves are a wrapper for some relatively simple math, and knowing the specific layout of the buffers (plus containing fault code defines). All three MAX variants Marlin supports could easily be rolled into a single lib file that we'd likely not need to touch again once implemented, since it's not likely that the underlying hardware will change without a new model number being rolled out. This could have the advantage of size and optimization, while still providing the full feature set for the supported chips. I'm happy to make that and host my own repository to be included via platformio if that would be preferred.

Of course, the best library is one that "just works."

Heh, agreed. I absolutely see the advantage of having the library external to Marlin; MAX chips aren't really the focus of Marlin, just a small part of a large ecosystem that only some people use. Having something we can include that will work reliably without monkeypatching is always the best option.

[slowbro]

@thinkyhead, I did a lot of rebasing in order to bump the MAX31865 library changes under the more-critical changes, in the hopes that a few commits could be cherry-picked to resolve bugs.

ce8805e Fixes the majority of the broken MAX31865 (using Adafruit) and other miscellaneous TC bugs
20075de Fixes issues with Software SPI but relies on 956c891

956c891 Standardizes CS/MOSI/MISO/SCK defines for MAX TC. Not critical, but nice. I can probably rebase 20075de around this if you want to save it for later.

Overall, distilled the original 22 commits into just 9.

[thisiskeithb]

Here's another related issue: #22289

[thinkyhead]

If this PR is not introducing any new issues, then I will go ahead and merge it into bugfix for further testing.

[slowbro]

Pushed a final set of changes from my testing, mostly comments - but also restores the LIB_USR_MAX31865 functionality.

[slowbro]

Thank you!