upgrade STM32CubeF4 to v1.13.0 #2408
Conversation
|
Thanks for this. I'm a bit concerned about the increase in the BSS by 100 bytes. Do you know what it is from? |
|
We can check that. --- before.txt 2016-09-14 12:14:27.901667159 +0200
+++ after.txt 2016-09-14 12:14:52.985549774 +0200
@@ -1,14 +1,14 @@
00000092 b CDC_ClassData
00000004 b CDC_fops
00000001 b cdc_iface_num
-00000001 b cfgidx.9898
+00000001 b cfgidx.10035
00000024 b DAC_Handle
00000001 b dev_is_connected
00000004 b dma_enable_mask
00000064 b dma_handle
00000002 B dma_idle
00000016 b dma_last_sub_instance
-00000001 b enabled.11367
+00000001 b enabled.11504
00000016 b FatFs
00000001 B ff_CurrVol
00000004 b flash_cache_sector_id
@@ -26,15 +26,15 @@
00000001 b hid_in_ep
00000004 b hid_report_desc
00000548 B hUSBDDevice
-00000064 B I2CHandle1
-00000064 B I2CHandle2
+00000092 B I2CHandle1
+00000092 B I2CHandle2
00000001 b led_pwm_state
00000256 b lfn
00000512 b LfnBuf
00000708 B mp_state_ctx
00002156 b MSC_BOT_ClassData
00000004 b MSC_fops
-00000948 B pcd_fs_handle
+00000956 B pcd_fs_handle
00000004 B pendsv_object
00000032 B pFlash
00000001 b pin_class_debug
@@ -46,15 +46,15 @@
00000001 b repl_display_debugging_info
00000004 b reset_cause
00000028 b rl
-00000012 b RNGHandle
+00000016 b RNGHandle
00000036 B RTCHandle
00000004 b rtc_info
00000001 b rtc_need_init_finalise
00000004 b rtc_startup_tick
00000001 b sdcard_started
00000096 b sd_handle
-00000084 b sd_rx_dma
-00000084 b sd_tx_dma
+00000100 b sd_rx_dma
+00000100 b sd_tx_dma
00000092 B SPIHandle1
00000092 B SPIHandle2
00000064 B TIM5_HandleSo we have:
|
|
A few more points/quetions:
|
So maybe it would be a good idea to remove all F2 related files before merging this PR and add it fresh when/if they will be needed? Leaving them may result in more compatibility issues in future.
Ok, but then we have to separate files from For generic CMSIS files inside lib/ we could consider using a submodule of ARM's official repo: ARM-software/CMSIS
IMHO they made it represent hardware more literally, since technically there is one 32 bit register called BSRR. So to conclude, before merge we need to:
|
|
Creating submodule from ARM-software/CMSIS is actually a bad idea, after pull it is over 356 MB! |
Yes please, no submodule :)
Yes, move up one dir to simply stmhal/cmsis
Yes. |
03af904 to
626ad8d
Compare
|
Is there a timeline to merge this? It enables adding support for the new F412 family, which I'm currently trying to do. |
|
You can use my branch to add F412, it would be a good verification for this PR since I've tested it only on pyboard (F405RG) and something could slip through automated tests. |
|
I'm trying to do exactly that. I got it compiling, but not working yet. See here. |
|
@blazewicz is this ready to merge? Now that v1.8.5 is tagged we can proceed with this. |
3994a9e to
a92bb90
Compare
|
Branch is based on current master, all CI checks pass, all tests on pyboard pass. I think we are ready to merge it in. |
|
Hello, I noticed that there is a recent patch that upgrades to Cube HAL 1.13.1 which fixes in particular some issues with I2C peripheral:
would it be worth to integrate 1.13.1 directly so we are aligned to the very latest version ? David |
|
@siorpaes was that version already released? The latest package from http://www.st.com/en/embedded-software/stm32cubef4.html is still 1.13.0 with latest changes in HAL from 01-July-2016. I think its a good idea to use latest possible version if we make that upgrade. |
|
Yes, it has already been released but in form of a patch that goes on top of 1.13.0. |
|
It would be good to use v1.13.1 with the extra patch. |
|
I have merged the patch into |
|
Would it be possible to include the following files, even though they are not used currently in Micropython? I am using these in a project of mine and I guess other developers could benefit from inclusion of these, but I understand if you don't want to include unused files. |
@blazewicz how is this going? |
|
I've been pretty busy with work lately but I'll continue to work on this
after the weekend.
|
a92bb90 to
7b5679c
Compare
|
@blazewicz I see you've made some new commits... is it ready to merge? Also, is it important to keep the upgrade v1.13.0 separate to v1.13.1, ie not squash them into a single upgrate? |
Yes, I've rebased my branch on top of the upstream master.
Actually after c4e58ea all pyboard tests pass, this is because the problem lays between I2C and DMA, and test
You mean commits below? The v1.13.1 is a patch release and it only changes the HAL driver ( I've kept those commits separate having in mind note from
|
My 2 cents: yes, it would be be important to keep them separate, to debug (bisect) any later issues. (And to just avoid "vendor fork == mess" syndrome). I'm not sure if @blazewicz was quick to squash something indeed, but what I see now is:
I.e, on Sep 6, it was upgraded to 1.13.1, then on Oct 12, "upgraded" to 1.13.0. There's apparently a mistake in the commit message, as it looks like a downgrade. |
7b5679c to
13e9328
Compare
|
The CMSIS/Device part was not affected by |
If you want to keep v1.13.0 and v1.13.1 separate we could rebase/split commits like this: |
|
@blazewicz : You might notice that the root of my confusion was that commits were apparently in reverse order to what logically could be expected - both in linear order and by dates. If that was indeed true, then swapping commit order would be the way to address it. Anyway, I don't call for any changes myself, I'm not stm32 maintainer. We just had an interesting discussion in some other ticket on how easy would it be supporting this or that vendor-specific feature for an arbitrary MCU, so I took a chance to peek at how easy this goes for stm32 which is already well supported. I don't find associated maintenance easy at all. Anyway, thanks for calling for these changes (and I hope they won't break much indeed ;-) ). |
Yes, you are right, seeing first @dpgeorge what do you think about the proposition form my previous comment? Then you could merge this PR as it is right now (most of functionality is working fine, and some people are waiting for it), and move the I2C/DMA problem to its own issue - the breaking change could be easily extracted/reverted. the complete branch log would look like this: In this state tests break only with this change: diff --git a/tests/pyb/i2c.py b/tests/pyb/i2c.py
index a220f8e..0b3ba93 100644
--- a/tests/pyb/i2c.py
+++ b/tests/pyb/i2c.py
@@ -11,7 +11,7 @@ for bus in (-1, 0, 1, 2, 3, "X", "Y", "Z"):
i2c = I2C(1)
-i2c.init(I2C.MASTER, baudrate=400000)
+i2c.init(I2C.MASTER, baudrate=400000, dma=True)
print(i2c.scan())
i2c.deinit() |
I agree that we can put the I2C DMA problem in an issue of its own later, and that what you've done so far is good for merging. I've had other reports that the latest HAL actually improves the I2C driver in some repects so there will be some test to do to see exactly what the issues are. |
|
Ok, and do you want me to make 1.13.1 patch a separate commit? As @pfalcon said, it would facilitate debugging. |
Yes, doing it that way looks good. If you don't mind reorganising it like that and then I can merge, thanks! |
changes include: * use single GPIO.BSRR instead of BSRRH and BSRRL * change HSE_STARTUP_TIMEOUT to 100 ms * define LSE_STARTUP_TIMEOUT to 5 s
CMSIS v2.5.0 removed all uint32_t casts and uses only Misra Cast (U)
This is required by HAL Driver for error handling since v1.5.0
Current version of HAL drivers optimize IRQ handler by using precalculated DMA register address and stream bitshift instead of calculating it on every interrupt. Since we skip call to `HAL_DMA_Init` on reused DMA, fields StreamBaseAddress and StreamIndex of DMA handle are not initialized and thus leads to SegFault in `DMA_IRQHandler`. HAL_DMA_Init is a big routine and we do not need to call it on each use of DMA (ex.: series of I2C operations) and DMA_CalcBaseAndBitshift is really small and releasing it increases code size by only 8 bytes.
Current version of HAL drivers checks if `hdma->State == HAL_DMA_STATE_READY` before executing some functions.
HAL Driver before v1.4.2 had a bug which caused clearing all pending flags in MSR, TSR, RF0R and RF1R instead of only the requested one. This is why micropython got away without explicitly clearing flags in IRQ handler.
Use custom handlers providing minimal required functionality because those provided by ST increase code size by almost 2 KiB.
stdio.h was included in all HAL files only to provide definition of NULL symbol "stdio.h" includes "types.h" which contains some conflicting definitions with "drivers/cc3000/inc/socket.h"
13e9328 to
4d9dce7
Compare
|
rebased & reorganized gitlog: |
|
Merged, see 4d9dce7 for last commit. A very big thanks @blazewicz! Thanks for great commit messages and splitting the commits as appropriate. There are 2 bugs (so far...):
|
This is the newest version of STM32CubeF4 from ST.
There is a lot of changes since previous HAL version was over 2 years older.
Key benefits:
For complete reference of STM32CubeF4 changes please see
Release_Notes.htmlprovided by ST in Cube package, you can download it from here.Tested on pyboard (PYBV10), all tests pass, SD card works, I didn't test anything beyond that. It doesn't break build for any other MCU series.
Executable size increased very slightly, exact numbers can be found below:
before:
after:
Any volunteers to test this PR are welcome.