Skip to content
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

CMSIS RTOS Feature #227

Closed
wants to merge 34 commits into from
Closed

CMSIS RTOS Feature #227

wants to merge 34 commits into from

Conversation

robamu
Copy link
Contributor

@robamu robamu commented Jul 4, 2021

This branch has the complete feature set for CMSIS RTOS support.
Builds on #190 and #189 , a lot of work provided by @g-arjones , extended with documentation and an updated freertos example with the same architetecture as #225

Closes #188 . Closes #160 issues
Closes #190 Closes #189 PR

The updated freertos example can now be compiled with CMSIS RTOS by passing -DUSE_CMSIS_RTOS=ON and CMSIS RTOSV2 by passing -DUSE_CMSIS_RTOS_V2=ON . When doing this, it is not necessary to set FREERTOS_PATH anymore, because the build system can use the FreeRTOS and CMSIS RTOS sources inside the Cube repository. See updated README for more details

@robamu robamu changed the title WIP: CMSIS RTOS Feature CMSIS RTOS Feature Jul 4, 2021
@robamu robamu changed the title CMSIS RTOS Feature WIP: CMSIS RTOS Feature Jul 4, 2021
@robamu robamu changed the title WIP: CMSIS RTOS Feature CMSIS RTOS Feature Jul 4, 2021
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@atsju atsju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cmake is too complex for me without comments.

I'm not expert and the most difficult is to understand what each variable contains. Var names are near each others and often composed names. I know there were historically no comments in the repo but it needs to change as the repo grows and is used by many. in my opinion

examples/freertos/CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines +7 to +8
option(USE_CMSIS_RTOS "Use CMSIS RTOS provided by Cube repository" OFF)
option(USE_CMSIS_RTOS_V2 "Use CMSIS RTOS_V2 provided by Cube repository" OFF)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
option(USE_CMSIS_RTOS "Use CMSIS RTOS provided by Cube repository" OFF)
option(USE_CMSIS_RTOS_V2 "Use CMSIS RTOS_V2 provided by Cube repository" OFF)
option(USE_CMSIS_RTOS "Use CMSIS RTOS provided by Cube repository" ON)
option(USE_CMSIS_RTOS_V2 "Use CMSIS RTOS_V2 provided by Cube repository" OFF)

This assumes an RTOS path is given by command line or PATH variable. I suppose it's better to have a working example from start.

Sorry I am a novice but aren't freeRTOS/CMSIS_RTOS/CMSIS_RTOS_V2 supposed to be 3 different things ? Shouldn't there be a third option for freeRTOS ?

Copy link
Contributor Author

@robamu robamu Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMSIS_RTOS and CMSIS_RTOS_V2 build on top of FreeRTOS. In the application, you can still use the regular FreeRTOS API, but with CMSIS you can use any library which expects/uses a CMSIS RTOS API as well. (e.g. LwIP). If you think this explanation would help, I can add it to the README as well

Copy link
Collaborator

@atsju atsju Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As seen in later comments, the example should build on FreeRTOS. But I need to modify the cmakelist to be able to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is solves by the new third option USE_CUBE_FREERTOS ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved for H7 but not for F4 missing if(USE_CUBE_FREERTOS)

examples/freertos/CMakeLists.txt Show resolved Hide resolved
The following additional FreeRTOS targets are available in general to use the FreeRTOS provided
in the Cube repository

* `FreeRTOS::STM32::<Family>`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a cube path is provided this should be populated and usable but it is not the case from my tests.
Maybe FREERTOS_PATH should not be mandatory when a STM32_CUBE_<FAMILY>_PATH is provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you added the STM32<Family> to the FreeRTOS components? This is necessary to use the target inside the namespace I think

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I have not. And it was not enough to solve the issue.
But this did:

#if(USE_CMSIS_RTOS OR USE_CMSIS_RTOS_V2)
        list(APPEND FREERTOS_COMP_LIST STM32H7)
        set(FREERTOS_H7_NAMESPACE ${FREERTOS_NAMESPACE}::STM32::H7::M7)
#else()
#    set(FREERTOS_H7_NAMESPACE ${FREERTOS_NAMESPACE})
#endif()

You are more expert that I am. Does the example need update ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah was that the example? That is weird, FREERTOS_PATH should not be necessary.. What was the build generation command?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested it. I loaded the STM32_CUBE_H7_PATH by setting it as an environment with
export STM32_CUBE_H7_PATH=<pathToCube> and the toolchain with export STM32_TOOLCHAIN_PATH=<pathToToolchain>

I the ran the following command inside example/freertos/build:

cmake -DUSE_CMSIS_RTOS=ON -DFREERTOS_H743ZI_EXAMPLE=ON -DSTM32_TOOLCHAIN_PATH=$STM32_TOOLCHAIN_PATH ..

For CMSIS V2 I ran

cmake -DUSE_CMSIS_RTOS_V2=ON -DFREERTOS_H743ZI_EXAMPLE=ON -DSTM32_TOOLCHAIN_PATH=$STM32_TOOLCHAIN_PATH ..

Copy link
Contributor Author

@robamu robamu Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, if neither USE_CMSIS_RTOS nor USE_CMSIS_RTOS_V2 is specified the example could theoretically use both external kernel or STM32H7 kernel. But then what is taken as the default choice? Maybe introduce another option for that? For CMSIS, it is recommended to take the Cube kernel, so I just hardcoded it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I updated it. It should work for your case now as well. There is an option to specify whether the Cube FreeRTOS or an external kernel is picked.

@@ -7,6 +9,32 @@ if(STM32H7 IN_LIST CMSIS_FIND_COMPONENTS)
endif()
list(REMOVE_DUPLICATES CMSIS_FIND_COMPONENTS)

foreach(COMP ${CMSIS_FIND_COMPONENTS})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not expert so I'm sorry if it's supposed to be trivial for me. But if you find it relevant to beginners and maintainers: please add (relevant) comments into the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added more documentation for all component list filler loops

cmake/FindFreeRTOS.cmake Outdated Show resolved Hide resolved
@robamu robamu changed the title CMSIS RTOS Feature WIP: CMSIS RTOS Feature Jul 15, 2021
@robamu robamu changed the title WIP: CMSIS RTOS Feature CMSIS RTOS Feature Jul 15, 2021
@robamu
Copy link
Contributor Author

robamu commented Jul 15, 2021

I added am minor improvement for the FREERTOS_PATH handling:

if(NOT FREERTOS_PATH)
    set(FREERTOS_PATH $ENV{FREERTOS_PATH} CACHE PATH "Path to FreeRTOS")
endif()

if(NOT FREERTOS_PATH)
    set(DEFAULT_FREERTOS_PATH "/opt/FreeRTOS")
    if(EXISTS ${DEFAULT_FREERTOS_PATH})
        set(FREERTOS_PATH ${DEFAULT_FREERTOS_PATH} CACHE PATH "Path to FreeRTOS")
        message(STATUS "No FREERTOS_PATH specified using default: ${DEFAULT_FREERTOS_PATH}")
    else()
        message(STATUS
            "No FreeRTOS folder found at default location ${DEFAULT_FREERTOS_PATH}. "
            "Leaving empty
        )
    endif()
endif()

CMake will check whether the default path exists and just leave it empty if it does not exist

@atsju
Copy link
Collaborator

atsju commented Jul 15, 2021

CMake will check whether the default path exists and just leave it empty if it does not exist

Excellent idea. Could be the same for all default path in a separate PR (I'm on widows so they are all/always non-sense).

@robamu
Copy link
Contributor Author

robamu commented Jul 15, 2021

New issue for this #232

Copy link
Collaborator

@atsju atsju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have an issue with USE_CUBE_FREERTOS on F4

Comment on lines +7 to +8
option(USE_CMSIS_RTOS "Use CMSIS RTOS provided by Cube repository" OFF)
option(USE_CMSIS_RTOS_V2 "Use CMSIS RTOS_V2 provided by Cube repository" OFF)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved for H7 but not for F4 missing if(USE_CUBE_FREERTOS)

examples/freertos/CMakeLists.txt Show resolved Hide resolved
@atsju
Copy link
Collaborator

atsju commented Aug 1, 2021

Hello @robamu,
Sorry for delay. Can you please fix conflict?
Also it would be great to rebase the PR to have a cleaner history (there are 31 commits here). If it's OK for you I can rebase it myself but you will lose commit signatures (I would keep you as author though).

@robamu
Copy link
Contributor Author

robamu commented Aug 1, 2021

I will look into rebasing it for better history

@robamu
Copy link
Contributor Author

robamu commented Aug 1, 2021

I really can't wrap my head around the rebase feature, I would still need to force push (I guess there is no way around this though?).

I will just create a clean squash merge and a new PR, and reference the full discussion there. I hope that is okay.

@robamu robamu mentioned this pull request Aug 1, 2021
@atsju
Copy link
Collaborator

atsju commented Aug 1, 2021

I really can't wrap my head around the rebase feature, I would still need to force push (I guess there is no way around this though?).

I will just create a clean squash merge and a new PR, and reference the full discussion there. I hope that is okay.

Force push is not avoidable in this case. Having a new PR is fine also. I will have a look at #253
Closed in favor of #253

@atsju atsju closed this Aug 1, 2021
@robamu robamu deleted the feature/rtos branch August 8, 2021 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using FreeRTOS from STM32Cube Handle CMSIS-RTOS and CMSIS-RTOS2 as CMSIS components
3 participants