feat: ported APDS9960 sensor screen. #2838
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideImplemented full APDS9960 sensor support by adding its I2C communication class, provider for state and data polling, a dedicated UI screen with charts and controls, integration into the main navigation, and necessary localization and theming updates. Sequence diagram for APDS9960 sensor data polling and UI updatesequenceDiagram
actor User
participant APDS9960Screen
participant APDS9960Provider
participant APDS9960
participant I2C
User->>APDS9960Screen: Selects APDS9960 sensor
APDS9960Screen->>APDS9960Provider: initializeSensors(i2c, scienceLab)
APDS9960Provider->>APDS9960: create(i2c, scienceLab)
APDS9960->>I2C: I2C initialization
APDS9960Provider->>APDS9960: getRawData(mode)
APDS9960->>I2C: Read sensor data
APDS9960-->>APDS9960Provider: Return sensor data
APDS9960Provider-->>APDS9960Screen: Notify UI update
APDS9960Screen-->>User: Display updated sensor data and charts
Class diagram for APDS9960 sensor supportclassDiagram
class APDS9960 {
+I2C i2c
+List<int> colorData
+double lux
+int proximity
+int gesture
+Future<void> _initialize(ScienceLab)
+Future<int> getProximity()
+Future<List<int>> getColorData()
+Future<double> getLux()
+Future<int> getGesture()
+Future<Map<String, dynamic>> getRawData(int mode)
+String getGestureString(int gestureValue)
}
class APDS9960Provider {
+APDS9960? _apds9960
+int red
+int green
+int blue
+int clear
+double lux
+int proximity
+int gesture
+String gestureString
+List<ChartDataPoint> luxData
+List<ChartDataPoint> proximityData
+bool isRunning
+bool isLooping
+int timegapMs
+int numberOfReadings
+int mode
+Future<void> initializeSensors(...)
+void setMode(int)
+void toggleDataCollection()
+void clearData()
}
class APDS9960Screen {
+StatefulWidget
+APDS9960Provider provider
+Widget _buildConfigureSection(APDS9960Provider)
+Widget _buildRawDataSection(APDS9960Provider)
}
APDS9960Provider --> APDS9960
APDS9960Screen --> APDS9960Provider
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Build successful. APKs to test: https://github.com/fossasia/pslab-app/actions/runs/17005200742/artifacts/3778921953 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Localize hard-coded UI strings in APDS9960Screen (modeOptions entries and the scaffold title) instead of using direct English literals.
- Extract the duplicate Snackbar creation logic in APDS9960Screen into a shared helper or widget to avoid repeating error and success snackbars.
- The APDS9960 class currently mixes low-level I2C communication with complex gesture detection; consider refactoring by extracting gesture processing or data conversion into separate smaller classes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Localize hard-coded UI strings in APDS9960Screen (modeOptions entries and the scaffold title) instead of using direct English literals.
- Extract the duplicate Snackbar creation logic in APDS9960Screen into a shared helper or widget to avoid repeating error and success snackbars.
- The APDS9960 class currently mixes low-level I2C communication with complex gesture detection; consider refactoring by extracting gesture processing or data conversion into separate smaller classes.
## Individual Comments
### Comment 1
<location> `lib/communication/sensors/apds9960.dart:126` </location>
<code_context>
+ await setBit(enable, bitMaskEnableColor, value);
+ }
+
+ Future<void> setProximityInterruptThreshold(List<int> settingArray) async {
+ if (settingArray.isNotEmpty &&
+ settingArray[0] >= 0 &&
</code_context>
<issue_to_address>
Potential issue: Both threshold values are written to the same register.
Please check if the sensor requires separate registers for each threshold and confirm the correct register mapping with the datasheet.
</issue_to_address>
### Comment 2
<location> `lib/communication/sensors/apds9960.dart:151` </location>
<code_context>
+ }
+
+ Future<void> setColorIntegrationTime(int value) async {
+ await i2c.write(address, [256 - value], atime);
+ }
+
</code_context>
<issue_to_address>
Possible off-by-one error in color integration time calculation.
Verify that the calculation aligns with the sensor's specification for integration time. If the expected range or formula differs, adjust accordingly.
</issue_to_address>
### Comment 3
<location> `lib/providers/apds9960_provider.dart:112` </location>
<code_context>
+ }
+
+ void _startDataCollection() {
+ if (_apds9960 == null) return;
+
+ _isRunning = true;
</code_context>
<issue_to_address>
Silent return if sensor is not initialized may hide errors.
Log a warning or notify the user when _apds9960 is null to improve error visibility and debugging.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
void _startDataCollection() {
if (_apds9960 == null) return;
_isRunning = true;
=======
void _startDataCollection() {
if (_apds9960 == null) {
// Log a warning for debugging purposes
debugPrint('Warning: Tried to start data collection but _apds9960 is not initialized.');
return;
}
_isRunning = true;
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `lib/theme/colors.dart:87` </location>
<code_context>
Color sensorControlIconColor = Colors.grey.shade600;
List<Color> bmp180ChartColors = [Colors.blue, Colors.green, Colors.red];
Color chartHintTextColor = Colors.yellow;
+List<Color> apds990ChartColors = [Colors.blue, Colors.yellow];
</code_context>
<issue_to_address>
Typo: Variable name should be apds9960ChartColors.
Rename the variable to apds9960ChartColors to match the sensor name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| await setBit(enable, bitMaskEnableColor, value); | ||
| } | ||
|
|
||
| Future<void> setProximityInterruptThreshold(List<int> settingArray) async { |
There was a problem hiding this comment.
issue (bug_risk): Potential issue: Both threshold values are written to the same register.
Please check if the sensor requires separate registers for each threshold and confirm the correct register mapping with the datasheet.
| } | ||
|
|
||
| Future<void> setColorIntegrationTime(int value) async { | ||
| await i2c.write(address, [256 - value], atime); |
There was a problem hiding this comment.
question (bug_risk): Possible off-by-one error in color integration time calculation.
Verify that the calculation aligns with the sensor's specification for integration time. If the expected range or formula differs, adjust accordingly.
|
@Yugesh-Kumar-S I just tested the PR with a sensor and I noticed that I get high numbers for the distance when my hand is close to the sensor and low values, when my hand is moving away from the sensor. I don't really remember if this is how the sensor works or if maybe there is something wrong. Do you know off the top of your head? If not, I will check the data sheet. |
Yes sir , this is how it works . We get proximity value from 0 to 255 (255 when closer and decreases as we move away ) |
|
@Yugesh-Kumar-S Could you please have a look at the remarks of the sourcery.ai bot before I approve the PR? I think at least the remark about the variable name (apds990ChartColors instead of apds9960ChartColors) makes sense. |
Sure sir , i will make the changes required. |
marcnause
approved these changes
Aug 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2837
Changes
Screenshots / Recordings
screen-20250816-105628.mp4
screen-20250816-113844.mp4
Checklist:
strings.xml,dimens.xmlandcolors.xmlwithout hard coding any value.strings.xml,dimens.xmlorcolors.xml.Summary by Sourcery
Add full support for the APDS9960 sensor by implementing its communication, provider, and UI screen, integrating it into the SensorsScreen, and updating localization and theming
New Features:
Bug Fixes:
Summary by Sourcery
Port full support for the APDS9960 sensor by implementing its communication layer, state provider, and UI screen, integrating it into the main SensorsScreen, and updating localization and theming
New Features:
Bug Fixes: