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.

Sign up for GitHub

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

Jump to bottom

Blinky example minor improvement #225

Merged
merged 4 commits into from
Jul 3, 2021
Merged

Blinky example minor improvement #225

merged 4 commits into from
Jul 3, 2021

Conversation

robamu
Copy link
Contributor

@robamu robamu commented Jul 2, 2021 •
edited
Loading

I updated blinky example to allow choosing the architecture via CMake options, with F4 being the default. I think this is a cleaner solution than commenting out the unneeded code or cloning 3 Cube repositories. What do you think?

@robamu robamu changed the title reworked example a bit Blinky example minor improvement Jul 2, 2021
atsju
atsju requested changes Jul 3, 2021
View reviewed changes
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 idea seams good. This method could be used in #206 ? @Hish15 what do you think ?

examples/blinky/CMakeLists.txt Outdated
find_package(CMSIS COMPONENTS STM32L0 STM32F1 STM32F4 REQUIRED)
find_package(HAL COMPONENTS STM32L0 STM32F1 STM32F4 RCC GPIO CORTEX REQUIRED)
# Find all drivers:
# F4 example enabled by default
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
# F4 example enabled by default
# Configure here which STM32 target(s) to build
# F4 example enabled by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented

Copy link
Contributor Author

@robamu robamu Jul 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Using it in #206 seems a good idea. Do you want to merge the two separate folders together again?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rmspacefish yes please go ahead. This is a clean way to do it.

examples/blinky/CMakeLists.txt Outdated

if(L0_EXAMPLE)
find_package(CMSIS COMPONENTS STM32L0 REQUIRED)
find_package(HAL COMPONENTS STM32L0 REQUIRED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't test but shouldn't it be this ?

Suggested change
find_package(HAL COMPONENTS STM32L0 REQUIRED)
find_package(HAL COMPONENTS STM32L0 RCC GPIO CORTEX REQUIRED)

Copy link
Collaborator

Choose a reason for hiding this comment

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

selecting no component will look up all components, so this should work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But is it what we want ? and it's not same as L4 lines.
If we add a comment it's OK for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use find_package with one call by filling component lists

examples/blinky/CMakeLists.txt Outdated
find_package(HAL COMPONENTS STM32L0 STM32F1 STM32F4 RCC GPIO CORTEX REQUIRED)
# Find all drivers:
# F4 example enabled by default
option(F4_EXAMPLE "Compile F4 example" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's strange to have an option with no effect. Add the required if(F4_EXAMPLE)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, make it ON by default, but allow to turn it OFF using the adequate if(F4_EXAMPLE)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@atsju atsju requested a review from Hish15 July 3, 2021 07:20
Hish15
Hish15 requested changes Jul 3, 2021
View reviewed changes
Copy link
Collaborator

@Hish15 Hish15 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi!
I love what you have done here 👍
some minor changes :
The scopes are quite large with cmake, so I would use BLINKY_EXAMPLE_F4 kind of name instead of just F4_EXAMPLE. This will limit possible collisions.

examples/blinky/CMakeLists.txt Outdated
find_package(HAL COMPONENTS STM32L0 STM32F1 STM32F4 RCC GPIO CORTEX REQUIRED)
# Find all drivers:
# F4 example enabled by default
option(F4_EXAMPLE "Compile F4 example" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, make it ON by default, but allow to turn it OFF using the adequate if(F4_EXAMPLE)

@robamu robamu mentioned this pull request Jul 3, 2021
Closed
@atsju atsju merged commit 5381dc1 into ObKo:master Jul 3, 2021
@robamu robamu mentioned this pull request Jul 4, 2021
Closed
@robamu robamu deleted the mueller/blinky-example-update branch July 12, 2021 17:54
robamu added a commit to us-irs/stm32-cmake that referenced this pull request Aug 1, 2021
@robamu
CMSIS RTOS feature
a642c5e 
Adds the full features set for CMSIS RTOS support, with a lot
work provided by g-arjones. Builds on ObKo#190
and ObKo#189.

It allows to use the CMSIS support provided in the STM32 cube
repository.

Also adds documentation and an updated freertos example with the
same architecture as ObKo#225
robamu added a commit to us-irs/stm32-cmake that referenced this pull request Aug 1, 2021
@robamu
CMSIS RTOS feature
ddabe51 
Adds the full features set for CMSIS RTOS support, with a lot
work provided by g-arjones. Builds on ObKo#190
and ObKo#189.

It allows to use the CMSIS support provided in the STM32 cube
repository.

Also adds documentation and an updated freertos example with the
same architecture as ObKo#225
@robamu robamu mentioned this pull request Aug 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atsju atsju atsju approved these changes

@Hish15 Hish15 Hish15 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robamu @atsju @Hish15