New HardwareTimer for STM32 5.7.0

[LinoBarreca]

Requirements

STM 5.7.0 and up

Description

Since new STM Platform 5.7.0 uses a new timer object uniform across all the STM32 models this update makes Marlin use the new object.

Benefits

• code compiles again against up-to-date libraries
• STM 5.7.0 already has a SoftwareSerial (which will make Marlin work with UART and updated TMCStepper without needing the outdated one from BigtreeTech)

Related Issues

https://github.com/MarlinFirmware/Marlin/issues/15538 https://github.com/MarlinFirmware/Marlin/pull/15530

[LinoBarreca]

Do not merge yet. STM32F407VE_black needs work since it used the old STM32. I wanna update that too to 5.7.0

[xC0000005]

This looks reasonable. I should have my printers set up again this week and can test on the M200, Lerdge, and Chitu systems.

[LinoBarreca]

This looks reasonable. I should have my printers set up again this week and can test on the M200, Lerdge, and Chitu systems.

Wait @xC0000005 . It probably doesn't work. According to my last comment the timer isn't started actually while the method name is "start" so I guess it should start the timer too. I'm pushing a new commit which starts it and does additional checks (moreover I do some checks here and there)

[kingofmonkeys]

Do we think this can be tested with a real skr-pro at this point? If so how can I tell its working vs not working correctly?

[viper93458]

I just tried compiling it myself (with my limited understanding of how to use this particular pull request manually) and it did indeed compile, but I can't test till next week when I return from being out of the office.

There is likely an easier way to automatically test a pull request, but my knowledge hasn't extended that far yet as I am not a developer and haven't learned a better way. :)

Manually looking at each file in raw and copying/replacing the original files is pain in the butt. ;)

-William

[kingofmonkeys]

So I can get this to compile but it wont run on the skr pro. It acts like it is crashing on startup. I am not very experienced with this stuff but i can get the firmware on the bigtree git hub to run on the board but not this PR code. I am not sure how to even begin debugging the failure.

[LinoBarreca]

So I can get this to compile but it wont run on the skr pro. It acts like it is crashing on startup. I am not very experienced with this stuff but i can get the firmware on the bigtree git hub to run on the board but not this PR code. I am not sure how to even begin debugging the failure.

@kingofmonkeys did you test the last commit? after 881fc it should work...

[kingofmonkeys]

I was using 881fc and i just pulled down your latest to try and its still doing the same. Its possible I am doing something wrong but I'm not sure what that could be right now. What should I be using for the TMCStepper? Right now i'm just using TMCStepper which I assume gets the latest, I have seen people using various versions but this one complies for me. I looked thru the build output and I see a bunch of warnings related to "HAL_PCD_MODULE_ENABLED" redefined, not sure if that might be a problem or not.

[LinoBarreca]

@kingofmonkeys latest commit doesn't fix anything, in reality. It's just there "for correctness": just to make the HAL behave like the others but with the current use of the hals it doesn't change anything in the theoric behavior. I don't own any STM32 (my 3d-printer is an ender 3) and I developed this just "on the paper" so "in theory" it should work but I didn't test it. Can you please tell me exactly what happens when you boot your board with it? So I can have a clue about what to look at....because if it doesn't work there must be something which looks good but is not...and I gotta understand what...but it's rather difficult without the possibility to debug it. can you please be more specific? what happens on your end? As regards the build warnings you might be right. I noticed it too. they are related to the inclusion of the define in the platformio.ini regarding a hal module which should already be included. I got a hint here https://github.com/platformio/platform-ststm32/issues/305 because without that define I got a linker error. I will try to look at it on tuesday 'cause I'm in the Netherlands right now :)

[kingofmonkeys]

So the behavior I see with this code is this: I power the board on and have it hooked to the computer. The computer plays the sound that it connects but a few seconds later plays the disconnect sound. When i attempt to connect via pronterface it will sometimes start to connect but then i start to receive errors saying the board isnt responding. I can see the lcd screen being powered on but the status screen never comes up and I can see a quick flash on the screen when the board disconnects from the computer.

All of this leads me to believe the firmware is attempting to start up but at some point it hits a fault and restarts. I am currently not sure how to troubleshoot it further.

[MagoKimbra]

I say mine, for the past month MK4duo has been running on a Rumba32 with STM32F446 and I have implemented the new ARDUINO-STM32 library with the new Hardware Timer function. First, I noticed that starting the stepper timer with frequency does not work, at least maintaining compatibility with Arduino DUE and AVR. Starting it instead with the prescaler works great. Second, the setcompare and getcompare functions do not have to act upon compare of the timers, but on overflow. This may HAL_timers on MK4duo:

FORCE_INLINE static void HAL_timer_start(const uint8_t timer_num, const uint32_t frequency) {

  if (!HAL_timer_initialized(timer_num)) {

    switch (timer_num) {
      case STEPPER_TIMER_NUM:
        MK_timer[STEPPER_TIMER_NUM] = new HardwareTimer(STEP_TIMER);
        MK_timer[STEPPER_TIMER_NUM]->setPrescaleFactor(STEPPER_TIMER_PRESCALE);
        MK_timer[STEPPER_TIMER_NUM]->attachInterrupt(Step_Handler);
        MK_timer[STEPPER_TIMER_NUM]->resume();
        HAL_timer_is_active[STEPPER_TIMER_NUM] = true;
        break;

      case TEMP_TIMER_NUM:
        MK_timer[TEMP_TIMER_NUM] = new HardwareTimer(TEMP_TIMER);
        MK_timer[TEMP_TIMER_NUM]->setOverflow(frequency, HERTZ_FORMAT);
        MK_timer[TEMP_TIMER_NUM]->attachInterrupt(Temp_Handler);
        MK_timer[TEMP_TIMER_NUM]->resume();
        HAL_timer_is_active[TEMP_TIMER_NUM] = true;
        break;
    }

  }
}

FORCE_INLINE static void HAL_timer_set_count(const uint8_t timer_num, const uint32_t count) {
  if (HAL_timer_initialized(timer_num)) {
    MK_timer[timer_num]->setOverflow(count);
    if (MK_timer[timer_num]->getCount() >= count)
      MK_timer[timer_num]->refresh(); // Generate an immediate update interrupt
  }
}

FORCE_INLINE static uint32_t HAL_timer_get_current_count(const uint8_t timer_num) {
  return HAL_timer_initialized(timer_num) ? MK_timer[timer_num]->getOverflow() : 0;
}

[thinkyhead]

Should this be replacing the timers code for our separate STM32F4/F7 HAL? At the moment HAL_STM32/timers.cpp simply won't compile for the STM32F4/F7 HAL. But we can make a copy of the HAL_STM32/timers.* files and place them into the STM32F4/F7 folder for completeness.

[thinkyhead]

@MagoKimbra — Seems that the current Marlin doesn't define or use HAL_timer_set_count anyplace. I'm assuming that HAL_timer_get_count has replaced HAL_timer_get_current_count.

[thinkyhead]

Squashed and rebased. To get the latest commits into your working copy use the Git console:

git fetch origin
git checkout NewTimer
git reset --hard origin/NewTimer

[thinkyhead]

@kingofmonkeys I added a call to ->resume() on timer start. Maybe that will help.

[MagoKimbra]

@thinkyhead In Marlin HAL_timer_set_compare in MK4duo is HAL_timer_set_count, is same apart from the name of the void. In Marlin HAL_timer_get_compare in Mk4duo is HAL_timer_get_current_count. For me the problem is that it doesn't have to use setCaptureCompare, but setOverflow, and it doesn't have to use getCaptureCompare, but getOverflow. I've been using it for more than a month on rumba32 with STM32F446.

[thinkyhead]

Ok, I can easily make the suggested changes for set_compare/get_compare. The OP seems pretty confident about According to documentation the prescale factor is computed automatically if setOverflow is in HERTZ_FORMAT. So it might be that the HAL_timer_start initialization code using setOverflow is ok. But I see in your code you don't setMode at all. Is the default mode trusted?

[darknode]

Agree with @MagoKimbra, it should be based on overflow:

FORCE_INLINE static void HAL_timer_set_compare(const uint8_t timer_num, const uint32_t compare) {
  #ifdef HW_TIMERS
    TimerHandle[timer_num]->setOverflow(compare, TICK_FORMAT);
    uint32_t cnt = TimerHandle[timer_num]->getCount(TICK_FORMAT);
    if (cnt >= compare) {
      TimerHandle[timer_num]->refresh();
    }
  #else
  __HAL_TIM_SET_AUTORELOAD(&TimerHandle[timer_num].handle, compare);
  if (HAL_timer_get_count(timer_num) >= compare)
    TimerHandle[timer_num].handle.Instance->EGR |= TIM_EGR_UG; // Generate an immediate update interrupt
  #endif
}

FORCE_INLINE static hal_timer_t HAL_timer_get_compare(const uint8_t timer_num) {
  #ifdef HW_TIMERS
    return TimerHandle[timer_num]->getOverflow(TICK_FORMAT);
  #else
    return __HAL_TIM_GET_AUTORELOAD(&TimerHandle[timer_num].handle);
  #endif
}

[MagoKimbra]

From the Hardware timer library of arduino stm32

void setOverflow (uint32_t val, TimerFormat_t format = TICK_FORMAT);
uint32_t getOverflow (TimerFormat_t format = TICK_FORMAT);

By default it is tick_format, if you want you can add it for security.

@thinkyhead what you set with HAL_timer_set_compare is the number of ticks for the next interrupt, not the frequency.

// Set the next ISR to fire at the proper time
HAL_timer_set_compare (STEP_TIMER_NUM, hal_timer_t (next_isr_ticks));

For this reason I never called HAL_timer_set_compare, but HAL_timer_set_count, but what's called counts nothing, what the function does is set the number of ticks for the next interrupt.

[thinkyhead]

The PR has already been patched to duplicate the given suggestions from @MagoKimbra. Are the software-based timer changes from @darknode also going to be required?

[MagoKimbra]

What @darknode does is for those who have not updated the STM32 libraries and therefore the old code remains.

#ifdef HW_TIMERS
    return TimerHandle[timer_num]->getOverflow(TICK_FORMAT); // New code for Hardware Timer
  #else
    return __HAL_TIM_GET_AUTORELOAD(&TimerHandle[timer_num].handle); // old Code for old library
  #endif

But it should be done for all the functions also for HAL_timer_start and the rest.

[darknode]

That's true, but keep sw times or not in my opinion mostly depends on how backward compatible it should be. I'm not sure first version of hw times will be as stable as current implementation :)

[thinkyhead]

…for those who have not updated…

Ah, but of course. So I don't think that needs to be preserved for this PR, unless we want to add the conditional everyplace else and bring back the files that were deleted.

[MagoKimbra]

On this, in theory it should be so, but I have not yet understood if this is indeed the case, I am also verifying. The problem is to start with the frequency. In stepper.cpp we start with 122

HAL_timer_start (STEP_TIMER_NUM, 122);

But this is not good in this case, we should change it, but this is no longer compatible with the other processors. One possibility is to put a #define for each HAL with the input frequency 122 for the other processors and the desired frequency for this processor. Anyway in theory the prescaler calculates the frequency and since the prescaler is given by the frequency of the processor divided by the desired frequency (HAL_TIMER_RATE) / (STEPPER_TIMER_RATE) should be the desired one. But so my delta flies and goes perfectly even at high speeds.

[thinkyhead]

The problem is to start with the frequency. In stepper.cpp we start with 122.

The other timers use master_clock / prescaler / frequency to set a reasonable compare value from the given frequency and it seems reasonable that we should do the same for STM32.

[kingofmonkeys]

So much activity while ive been away. Im currently getting the bigtreetech FW version setup to run my printer but i can pretty quickly attempt to run this PR and report back when i get a chance.

With everything going on above are you ready for me to try again or is there still work being done?

[thinkyhead]

…are you ready for me to try again…?

This is a good point for testing.

On a side note, at the moment I am also unable to get STM32F4 and STM32F7 builds to succeed. It would be good to get all these sorted out soon!

[kingofmonkeys]

Maybe I'm doing something wrong but i can't get the current branch to build. I am getting alot of errors like this: "Marlin\src\HAL\HAL_STM32\timers.h:137:59: error: request for member 'handle' in 'timer_instance[((int)timer_num)]', which is of pointer type 'HardwareTimer*' (maybe you meant to use '->' ?)"

The only things I have changed is to make the plateform and board BIGTREE_SKR_PRO and changed the x,y,z,e drivers to 2209.

[Spawn32]

kingofmonkeys, travis failed, so the commit's is not pushed out yet ..

[MagoKimbra]

@thinkyhead I saw your latest changes and I think there's something wrong. If working in TICK_FORMAT and not in HERTZ_FORMAT or MICROSEC_FORMAT the prescaler is not set.

From HardwareTimer.cpp…

void HardwareTimer::setOverflow(uint32_t overflow, TimerFormat_t format)
{
  uint32_t ARR_RegisterValue;
  uint32_t Prescalerfactor;
  uint32_t period_cyc;
  // Remark: Hardware register correspond to period count-1. Example ARR register value 9 means period of 10 timer cycle
  switch (format) {
    case MICROSEC_FORMAT:
      period_cyc = overflow * (getTimerClkFreq() / 1000000);
      Prescalerfactor = (period_cyc / 0x10000) + 1;
      __HAL_TIM_SET_PRESCALER(&_HardwareTimerObj.handle, Prescalerfactor - 1);
      _HardwareTimerObj.handle.Init.Prescaler = Prescalerfactor - 1;
      ARR_RegisterValue = (period_cyc / Prescalerfactor) - 1;
      break;
    case HERTZ_FORMAT:
      period_cyc = getTimerClkFreq() / overflow;
      Prescalerfactor = (period_cyc / 0x10000) + 1;
      __HAL_TIM_SET_PRESCALER(&_HardwareTimerObj.handle, Prescalerfactor - 1);
      _HardwareTimerObj.handle.Init.Prescaler = Prescalerfactor - 1;
      ARR_RegisterValue = (period_cyc / Prescalerfactor) - 1;
      break;
    case TICK_FORMAT:
    default :
      ARR_RegisterValue = overflow - 1;
      break;
  }

  __HAL_TIM_SET_AUTORELOAD(&_HardwareTimerObj.handle, ARR_RegisterValue);
  _HardwareTimerObj.handle.Init.Period = ARR_RegisterValue;
}

As you can see, the prescaler register is set only if the format is in HERTZ or MICROSECOND.

So for me it always pays to set the prescaler to the desired value STEPPER_TIMER_PRESCALE and then it's ok to set the input overflow as you did.

timer_instance[timer_num]->setPrescaleFactor(STEPPER_TIMER_PRESCALE);
timer_instance[timer_num]->setOverflow(_MIN(hal_timer_t(HAL_TIMER_TYPE_MAX), (STEPPER_TIMER_RATE) / frequency), TICK_FORMAT);

This way it works well.

This timer system inserted in the stm32 library, works differently from how we worked on AVR and SAM, here the frequency of the timer remains fixed, but we say how many ticks it must count before the next interrupt. First, however, the frequency varied with the speed of the stepper to give the next interrupt when needed. So if you want to work in this way, frequency variation is not fixed, you need to change the ticks in new frequency and then set overflow so that the timer prescaler changes, changing the desired frequency accordingly. I'll do some testing on this.

[kingofmonkeys]

@Spawn32 I'm trying to test the code in this pull request, sorry if my wording was confusing.

[darknode]

Get that branch built (removed lib_ignore = SoftwareSerialM for BIGTREE_SKR_PRO to compile it), flashed and it looks like Marlin restarting infinitely. Connected with debugger to the board, but I don't have swi support at the moment, so can't see anything going from the board :(. Is there any way to catch error of something?

[sjasonsmith]

Has this been tested on any boards yet, or are the failures with the SKR Pro the first attempt to use it anywhere?

[kingofmonkeys]

I think darknode and myself were the only ones who have tried it. Its been a few days since i have but I experienced the same behavior that he is seeing now. Sounds like we both have skr pro boards.

[Spawn32]

Try to compile it with the Watch Dog turned off, just to see if it then starts up...

I ordered my board with DHL shipping, but BIGTREETECH thinks it's nice to not even send the board before at least 8 days have passed, so can't test / debug anything yet :/

[sjasonsmith]

If I switch my steppers to TMC2208_STANDALONE I can boot. Movement and BLTouch work. I think that the BLTouch imlpementation uses HardwareTimer, so the reboot issue might be related to TMC/SoftwareSerial rather than the timing change.

(edit: Fix HardwareTimer, accidentally said HardwareSerial)

[MagoKimbra]

No the new TMC libraries that work on the new STM libraries, they call the library included in STM SoftwareSerial, which uses the timer 7 which is the same that the servo uses, blocking it.

[thinkyhead]

At least one environment seems to require ststm32@5.4.3, so I will need to patch this up to be able to use the old timer code or the new timer code. It would be nice to have unified timer.* files for STM32 that include F4/F7 but there are enough differences that it seems they will need to be kept separate.

[thinkyhead]

they call the library included in STM SoftwareSerial, which uses the timer 7 which is the same that the servo uses

Is this the general issue that the Arduino-based Servo libraries tend to steal all the timers? It seems like we might need to write our own custom Servo libraries in order to get control of that issue in the long term.

[MagoKimbra]

Unfortunately you are right. I solved it by modifying the variants.h of the board rumba32, but you have to do it on all the boards and then distribute the library ..

[thinkyhead]

😱

[MagoKimbra]

In library sofwareserial.cpp in STM32

#if !defined(TIMER_SERIAL)
#if defined (TIM18_BASE)
#define TIMER_SERIAL TIM18
#elif defined (TIM7_BASE)
#define TIMER_SERIAL TIM7
#elif defined (TIM6_BASE)
#define TIMER_SERIAL TIM6
#elif defined (TIM22_BASE)
#define TIMER_SERIAL TIM22
#elif defined (TIM21_BASE)
#define TIMER_SERIAL TIM21
#elif defined (TIM17_BASE)
#define TIMER_SERIAL TIM17
#elif defined (TIM16_BASE)
#define TIMER_SERIAL TIM16
#elif defined (TIM15_BASE)
#define TIMER_SERIAL TIM15
#elif defined (TIM14_BASE)
#define TIMER_SERIAL TIM14
#elif defined (TIM13_BASE)
#define TIMER_SERIAL TIM13
#elif defined (TIM11_BASE)
#define TIMER_SERIAL TIM11
#elif defined (TIM10_BASE)
#define TIMER_SERIAL TIM10
#elif defined (TIM12_BASE)
#define TIMER_SERIAL TIM12
#elif defined (TIM19_BASE)
#define TIMER_SERIAL TIM19
#elif defined (TIM9_BASE)
#define TIMER_SERIAL TIM9
#elif defined (TIM5_BASE)
#define TIMER_SERIAL TIM5
#elif defined (TIM4_BASE)
#define TIMER_SERIAL TIM4
#elif defined (TIM3_BASE)
#define TIMER_SERIAL TIM3
#elif defined (TIM2_BASE)
#define TIMER_SERIAL TIM2
#elif defined (TIM20_BASE)
#define TIMER_SERIAL TIM20
#elif defined (TIM8_BASE)
#define TIMER_SERIAL TIM8
#elif defined (TIM1_BASE)
#define TIMER_SERIAL TIM1
#else
#error No suitable timer found for SoftwareSerial, define TIMER_SERIAL in variant.h
#endif
#endif

Practically the library takes the first timer available according to the processor, in the case of the rumba the first is TIM7 which is the same that uses the Servo library. The only one is defining TIMER_SERIAL, but since this is in a .cpp it cannot be done in the firmware, but in the variants.h which is included by the library .. The problem is that they should do it ... Also so there is also the risk that the library uses a timer used by the firmware for the steppers or the temperature .. They should fix this directly to them. Maybe if you ask, you are more likely to listen than you count. ;)

[sjasonsmith]

On an SKR Pro it is colliding with the temperature timer on TIM7. I don't see TIMER_SERIAL currently in the variants.h at all, I'm experimenting with overriding it from the platformio.ini file.

[MagoKimbra]

Add it. #define TIMER_SERIAL TIM8 for example The Tone and Servo timers are defined in Variants.h of your card, if it coincides with the temperature one, you must also change that.

[sjasonsmith]

Adding -DTIMER_SERIAL=TIM14 to build_flags prevents the hang on boot. I still have a TMC Connection Issue, but at least it isn't hanging! I'm continuing to debug, but recompiling takes way too long on the old computer sitting by my printers.

[KiteLab]

BLTouch imlpementation uses HardwareTimer

BLTouch does not use a HardwareTimer. BLTouch needs the servo code. The servo code needs a hardware timer. The software serial also needs a hardware timer. If not told to use a specific timer it will select one by its own. https://github.com/stm32duino/Arduino_Core_STM32/blob/master/libraries/SoftwareSerial/src/SoftwareSerial.cpp#L43-L93

// It's best to define TIMER_SERIAL in variant.h. If not defined, we choose one here
// The order is based on (lack of) features and compare channels, we choose the simplest available
// because we only need an update interrupt
#if !defined(TIMER_SERIAL)
#if defined (TIM18_BASE)
#define TIMER_SERIAL TIM18
#elif defined (TIM7_BASE)
#define TIMER_SERIAL TIM7 // !!!! First available on the F407s
#elif defined (TIM6_BASE)
#define TIMER_SERIAL TIM6

Randomly this could be the same timer as an other part of the code is using.

You could try to fix that by defining an other servo-timer https://github.com/MarlinFirmware/Marlin/blame/bugfix-2.0.x/buildroot/share/PlatformIO/variants/BIGTREE_GENERIC_STM32F407_5X/variant.h#L241-L246

// Timer Definitions
//Do not use timer used by PWM pins when possible. See PinMap_PWM in PeripheralPins.c
#define TIMER_TONE              TIM6

// Do not use basic timer: OC is required
#define TIMER_SERVO             TIM2  // !!!! Ups !!!!

but that will not help because for the BTT SKR PRO it's the temp timer what is crashing. https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/HAL/HAL_STM32/timers.h#L60-L72

#elif defined(STM32F4xx) || defined(STM32F7xx)

  #define HAL_TIMER_RATE (F_CPU/2) // frequency of timer peripherals

  #ifndef STEP_TIMER
    #define STEP_TIMER 5
  #endif

  #ifndef TEMP_TIMER
    #define TEMP_TIMER 7
  #endif

#endif

That perfectly explains an early reset by the watchdog.

So we could try to change the TEMP_TIMER - but that's for the lazy ones. Better would be to tell the SoftwareSerial.cpp what timer to use. Changing in SoftwareSerial.cpp. No! Don't edit a core library. Changing in SoftwareSerial.h. No! Dito. So changing the one other include when compiling this - Ardino.h? No! What else? Edit platformio.in in the [env:BIGTREE_SKR_PRO] section to add a -DTIMER_SERIAL=XXX in the build_flags. Find the right expression for XXX yourself. Could be X, TIMX or TIMX_BASE with X being a number.

[darknode]

It looks like at least for 405/407 we have a few to choose from: Will try TIM14

[sjasonsmith]

I've tried TIM14 and TIM4. Both boot but don't communicate. I'm current scoping the pin while using TIM4. I can see serial commands going out, but nothing seems to be coming back. I'm about to revert to my older working firmware, so I know what it looks like when it is actually working.

[sjasonsmith]

Ok, comparing it to the old working version reveals that SoftwareSerial doesn’t seem to be handling the pin state properly. I’ll look into it more later tonight. This TMC connection issue seems to be a distinct issue that is probably not caused by the HardwareTimer changes in this pull request.

[kingofmonkeys]

Sjasonsmith, just wanted to pop in and say thank you for working on this. I would love to get my skr pro on the latest Marlin but i wouldn't know where to start