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

New HardwareTimer for STM32 5.7.0 #15655

Merged
merged 26 commits into from
Nov 13, 2019

Conversation

LinoBarreca
Copy link
Contributor

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

#15538
#15530

@LinoBarreca LinoBarreca changed the title New timer New HardwareTimer for STM32 5.7.0 Oct 24, 2019
@LinoBarreca
Copy link
Contributor Author

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

@thinkyhead thinkyhead added A: STM32 T: HAL & APIs Topic related to the HAL and internal APIs. Needs: Work More work is needed S: Don't Merge Work in progress or under discussion. labels Oct 24, 2019
@xC0000005
Copy link
Contributor

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

@LinoBarreca
Copy link
Contributor Author

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

@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 platformio/platform-ststm32#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
Copy link

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.

platformio.ini Outdated Show resolved Hide resolved
@MagoKimbra
Copy link
Contributor

MagoKimbra commented Oct 25, 2019

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
Copy link
Member

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
Copy link
Member

thinkyhead commented Oct 25, 2019

@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
Copy link
Member

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
Copy link
Member

thinkyhead commented Oct 25, 2019

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

@MagoKimbra
Copy link
Contributor

MagoKimbra commented Oct 25, 2019

@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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

MagoKimbra commented Oct 25, 2019

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
Copy link
Member

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
Copy link
Contributor

MagoKimbra commented Oct 25, 2019

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
Copy link
Contributor

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
Copy link
Member

thinkyhead commented Nov 13, 2019

I dont use Arduino ide and it's something I find useless

I don't use it either. But Marlin is not just about you and me.

...and "specific hal to arduino ide" makes no sense to me. The ide is an ide. The hal is a hal.

Arduino IDE is likely to use different platforms than PlatformIO for the 32-bit boards it supports.

In any case, the boards for which we have supported building in Arduino IDE should continue to be supported for build in Arduino IDE.

Has anyone tested the STM32 builds currently supported for Arduino IDE to make sure they are not broken by this PR? We will want to do that as soon as possible, and then do whatever needs to be done to make sure our currently-supported Arduino IDE 32-bit builds are all still working.

The added complexity of multiple HALs per framework depending on build environment seems a bit pointless and hard to maintain.

We want to continue to support Arduino IDE builds for as long as Arduino IDE is still being developed. If there are some 32-bit boards which both Arduino IDE and PlatformIO support, it would be good to have both of those builds working.

@thinkyhead thinkyhead merged commit ac71cdc into MarlinFirmware:bugfix-2.0.x Nov 13, 2019
@xC0000005
Copy link
Contributor

xC0000005 commented Nov 13, 2019 via email

@LinoBarreca
Copy link
Contributor Author

@thinkyhead yeah. I am sorry maybe I didn't say it right. Arduino is just an IDE. it doesn't matter or changes the code. As long as you have the code, the proper libraries and the toolchain Arduino or Notepad are just the same. That's what I wanted to say.
So...since arduino is like a multi-tab notepad I was wondering why someone today could use it with marlin.
That being said..the pr doesn't alter it's compatibility with any IDE. On platformio it has instructions on how to download libraries, toolchain and so on. On Arduino, instead, you have to do it "by hand" as you always did.

@LinoBarreca
Copy link
Contributor Author

And building this with arduino isn't simple (but still possible), let me explain why: the current "folder structure" has to be reworked. Since it's a board not present yet (a variant) you can't select "skr pro" among the boards available. When I will finish with spi I will move to USB host. By then our "variant" should be perfect. So I will make a pr to stmarduino to include that among the boards handled. Until that day the users have to move files manually from the variant folder into marlin.ino folder

@theofficialgman
Copy link

What is the <timer.h> library that should be included? I haven't been able to compile under the arduino IDE since this release.

@xC0000005
Copy link
Contributor

xC0000005 commented Nov 13, 2019 via email

@LinoBarreca
Copy link
Contributor Author

What is the <timer.h> library that should be included? I haven't been able to compile under the arduino IDE since this release.

attach build log, please. which file is searching for timer.h?

@theofficialgman
Copy link

Arduino: 1.8.9 (Windows Store 1.8.21.0) (Windows 10), Board: "Arduino/Genuino Mega or Mega 2560, ATmega2560 (Mega 2560)"

sketch\src\HAL\HAL_STM32\SoftwareSerial.cpp:38:19: fatal error: timer.h: No such file or directory

compilation terminated.

exit status 1
Error compiling for board Arduino/Genuino Mega or Mega 2560.

@LinoBarreca
Copy link
Contributor Author

sketch\src\HAL\HAL_STM32\SoftwareSerial.cpp:38:19: fatal error: timer.h: No such file or directory

@theofficialgman thanks that was useful. you shouldn't be compiling that file on Mega.
@sjasonsmith forgot the #if :) I'm making a pr with the fix. wait please.

@LinoBarreca
Copy link
Contributor Author

@theofficialgman take the sources from here:
https://github.com/LinoBarreca/Marlin/archive/BuildFix.zip
add your config files, try to build again and let me know if it works.

@sjasonsmith
Copy link
Contributor

@sjasonsmith forgot the #if :) I'm making a pr with the fix. wait please.

I didn't have an #if because when building in platform I/O path filters are used to only build appropriate HALs.

I guess inside Arduino it builds everything and depends on #if statements to exclude anything unwanted?

@theofficialgman
Copy link

@LinoBarreca Works perfectly.

@LinoBarreca
Copy link
Contributor Author

@sjasonsmith I wasn't blaming you ❤️ I tagged you so that next time you will remember. 🤗
I also made a mistake on the fan0/extruder2 pins. In Italy we say "only those who do can fail" meaning that if you do something there's the chance to make a mistake.
@theofficialgman nice to know I already aked to be merged. have a nice day.

@thinkyhead
Copy link
Member

thinkyhead commented Nov 14, 2019

since arduino is like a multi-tab notepad I was wondering why someone today could use it with marlin

Generally, we expect most users to open the .ino, flash Marlin, and quit Arduino. Maybe they'll tweak a setting or two in its quirky Java-based text editor. PlatformIO requires more setup than the average end-user wants to deal with, so it is not promoted where Arduino IDE will suffice.

@thinkyhead
Copy link
Member

fatal error: timer.h

Sorry about that. Now patched.

@thinkyhead
Copy link
Member

thinkyhead commented Nov 14, 2019

I guess inside Arduino it builds everything and depends on #if statements to exclude anything unwanted?

Most IDEs, Arduino included, will build all the .cpp files in the source folder, except those which are excluded by a flag to the compiler. Since Arduino has no way to pass flags to the compiler per-platform, and since some files are only used with certain features regardless of the IDE, almost every .cpp file is wrapped in an appropriate #if condition to prevent or allow compilation.

@thinkyhead
Copy link
Member

Be sure to print and test a lot over the coming days! Once we sort out remaining issues, it's time to tag the first 2.0 release-candidate.

@LinoBarreca
Copy link
Contributor Author

@thinkyhead I made some pr to fix some bugs, please review and merge.

@thinkyhead
Copy link
Member

Thanks! They are now merged. I'll be back to check on things later today. Ciao!

@SirRenix
Copy link

SirRenix commented Nov 14, 2019

Hey guys, thanks for your work for the skr pro. I have a question about the pin.h , why must I use of both pin tx rx the same pin number and not the pin numbers what the board have?
Because when I use this:

  #define X_SERIAL_TX_PIN  PE4
  #define X_SERIAL_RX_PIN  PC13

  #define Y_SERIAL_TX_PIN  PE2
  #define Y_SERIAL_RX_PIN  PE3

  #define Z_SERIAL_TX_PIN  PE0
  #define Z_SERIAL_RX_PIN  PE1

  #define E0_SERIAL_TX_PIN PD2
  #define E0_SERIAL_RX_PIN PD4

I get a tmc connection error. When I use this it work good now:

  #define X_SERIAL_TX_PIN  PC13
  #define X_SERIAL_RX_PIN  PC13

  #define Y_SERIAL_TX_PIN  PE3
  #define Y_SERIAL_RX_PIN  PE3

  #define Z_SERIAL_TX_PIN  PE1
  #define Z_SERIAL_RX_PIN  PE1

  #define E0_SERIAL_TX_PIN PD4
  #define E0_SERIAL_RX_PIN PD4

I load the last marlin from today.

@sjasonsmith
Copy link
Contributor

@PaschyS, BigTreeTech did not populate the resistors that connect the TX pins. You have to add those if you want to use separate pins.

I don't think there is any advantage of using separate pins. There is only one pin on the TMC driver anyway.

@KiteLab
Copy link
Contributor

KiteLab commented Nov 14, 2019

@PaschyS
The TX pins do end in the nirvana.
At least on some boards. Please check at your board. bigtreetech/BIGTREETECH-SKR-PRO-V1.1#70 (comment)

@LinoBarreca
Copy link
Contributor Author

LinoBarreca commented Nov 14, 2019

You should not care anyway. Since when the resistor is present It shorts the rx & tx together. I Guess it was a weird attempt to support different logic levels on different ports. If 3.3v use one port and disable the other, if it's 5v disable the 3.3 and use the one with the resistor to drop the voltage to MCU compatible levels

@sjasonsmith
Copy link
Contributor

You should not care anyway. Since when the resistor is present It shorts the rx & tx together. I Guess it was a weird attempt to support different logic levels on different ports. If 3.3v use one port and disable the other, if it's 5v disable the 3.3 and use the one with the resistor to drop the voltage to MCU compatible levels

The resistor simply allows the TMC to drive the line stronger than the STM32's TX pin. That way you can transmit and receive on a single wire without disabling output on the TX pin.

philippniethammer pushed a commit to philippniethammer/Marlin that referenced this pull request Dec 21, 2019
christran206 pushed a commit to christran206/Marlin2.0-SKR-Mini-E3-1.2 that referenced this pull request Dec 30, 2019
@diabl0w
Copy link

diabl0w commented Apr 9, 2020

this is not true for the skr mini e3 which is still configured to use the old rogerclarkemelbourne repo (through tool-stm32duino in platformio). this repo doesnt have HardwareTimer function integrated from what I understand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: STM32 Needs: Work More work is needed S: Don't Merge Work in progress or under discussion. T: HAL & APIs Topic related to the HAL and internal APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.