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

P2 h/hal comp scan #301

Closed
wants to merge 4 commits into from
Closed

P2 h/hal comp scan #301

wants to merge 4 commits into from

Conversation

Hish15
Copy link
Collaborator

@Hish15 Hish15 commented Oct 7, 2022

This does a lot of changes, but I did clean commits.
I suggest reviewing commit by commit.

close #268

Closed in favor of #302

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.

Big and complex PR for me.
Please start by fixing automated builds...

)
# This function gets a list of hal_driver using a given prefix and suffix
#
# out_list_hal_drivers list of hal_drivers foud
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
# out_list_hal_drivers list of hal_drivers foud
# out_list_hal_drivers list of hal_drivers found

#Keeping only matching files
list(FILTER filtered_files INCLUDE REGEX ${file_pattern})
#Cleaning files name to have the hal_drivers only (adc/uart/...)
list(TRANSFORM filtered_files REPLACE ${file_pattern} "\\1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand how this line works (even with cmake manual)

foreach(COMP ${HAL_FIND_COMPONENTS})
string(TOLOWER ${COMP} COMP_L)
string(TOUPPER ${COMP} COMP_U)
string(REGEX MATCH "^STM32([FGHLMUW]P?[0-9BL])([0-9A-Z][0-9M][A-Z][0-9A-Z])?_?(M0PLUS|M4|M7)?.*$" COMP_U ${COMP_U})
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for #284

foreach(COMP ${HAL_REQUESTED_FAMILIES})
string(TOLOWER ${COMP} COMP_L)
string(TOUPPER ${COMP} COMP_U)
string(REGEX MATCH "^STM32([FGHLMUW]P?[0-9BL])([0-9A-Z][0-9M][A-Z][0-9A-Z])?_?(M0PLUS|M4|M7)?.*$" COMP_U ${COMP_U})
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for #284

string(TOUPPER ${COMP} COMP_U)
string(REGEX MATCH "^STM32([FGHLMUW]P?[0-9BL])([0-9A-Z][0-9M][A-Z][0-9A-Z])?_?(M0PLUS|M4|M7)?.*$" COMP_U ${COMP_U})
if(CMAKE_MATCH_1)
set(FAMILY ${CMAKE_MATCH_1})
Copy link
Collaborator

Choose a reason for hiding this comment

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

unused ?
or supposed to be used in following line instead of COMP_U ?

string(TOLOWER ${COMP} COMP_L)
string(TOUPPER ${COMP} COMP_U)
string(REGEX MATCH "^STM32([FGHLMUW]P?[0-9BL])([0-9A-Z][0-9M][A-Z][0-9A-Z])?_?(M0PLUS|M4|M7)?.*$" COMP_U ${COMP_U})
if(CMAKE_MATCH_1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add a comment (example) about what is matched. because https://cmake.org/cmake/help/latest/variable/CMAKE_MATCH_n.html

string(TOLOWER ${COMP} COMP_L)
string(TOUPPER ${COMP} COMP_U)
string(REGEX MATCH "^STM32([FGHLMUW]P?[0-9BL])([0-9A-Z][0-9M][A-Z][0-9A-Z])?_?(M0PLUS|M4|M7)?.*$" COMP_U ${COMP_U})
if(CMAKE_MATCH_1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add a comment (example) about what is matched

set(FAMILY ${CMAKE_MATCH_1})
list(APPEND HAL_FIND_COMPONENTS_FAMILIES ${COMP_U})
else()
message(FATAL_ERROR "\"${COMP}\" is not a valid FAMILY")
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this already filtered by previous foreach ?

Comment on lines +66 to +69
get_list_hal_drivers(HAL_DRIVERS_${FAMILY} ${HAL_${FAMILY}_PATH} "hal")
get_list_hal_drivers(HAL_LL_DRIVERS_${FAMILY} ${HAL_${FAMILY}_PATH} "ll")
list(APPEND HAL_DRIVERS ${HAL_DRIVERS_${FAMILY}})
list(APPEND HAL_LL_DRIVERS ${HAL_LL_DRIVERS_${FAMILY}})
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems _ex drivers are not managed

Comment on lines -243 to -248
string(REGEX MATCH "^STM32([FGHLMUW]P?[0-9BL])([0-9A-Z][0-9M][A-Z][0-9A-Z])?_?(M0PLUS|M4|M7)?.*$" COMP_U ${COMP_U})
if(CMAKE_MATCH_1)
list(APPEND HAL_FIND_COMPONENTS_FAMILIES ${COMP})
message(TRACE "FindHAL: append COMP ${COMP} to HAL_FIND_COMPONENTS_FAMILIES")
continue()
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I cant comment in PR bellow this line but please have a look at next occurences of HAL_FIND_COMPONENTS_FAMILIES it might explain you are breaking the multicore families tests + notice the management of empty list to manage all families. I think your work is not finished

@Hish15 Hish15 closed this Oct 15, 2022
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.

HAL drivers detection without knowing the list of expected drivers
2 participants