feat: added sensors bh1750 & tsl2561 to complete Luxmeter #2918
+399
−35
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 GuideThis PR enriches the LuxMeter feature by refactoring the state provider to dynamically initialize and switch between in-built, BH1750, and TSL2561 sensors based on configuration, adding dedicated driver classes for both new sensors, and improving configuration persistence and UI integration. Sequence diagram for sensor initialization and switching in LuxMeterStateProvidersequenceDiagram
participant UI
participant LuxMeterStateProvider
participant LuxMeterConfigProvider
participant BH1750
participant TSL2561
participant Light
participant ScienceLab
participant I2C
UI->>LuxMeterConfigProvider: updateActiveSensor(sensorType)
LuxMeterConfigProvider-->>LuxMeterStateProvider: notifyListeners()
LuxMeterStateProvider->>LuxMeterStateProvider: _onConfigChanged()
alt In-built Sensor
LuxMeterStateProvider->>Light: initializeInbuiltSensor()
else BH1750
LuxMeterStateProvider->>ScienceLab: getIt<ScienceLab>()
LuxMeterStateProvider->>I2C: new I2C(scienceLab.mPacketHandler)
LuxMeterStateProvider->>BH1750: new BH1750(i2c)
LuxMeterStateProvider->>BH1750: setRange(gain)
else TSL2561
LuxMeterStateProvider->>ScienceLab: getIt<ScienceLab>()
LuxMeterStateProvider->>I2C: new I2C(scienceLab.mPacketHandler)
LuxMeterStateProvider->>TSL2561: new TSL2561(i2c, scienceLab)
LuxMeterStateProvider->>TSL2561: setGain(gain)
end
Class diagram for new and updated LuxMeter sensor classesclassDiagram
class LuxMeterStateProvider {
- double _currentLux
- Light? _light
- I2C? _i2c
- ScienceLab? _scienceLab
- BH1750? _bh1750
- TSL2561? _tsl2561
- LuxMeterConfigProvider? _configProvider
+ setConfigProvider(configProvider)
+ initializeInbuiltSensor(onError)
+ initializeBH1750Sensor(onError)
+ initializeTSL2561Sensor(onError)
+ disposeSensors()
+ _onConfigChanged()
+ _resetLuxData()
}
class BH1750 {
+ BH1750(i2c)
+ init()
+ setRange(g)
+ getVals(numBytes)
+ getRaw()
- I2C i2c
- int address
}
class TSL2561 {
+ TSL2561(i2c, scienceLab)
+ getID()
+ getRaw()
+ setGain(gainValue)
+ enable()
+ disable()
- I2C i2c
- int address
- int timing
- int gain
}
LuxMeterStateProvider --> BH1750
LuxMeterStateProvider --> TSL2561
LuxMeterStateProvider --> Light
LuxMeterStateProvider --> I2C
LuxMeterStateProvider --> ScienceLab
BH1750 --> I2C
TSL2561 --> I2C
TSL2561 --> ScienceLab
Class diagram for updated LuxMeterConfigProviderclassDiagram
class LuxMeterConfigProvider {
- LuxMeterConfig _config
+ updateConfig(newConfig)
+ updateActiveSensor(activeSensor)
+ _loadConfigFromPrefs()
+ _saveConfigToPrefs()
}
LuxMeterConfigProvider --> LuxMeterConfig
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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:
- Consider replacing the string literals for sensor types with a dedicated enum or constants to ensure type safety and simplify sensor-selection logic.
- You have nearly identical polling/setup code in both BH1750 and TSL2561; extract the shared Timer-based read/update logic into a common helper or base class to reduce duplication.
- Avoid using an immediately‐invoked async closure in TSL2561’s constructor—move the address discovery and initialization into a separate async init() method for clearer sequencing and error handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider replacing the string literals for sensor types with a dedicated enum or constants to ensure type safety and simplify sensor-selection logic.
- You have nearly identical polling/setup code in both BH1750 and TSL2561; extract the shared Timer-based read/update logic into a common helper or base class to reduce duplication.
- Avoid using an immediately‐invoked async closure in TSL2561’s constructor—move the address discovery and initialization into a separate async init() method for clearer sequencing and error handling.
## Individual Comments
### Comment 1
<location> `lib/communication/sensors/bh1750.dart:45` </location>
<code_context>
+
+ Future<double> getRaw() async {
+ List<int> vals = await getVals(2);
+ if (vals.length == 3) {
+ return ((vals[0] << 8) | vals[1]) / 1.2;
+ } else {
</code_context>
<issue_to_address>
Possible off-by-one error in getRaw: expects 3 bytes, but BH1750 typically returns 2.
The method checks for three bytes, but the sensor returns two. This could cause incorrect results. Please confirm the expected response length and update the condition if needed.
</issue_to_address>
### Comment 2
<location> `lib/communication/sensors/tsl2561.dart:66` </location>
<code_context>
+ try {
+ List<int> idList = await i2c.readBulk(address, registerId, 1);
+ if (idList.isEmpty) return -1;
+ int id = int.parse(idList[0].toRadixString(16), radix: 16);
+ logger.d("$tag: ID: $id");
+ return id;
</code_context>
<issue_to_address>
Unnecessary conversion in getID: int.parse on a value already an int.
The conversion from int to hex string and back to int is unnecessary; you can return idList[0] directly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
int id = int.parse(idList[0].toRadixString(16), radix: 16);
logger.d("$tag: ID: $id");
return id;
=======
int id = idList[0];
logger.d("$tag: ID: $id");
return id;
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `lib/providers/luxmeter_state_provider.dart:145` </location>
<code_context>
+
+ int lastUpdate = 0;
+
+ _luxTimer = Timer.periodic(Duration(milliseconds: 10), (timer) async {
+ try {
+ final lux = await _bh1750?.getRaw();
</code_context>
<issue_to_address>
High-frequency polling (10ms) may impact performance.
Polling sensors every 10ms may cause excessive CPU and battery usage. Consider matching the polling interval to the updatePeriod or implementing an adaptive strategy.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
_luxTimer = Timer.periodic(Duration(milliseconds: 10), (timer) async {
=======
_luxTimer = Timer.periodic(Duration(milliseconds: intervalMs), (timer) async {
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+66
to
+68
| int id = int.parse(idList[0].toRadixString(16), radix: 16); | ||
| logger.d("$tag: ID: $id"); | ||
| return id; |
There was a problem hiding this comment.
suggestion: Unnecessary conversion in getID: int.parse on a value already an int.
The conversion from int to hex string and back to int is unnecessary; you can return idList[0] directly.
Suggested change
| int id = int.parse(idList[0].toRadixString(16), radix: 16); | |
| logger.d("$tag: ID: $id"); | |
| return id; | |
| int id = idList[0]; | |
| logger.d("$tag: ID: $id"); | |
| return id; |
|
|
||
| int lastUpdate = 0; | ||
|
|
||
| _luxTimer = Timer.periodic(Duration(milliseconds: 10), (timer) async { |
There was a problem hiding this comment.
suggestion (performance): High-frequency polling (10ms) may impact performance.
Polling sensors every 10ms may cause excessive CPU and battery usage. Consider matching the polling interval to the updatePeriod or implementing an adaptive strategy.
Suggested change
| _luxTimer = Timer.periodic(Duration(milliseconds: 10), (timer) async { | |
| _luxTimer = Timer.periodic(Duration(milliseconds: intervalMs), (timer) async { |
|
@rahul31124 Looks like there's a minor analysis issue. Could you check it please ? |
Build StatusBuild successful. APKs to test: https://github.com/fossasia/pslab-app/actions/runs/17795774306/artifacts/4033786100. ScreenshotsAndroid Screenshots
iPhone Screenshots
iPad Screenshots
|
||||||||||||||||||||||||
-1_instruments_screen.png?raw=true)
-2_nav_drawer.png?raw=true)
-3_accelerometer.png?raw=true)
-4_power_source.png?raw=true)
-5_multimeter.png?raw=true)
-6_wave_generator.png?raw=true)
-7_oscilloscope.png?raw=true)
AsCress
approved these changes
Sep 15, 2025
mariobehling
approved these changes
Sep 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Changes
Recording
lux_meter.mp4
Summary by Sourcery
Add support for BH1750 and TSL2561 external light sensors and extend the LuxMeter state and config providers to apply active sensor selection, update period, and gain settings dynamically with proper persistence and error handling.
New Features:
Enhancements: