-
Notifications
You must be signed in to change notification settings - Fork 24
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
2016 RLX attempt 1 #51
base: master-new
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request adds support for the 2016 Acura RLX. Entity relationship diagram for ECU fingerprintserDiagram
ACURA_RLX ||--o{ ECU : contains
ECU {
string vsa
string fwdRadar
string srs
}
ECU ||--o{ FIRMWARE : has
FIRMWARE {
string version
}
note "New ECU fingerprints added for Acura RLX"
Class diagram for the updated Honda car configurationclassDiagram
class CAR {
+ACURA_RLX: HondaNidecPlatformConfig
}
class HondaNidecPlatformConfig {
+CarDocs[]
+CarSpecs
+radar_dbc_dict
+flags
}
class ACURA_RLX_Config {
+mass: 1836.0 LB_TO_KG
+wheelbase: 2.70
+steerRatio: 13.0
+centerToFrontRatio: 0.3794
+tireStiffnessFactor: 0.444
+min_steer_speed: 0 MPH_TO_MS
}
CAR --> HondaNidecPlatformConfig
ACURA_RLX_Config --> HondaNidecPlatformConfig
note for ACURA_RLX_Config "New configuration added for 2016 Acura RLX"
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 @thirdstick - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please improve the PR title and description to explain what changes are being made and why. Include information about any testing performed.
- The vehicle parameters (mass, wheelbase, steerRatio, etc.) appear to be copied from the RDX. Please verify these against actual 2016 RLX specifications.
- Using the RDX's DBC file for the RLX needs justification. Are you certain the CAN messages are identical between these different models?
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 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.
@@ -246,6 +246,12 @@ class CAR(Platforms): | |||
radar_dbc_dict('acura_rdx_2018_can_generated'), | |||
flags=HondaFlags.NIDEC_ALT_SCM_MESSAGES, | |||
) | |||
ACURA_RLX = HondaNidecPlatformConfig( | |||
[HondaCarDocs("Acura RDX 2016-18", "AcuraWatch Plus", "All", min_steer_speed=0. * CV.MPH_TO_MS)], |
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.
issue (bug_risk): The car model in HondaCarDocs appears to be incorrect - this should be for RLX, not RDX
This could cause confusion about which vehicle models are supported and might lead to compatibility issues.
2016 RLX attempt 1
Summary by Sourcery
New Features: