feat: add Dust Sensor instrument screen#3030
feat: add Dust Sensor instrument screen#3030manishraj-003 wants to merge 1 commit intofossasia:flutterfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @manishraj-003, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Build StatusBuild workflow failed. Please check the logs for more information. ScreenshotsNot able to fetch screenshots. |
manishraj-003
force-pushed
the
issue2988
branch
from
0fd7067 to
4184be9
Compare
manishraj-003
force-pushed
the
issue2988
branch
from
4184be9 to
296f99b
Compare
|
Hi @manishraj-003 ,please make use of the guide_widget.dart for the meter .It would help to maintain consistent UI. |
There was a problem hiding this comment.
Pull request overview
Adds a new Dust Sensor instrument entry and a corresponding Flutter screen to bring feature parity with the legacy Java app (issue #2988), including navigation wiring and initial UI scaffolding.
Changes:
- Enabled “Dust Sensor” in the Instruments list and added a
/dustsensorroute. - Added
DustSensorScreenUI (PPM/air-quality display, gauge, and chart area). - Introduced
DustSensorStateProviderfor screen state and (placeholder) streaming behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/view/instruments_screen.dart | Adds Dust Sensor to the instruments list for navigation. |
| lib/view/dust_sensor_screen.dart | New Dust Sensor UI screen with menu actions, gauge, and chart placeholder. |
| lib/providers/dust_sensor_state_provider.dart | New state provider for dust sensor value/graph and streaming lifecycle. |
| lib/main.dart | Registers the Dust Sensor route and imports the screen. |
| lib/constants.dart | Adjusts instrument icon list to include a temporary dust/sound icon entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO: Replace timer body with real sensor reading. | ||
| // Example future flow: | ||
| // 1) Read dust sensor value from PSLab | ||
| // 2) _ppm = newValue | ||
| // 3) _spots.add(FlSpot(_time, _ppm!)) | ||
| // 4) _time += 1 | ||
| // 5) notifyListeners() | ||
|
|
||
| _timer = Timer.periodic(const Duration(milliseconds: 700), (_) { | ||
| // No mock values in perfect PR. | ||
| // Keep running idle until real data integration is added. | ||
| notifyListeners(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Timer.periodic calls notifyListeners() every 700ms without changing any state. That will force continuous rebuilds and waste battery/CPU. Until real sensor integration exists, consider not starting a timer at all (or only notify when a new reading is actually added/changed).
| // TODO: Replace timer body with real sensor reading. | |
| // Example future flow: | |
| // 1) Read dust sensor value from PSLab | |
| // 2) _ppm = newValue | |
| // 3) _spots.add(FlSpot(_time, _ppm!)) | |
| // 4) _time += 1 | |
| // 5) notifyListeners() | |
| _timer = Timer.periodic(const Duration(milliseconds: 700), (_) { | |
| // No mock values in perfect PR. | |
| // Keep running idle until real data integration is added. | |
| notifyListeners(); | |
| }); | |
| // TODO: Replace this placeholder with real sensor reading. | |
| // Example future flow: | |
| // 1) Read dust sensor value from PSLab | |
| // 2) _ppm = newValue | |
| // 3) _spots.add(FlSpot(_time, _ppm!)) | |
| // 4) _time += 1 | |
| // 5) notifyListeners() | |
| // | |
| // Until real sensor integration is implemented, avoid starting a | |
| // periodic timer that triggers rebuilds without any state changes. |
| if (ppm < 1.0) return "Good"; | ||
| if (ppm < 2.5) return "Moderate"; | ||
| if (ppm < 4.0) return "Poor"; | ||
| return "Hazardous"; |
There was a problem hiding this comment.
Air-quality labels are hard-coded English strings (Good/Moderate/Poor/Hazardous). These should come from AppLocalizations so the screen is translatable like the rest of the app, and so the PR checklist claim of no hard-coded strings holds.
| if (ppm < 1.0) return "Good"; | |
| if (ppm < 2.5) return "Moderate"; | |
| if (ppm < 4.0) return "Poor"; | |
| return "Hazardous"; | |
| final loc = AppLocalizations.of(context)!; | |
| if (ppm < 1.0) return loc.airQualityGood; | |
| if (ppm < 2.5) return loc.airQualityModerate; | |
| if (ppm < 4.0) return loc.airQualityPoor; | |
| return loc.airQualityHazardous; |
| const PopupMenuItem( | ||
| value: 'clear', | ||
| child: Text("Clear graph"), | ||
| ), | ||
| PopupMenuItem( | ||
| value: 'toggle', | ||
| enabled: connected, | ||
| child: Text(provider.isStreaming ? "Stop" : "Start"), | ||
| ), |
There was a problem hiding this comment.
The options menu items use hard-coded strings (e.g., "Clear graph", "Start"/"Stop"). Please replace these with localized strings (ARB + generated getters) to match the app’s localization pattern.
| Text( | ||
| "⚠️ Connect PSLab device to start measuring.", | ||
| style: Theme.of(context).textTheme.bodySmall, |
There was a problem hiding this comment.
This user-facing message is hard-coded and includes an emoji. For localization and accessibility/screen-reader consistency, use a localized string (and consider using an Icon widget instead of an emoji if you want a warning indicator).
| Text( | |
| "⚠️ Connect PSLab device to start measuring.", | |
| style: Theme.of(context).textTheme.bodySmall, | |
| Row( | |
| mainAxisAlignment: MainAxisAlignment.center, | |
| children: [ | |
| Icon( | |
| Icons.warning_amber_rounded, | |
| size: 16, | |
| color: Theme.of(context).colorScheme.error, | |
| ), | |
| const SizedBox(width: 8), | |
| Text( | |
| "Connect PSLab device to start measuring.", | |
| style: Theme.of(context).textTheme.bodySmall, | |
| ), | |
| ], |
| @@ -26,9 +26,8 @@ List<String> instrumentIcons = [ | |||
| 'assets/icons/gyroscope_logo.png', | |||
| 'assets/icons/thermometer_logo.png', | |||
| 'assets/icons/robotic_arm.png', | |||
There was a problem hiding this comment.
instrumentIcons now has fewer entries than instrumentNames, and it also changes the assumed index mapping used elsewhere (e.g., instrumentIcons[15] in SoundMeter and LoggedDataScreen’s instrumentNames.indexOf(...) lookup). This can cause RangeError crashes and/or wrong icons. Keep instrumentIcons and instrumentNames aligned (same ordering/length), or refactor to a keyed mapping so screens don’t rely on hard-coded indices.
| 'assets/icons/robotic_arm.png', | |
| 'assets/icons/robotic_arm.png', | |
| 'assets/icons/tile_icon_gas.png', // gas/dust sensor (temporary icon) |
| return ChangeNotifierProvider( | ||
| create: (_) => DustSensorStateProvider(), | ||
| child: Consumer<DustSensorStateProvider>( | ||
| builder: (context, provider, _) { |
There was a problem hiding this comment.
ChangeNotifierProvider(create: ...) is inside build(), so any rebuild of DustSensorScreen (e.g., toggling _showGuide) will recreate DustSensorStateProvider and reset state/timers/spots. Instantiate the provider once (e.g., in initState as a late final field) and provide it via ChangeNotifierProvider.value, or lift the provider above this widget (route-level provider).
| provider.stop(); | ||
| provider.clearGraph(); |
There was a problem hiding this comment.
Calling provider.stop() / provider.clearGraph() from inside the Consumer builder introduces side effects during build and can trigger "markNeedsBuild called during build" style issues (notifyListeners while building). Move this disconnect cleanup into a listener (addListener/Selector) or schedule it post-frame when the connection state transitions.
| provider.stop(); | |
| provider.clearGraph(); | |
| // Avoid side effects during build: schedule stop/clear after this frame. | |
| WidgetsBinding.instance.addPostFrameCallback((_) { | |
| if (!mounted) return; | |
| final dustProvider = context.read<DustSensorStateProvider>(); | |
| if (! _isDeviceConnected() && dustProvider.isStreaming) { | |
| dustProvider.stop(); | |
| dustProvider.clearGraph(); | |
| } | |
| }); |
| /// Starts polling/streaming dust values. | ||
| /// In this PR we only enable the UI + route; hardware streaming will be added later. | ||
| void start() { | ||
| if (_isStreaming || !_isConnected) return; |
There was a problem hiding this comment.
start() is gated on _isConnected, but nothing in this PR ever calls setConnectionStatus(true). As a result, start() will always early-return and streaming can never begin even when the device is connected. Either remove _isConnected from the provider and rely on the UI’s connected check, or ensure the UI updates the provider’s connection state when ScienceLab.isConnected() changes.
| if (_isStreaming || !_isConnected) return; | |
| if (_isStreaming) return; |
|
Thank you for your contribution. This PR is scheduled for closure due to inactivity. It would be fantastic if you could finalize this PR. Otherwise we unfortunately have to close this PR in a few days. |
|
Also a process note. We have automatic Copilot PR reviews enabled on this repository. These reviews are only triggered if the contributor has GitHub Copilot enabled and an active license on their own account. Please enable Copilot in your GitHub settings if you have access. In many regions, free licenses are available through educational institutions or developer programs. Enabling Copilot helps us speed up the auto review process and reduces manual review overhead for the core team. |
Fixes #2988
Changes
/dustsensorScreenshots / Recordings
Checklist:
constants.dartor localization files instead of hard-coded values.dart formator the IDE formatter.flutter analyzeand tests run influtter test.