Web: Fix CAN options visibility for non-CAN boards#227
Web: Fix CAN options visibility for non-CAN boards#227garvit000 wants to merge 3 commits intoArduPilot:mainfrom
Conversation
|
Before I do a review, can you please tell did you test it? And what cases did you cover? Also, it is generally not recommended to close existing PRs without a good reason. We lose review history that way. |
|
@shiv-tyagi Yes I tested it with cubeorange and mupilot where CAN was visible and kakutef4mini where CAN was hidden. |
|
Waiting for reply @shiv-tyagi |
shiv-tyagi
left a comment
shiv-tyagi
left a comment
There was a problem hiding this comment.
Hi @garvit000. Sorry for replying late. I am busy with some other stuff.
Thanks for all your efforts.
I still need to take out some time to test this. Also, the structuring of this code is problematic. We need organise it better. I have posted some comments. I might post comments again once you fix there till we reach a good shape. But I cannot promise if I would be able to reply fast.
| - 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. |
There was a problem hiding this comment.
Return statement shows two element tuple but comment says four why?
There was a problem hiding this comment.
I think while rebasing i accepted both changes, have fixed that now
| # 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 | ||
|
|
There was a problem hiding this comment.
We can remove this. I mentioned this in one of my previous reviews as well that we can handle this cleanup manually.
web/services/vehicles.py
Outdated
| vehicle_id=vehicle_id, | ||
| ) | ||
|
|
||
| board_dicts = [board.to_dict() for board in boards] |
There was a problem hiding this comment.
Do we need to convert board to dict?
web/services/vehicles.py
Outdated
| for board in boards: | ||
| if board.id == board_id or board.name == board_id: | ||
| board_has_can = bool(board.attributes.get("has_can")) | ||
| break | ||
|
|
There was a problem hiding this comment.
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.
| 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), | ||
| ) |
There was a problem hiding this comment.
Can we keep the sorting piece of code out of checkout locking and build board metadata early?
|
I have moved the logic from vehicles to meta data fetcher and addressed other comments, have a look at them when you are not busy. |
2 participants
This PR fixes #151 where the "CAN" feature category remained visible even when selecting a board that does not support CAN.
Changes:
Test Plan:
Supersedes #209 with a cleaner implementation addressing all review feedback.
Closes #151