Skip to content

Comments

Fix BLTOUCH and FAN PWM conflicts on SKR E3 boards#15547

Merged
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
sjasonsmith:SKR_E3_BLTOUCH
Oct 15, 2019
Merged

Fix BLTOUCH and FAN PWM conflicts on SKR E3 boards#15547
thinkyhead merged 2 commits intoMarlinFirmware:bugfix-2.0.xfrom
sjasonsmith:SKR_E3_BLTOUCH

Conversation

@sjasonsmith
Copy link
Contributor

Description

Fix a conflict between the PWM timer used by FAN_PIN and the timer used for Software PWM output for SERVO0.

This impacts all of the SKR "E3" boards (E3 Mini, E3 Mini 1.2, and E3 DIP). The MKS Robin Lite is also impacted, although it doesn't currently implement BLTouch support. These all use the PA8 pin for FAN_PIN, which corresponds to timer 1's TIM1_CH1 PWM pin.

I'm including scope captures of the SERVO0 output line, prior to this change, on an SKR E3 Mini.

All my testing was done with an SKR E3 Mini. I am assuming the same solution works for the other boards including in the change.

Immediately After Boot (Without fix)
Logic is held high with a 1.4us glitch low every 20ms (PWM period).
Default_After_Boot

When commanding a BLTouch probe to deploy, using M280 P0 S10 (Without Fix)
Logic drops low, and only two pulses are observed before returning to prior erroneous state. Occasionally up to 5 pulses are sent. The BLTouch seems to recognize the command when at least 4 pulses are sent.
Default_ProbeDown_M280

Benefits

This makes manual control very reliable. I'm using the Octoprint BLTouch plugin, but that is just sending M280 commands.

I have also not had any probing failures since making this change. Prior to this change about 60-80% of my probes would crash into the bed. After this change, I have performed at least 125 probes with no failures. 25 of those were at temperature, with the bed at 100C and nozzle at 230C.

I suspect that the erroneous behavior of holding the signal line high between commands is preventing the BLTouch from performing correctly.

Scope captures show the "Fixed" behavior with this change, on the same board. I verified with my old Melzi board that this matches the behavior of my old working BLTouch system.

Immediately After Boot (With Fix)
Logic low, with proper continuous "Probe Up" pulses every 20ms.
TIMER_8_Boot

When commanding a BLTouch probe to deploy, using M280 P0 S10 (With Fix)
New command pulse continues until another command replaces it.
TIMER_8_ProbeDown_M280_ZoomedOut

Related Issues

For as many complaints as I can find on Reddit about BLTouch being unreliable on the SKR E3 boards, I can't find specific issues in Marlin. I feel like I must be a little blind today, they must be there somewhere!

@sjasonsmith
Copy link
Contributor Author

@Roxy-3D and I discussed this on Discord, and they recommended working around this in timers.h for now. Unfortunately I had to change the include order in a couple other files to ensure it always behaved correctly, which may or may not be ok.

I didn't change all STM32F1 boards to use timer 8 because (1) not all F1 chips has a TIM8, and (2) other boards have their fans attached to TIM8.

I think long-term this needs restructured to allow the pins files to override all the timer selections in timers.h. It's not really a board flaw to connect a FAN to an arbitrary timer on the board. There should be some way to configure the HAL accordingly. I attempted to implement this approach, but it wasn't as simple as adding some ifdefs due to the include order of the HAL and pins files.

@sjasonsmith
Copy link
Contributor Author

Another improvement I just noticed is that when I sent a Self-Test command to my BLTouch, it consistently performs 10 Probe/Stow cycles. Prior to this, it would perform 2-5 cycles, seemingly randomly.

It seems that the BLTouch "forgets" what mode it is in without this fix. That could explain why when probing it would simply ignore the pin being pressed into the probe, and why it was more likely to happen if the bed started a long distance from the probe.

@sjasonsmith
Copy link
Contributor Author

sjasonsmith commented Oct 13, 2019

I just finished 250 successful probes with this change (5 iterations of M48 P50). It seems like maybe all those complaints about hardware could be solved in software after all!

Edit: This was done with bed at 100C and nozzle at 230C. Infrared thermometer shows voltage regulator area on the board is at 50C.

@thisiskeithb
Copy link
Contributor

Great work.

I wonder if a similar patch can be applied to the SKR 1.3 since the servo & fans share the same timer making FAST_PWM_FAN incompatible with BLTOUCH.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be replaced by #if STM32_HAVE_TIMER(8)

only the CB/C8 card need timer 1... STM32F103CB_malyan

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not do this, because other boards have FAN pins on timer 8. It would trade a fix on the SKR E3 boards for potential problems on other boards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but also locks 2 timers in Marlin for the same feature...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see if I can solve the problem by switching to different channels, rather than a different timer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant the list of boards could take 5 pages in a few months, using this kind of code :p better to do the reverse... if the timer 8 doesnt exist...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i coded it like that first, someone has put it then in this file.... was intended to be set in the pins file if used

Copy link
Contributor

@tpruvot tpruvot Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something to try:
#ifndef SERVO0_TIMER_NUM
#define SERVO0_TIMER_NUM 1
#endif

and define 8 in your board file, in theory the Servo cpp file will load the config before this .h

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the first thing I tried, and it did not work. This file ends up included before the pins file. That would work if I moved this #DEFINE into Servo.cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed like there was some intention to have the timer numbers listed all in timers.h. If that is not true, then moving it to Servos.cpp and adding the #ifndef would be the easy fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe let a comment in the .h ... #14407 seems related

@sjasonsmith
Copy link
Contributor Author

Based on @tpruvot's concern regarding this making fewer timers free for future use, I experimented with moving the Servo output to different channels on Timer 1, rather than to a completely new timer. Images were taken with the fan at 50%, to produce a square wave.

This still resolves the BLTouch issues (I think, I only probed it, did not test it), but changes the fan PWM period to 20ms.

(Notice that the X scale is changed in the second picture, so that you can clearly see the higher frequency PWM signal. The Servo pulse is the same length in all images.)

FAN_TIM1_SERVO0_TIM1
FAN_TIM1_SERVO_TIM8_reference
FAN1_TIM1_SERVO_TIM1CH3CH4

@sjasonsmith
Copy link
Contributor Author

I wonder if a similar patch can be applied to the SKR 1.3 since the servo & fans share the same timer making FAST_PWM_FAN incompatible with BLTOUCH.

I don't know. I've never had issues with BLTouch on my SKR 1.3, I guess because I use FAN_SOFT_PWM. My fan won't turn at less than 100% with the hardware PWM.

@thinkyhead thinkyhead merged commit 53abfdc into MarlinFirmware:bugfix-2.0.x Oct 15, 2019
@sjasonsmith sjasonsmith deleted the SKR_E3_BLTOUCH branch October 18, 2019 00:12
rolkun pushed a commit to rolkun/Marlin that referenced this pull request Oct 25, 2019
@sustmi
Copy link
Contributor

sustmi commented Oct 31, 2019

Nice work @sjasonsmith !

I may not fully understand how are timers configured and how are the pins assigned. However, I can see in https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/master/STM32F1/variants/generic_stm32f103r/board.cpp#L124 and also in the STM32F103RCT6 datasheet (on page 34) that timer 8 is assigned to pins PC6, PC7, PC8 and PC9.
According to SKR E3 DIP V1.0 schematic, pin PC8 is used for HE0_PWM signal and PC9 for BED-PWM.

Correct me if I am wrong but isn't the BLTOUCH servo now in conflict with those two PWMs?

As you shown here #15547 (comment), setting SERVO0_TIMER_NUM to 1 causes change of the PWM period from ~1.8 ms to 20 ms. Isn't something similar happening now with hotend and heated bed PWMs?

I can see in https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/HAL/HAL_STM32F1/Servo.cpp#L180 that the servo control code uses channels 1 and 2 of the timer while PC8 and PC9 correspond to channels 3 and 4 but the timer_set_prescaler() setting is common for all channels.

@sustmi
Copy link
Contributor

sustmi commented Oct 31, 2019

Oh, I can see by looking here:

// Use software PWM to drive the fan, as for the heaters. This uses a very low frequency

and here:
static SoftPWM soft_pwm_hotend[HOTENDS];

... that hotend and heated bed actually use software PWM (ie. generated by directly setting the output signals to 0 or 1; not a hardware PWM where the signals are generated by appropriate timer channels).

And the code that generates software PWMs for heaters uses timer 2 according to:

HAL_timer_isr_prologue(TEMP_TIMER_NUM);

and:
#define TEMP_TIMER_NUM 2 // index of timer to use for temperature

So there should be no new conflict after this fix. 👍

@sustmi
Copy link
Contributor

sustmi commented Nov 3, 2019

Update: I found there actually is a conflict after changing SERVO0_TIMER_NUM to 8. 😞

I noticed that after this fix the buzzer sound played during splash screen (when the printer is turned on) sounds a bit different - it's longer and sounds distorted.
I found out that timer 8 is actually used by tone generator: https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/83ae7019d8085181e8d9b622ce90201b61b5c719/STM32F1/cores/maple/tone.cpp#L31 .
And this tone generator is used by Marlin's Buzzer class:

::tone(BEEPER_PIN, state.tone.frequency, state.tone.duration);

Side note: Timer 8 is a default value that can be overwritten by calling setToneTimerChannel().

This is the current assignment of timers for SKR E3 boards

As far as I can tell, the other timers are not used. According to Table 4. High-density timer feature comparison in STM32F103RCT6 datasheet (page 19), timers 6 and 7 have zero capture/compare channels so they are not suitable for SERVO0_TIMER_NUM. So we are left with timer 3 or timer 4.

Since on some boards (that have "medium-density" STM32F103xx chips with just 4 timers) timer 4 is used for steppers instead of timer 5, I would choose timer 3 (to avoid confusion) as SERVO0_TIMER_NUM while hoping that we do not find some other feature that already uses this timer.

What do you think @sjasonsmith ?

@sjasonsmith
Copy link
Contributor Author

@sustmi All I can do it laugh 🤣

These timer conflicts are such a pain. I wonder if something could be done in the STM32 Core to make it easier to detect conflicts at compile time.

Can you submit a pull request to fix this? I don't have that board connected anymore, so cannot test a fix. If you cannot fix it right away you should create a new issue with the details you included here.

@sjasonsmith
Copy link
Contributor Author

At least a weird sounding buzzer is better than crashing during probes 😄

@sustmi
Copy link
Contributor

sustmi commented Nov 3, 2019

🤦‍♂️ I've just found that the sound does not actually come from the buzzer but it's a noise from the hotend fan that gets spinned-up for a short time when the printer is turned on.
As the fan PWM period is now different after your fix (not 20ms forced by SERVO0) it makes sense that the noise is different too.

At least a weird sounding buzzer is better than crashing during probes

As long as the buzzer does not change timer's pre-scaler or output compare value for channels 1 and 2 during probing, I guess it should not introduce any probing errors.

sustmi added a commit to sustmi/Marlin that referenced this pull request Nov 3, 2019
…ware#15547)

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: MarlinFirmware#15547 (comment)
@donjakobo
Copy link

I know this is closed and an old issue, but if it helps anyone. None of the above fixed my BLTouch issue. I swapped hardware (Bltouch) with a backup, tweaked firmware multiple ways. The ONLY fix that eliminated the "stuttering probe" issue was installing a 5V stepdown (24v -> 5V) and running my BLtouch power/ground off that vs the board. I don't know if its noise or weak 5v line, I tried as a last ditch effort on this board and just a simple pololu stepdown did the trick beautifully. Now all my probing is reliable and works great.

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.

6 participants