-
-
Notifications
You must be signed in to change notification settings - Fork 895
Arduino_core_STM32 support (improved) #1571
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
base: development
Are you sure you want to change the base?
Conversation
|
@dirkju Thank you for this PR. Please see my review comments that need some attention. |
tekka007
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dirkju, please see my comments. I believe it makes sense to merge the (old/legacy) STM32F1 implementation to have one unified STM32 - if possible. What do you think?
hal/architecture/STM32/MyHwSTM32.cpp
Outdated
| #ifdef IWDG | ||
| // Reset independent watchdog if enabled | ||
| // Note: Watchdog must be configured separately in sketch if needed | ||
| #if defined(HAL_IWDG_MODULE_ENABLED) | ||
| // Using STM32 HAL | ||
| // Implementation depends on whether user has initialized IWDG | ||
| // For safety, we only reset if it's running | ||
| #endif | ||
| #endif | ||
| // No-op if watchdog not enabled - safer default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here some code is missing to actually reset the wdt if enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added.
| // Read internal voltage reference to calculate VDD | ||
| // VREFINT is typically 1.2V (varies by STM32 family) | ||
|
|
||
| uint32_t vrefint = analogRead(AVREF); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AVREF is accessed without checking the VREF_AVAILABLE flag, see above in hwRandomNumberInit()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added condition
| // Read internal temperature sensor | ||
| // Note: Requires calibration values for accurate results | ||
|
|
||
| int32_t temp_raw = analogRead(ATEMP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ATEMP is accessed without checking the TEMP_SENSOR_AVAILABLE flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added condition
| * STM32duino core provides main() function and serialEvent() handlers. | ||
| * No additional implementation needed - the framework handles setup()/loop() calls. | ||
| */ | ||
|
|
||
| // Note: STM32duino core already provides weak serialEvent handlers | ||
| // We don't need to redefine them here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hook to _begin() and _process() is missing, see other HAL implementation for reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added.
@tekka007 Completely agree, this is my goal. I was not sure though, if we should do that in a single step. Wdyt - should we try to get out the 2.4.0 release with both STM32 and /F1 and thereafter produce a 2.5 with STM32F1 removed? |
Adding Arduino_core_STM32 support
Three previous attempts of adding STM32 support were abandoned (#1422, #1437, #1486).
@tekka007 s branch also served as inspiration. Thanks to everyone of you.
I'm asking you to please consider my attempt :-) @tekka007 @mfalkvidd
Risks
The change is rather local in hal/STM32 and should not cause problems with other architectures.
I've kept the existing legacy hal/STM32F1 unchanged and the respective constant is checked in MySensors.h before my STM32 addition. This should maximize backward-compatibility for STM32F1 (blue pill) legacy users.
Implemented in this version
Planned
Build
I developed under VS Code + Platformio on Windows. I test-compiled examples under the Arduino IDE (also Windows).
Testing
I've tested mostly on a generic black pill STM32F401CE (gateway, node using Ra-01 LoRa and nRF24L01+).
I encourage to test on a broader variety of boards.
Final note
The code has mostly been generated using AI. I asked for a comprehensive review of the previously mentioned three abandoned STM32 PRs and a comparison between building on STM32duino vs the original STM32Cube HAL. Both are feasible but the simplicity of the STM32duino solution stood out.