search

ambrop72 / aprinter

3D printer firmware written in C++
Other
144 stars 42 forks source link

Added support for STM32F2 #33

Open svhelper opened 6 years ago

svhelper commented 6 years ago
ambrop72 commented 6 years ago

Hi,

Looks like most of the HAL code was copied from the F4 code and just minor adaptations were made. I'm not comfortable merging this because this means I need to maintain mostly duplicated code. I rather think the existing code should be modified to support F2 as well. It may have been a mistake to name the things "Stm32f4" in the first place but that is a minor concern.

By the way, do you indeed use the SD/SDIO code?

svhelper commented 6 years ago

Hi,

Yes, the HAL has been copied due to most of periphery has the same features. More over, the ST save compatibility between their controllers not only STM32, but all, include STM8 (with small difference in features, you even can have common HAL and switch between 32 and 8 bit MCU).

I was not sure about your preferences for making changes to your code by third-party developers, but now all clear - you should not merge this code, but simple use "Stm32" for full ST family line.

And yes - I use this SD/SDIO without any changes - the F2 and F4 has different cores Cortex M3 and M4, but the same periphery and their features (like DMA).

By the way, if you d'like to replase Stm32f4 with Stm32, I think you should also make simple adjustment of system clock - just now the core clock, bus clocks (periphery), PLL and so, made as board specific. That make impossible to change MCU not between family lines, but even with in single family line.

Can you also export from board script to the web interface (json file) at least the following:

If I can help you to improve this project, please touch me feel free.

PS: my the next steps - as additional storage, like SD, I going to add support of USB host (MSC over USB-FS, can provide speed up to 1 MBytes that should enough for most of projects). And then add TFT display over serial interface (initially SPI, then may be I will add parallel), but this is another story. But the current code related to SD (at least by name and/or state) instead of abstract "storage". And due to your concept - to do not make duplicates, I'm going to change names of related code.

Sorry for my English And best regargs, BOBAH

ambrop72 commented 6 years ago

Thanks for the explanation.

About the board scripts, they are a remnant from time time ago and ideally all would be configurable via the "board" section in the configurator. I'm closing this since I don't want to merge it as is. I may be able to incorporate F2 support based on this when I find time for this, together with improvements to make more things configurable in "board" settings and remove the "board scripts".

svhelper commented 6 years ago

Hi,

I apologize for returning to this issue. Unfortunately I cannot continue work with USB host, because it will be dependent on F2. And if I will continue work with my branch, merging of the USB host driver with your branch may be not so easy.

Is it OK, if I will try to make common STM32/GD32 drivers for already supported devices as separate configuration?

Best regards, BOBAH

ambrop72 commented 6 years ago

Sure that would be great if you try to merge this code. I am mostly concerned about the "real" code in the hal/ directory that I wrote. You can rename from Stm32F4Something to Stm32FSomething and adapt the generate.py script. Using preprocessor checks to see which CPU is used is fine. Don't forget that existing config files for F4 should keep working, so don't do the same thing for the config file structure.

svhelper commented 6 years ago

The existing config files for F4 is working for Cortex-M4, that has DSP instruction set and hardware floatpoints. The F2 has core Cortex-M3 (without DSP and FP), and it should have different build configuration.

So, not only sources F4 should be adoptated to support Cortex-M3 cores, but linker, board config and build script files. The Keil is supported the GD32 devices. I.e. at least it has correct declaration of registers for those devices, that has same names as STM32 for similar features. default As you should see, those MCU has interesting characteristics - they may be more preferred for devices with WIFI, Ethernet or TFT screen.

And if will be made very abstract configurator (at least for MCU core), the project may start support large number of MCU. Not only full line of STM32 but MCU that has STM32-like periphery (for example GD32).

I'm agree, this way is very hard, but interesting.

Any way, I will start from your recommendations - adopt F4 for full line. Then will wrote my own drivers for F2 in your style and checked on STM32F205RE and STM32F407VG.

svhelper commented 6 years ago

Hi, I made initial refactoring of STM32F4 to STM32-common If you have time, you may make simple review changes in my repository:

The next my steps - moving hard coded board declarations to the board section of configurator.

If you have some comments, please leave them.

br, BOBAH

svhelper commented 6 years ago

Hi, The second part of refactoring finished - moved hardcoded configs to the configurator. You can find changes in the commit https://github.com/svhelper/aprinter/commit/dee80e2b24156a52c96cf93ccd5e96b0ea08d563

I can create merge request for applying of those changes to your branch.

Now I can continue to refactor other low-level and middle layers drivers (like USBD and SDIO) to remove dependencies from Stm32CubeMX (for F4 its weight about 400MB)

br, BOBAH

ambrop72 commented 6 years ago

Hi, Thanks. I've reviewed your changes and generally it looks great.

I have the following comments/questions.

When the above issues are addressed I will do some sanity tests with my boards, if everything is fine I will then merge.

ambrop72 commented 6 years ago

Do you really need these changes in Preprocessor.h (I assume all changes are to support APRINTER_IF_DEFINED)? I would like to avoid unnecessary expansion of this mess :)

Please ignore this I later saw the use for GPIO.

svhelper commented 6 years ago

Hi, Thank you for detailed review.

Please find below my answers on your comments and questions.

  • What's the reason for the changes in Preprocessor_MacroMap.h (APRINTER_AS_NUM_MACRO_ARGS...)? Is some compiler processing it in a way that it doesn't work?

This macros should works on all GNU compilers (include clang). But it has small issue - it returns "1" when no arguments has been passed to it. This fix extends your macro, and make ability to detect zero arguments (as wrote in comments to this commit https://github.com/svhelper/aprinter/commit/9ced5b993e0a664cf812b610d92dcdf694faf555)

  • Do you really need these changes in Preprocessor.h (I assume all changes are to support APRINTER_IF_DEFINED)? I would like to avoid unnecessary expansion of this mess :)

Ignored ;)

  • Stm32UsbSerial: UsbCoreClock should not be size_t because it has nothing to do with object sizes. Maybe uint32_t (#include )? Make sure to fix both the type and the cast.

It used during check at compilation time (in static_assert), so it should not be a problem. But you right - in static_assert was compared unsigned (size_t) with signed (48000000) sizeless ints. And it should be fixed.

  • What's the deal with aprinter/platform/stm32/startup_stm32.? I see previously different statup files from Cube were used based on the chip. How is the same code generally suitable now, and why does it have to be bundled? From a license perspective it seems OK though.

If you compare all startup files for F2 and F4, you may observe that main differense:

So, this single startup file may be used for both of lines - F2 and F4

  • aprinter/platform/stm32/stm32f4xx_hal_conf.h - is this relevant only for F4 or did you forget to rename it?

The current state of branch - using of F4 HAL as reference, because switching between HALs required large number of changes. As I wrote above (https://github.com/ambrop72/aprinter/pull/33#issuecomment-419715395) I'm going to remove dependency from HAL (as you already do in GPIO and partially in SDIO) and use СMSIS directly. More over - I'm going to use common CMSIS for all STM32, that named "legacy". So, in the future this file will be removed.

  • What is the reason for the changes in usbd_desc.c (serial number, device class, subclass)?

This is very interesting change - very simple, but very difficult. The initial declaration of USB descriptor for F4 creates composite usb-device (there defined virtual com port only, but may be extended by adding some features like U-disk). But this driver works OK on Win10, where is not required some additional drivers for virtual comports. And on previous Win OS, it make some issues:

So, in this fix, I simple rollback the driver type (from "composite usb-device" to "virtual comport"), beacuse it make compatibility with previous Win OS. Looks like in the near future you are not going to add access to sd-card via USB (U-disk/mass storage etc), so this fix should be OK.

  • boards.nix: For F205 support why do you define -DSTM32F405xx? This is really compatible?

For modules, that used in APrinter, it's OK. If you checks using of this definition, you can find ADC module only. But there in good way should be checked pinout (MCU package). So, this definition has not principal meaning (especially if somebody going to use for example F207 without any changes). And as I wrote above, after switching to Legacy CMSIS, this may be removed or changed.

  • boards.nix: You changed PLL_N_VALUE and PLL_M_VALUE to be calculated based on FCPU_VALUE from the configuration file - why? Isn't it more flexible to specify these two in the configuration file? I cannot see how your formulas could always give the best solution, so I would rather not implement such automatic configuration and have the developer directly specify the PLL configuration.

You right, it will be more flexible. But my opinion - the most of users for APrinter (3D printer, CNC, engravers and so) d'like to get the highest performance for existing MCU (and sometimes may overclock the MCU) :) So, they will define the highest core clock frequency for his MCU. And my formulas are do it only. But the max core clock may not be compatible with some modules, for example USB and SDIO. And I added at compilation time a check for those issues (at least for USB, remember UsbCoreClock issue).

  • boards.nix: I see no remaining use of STM_CHIP so these definitions should be removed.

You are right - the last commit removes necessary in it. This is my omission.

  • Please fix the include guard name in platform/stm32/stm32_support.h.

This is my omission also.

When the above issues are addressed I will do some sanity tests with my boards, if everything is fine I will then merge.

This merge request refs to the "master" branch, but my changes created in "STM32 common". So, you should not reopen this merge request. I can open new merge reques for "STM32 common" branch, there we can continue discussion. And this is also my omission.

I hope, my answers is clear for you (due to my poor English, where Google translate may give me odds)

br, BOBAH

svhelper commented 6 years ago
  • What's the reason for the changes in Preprocessor_MacroMap.h (APRINTER_AS_NUM_MACRO_ARGS...)? Is some compiler processing it in a way that it doesn't work?

I double checked my changes and I became very surprised - with gcc 7.3 (i.e. on aprinter environment) this fix does not worked. The feature with deleting of comma , ## __VA_ARGS__ does not worked

svhelper commented 6 years ago

Hi,

Sorry for ask there. I try to refactor Stm32Sdio.h and collided to strange using of call NVIC_ClearPendingIRQ(SDIO_IRQn). What are you try to do there?

Another question - what reason to not use the DMA with the SDIO module?

br, BOBAH

ambrop72 commented 6 years ago

Thanks for the informative answers. Yes please open a pull request with what you think I should merge, and I will give it a bit of sanity testing, to check that platforms I have still compile and work.

I try to refactor Stm32Sdio.h and collided to strange using of call NVIC_ClearPendingIRQ(SDIO_IRQn). What are you try to do there?

Please check the long comment just below the declaration of buffer_diff. I hope that answers the question. Basically it's due to a theoretical concern that when the FIFO is filled while the interrupt is emptying it, we get a spurious second interrupt that actually will not be able to transfer data.

Another question - what reason to not use the DMA with the SDIO module?

Basically, because the hardware is broken. We need to be able to do a multi-block transfer where the blocks are not contiguous in RAM (the architecture of the FAT driver (BlockCache) requires this). The manual says that when DMA is used with SDIO, hardware handshaking between the DMA and SDIO should be enabled so that the DMA automatically knows when the data ends. This implies that the entire transfer of possibly multiple blocks is to a contiguous range of RAM, which is not what we need. I had tried to disable hardware handshaking and write the code such that when DMA transfers one block, I get an interrupt and the interrupt starts DMA for the next block (all while the SDIO is still active). But it did not work (don't remember exactly what happens, I think it failed to read the last few bytes?). Presumably the hardware was never tested in that configuration.

Now, it is possible to disable multi-block transfers, then DMA can work. Before I implemented multi-block transfers, this driver did use DMA. Here is the commit where I implemented multi-block and changed the driver to not use DMA: https://github.com/ambrop72/aprinter/commit/25ba98f045f760494b10fc9467eaf5f742573236

If you want you can change the driver to do DMA; if you do this you will have to change the MaxIoBlocks (near the top) to 1 (the FAT driver will then know not to do multi-block transfers).

svhelper commented 6 years ago

Thank you for prompt answer.

I will try to understand implementation of SDIO not only as sd-card module, but with your implementation FAT and main logic of aprinter. Also I will check your previous DMA implementation. According your description, at first glance it seems, that there will be suitable the Double Buffer mode of DMA stream.

After all I will open new merge request.

br, BOBAH