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

Added support for Radxa Rock 5C #357

Merged
merged 3 commits into from
Jun 6, 2024
Merged

Conversation

hajimef
Copy link
Contributor

@hajimef hajimef commented Jun 1, 2024

I added support for Radxa Rock 5C.
I also added support for Radxa Rock 5C to Adafruit_Blinka.

Copy link
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

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

To speed it up, you could just check for "ROCK 5C" before checking for "ROCK 5".

Copy link
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

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

Close. Can you just put the check for 5C before the 5? The reason is because the 5C would always match the 5 string as well.

@hajimef
Copy link
Contributor Author

hajimef commented Jun 5, 2024

Close. Can you just put the check for 5C before the 5? The reason is because the 5C would always match the 5 string as well.

Because each board is judged using an if statement, if the if statement for ROCK 5C is executed first, the result will be overwritten by the if statement for ROCK 5 that follows.
For this reason, I have made the judgement for ROCK 5C happen last.
It may be better to modify it to an if ... elif form rather than an if ... if form.

def _rock_pi_id(self) -> Optional[str]:
    """Check what type of Rock Pi board."""
    board_value = self.detector.get_device_model()
    board_value_upper = board_value.upper()
    board = None
    if board_value and "ROCK Pi S" in board_value:
        board = boards.ROCK_PI_S
    elif board_value and "ROCK PI 4" in board_value_upper:
        board = boards.ROCK_PI_4
    elif board_value and "ROCK PI E" in board_value_upper:
        board = boards.ROCK_PI_E
    elif self.detector.check_board_name_value() == "ROCK Pi X":
        board = boards.ROCK_PI_X
    elif board_value and "ROCK 5C" in board_value_upper:
        board = boards.ROCK_PI_5C
    elif board_value and "ROCK 5" in board_value_upper:
        board = boards.ROCK_PI_5
    elif board_value and "RADXA ROCK 4C+" in board_value_upper:
        board = boards.ROCK_PI_4_C_PLUS
    elif board_value and "RADXA ROCK 4SE" in board_value_upper:
        board = boards.ROCK_PI_4_SE
    elif board_value and "ROCK3 Model A" in board_value:
        board = boards.ROCK_PI_3A
    return board

@makermelissa
Copy link
Collaborator

Because each board is judged using an if statement, if the if statement for ROCK 5C is executed first, the result will be overwritten by the if statement for ROCK 5 that follows.

There isn't anything that is being "overwritten". An if/else if statement like this stops looking once it finds a match. So once it sees that "ROCK 5" matches, it uses that and stops looking.

@hajimef
Copy link
Contributor Author

hajimef commented Jun 6, 2024

I tested this commit with Rock 5B and Rock 5C with the following code.

import adafruit_platformdetect

detector = adafruit_platformdetect.Detector()
print("Chip id: ", detector.chip.id)
print("Board id: ", detector.board.id)

The results for Rock 5B are as follows:

Chip id: RK3588
Board id: ROCK_PI_5

The results for Rock 5C are as follows:

Chip id: RK3588
Board id: ROCK_PI_5C

Copy link
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you.

@makermelissa makermelissa merged commit 565e00b into adafruit:main Jun 6, 2024
1 check passed
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.

2 participants