Fix BLTOUCH and SPEAKER conflict on SKR E3 boards

[sustmi]

Description

Previous fix https://github.com/MarlinFirmware/Marlin/pull/15547/commits solved the conflict when both SERVO0 and FAN0 PWM were using TIMER 1 by assigning TIMER 8 to SERVO0.
However, when SPEAKER is enabled on SKR E3 boards, TIMER 8 is actually used by libmaple's tone.cpp to generate tones.

This fix changes SERVO0_TIMER_NUM to TIMER 3 on SKR E3 boards. As far as I can tell this timer is not used by any other functionality.

See also this discussion: #15547 (comment) .

Benefits

This PR fixes possible problems with BLTOUCH on SKR E3 boards when SPEAKER is enabled in Configuration.h.

Related Issues

• https://github.com/MarlinFirmware/Marlin/pull/15547/commits

[sjasonsmith]

I would love to test this change myself, since I made the last change to this. Unfortunately that board is disconnected right now, as I have been working on SKR Pro issues.

[boelle]

@sustmi or/and @sjasonsmith do we have links to issues that this PR might solve?

[sjasonsmith]

@sustmi or/and @sjasonsmith do we have links to issues that this PR might solve?

I don't think so. Even when I made the original fix I couldn't find any, even though the BTT repo and reddit had reports of BLTouch issues everywhere.

@sustmi commented on my original PR that he discovered there was still an issue and posted a PR, without bothering to report an issue, I think.

[sjasonsmith]

posted a PR, without bothering to report an issue, I think.

I myself have questioned whether it is better to report an issue and follow it with a PR, or just post the PR if I already intend to fix it immediately.

[sustmi]

@boelle: The original PR #15547 from @sjasonsmith may have fixed these issues:

• while homing z bltouch doesn't always stop the z axis bigtreetech/BIGTREETECH-SKR-E3-DIP-V1.0#13
• BLTouch Configuration bigtreetech/BIGTREETECH-SKR-mini-E3#6

But I did not find any issue (except the @sjasonsmith's PR) in https://github.com/MarlinFirmware/Marlin/ that would describe exactly this problem.

Speaking of my pull request, as the @sjasonsmith's PR was merged only 22 days ago and Creality Ender 3 printers only have buzzer (not speaker) by default, I guess that nobody run into any issue with the BLTOUCH and SPEAKER conflict yet because nobody with SKR E3 board probably enabled SPEAKER.
However, considering how hard would it be to debug this issue I think that it's better to use a timer that is not in conflict with SPEAKER (for SKR E3 boards).
Also, for this very reason I also added the comment in timers.h describing what other timers are used and where.

---

Side note: Actually, I found this potential timer collision only because I noticed that my printer made a different noise when turned on after applying @sjasonsmith's fix. I started to dig deep into the code and found that SPEAKER would use TIMER 8 too.
In the end I realized that the audible noise does not have anything in common with buzzer/speaker. It is actually emitted by FAN0 itself because of the low PWM frequency. And the noise is different now because before @sjasonsmith's fix, the PWM period of FAN0 was changed to 20 ms by SERVO0 code (and the default PWM period/frequency for FAN0 is probably different).

[boelle]

those 2 links point to outside marlin??

[sjasonsmith]

those 2 links point to outside marlin??

That is what we are saying. Issues were very widespread, but all the reports went to the BTT repo. I don't use BTT firmware, so I debugged and fixed the original issue here.

[boelle]

talk about confusion..... :-D

[sustmi]

Some people use those BigTreeTech's GitHub projects to track issues with SKR boards (whether hardware or software ones).
Maybe it has historic reasons (from times when SKR boards were not officially supported in upstream Marlin - only in BigTreeTech's forks).

[sustmi]

Oh no. Do not merge this PR!

Until now, I tested this change only by leveling the bed.
Now I tried to print a file from SD card and while using TIMER 3 for SERVO0, the printer hangs during "E Heating..." and actually causes thermal runaway because the heating is kept turned on.

This is strange. Maybe TIMER 3 is already used by something else.

[sustmi]

Well, I found that TIMER 3 is actually not used but it gets disabled in SPI.cpp when "Print from Media" menu item is selected.
The SD card uses SPI interface and when an SPI gets initialized, it disables timers that belongs to NSS, SCK, MISO and MOSI pins.

On SKR E3 board, SD card is connected to SPI1.
Note that:

• BOARD_SPI1_MISO_PIN is PA6
• BOARD_SPI1_MOSI_PIN is PA7

which both correspond to TIMER 3.

I was trying to find the reason why are the timers disabled but I only found that the SPI code was copied from https://github.com/rogerclarkmelbourne/Arduino_STM32/tree/master/STM32F1/libraries/SPI in PR #15002 .
And when I looked into history of this library, I fount that the disable_pwm() calls are there since the very beginning: https://github.com/rogerclarkmelbourne/Arduino_STM32/blame/d49df93f3843f13a60635b1c9df190cd0c196fdb/STM32F1/libraries/SPI/src/SPI.cpp#L598 .

So it looks like SPI code also "eats" timers. ☹️

Btw: The hang happens when a command is then sent to BLTOUCH over SERVO0 after the timer channel was disabled. Because when timer channel gets disabled by disable_channel(), the interrupt handler is set to NULL. When BLTOUCH command is sent, libServo::pwmSetDuty() enables IRQ because it believes that the interrupt handler is set to Servo_IRQHandler.

I need to sleep on this. 😪

[sjasonsmith]

These timers are such a pain.

As we learn more about what these frameworks do, I think we need to find a way to break the build if conflicting timers are selected.

[sjasonsmith]

Oh, I didn't realize that was actually Marlin code doing that. I thought that when used for GPIO the pins would be muxed away from the timer, so actually interfering with the timer would be unnecessary.

[thinkyhead]

It is our destiny that we will have to make our own SPI, Servo, and Serial classes for use within Marlin in order to get control over which timers we can use. It would be a good idea to have a class for general timer allocation so that we can grab the "best available" timer / channel / interrupt for each feature.

[diabl0w]

@sustmi or/and @sjasonsmith do we have links to issues that this PR might solve?

I would just like to add that I think the reason why there are not many issues reported on this is because BigTreeTech instructs us to disable SPEAKER and enable SOFT_FAN_PWM :
https://github.com/bigtreetech/BIGTREETECH-SKR-mini-E3/blob/master/firmware/V1.2/readme.md

In addition, their default firmware that comes preloaded and also available for download from their github has SPEAKER disabled as well.

So this is still important despite the lack of reports

[sustmi]

@thinkyhead : Thank you for your corrections in the comment.

However, I think that you may have been too fast to merge this PR. The situation with timers allocation is quite messy and I needed more time to wrap my head around it. I did not expect this to be merged since @boelle added the "Do not merge" label.

Did you test the changes with a real machine? I am afraid that this change may cause hangs and thermal runaway with some boards.

[thinkyhead]

Now that timers are overhauled, concerns about timer allocation should be resolved.

[sustmi]

Can you be more specific what do you mean, @thinkyhead ?
Could you provide a link to a pull request or commits?

[sjasonsmith]

Perhaps referring to the changes made in HAL_STM32, although those wouldn't impact these boards using HAL_STM32F1.

[sustmi]

Oh, I found that platformio/platform-ststm32 v5.7.0 now uses stm32duino/Arduino_Core_STM32 v1.7.0 and this version brings a new API for timers: https://github.com/stm32duino/wiki/wiki/HardwareTimer-library
So you probably meant this, did you?

concerns about timer allocation should be resolved.

Are they already? My build env is STM32F103RC_bigtree which uses board genericSTM32F103RC. Does this board use the new timers API?

By searching for example TIMER_TONE in code, I can see that it is defined only in MARLIN_F407VE/variant.h and BIGTREE_SKR_PRO_1v1/variant.h.

[thinkyhead]

concerns about timer allocation should be resolved.

Are they already?

They "should" be. But you will have to confirm whether they are.

[diabl0w]

Oh, I found that platformio/platform-ststm32 v5.7.0 now uses stm32duino/Arduino_Core_STM32 v1.7.0 and this version brings a new API for timers: https://github.com/stm32duino/wiki/wiki/HardwareTimer-library
So you probably meant this, did you?

concerns about timer allocation should be resolved.

Are they already? My build env is STM32F103RC_bigtree which uses board genericSTM32F103RC. Does this board use the new timers API?

By searching for example TIMER_TONE in code, I can see that it is defined only in MARLIN_F407VE/variant.h and BIGTREE_SKR_PRO_1v1/variant.h.

Marlin still has us use tool-stm32duino which is using the libs from rogerclarkmelbourne's repo, so no we are not seeing any benefits of the new API

[minosg]

Trying to wrap my head around this issue.

This pull request changes the SERVO0_TIMER_NUM to TIMER3 which is disabled by SPI?

[sustmi]

Hello @minosg . Sorry for a late reply.

This pull request changes the SERVO0_TIMER_NUM to TIMER3 which is disabled by SPI?

Well, actually not. I think that the TIMER3 is not used by SERVO0 on any board configuration because as far as I know, neither of BIGTREE_SKR_MINI_E3, BIGTREE_SKR_E3_DIP, BTT_SKR_MINI_E3_V1_2 and MKS_ROBIN_LITE boards have a low density MCU. So I believe that the conditions needed for the TIMER3 to be used are never satisfied.

This pull request originally tried to change the SERVO0_TIMER_NUM from TIMER8 to TIMER3 for the aforementioned boards because those boards are STM32_HIGH_DENSITY and tone generator for SPEAKER functionality uses TIMER8 on such boards. However then I found out that TIMER3 cannot be used because it gets disabled by SD cards SPI initialization code (even though the SPI does not use the timer).

Then the code in this PR got updated by @thinkyhead who changed it almost to it's final form.

Don't feel bad if the final code does not make much sense to you. It does not actually do what it says in the PRs title. It does not fix the conflict with the SPEAKER functionality. It only added one condition that is probably never satisfied, few cosmetic changes and two misleading comments because they say that "tone.cpp uses Timer 4" for STM32_HIGH_DENSITY and "Timer 8" for other boards which is actually the other way around. To be honest, I feel bad that the PR got merged and I was the one who started it.