Update internal CMSIS and HAL #582
Conversation
|
Hi @stellar-aria! Thanks for the massive contribution. This may take a little to look over and test, but most things look okay at a glance. Regarding the OPEN_DRAIN/OD swap: #define GPIO_MODE_OUTPUT_OD (MODE_OUTPUT | OUTPUT_OD) Seems like the offending line you mentioned. I'm not sure this is is an actual naming conflict with the Re. Code size -- this has been creeping up over the past several versions: Long term, there are a few things we can do (convert HAL stuff to LL drivers, reduce const HAL-mapping tables in peripheral APIs, etc. |
| #define OUTPUT_TYPE_Pos 4u | ||
| #define OUTPUT_TYPE (0x1uL << OUTPUT_TYPE_Pos) | ||
| #define OUTPUT_PP (0x0uL << OUTPUT_TYPE_Pos) | ||
| #define OUTPUT_OD (0x1uL << OUTPUT_TYPE_Pos) |
There was a problem hiding this comment.
This is the location of the offending macro that causes name-collision
There was a problem hiding this comment.
Since macros are basically just global find-and-replace text replacement, the preprocessor is inserting it into the enum regardless of the code (and scoping) around it :/
There was a problem hiding this comment.
Ah, yup! You're totally right. I forgot that macros just do a brute-force text replace everywhere. This is totally fine.
Thanks for the added detail.
|
Hi! Thanks for the feedback, I've added a comment to the line number of the file which causes the name-collision. As you said, readability is also a concern, if just because it's better to be explicit for users, however if this wasn't a critical compilation failure I'd be fine just leaving it as it was. Yeah, I don't really think there's a quick easy fix for the HAL's size creep, especially if the Daisy platform adopts other CPUs in the future (for say, a Seed 2 or 3) and doesn't want to take on the task of writing custom platform-specific adapters between the higher-level libDaisy classes and the processor-specific LL stuff. It's all tradeoffs, and the HAL is overall just more convenient. |
|
Thanks for the update. We'll keep you posted if there's anything that needs changing. |
|
After giving it a lot of thought, the DSP subfolder of the CMSIS driver should probably also be swapped out for a submodule and pinned to a release tag now that it's been spun off into its own project. It'll also keep the code turnover in this PR down once the commits are squashed and merged. |
|
This will also make #578 unnecessary, as the DSP library will be up-to-date and now includes arm_clip_f32 (at Include/dsp/basic_math_functions.h) |
|
@stellar-aria thanks for the update! I like the conversion of the DSP to a submodule. Most of those edits can be found here However, I think there may have been one or two other ones. I'll try to track down any details on other changes, that we've made, but I figured I'd mention them here so that we can have them in mind as we go through integration, and regression testing. Now that we've signed off on the changes in #581 we'll start doing some testing on this. I can update here as we progress, and possibly provide a little table or checklist of proposed/completed tests as we go. |
|
Yeah, I actually re-incorporated those noted edits into the updated version of the HAL driver files. If there's problems with the SDMMC DMA stuff, it'll be because I didn't see any notes and it got undone 😅. I'll do a diff of the driver pre-and-post and check what's going on in a few minutes. In terms of switching to submodules, we can probably at least go ahead with the CMSIS driver as well then, since the preliminary commit here was just based on the source at the latest release tag. |
|
@stephenhensley
diff -ru stm32h7xx_hal_driver/Src/stm32h7xx_hal.c libDaisy/Drivers/STM32H7xx_HAL_Driver/Src/stm32h7xx_hal.c
--- stm32h7xx_hal_driver/Src/stm32h7xx_hal.c 2023-06-12 14:13:05.245624400 -0400
+++ libDaisy/Drivers/STM32H7xx_HAL_Driver/Src/stm32h7xx_hal.c 2023-06-12 13:17:58.057026300 -0400
@@ -187,8 +187,8 @@
HAL_StatusTypeDef HAL_DeInit(void)
{
/* Reset of all peripherals */
- __HAL_RCC_AHB3_FORCE_RESET();
- __HAL_RCC_AHB3_RELEASE_RESET();
+ //__HAL_RCC_AHB3_FORCE_RESET();
+ //__HAL_RCC_AHB3_RELEASE_RESET();
__HAL_RCC_AHB1_FORCE_RESET();
__HAL_RCC_AHB1_RELEASE_RESET();However, the only place that HAL_DeInit() was called was in
diff -ru stm32h7xx_hal_driver/Src/stm32h7xx_ll_spi.c libDaisy/Drivers/STM32H7xx_HAL_Driver/Src/stm32h7xx_ll_spi.c
--- stm32h7xx_hal_driver/Src/stm32h7xx_ll_spi.c 2023-06-12 14:12:53.654625100 -0400
+++ libDaisy/Drivers/STM32H7xx_HAL_Driver/Src/stm32h7xx_ll_spi.c 2023-06-12 13:17:59.039027000 -0400
@@ -24,7 +24,9 @@
#ifdef USE_FULL_ASSERT
#include "stm32_assert.h"
#else
+#ifndef assert_param
#define assert_param(expr) ((void)0U)
+#endif
#endif /* USE_FULL_ASSERT */
/** @addtogroup STM32H7xx_LL_Driver
uint32_t count = 0U;
do {
if (++count > 200000U) {
return TIMEOUT
}
} while (condition == 0)
In the short-term, what would you like me to do about this? |
|
Okay, after some thinking at lunch, the easiest thing to do would probably be to swap to using a submodule, but retain those singular patched source files in-tree until the changes are merged upstream, at which point the build managers can just be pointed back to the originals. This is what I'm going to work on for now, unless something changes? Alternatively, the daisy project could fork the HAL, introduce the required bugfixes, and then submit a PR or just keep the separate fork and then have that be the submodule. |
|
@stellar-aria thanks for being super thorough and digging through the changes to the previous HAL files. I'll have a chance to go through and make a bit of a test plan later this week. Ideally we'll be doing some actual integration testing on our end (both to confirm there aren't any regressions, and to see if the HAL update resolved some of the patches we needed to do in the past). Once I've had a chance to dig in a bit more, I'll respond more specifically to some of your comments above wrt specific changes, submodule-ification of the HAL, etc. |
stellar-aria
force-pushed
the
feature/driver_and_hal_update
branch
from
c9f131b to
a6ece3f
Compare
stellar-aria
force-pushed
the
feature/driver_and_hal_update
branch
from
a6ece3f to
0366831
Compare
|
Another potential source of regressions is the Middleware drivers now being out of date, but unless they do cause problems, upgrading them is probably best left for a future PR. So I'm just noting some things here for the future: The USB Device Middleware can probably be upgraded (and swapped to a submodule) without any issues. However there's some notes on the Host library that dynamic allocation was disabled for one of the internal Handle types, and then later notes that USBH_Free was also disabled, and the main barrier to using the Middlewares as-designed (with malloc and free) was that the heap is in DTCMRAM. As far as I can tell the only memory configuration with the heap located there is the SRAM one, but I also don't think I understand the architecture well enough yet to understand why it was placed there initially. Host is also the one I'm most interested in upgrading eventually, since there's now in-built components for USB Host Audio and CDC classes. |
|
Hey just wondering where everything's at, I figure testing this is a real pain. I see there's some merge conflicts from upstream, should I go ahead and resolve those or will that impact the build that's being used? |
|
You can resolve the conflicts, that shouldn't cause any issues with testing. I'm hoping to get dig into it quite a bit deeper this week, and do a few updates on some older, but more complex programs that use USB, SDMMC, etc. and do some side-by-side comparison of some of the non-trivial changes. If nothing breaks/doesn't stand out, then I think we might be able to merge this this week 😄 |
|
Since I Introduced this PR over six months ago, I've learned a lot more about the state of embedded development toolchains and build systems, primarily from my work managing such for the Deluge Community Firmware project. In that time, my overall outlook w.r.t. the management of dependencies (such as the CMSIS ones here) and build systems in the Daisy project has changed, and it boils down to a few key points:
In the long run migrating the current build system to more idiomatic CMake is probably a good path forward, and will reduce a lot of issues with other kinds of integrations such as alternate toolchains (I've had a Clang-support PR waiting in the wings for a while now). However in the short term I think submodules are an acceptable stopgap solution for the dependency version problem, even if it requires an additional VCS step in the form of I also kind of just want to see this PR merged so I can work on others, haha 😅 |
stellar-aria
force-pushed
the
feature/driver_and_hal_update
branch
from
194e0be to
7367232
Compare
|
Happy New Year! We are doing some internal testing on this, and that should progress fairly quickly. We are aiming to have either approval, or in the case of finding any issues, feedback by the end of next week! Regarding your comments on CMake, I've converted that into and issue (#606) so that we could discuss it separately since it is a worthwhile discussion, but a bit outside the scope of this PR. |
|
Just checking in on this! |
|
@stellar-aria alright! @beserge and I have both gone through quite a bit of testing at this point. USB DeviceI am unable to use the internal USB port (I haven't tested the external yet) with the updates. I have a feeling this could be an incompatibility in our class interface with at least one of the changes that have happened over the years. Did you run into this on your end with either of the device classes? USB HostSimilar to the above, I setup some test code that worked on the current master branch that no longer works correctly on the new branch. Again, I would guess this is an incompatibility with the latest changes (interestingly it looks like the USB Host middleware has been left the same. So it might be on the HAL side that's causing this particular issue. SDRAM/FMCSo I haven't personally gotten any abnormal behavior yet, but Ben mentioned having an example not start up. Afterwards it worked okay. I haven't done any strict profiling to see if there's been any unexpected change in performance yet either. I'm going to dig into the USB stuff a little bit today/tomorrow to see if I can find the particular issue(s) remaining there. If you have any insight from testing you've done so far that would be welcome! Aside from getting USB up and running, I'm thinking the rest of this is ready to go. |
|
I haven't played much with the USB stuff yet, tbqh. Part of the incompatibility might just come from using the older middleware libraries with the newer HAL, as I noted earlier:
So it might just be worth it to upgrade the device library and see if that fixes the device problems. If it does, we might have to upgrade/merge the host library manually. It doesn't look like the heap is stored in the DTCMRAM anymore, rather in SRAM for the QSPI and Flash loaders, and the D2 RAM for the SRAM loader, so does that note need to be edited/fixed? I haven't done any strict profiling for the SDRAM, if you need something like that I do have some profiling tools. Was this code execution from the SDRAM or just data storage there? |
|
Okay! I have a feeling you're correct, I just wanted to confirm if you had either experienced the same thing, or had a different experience before we went super far down the rabbit hole. As far as I can tell, the USB device/host issues are the only things left to resolve, and then I think this is good to go. I don't think there's much to do wrt to the SDRAM, the performance seemed pretty similar to me. The only red flag was the report of an app not starting, but that could have been something unrelated as it has not been reproduced since. I didn't do any quantitative profiling, but I have a program that just barely underruns when copying the input around the SDRAM a ton of times, and then to the output, and it was audibly the same when running it between both the master branch, and this PR. |
|
@stellar-aria Just wanted to keep you up to date as we progress. We've made some decent headway in identifying the issues with USB, and are hoping to have a patch for getting USB device stuff working before the end of the week. |
|
FYI I have been working on MIDI host USB. It works on first boot but
unplug/plug cycles are unstable. I am continuing to debug this issue. I can
point you at my git repo if you are interested.
…On Thu, Feb 15, 2024, 1:09 PM Stephen Hensley ***@***.***> wrote:
@stellar-aria <https://github.com/stellar-aria> Just wanted to keep you
up to date as we progress.
We've made some decent headway in identifying the issues with USB, and are
hoping to have a patch for getting USB device stuff working before the end
of the week.
If that's the case, we should have a decent idea of how to get USB host
working again as well, and I'm hoping to have that knocked out in the first
half of next week. So if that timeline works out, we can potentially be
doing final testing, and be ready to merge this by the end of next week.
—
Reply to this email directly, view it on GitHub
<#582 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACESB6YVGXDTI2FPC4F25ETYTZ2Q3AVCNFSM6AAAAAAYJXMWKKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBXGM2DGMBRHA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
@GregBurns Awesome! Please do. I'm going to look into this myself a bit now. I've just pushed my patches to the USB Device Middleware required to use both USB Midi and CDC. |
|
Ben,
This is my fork
https://github.com/GregBurns/libDaisy
Greg
…On Fri, Feb 16, 2024, 6:59 AM Ben Sergentanis ***@***.***> wrote:
@GregBurns <https://github.com/GregBurns> Awesome! Please do. I'm going
to look into this myself a bit now.
I've just pushed my patches to the USB Device Middleware required to use
both USB Midi and CDC.
As part of that I updated the ref for the HAL Driver.
—
Reply to this email directly, view it on GitHub
<#582 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACESB64YJ4CN2L4MULABDTLYT5X6RAVCNFSM6AAAAAAYJXMWKKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBYGUZTSNRSGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Ok, so I was finally able to successfully test USB Host and get it working. It was a combination of issues on my end.
Inexplicably the app still was able to be debugged, and the issues didn't present as if they were bootloader related, not really sure why yet. Couple of things I want to do:
That being said, this PR is finally tested, and ready to be merged! Thanks for your hard work, glad we're finally able to give it the signoff! 🚀 |
|
Ready for merge! Thanks once again for your stellar contribution! |
This upgrades the built-in CMSIS library to v5.9.0, the CMSIS H7 Device to v1.10.3, and the STM32 HAL to v1.11.1.
This addresses issue #538.
This change has been tested with DaisyExamples and a Daisy Patch, and tested against the included test suite.
Additions:
The new HAL modules have been added to
src/sys/stm32h7xx_hal_conf.h, and have been enabled to match the others already present. These include modules such as the Digital Temperature Sensor, a Filter Math Accelerator (aka Fixed-point Multiplier and Accumulator or FMAC), an Octo-SPI controller (OSPI), a Digital Filter for Sigma-Delta modulation, and of course, the CORDIC co-processor block, which can accelerate trigonometric functions like sine, cosine, arctan, and square root for q1.31 and q1.15 fixed point signed numbers.Makefiles have been updated to reflect the source code structure changes, including
Makefile,core/Makefile,CMakeLists.txt, and thelibdaisy.vcxproj[.filters]files.Major Breaking Changes:
GPIO::Mode::OUTPUT_ODhas been renamed toGPIO::Mode::OPEN_DRAINdue to a name conflict collision with a macro now defined in stm32h7xx_hal_gpio.h (the primary HAL file for gpio). This is instead of a kludge like#undefing the macro, as it may crop up elsewhere depending on include order.Documentation has been updated accordingly.
Minor Breaking Changes:
Compiled code size has increased by up to 7%. This means that the
patch/Nimbusexample in DaisyExamples will not compile any longer and may need to be reworked to be stored somewhere other than FLASH.Minor Fixes:
A very minor (single-character) bugfix was done in
CpuLoadMeter_gtest.cppso that a number would no longer overflow when bitshifting left, and so that I could actually compile and run the tests.The relevant external repos:
https://github.com/ARM-software/CMSIS_5
https://github.com/STMicroelectronics/cmsis_device_h7
https://github.com/STMicroelectronics/stm32h7xx_hal_driver