Rework the STM32F7xx HAL as a generic STM32 HAL#36
Rework the STM32F7xx HAL as a generic STM32 HAL#36thinkyhead merged 4 commits intothinkyhead:bf2_STM32F7xx_HALfrom hasenbanck:hal-rework
Conversation
The offical arduino STM32 core is abstracting quite much for us already, so the only real device specific parts are for the timer configuration. Memory backtrace information should be handled on a motherboard basis, since STM32 chips are pretty configurable, as you can already see in the difference between the chips used in THE_BORG and REMRAM_V1. If a board wants to use this HAL, it needs a variant definition in the upstream STM32 core.
| #elif defined(STM32F7xx) | ||
| #define HAL_PLATFORM HAL_STM32F7xx | ||
| #elif defined(STM32F0xx) || defined(STM32F1xx) || defined(STM32F4xx) || defined(STM32F7xx) | ||
| #define HAL_PLATFORM HAL_STM32 |
There was a problem hiding this comment.
If this segment is moved up then the ! clauses above can be removed.
There was a problem hiding this comment.
Fixed. Btw: We could use the "ARDUINO_ARCH_STM32" symbol to further simplify things. But the STM32GENERIC core also defines that symbol and maybe this could lead to problems when debugging issues later on.
| */ | ||
|
|
||
| #ifdef STM32F7xx | ||
| #if defined STM32F0xx || defined STM32F1xx || defined STM32F4xx || defined STM32F7xx |
There was a problem hiding this comment.
By convention the defined directive should always include parentheses.
There was a problem hiding this comment.
Maybe "ARDUINO_ARCH_STM32" would bet better to use? See comment above.
There was a problem hiding this comment.
I agree. Whatever is most concise.
|
Looks very good, and much cleaner. I’ll just want to adjust some formatting before I merge it. |
| #endif | ||
|
|
||
| #ifdef STM32F7 | ||
| #if MB(THE_BORG) |
There was a problem hiding this comment.
There are symbols in the linker script for these that we can use for these if you want to remove these defs entirely.
There was a problem hiding this comment.
These definitions are indeed a pain point for me. I had a similar idea 1 1/2 month back (stm32duino/Arduino_Core_STM32#294) but couldn't find a way to pull out the information out of the linker script. Could you probably show how we could extract these symbols and include them in this file?
There was a problem hiding this comment.
Since the "MEMORY" symbols are in a different namespace and not accessible to a C program, we need to export these information somehow. Maybe:
SECTIONS
{
/* Export the memory areas */
__ram_start = ORIGIN(RAM);
__ram_end = ORIGIN(RAM)+ LENGTH(RAM);
__flash_start = ORIGIN(FLASH);
__flash_end = ORIGIN(FLASH)+ LENGTH(FLASH);
}#if defined(ARDUINO_ARCH_STM32)
// Pull out the needed information out of the linker script
extern int __ram_start;
extern int __ram_end;
extern int __flash_start;
extern int __flash_end;
#define START_SRAM_ADDR __ram_start
#define END_SRAM_ADDR __ram_end
#define START_FLASH_ADDR __flash_start
#define END_FLASH_ADDR __flash_end
#endifThere was a problem hiding this comment.
Here's how I access such a symbol in the STM32 code (using a ram vector table):
https://github.com/xC0000005/STM32GENERIC/blob/23aca4eddc122e178499e13d1c28c6392cd684b1/STM32/variants/MALYAN_M200_V2/variant.c#L9
There was a problem hiding this comment.
I don't think that the MEMORY entries are accessible that way. At least if I read the LD documentation correctly (https://sourceware.org/binutils/docs/ld/MEMORY.html)
The name is a name used in the linker script to refer to the region.
The region name has no meaning outside of the linker script.
Region names are stored in a separate name space, and will not
conflict with symbol names, file names, or section names.
| #include "watchdog_STM32.h" | ||
| #include <IWatchdog.h> | ||
|
|
||
| void watchdog_init() { |
There was a problem hiding this comment.
The watchdog code in ST's official core is problematic for boards with a bootloader running which starts it, but that will be a separate PR I'll tee up soon. This is fine for now.
There was a problem hiding this comment.
Good catch. Wouldn't it be better if we fix that in the core right away?
There was a problem hiding this comment.
Yes, but I'm not clear on whether the core is doing something wrong or not. The core's code works fine for the situation it was designed for, but it waits for the watchdog to enter a state it's never going to if it's already running (working from memory here), so re-init hangs if it's already running. My fix was to have support for an instance that doesn't get "initialized".
https://github.com/xC0000005/Marlin/blob/c1826d6d83cce049ca1a1cdab622516001d8cebf/Marlin/src/HAL/HAL_STM32Fx/watchdog.cpp#L31
There was a problem hiding this comment.
I guess we could just try to throw this issue into the guthub issue tracker and see what happens. Should be best if we could show which bootloader is affected.
|
Should I merge this now and we can carry on from here? |
|
@thinkyhead Let me push my "ARDUINO_ARCH_STM32" real fast. Then this PR is good to go. |
|
Ok, I'll hold off till after your new commit(s). |
|
Now it's good to go. |
3 participants
The official arduino STM32 core is abstracting quite much for us
already, so the only real device specific parts are for the timer
configuration.
Memory backtrace information should be handled on a motherboard
basis, since STM32 chips are pretty configurable, as you can already
see in the difference between the chips used in THE_BORG and REMRAM_V1.
If a board wants to use this HAL, it needs a variant definition
in the upstream STM32 core.