-
Notifications
You must be signed in to change notification settings - Fork 92
autopilot_manager: check if we are running on a pi4 for NavigatorPì4 #3204
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: master
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request modifies the Sequence diagram for NavigatorPi4.detect()sequenceDiagram
participant NavigatorPi4
NavigatorPi4->>NavigatorPi4: is_pi4()
alt is_pi4() == True
NavigatorPi4->>NavigatorPi4: check_for_i2c_device(bus, address) for each device
NavigatorPi4->>NavigatorPi4: all(check_for_i2c_device)
else is_pi4() == False
NavigatorPi4-->>NavigatorPi4: return False
end
Updated class diagram for NavigatorPi4classDiagram
class NavigatorPi4 {
-devices: Dict[int, Tuple[int, int]]
+__init__(self, i2c: I2C)
+get_release(self) string
+get_serials(self) List[Serial]
+is_pi5() bool
+is_pi4() bool
+detect() bool
+check_for_i2c_device(self, bus: int, address: int) bool
}
note for NavigatorPi4 "detect() now checks if the system is a Raspberry Pi 4 before running the i2c device check."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @Williangalvani - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the file reading logic into a separate helper function to avoid repetition in
is_pi5
andis_pi4
. - It might be clearer to rename
detect
tois_present
oris_supported
to better reflect its purpose.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This will make it so the detection doesn't run on pi3 and other boards
@@ -1,7 +1,7 @@ | |||
import platform | |||
from typing import Any, List | |||
|
|||
from commonwealth.utils.commands import load_file | |||
from commonwealth.utils.commands import CpuType, get_cpu_type, load_file |
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 that get_cpu_type
and CpuType
are in .general
from commonwealth.utils.commands import CpuType, get_cpu_type, load_file | |
from commonwealth.utils.commands import load_file | |
from commonwealth.utils.general import CpuType, get_cpu_type |
This will make it so the detection doesn't run on pi3 and other boards
waiting for CI to test it
Summary by Sourcery
Add Raspberry Pi 4 detection method for NavigatorPi4 flight controller
New Features:
Enhancements: