-
Notifications
You must be signed in to change notification settings - Fork 72
Web: Fix CAN options visibility for non-CAN boards #227
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,21 @@ | |
| import logging | ||
| import ap_git | ||
| import os | ||
|
|
||
| import re | ||
|
|
||
| class Board: | ||
| def __init__(self, id: str, name: str, attributes: dict): | ||
| self.id = id | ||
| self.name = name | ||
| self.attributes = dict(attributes) | ||
|
|
||
| def to_dict(self) -> dict: | ||
| out = { | ||
| "id": self.id, | ||
| "name": self.name, | ||
| "attributes": self.attributes, | ||
| } | ||
| return out | ||
|
|
||
| class APSourceMetadataFetcher: | ||
| """ | ||
|
|
@@ -192,6 +206,8 @@ def __get_boards_at_commit_from_cache(self, | |
| tuple: A tuple of two lists in order: | ||
| - A list contains boards for NON-'ap_periph' targets. | ||
| - A list contains boards for the 'ap_periph' target. | ||
| - A list of Boards for NON-'ap_periph' targets. | ||
| - A list of Boards for the 'ap_periph' target. | ||
|
|
||
| Raises: | ||
| RuntimeError: If the method is called when caching is disabled. | ||
|
|
@@ -220,6 +236,11 @@ def __get_boards_at_commit_from_cache(self, | |
| self.logger.exception(e) | ||
| return None | ||
|
|
||
| # Invalidate stale cache entries that contain dicts instead of Board objects | ||
| if non_periph_boards and isinstance(non_periph_boards[0], dict): | ||
| self.logger.debug("Stale cache entry found, treating as cache miss") | ||
| return None | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove this. I mentioned this in one of my previous reviews as well that we can handle this cleanup manually. |
||
| return ( | ||
| non_periph_boards, | ||
| periph_boards | ||
|
|
@@ -237,8 +258,94 @@ def __exclude_boards_matching_patterns(self, boards: list, patterns: list): | |
| ret.append(b) | ||
| return ret | ||
|
|
||
| def __board_has_can(self, hwdef_path: str) -> bool: | ||
| """Return True when the hwdef file or its includes advertise CAN support.""" | ||
| if not hwdef_path or not os.path.isfile(hwdef_path): | ||
| self.logger.debug( | ||
| "hwdef.dat not found while checking CAN support: %s", | ||
| hwdef_path, | ||
| ) | ||
| return False | ||
|
|
||
| visited_paths = set() | ||
|
|
||
| def read_hwdef_tree(file_path: str) -> str: | ||
| if not file_path or not os.path.isfile(file_path): | ||
| return "" | ||
|
|
||
| normalized_path = os.path.normpath(file_path) | ||
| if normalized_path in visited_paths: | ||
| return "" | ||
| visited_paths.add(normalized_path) | ||
|
|
||
| try: | ||
| with open(file_path, "r", encoding="utf-8", errors="ignore") as hwdef_file: | ||
| contents = hwdef_file.read() | ||
| except OSError as exc: | ||
| self.logger.warning( | ||
| "Failed to read hwdef file at %s: %s", | ||
| file_path, | ||
| exc, | ||
| ) | ||
| return "" | ||
|
|
||
| combined = contents | ||
|
|
||
| # Parse include directives (both .dat and .inc files) | ||
| include_matches = re.findall( | ||
| r"^\s*include\s+([^\s#]+\.(?:dat|inc))\s*$", | ||
| contents, | ||
| re.MULTILINE, | ||
| ) | ||
| for include_name in include_matches: | ||
| include_path = os.path.normpath( | ||
| os.path.join(os.path.dirname(file_path), include_name.strip()) | ||
| ) | ||
| if os.path.isfile(include_path): | ||
| combined += "\n" + read_hwdef_tree(include_path) | ||
| else: | ||
| self.logger.debug( | ||
| "Included hwdef not found while checking CAN support: %s", | ||
| include_path, | ||
| ) | ||
|
|
||
| return combined | ||
|
|
||
| combined_contents = read_hwdef_tree(hwdef_path) | ||
|
|
||
| return ( | ||
| "CAN1" in combined_contents | ||
| or "HAL_NUM_CAN_IFACES" in combined_contents | ||
| or "CAN_P1_DRIVER" in combined_contents | ||
| or "CAN_D1_DRIVER" in combined_contents | ||
| ) | ||
|
|
||
| def __build_board_metadata(self, board_names: list[str], hwdef_dir: str) -> list[Board]: | ||
| board_data: list[Board] = [] | ||
| for board_name in board_names: | ||
| hwdef_path = None | ||
| if hwdef_dir: | ||
| candidate_path = os.path.join(hwdef_dir, board_name, "hwdef.dat") | ||
| if os.path.isfile(candidate_path): | ||
| hwdef_path = candidate_path | ||
| else: | ||
| self.logger.debug( | ||
| "hwdef.dat not found for board %s at %s", | ||
| board_name, | ||
| candidate_path, | ||
| ) | ||
|
|
||
| has_can = self.__board_has_can(hwdef_path) if hwdef_path else False | ||
| board = Board( | ||
| id=board_name, | ||
| name=board_name, | ||
| attributes={"has_can": has_can}, | ||
| ) | ||
| board_data.append(board) | ||
| return board_data | ||
|
|
||
| def __get_boards_at_commit_from_repo(self, remote: str, | ||
| commit_ref: str) -> tuple[list, list]: | ||
| commit_ref: str) -> tuple[list[Board], list[Board]]: | ||
| """ | ||
| Returns the tuple of boards (for both non-periph and periph targets, | ||
| in order) for a given commit from the git repo. | ||
|
|
@@ -251,6 +358,8 @@ def __get_boards_at_commit_from_repo(self, remote: str, | |
| tuple: A tuple of two lists in order: | ||
| - A list contains boards for NON-'ap_periph' targets. | ||
| - A list contains boards for the 'ap_periph' target. | ||
| - A list of Boards for NON-'ap_periph' targets. | ||
| - A list of Boards for the 'ap_periph' target. | ||
| """ | ||
| with self.repo.get_checkout_lock(): | ||
| self.repo.checkout_remote_commit_ref( | ||
|
|
@@ -275,24 +384,24 @@ def __get_boards_at_commit_from_repo(self, remote: str, | |
| self.logger.debug(f"non_periph_boards raw: {non_periph_boards}") | ||
| self.logger.debug(f"periph_boards raw: {periph_boards}") | ||
|
|
||
| non_periph_boards = self.__exclude_boards_matching_patterns( | ||
| boards=non_periph_boards, | ||
| patterns=['fmuv*', 'SITL*'], | ||
| ) | ||
| self.logger.debug(f"non_periph_boards filtered: {non_periph_boards}") | ||
| non_periph_boards = self.__exclude_boards_matching_patterns( | ||
| boards=non_periph_boards, | ||
| patterns=['fmuv*', 'SITL*'], | ||
| ) | ||
| self.logger.debug(f"non_periph_boards filtered: {non_periph_boards}") | ||
|
|
||
| non_periph_boards_sorted = sorted(non_periph_boards) | ||
| periph_boards_sorted = sorted(periph_boards) | ||
| non_periph_boards_sorted = sorted(non_periph_boards) | ||
| periph_boards_sorted = sorted(periph_boards) | ||
|
|
||
| self.logger.debug( | ||
| f"non_periph_boards sorted: {non_periph_boards_sorted}" | ||
| ) | ||
| self.logger.debug(f"periph_boards sorted: {periph_boards_sorted}") | ||
| self.logger.debug( | ||
| f"non_periph_boards sorted: {non_periph_boards_sorted}" | ||
| ) | ||
| self.logger.debug(f"periph_boards sorted: {periph_boards_sorted}") | ||
|
|
||
| return ( | ||
| non_periph_boards_sorted, | ||
| periph_boards_sorted, | ||
| ) | ||
| return ( | ||
| self.__build_board_metadata(non_periph_boards_sorted, hwdef_dir), | ||
| self.__build_board_metadata(periph_boards_sorted, hwdef_dir), | ||
| ) | ||
|
Comment on lines
+378
to
+395
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we keep the sorting piece of code out of checkout locking and build board metadata early? |
||
|
|
||
| def __get_build_options_at_commit_from_repo(self, | ||
| remote: str, | ||
|
|
@@ -334,7 +443,7 @@ def __get_build_options_at_commit_from_repo(self, | |
| return build_options | ||
|
|
||
| def __get_boards_at_commit(self, remote: str, | ||
| commit_ref: str) -> tuple[list, list]: | ||
| commit_ref: str) -> tuple[list[Board], list[Board]]: | ||
| """ | ||
| Retrieves lists of boards available for building at a | ||
| specified commit for both NON-'ap_periph' and ap_periph targets | ||
|
|
@@ -352,6 +461,8 @@ def __get_boards_at_commit(self, remote: str, | |
| tuple: A tuple of two lists in order: | ||
| - A list contains boards for NON-'ap_periph' targets. | ||
| - A list contains boards for the 'ap_periph' target. | ||
| - A list of Boards for NON-'ap_periph' targets. | ||
| - A list of Boards for the 'ap_periph' target. | ||
| """ | ||
| tstart = time.time() | ||
| if not self.caching_enabled: | ||
|
|
@@ -396,7 +507,7 @@ def __get_boards_at_commit(self, remote: str, | |
| return boards | ||
|
|
||
| def get_boards(self, remote: str, commit_ref: str, | ||
| vehicle_id: str) -> list: | ||
| vehicle_id: str) -> list[Board]: | ||
| """ | ||
| Returns a list of boards available for building at a | ||
| specified commit for given vehicle. | ||
|
|
@@ -407,7 +518,7 @@ def get_boards(self, remote: str, commit_ref: str, | |
| vehicle_id (str): The vehicle ID to get the boards list for. | ||
|
|
||
| Returns: | ||
| list: A list of boards. | ||
| list: A list of Boards. | ||
| """ | ||
| non_periph_boards, periph_boards = self.__get_boards_at_commit( | ||
| remote=remote, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,14 +127,15 @@ def get_boards( | |
| vehicle_id=vehicle_id, | ||
| ) | ||
|
|
||
| board_dicts = [board.to_dict() for board in boards] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to convert board to dict? |
||
| return [ | ||
| BoardOut( | ||
| id=board, | ||
| name=board, | ||
| id=d['id'], | ||
| name=d['name'], | ||
| vehicle_id=vehicle_id, | ||
| version_id=version_id | ||
| ) | ||
| for board in boards | ||
| for d in board_dicts | ||
| ] | ||
|
|
||
| def get_board( | ||
|
|
@@ -174,13 +175,57 @@ def get_features( | |
| f'{version_info.remote_info.name} {version_info.commit_ref}' | ||
| ) | ||
|
|
||
| boards = self.ap_src_metadata_fetcher.get_boards( | ||
| remote=version_info.remote_info.name, | ||
| commit_ref=version_info.commit_ref, | ||
| vehicle_id=vehicle_id, | ||
| ) | ||
| board_has_can = False | ||
| for board in boards: | ||
| if board.id == board_id or board.name == board_id: | ||
| board_has_can = bool(board.attributes.get("has_can")) | ||
| break | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is still not the right way to do it. We need to have methods in ap src metadata fetcher to return features for boards. We should not pollute the code here. |
||
| # Get build options from source | ||
| with self.repo.get_checkout_lock(): | ||
| options = self.ap_src_metadata_fetcher.get_build_options_at_commit( | ||
| remote=version_info.remote_info.name, | ||
| commit_ref=version_info.commit_ref | ||
| ) | ||
|
|
||
| if board_has_can is False: | ||
| options = [ | ||
| option for option in options | ||
| if not option.category or ( | ||
| "CAN" not in option.category | ||
| and "DroneCAN" not in option.category | ||
| ) | ||
| ] | ||
|
|
||
| available_labels = {option.label for option in options} | ||
| pruned_options = [] | ||
| changed = True | ||
| while changed: | ||
| changed = False | ||
| pruned_options = [] | ||
| for option in options: | ||
| if not option.dependency: | ||
| pruned_options.append(option) | ||
| continue | ||
|
|
||
| dependencies = [ | ||
| label.strip() | ||
| for label in option.dependency.split(',') | ||
| if label.strip() | ||
| ] | ||
| if all(dep in available_labels for dep in dependencies): | ||
| pruned_options.append(option) | ||
| else: | ||
| changed = True | ||
|
|
||
| options = pruned_options | ||
| available_labels = {option.label for option in options} | ||
|
|
||
| # Try to fetch board-specific defaults from firmware-server | ||
| board_defaults = None | ||
| artifacts_dir = version_info.ap_build_artifacts_url | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return statement shows two element tuple but comment says four why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think while rebasing i accepted both changes, have fixed that now