-
Notifications
You must be signed in to change notification settings - Fork 820
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
docs: 2144 - ctrehan/2144 #2662
base: development
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request updates the I2C address of the MPU6050 sensor and adds new string resources for sensor data representation and a 'Documentation' navigation item. The sensor address was updated across multiple files to ensure consistency. New string resources were added to support the display of various sensor readings and a new navigation option in multiple languages. Updated class diagram for MPU6050classDiagram
class MPU6050 {
-int ADDRESS
+MPU6050()
+readSensor()
}
note for MPU6050 "Address changed from 0x68 to 0x69"
Updated class diagram for MPU925xclassDiagram
class MPU925x {
-int ADDRESS
+MPU925x()
+readSensor()
}
note for MPU925x "Address changed from 0x68 to 0x69"
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 @ChehakTrehan - I've reviewed your changes - here's some feedback:
Overall Comments:
- The commit message should reflect both the address change and the string additions.
- Be sure to check that MPU925x should also have its address updated.
Here's what I looked at during the review
- 🟡 General issues: 2 issues 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.
sensorAddr.put(0x69, "MPU6050"); | ||
sensorAddr.put(0x40, "SHT21"); | ||
sensorAddr.put(0x39, "TSL2561"); | ||
sensorAddr.put(0x69, "MPU925x"); |
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): Duplicate key mapping for 0x69 in sensorAddr may lead to unintended overrides.
Both MPU6050 and MPU925x are mapped to the same I2C address key (0x69), which means the latter mapping will overwrite the earlier one. It’s important to verify whether these devices should share the same address or if a distinct address or strategy (such as separate maps or composite values) is required.
sensorList.put(0x69, new String[]{"MPU-6050/GY-521 accel+gyro+temp", "ITG3200", "DS1307", "DS3231"}); | ||
sensorList.put(0x69, new String[]{"ITG3200"}); |
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): Conflicting entries for key 0x69 in sensorList could result in inadvertent data loss.
The first mapping for 0x69 (associating multiple sensor names including MPU-6050) is being overwritten by the second mapping (only "ITG3200") because map keys must be unique. Please double-check the intended sensor addressing to ensure both sets of sensor identifications are preserved or differentiated.
@ChehakTrehan: Is this a duplicate of #2660? Should the ID of the MPU6050 really be changes or is this a mistake? |
@ChehakTrehan You might want to go through the discussion here. |
Updated Address of MPU6050
Summary by Sourcery
Update sensor address for MPU6050 and add localization strings for various sensors and UI elements
Enhancements:
Documentation:
Chores: