fix: prevent LA pin crash in multimeter (#2955)#3053
Open
moksha-hub wants to merge 5 commits intofossasia:flutterfrom
Open
fix: prevent LA pin crash in multimeter (#2955)#3053moksha-hub wants to merge 5 commits intofossasia:flutterfrom
moksha-hub wants to merge 5 commits intofossasia:flutterfrom
Conversation
Contributor
Reviewer's GuideAdds null-safety and error-handling around logic analyzer frequency measurement to prevent crashes, and propagates error states to the multimeter UI with a user-friendly message instead of crashing. Sequence diagram for multimeter frequency measurement with null-safetysequenceDiagram
actor User
participant MultimeterUI
participant MultimeterStateProvider
participant ScienceLab
User->>MultimeterUI: Select frequency mode and channel
MultimeterUI->>MultimeterStateProvider: requestFrequencyMeasurement()
MultimeterStateProvider->>ScienceLab: getFrequency(selectedChannel)
activate ScienceLab
ScienceLab->>ScienceLab: getLAInitialStates()
alt getLAInitialStates throws
ScienceLab->>ScienceLab: log error
ScienceLab-->>MultimeterStateProvider: -1
else getLAInitialStates returns null
ScienceLab->>ScienceLab: log error (null data)
ScienceLab-->>MultimeterStateProvider: -1
else getLAInitialStates ok
ScienceLab->>ScienceLab: calculateDigitalChannel(channel)
alt channelNumber is null
ScienceLab->>ScienceLab: log error (invalid channel)
ScienceLab-->>MultimeterStateProvider: -1
else channelNumber valid
ScienceLab->>ScienceLab: fetchLAChannelFrequency(channelNumber, data)
ScienceLab-->>MultimeterStateProvider: frequencyHz
end
end
deactivate ScienceLab
alt frequency < 0
MultimeterStateProvider->>MultimeterStateProvider: value = Cannot measure!, unit = empty
else frequency >= 0
MultimeterStateProvider->>MultimeterStateProvider: value = formatted frequency, unit = Hz
end
MultimeterStateProvider-->>MultimeterUI: updated value and unit
MultimeterUI-->>User: Show result or Cannot measure!
Updated class diagram for ScienceLab and MultimeterStateProvider frequency logicclassDiagram
class ScienceLab {
+Future~double~ getFrequency(String channel)
+Future~void~ startOneChannelLA(String channel, int channelMode, int deviceMode)
-Future~List~ getLAInitialStates()
-int~?~ calculateDigitalChannel(String channel)
-Future~double~ fetchLAChannelFrequency(int channelNumber, List data)
}
class MultimeterStateProvider {
-ScienceLab _scienceLab
-bool isSwitchChecked
-int _selectedIndex
-List knobMarker
-String value
-String unit
-AppLocalizations appLocalizations
+Future~void~ requestFrequencyMeasurement()
}
class AppLocalizations {
+String unitHz
}
MultimeterStateProvider --> ScienceLab : uses
MultimeterStateProvider --> AppLocalizations : uses
note for ScienceLab "Now returns -1 on errors in getFrequency instead of throwing due to null values or invalid channel."
note for MultimeterStateProvider "Interprets frequency < 0 as error and sets value to Cannot measure! with empty unit."
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Using
-1as a magic error value forgetFrequencymakes the API harder to reason about; consider returningnull, aResult/wrapper type, or throwing and handling a specific exception instead of overloading a numeric return value. - The user-facing string "Cannot measure!" should be sourced from your localization resources instead of being hard-coded so it can be translated consistently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `-1` as a magic error value for `getFrequency` makes the API harder to reason about; consider returning `null`, a `Result`/wrapper type, or throwing and handling a specific exception instead of overloading a numeric return value.
- The user-facing string "Cannot measure!" should be sourced from your localization resources instead of being hard-coded so it can be translated consistently.
## Individual Comments
### Comment 1
<location> `lib/communication/science_lab.dart:612-621` </location>
<code_context>
await Future.delayed(const Duration(milliseconds: 250));
} catch (e) {
logger.e("Error in getFrequency: $e");
+ return -1;
+ }
+
+ // Null check to prevent crash when getLAInitialStates fails
+ if (data == null) {
+ logger.e("Error in getFrequency: getLAInitialStates returned null");
+ return -1;
}
- return await fetchLAChannelFrequency(
- calculateDigitalChannel(channel)!, data!);
+
+ int? channelNumber = calculateDigitalChannel(channel);
+ if (channelNumber == null) {
+ logger.e("Error in getFrequency: invalid channel $channel");
+ return -1;
+ }
+
+ return await fetchLAChannelFrequency(channelNumber, data);
}
</code_context>
<issue_to_address>
**suggestion:** Consider avoiding a magic sentinel value (-1) to indicate frequency read errors.
Using `-1` as an error marker tightly couples the protocol to a magic value and relies on all callers remembering to treat `< 0` as an error. Prefer making the error path explicit in the type (e.g. `Future<double?>` or a small result type with `value` and `error`). If changing the signature is too disruptive, at least extract the sentinel into a named constant (e.g. `const double frequencyError = -1;`) to reduce the risk of misuse.
Suggested implementation:
```
} catch (e) {
logger.e("Error in getFrequency: $e");
return frequencyError;
}
```
```
if (data == null) {
logger.e("Error in getFrequency: getLAInitialStates returned null");
return frequencyError;
}
```
```
int? channelNumber = calculateDigitalChannel(channel);
if (channelNumber == null) {
logger.e("Error in getFrequency: invalid channel $channel");
return frequencyError;
}
```
1. Define a shared sentinel constant in this file, for example near the top-level declarations (or close to the `getFrequency` function), to avoid repeating the magic value:
```dart
const double frequencyError = -1;
```
2. If `getFrequency` returns `int` rather than `double`, adjust the type accordingly:
```dart
const int frequencyError = -1;
```
3. Replace any other occurrences of `return -1;` from `getFrequency` (or related frequency-reading functions) with `return frequencyError;` to keep error handling consistent.
</issue_to_address>
### Comment 2
<location> `lib/providers/multimeter_state_provider.dart:249-250` </location>
<code_context>
- value = frequency.toStringAsFixed(2);
- unit = appLocalizations.unitHz;
+ if (frequency < 0) {
+ value = "Cannot measure!";
+ unit = "";
+ } else {
+ value = frequency.toStringAsFixed(2);
</code_context>
<issue_to_address>
**issue:** Use localized strings instead of a hardcoded error message for the UI.
This widget otherwise uses `appLocalizations`, but this error text is a hardcoded English string. Please add it to your localization resources (e.g. `appLocalizations.multimeterCannotMeasure`) and reference that here to keep the UI consistent and translatable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
moksha-hub
force-pushed
the
fix/2955-la-pin-crash
branch
from
1a0251a to
b32d204
Compare
Contributor
Build StatusBuild successful. APKs to test: https://github.com/fossasia/pslab-app/actions/runs/22219339477/artifacts/5587422776. 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)
Author
|
@h4ck3rm1k3 @fcartegnie @yarons @maxinbjohn Please have a review for the PR. Thank you. |
marcnause
changed the title
Author
|
@h4ck3rm1k3 @fcartegnie @yarons @maxinbjohn please have a review on it. |
marcnause
force-pushed
the
fix/2955-la-pin-crash
branch
from
81b4942 to
6c5b8c5
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/communication/science_lab.dart
Outdated
| return frequencyError; | ||
| } | ||
|
|
||
| return await fetchLAChannelFrequency(channelNumber, data); |
There was a problem hiding this comment.
fetchLAChannelFrequency() can still throw (e.g., via null-assertions inside it when LA buffer fetch fails or expected keys are missing). Since getFrequency is now using a sentinel error value (frequencyError), it should also wrap the fetchLAChannelFrequency call in a try/catch (or otherwise guarantee it can’t throw) and return frequencyError on failure.
Suggested change
| return await fetchLAChannelFrequency(channelNumber, data); | |
| try { | |
| return await fetchLAChannelFrequency(channelNumber, data); | |
| } catch (e) { | |
| logger.e("Error in getFrequency while fetching LA channel frequency: $e"); | |
| return frequencyError; | |
| } |
Contributor
|
@moksha-hub, could you please have a look at the comments by Copilot (#3053 (review))? I think they make sense. |
fetchLAChannelFrequency can throw exceptions (e.g., via null-assertions when LA buffer fetch fails or expected keys are missing). Wrap it in a try/catch so any failure returns frequencyError instead of propagating the exception and crashing the multimeter. Addresses Copilot review comment on PR fossasia#3053.
Author
|
@marcnause please have a review. |
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
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 #2955
Changes
Screenshots / Recordings
Checklist:
constants.dartor localization files instead of hard-coded values.dart formator the IDE formatter.flutter analyzeand tests run influtter test.Summary by Sourcery
Handle invalid or null logic analyzer data when measuring frequency to prevent multimeter crashes and report unmeasurable states to the UI.
Bug Fixes: