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

Normalize configuration of HAL/STM32 targets #17904

Conversation

sjasonsmith
Copy link
Contributor

@sjasonsmith sjasonsmith commented May 7, 2020

Add additional testing and resolve issues compiling with TMCStepper.
Add a variable to declare the version of framework-arduinoststm32.
Upgrade framework-arduinoststm32 to 4.10700
Remove previously unused ldscript filenames from board.json files.

Description

This improves consistency for environments using HAL/STM32. By adding a variable for the framework version and extending test coverage, future version changes will be simpler and safer.

I am upgrading to framework version 4.10700 and not all the way to 4.10800 because some timer issues have to be fixed to be compatible with 4.10800.

I've verified the following functionality with an SKR Pro on my bench. I have not tested actual prints or other hardware. Hopefully others could help try out the change.

Functionality I verified with my SKR Pro on the bench:

  • RepRapDiscount Full Graphics Display
  • BLTouch
  • TMC Communication (baud rate verified with analyzer)
  • Thermistor operation
  • Step generation (viewed by logic analyzer)

Benefits

  • Fixes build issues with framework 4.10700
  • Sets the stage for a cleaner upgrade to 4.10800 once timer issues are resolved.

Add additional testing and resolve issues compiling with TMCStepper.
Add a variable to declare the version of framework-arduinoststm32.
Upgrade framework-arduinoststm32 to 4.107
Remove previously unused ldscript filenames from board.json files.
.github/workflows/test-builds.yml Show resolved Hide resolved
platformio.ini Show resolved Hide resolved
platformio.ini Show resolved Hide resolved
@thisiskeithb
Copy link
Member

I’ll give this a test on a BTT002 after some sleep 🙂

@viper93458
Copy link

viper93458 commented May 7, 2020

I'll give it a shot now on my SKR Pro 1.1 with TMC2208 UART, BLTouch, i2C External EEPROM and RGB LEDs.

-William

@GhostlyCrowd
Copy link
Contributor

Building on SKR Pro 1.1 with 2209's right now will test

@viper93458
Copy link

Compiled OK, fixed the bootloop, i2c EEPROM not working, trying to determine if something else happened to it though. :)

-William

@viper93458
Copy link

viper93458 commented May 7, 2020

Movement works, Heating works, Layer cooling fan works, BLTouch works, LEDs work. Just trying to figure out what happened to my EEPROM.

@GhostlyCrowd
Copy link
Contributor

Everything is working as expected, printing a test cube right now. SD Eeprom is fine.

SKR Pro 1.1 with 2209's

@thisiskeithb
Copy link
Member

thisiskeithb commented May 7, 2020

I just ran a quick test print on a BTT002 (Prusa MK3S, TMC2209, PINDA V2, 2004 character LCD) and everything worked as expected.

@sjasonsmith
Copy link
Contributor Author

A little update on this. I have succeeded in getting timers to work with the framework updated to 4.10800, but unfortunately their SoftwareSerial appears to be broken again. I can still make it work with our own copy of SoftwareSerial, but deleting those files was my main motivation for wanting to go to 4.10800!

I will need a bit more time with the new framework to make sure I understand the changes. The two main changes I wanted from it was setting timer interrupt priorities and fixing SoftwareSerial, and at the moment is is seeming like both of those items may not be working as intended.

I don't think that should hold up this PR, though. @thinkyhead do you want to choose which of the new test files remain enabled, or do you want me to push a change to disable some of them? I'd like to keep the actual test files intact even if we don't enable them, since they are useful to test that environments remain buildable during refactors like this one.

@viper93458
Copy link

using bugfix2.0 from just now with the changes here has unfortunately caused the SKR pro 1.1 to go back into boot loops. :(

-William

…s/PR/STM32_Normalize_Rebase

# Conflicts:
#	.github/workflows/test-builds.yml
@sjasonsmith
Copy link
Contributor Author

FYI, that M115 issue is logged in issue #17922, and I've already posted a PR #17932 to fix it.

@viper93458
Copy link

Good news, disabling M115 Reporting has resolved the boot loop! Thanks for the help.

-William

@sjasonsmith
Copy link
Contributor Author

Good news, disabling M115 Reporting has resolved the boot loop! Thanks for the help.

That's great! I went ahead and merged in the latest bf2 changes, which include the fix for that.

@thinkyhead
Copy link
Member

Note that once the PlatformIO extension for VSCode catches up, we will be able to use the inherits_from parameter to inherit all the properties of another environment.

@thinkyhead
Copy link
Member

do you want to choose which of the new test files remain enabled

If they are still completed in the same amount of time as the existing brood, then I don't mind keeping them all. I think it would be fine to merge them all, and only if they become problematic disable a few.

@thinkyhead thinkyhead added A: STM32 PR: Improvement T: HAL & APIs Topic related to the HAL and internal APIs. PR: General Cleanup C: Build / Toolchain and removed T: HAL & APIs Topic related to the HAL and internal APIs. labels May 10, 2020
@sjasonsmith
Copy link
Contributor Author

If they are still completed in the same amount of time as the existing brood, then I don't mind keeping them all. I think it would be fine to merge them all, and only if they become problematic disable a few.

All the new ones are just simple sanity checks that build quite quickly. They were pretty useful for making sure the variants for each platform weren't broken.

@thinkyhead
Copy link
Member

Before this is merged, what's the latest with the ststm32 platform / framework version and SoftwareSerial?

@MarlinFirmware MarlinFirmware deleted a comment from randellhodges May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from randellhodges May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from sjasonsmith May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from sjasonsmith May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from sjasonsmith May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from sjasonsmith May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from sjasonsmith May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from GhostlyCrowd May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from sjasonsmith May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from thisiskeithb May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from viper93458 May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from sjasonsmith May 10, 2020
@MarlinFirmware MarlinFirmware deleted a comment from sjasonsmith May 10, 2020
@thinkyhead thinkyhead merged commit ba9a9bb into MarlinFirmware:bugfix-2.0.x May 10, 2020
@sjasonsmith
Copy link
Contributor Author

Before this is merged, what's the latest with the ststm32 platform / framework version and SoftwareSerial?

@thinkyhead, I'm glad you asked, I was about to ask you how you want to proceed on this.

The latest platform (1.8) still has some timer issues. I have it working, but it requires a small increase of code in timers.cpp and we cannot yet eliminate SoftwareSerial.

One issue is already fixed post-1.8, and I've posted a PR for the other. Once we have a new framework release with those changes, we can delete our SoftwareSerial implementation and some workarounds from timers.cpp. I've already tested this combination and verified the timers work.

We have two choices right now.

  1. Leave it how it is using 1.7
    I suspect this is probably broken for the Malyan M300, since it uses the latest framework and our timers don't currently work with that.

  2. Upgrade to 1.8 and add minor timer workarounds
    I don't know what else might have been fixed in 1.8. The only benefit I'm aware of at the moment is that it supports the Malyan M300.

If you'd like to upgrade now I can easily put together a PR for that, so that people could start testing it. It is possible there could be issues with PWM or other features I didn't notice.

These are the two framework changes we need to be able to eliminate our SoftwareSerial:
stm32duino/Arduino_Core_STM32#849
stm32duino/Arduino_Core_STM32#1062

@thinkyhead
Copy link
Member

Yes, please, there's no time like the present. And nothing makes me happier than getting rid of HAL code. I especially want to get past these obscure and hard to track timer issues and have full control over them.

To that end, it would help to have more detailed commentary in the HALs to make clear why all the parameters and timer indexes and so on are set to their chosen values. The HAL has reached a point where it's a bit of an API now, so the choice remains whether to stick the HALs in a namespace or make a HAL class…

vgadreau pushed a commit to vgadreau/Marlin that referenced this pull request May 29, 2020
jmp0x0000 pushed a commit to jmp0x0000/Marlin that referenced this pull request Aug 7, 2020
njibhu pushed a commit to njibhu/Marlin that referenced this pull request Aug 24, 2020
@sjasonsmith sjasonsmith deleted the Improvements/PR/STM32_Normalize_Rebase branch September 22, 2020 02:06
HairingX pushed a commit to HairingX/Marlin that referenced this pull request Jun 16, 2021
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.

5 participants