CANopenNode / CanOpenSTM32

CANopenNode on STM32 microcontrollers.
Other
281 stars 112 forks source link

STM32 Library overhaul #12

Closed HamedJafarzadeh closed 1 year ago

HamedJafarzadeh commented 2 years ago

I'm working on STM32 CANOpen stack for a while now and I'm porting it to different devices, during these times I learnt that the current folder structures are misleading and might be difficult to understand at first. Plus we had different STM32 Driver code for each individual microcontroller which wasn't ideal at all. hence I decided to overhaul the library and reorganize it in a way that would make it much more easier and universal. On top of these, I also included an video tutorial to facilitate the learning experience. In summary, following changes has been made :

Things to consider :

MaJerle commented 2 years ago

Main problem - for which I don't find real time - is to reorganize the projects to have one folder with drivers for any STM32. Currently it is all scattered all around the repository and this is ultra bad.

Generic STM32 driver is OK - but there is an abstraction needed between CAN and FDCAN.

Things to consider :

The universal driver has not been implemented and tested for RTOS

We need this test. Absolutely

MaJerle commented 2 years ago

Coding style is out of existing projects in stm32H7xx_xxxx examples. Please correct this. At least always curly brackets and 4-spaces for tab.

for instance

    if (htim == canopenNodeSTM32->timerHandle)
        canopen_app_interrupt();

shall be

    if (htim == canopenNodeSTM32->timerHandle) {
        canopen_app_interrupt();
    }
HamedJafarzadeh commented 2 years ago

Main problem - for which I don't find real time - is to reorganize the projects to have one folder with drivers for any STM32. Currently it is all scattered all around the repository and this is ultra bad.

So maybe I didn't explain good enough, but in the code I provided there is now a single "co_driver_stm32.c/.h" which serves for all STM32 without any change, it automatically detect if the device is using FDCAN, and use the FDCAN functions, otherwise it uses CAN functions. This is implemented which a bunch of #ifdef.

We need this test. Absolutely.

I included a example/legacy folder which includes source that you provided for RTOS application, and it compiles with no problems. I noted this in the readme file. Anyway, I might find time later to implement RTOS support in the universal driver.

Coding style is out of existing projects in stm32H7xx_xxxx examples. Please correct this. At least always curly brackets and 4-spaces for tab.

Sure, I will take care of that, if you have your coding style from eclipse, you can export it and send it over so I can use exact same version and I will include it in the repo for future developers.

MaJerle commented 2 years ago

not only that - problem is that several projects with same STM32 copy HAL/LL drivers inside - makes no sense

Here is one: https://github.com/MaJerle/c-code-style

MaJerle commented 2 years ago

I added .clang-format file to the repository

HamedJafarzadeh commented 2 years ago

Thanks Tilen, That's great. I'll get back to this pull request to fix the mentioned changes as soon as I can, That's why I kept it open for now.

MaJerle commented 1 year ago

Is this considered ready now?

HamedJafarzadeh commented 1 year ago

We need this test. Absolutely

Done. A new sample project provided as well.

I added .clang-format file to the repository

CanOpenSTM32Node folder is now formatted using this.

not only that - problem is that several projects with same STM32 copy HAL/LL drivers inside - makes no sense

I'm not agreeing on this Tilen, I believe each project folder should be independent from others. The only shared folders are CANOpenNode folder and CANOpenNodeSTM32 folder. Projects don't have many files in common as they are different MCU families (Except for G0 examples). I tried my best to keep the files as they are generated by STM32CubeMX so that minimum effort would be needed to port the CANOpen library to other projects.

Changes up to this point are as follow :

If you agree with me on these points, you can merge the changes.