feat: added Power Source screen #2792
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 GuideThis PR implements a new Power Source feature end-to-end: it extends the communication layer with DAC channel support and new commands, adds a provider to manage pin states and conversions, integrates routing and localization, and delivers a fully interactive UI screen with custom knob widgets for PV1/PV2/PV3/PCS controls. Sequence diagram for setting a power source value from the UIsequenceDiagram
actor User
participant PowerSourceScreen
participant PowerSourceStateProvider
participant ScienceLab
participant PacketHandler
participant Hardware
User->>PowerSourceScreen: Adjusts knob or enters value
PowerSourceScreen->>PowerSourceStateProvider: setPV1/setPV2/setPV3/setPCS(value)
PowerSourceStateProvider->>ScienceLab: setPV1/setPV2/setPV3/setPCS(value)
ScienceLab->>PacketHandler: sendByte/sendInt (DAC command)
PacketHandler->>Hardware: Send command
Hardware-->>PacketHandler: Acknowledgement
PacketHandler-->>ScienceLab: Ack
ScienceLab-->>PowerSourceStateProvider: Complete
PowerSourceStateProvider-->>PowerSourceScreen: Notify listeners
PowerSourceScreen-->>User: UI updates value
Class diagram for Power Source state and communicationclassDiagram
class PowerSourceStateProvider {
- double voltagePV1
- double voltagePV2
- double voltagePV3
- double currentPCS
- List<double> rangePV1
- List<double> rangePV2
- List<double> rangePV3
- List<double> rangePCS
- double step
- ScienceLab _scienceLab
+ setPV1(double)
+ setPV2(double)
+ setPV3(double)
+ setPCS(double)
+ setValue(double, Pin)
+ getValue(Pin)
+ valueToIndex(double, Pin)
+ indexToValue(double, Pin)
}
class ScienceLab {
+ Map<String, DACChannel> dacChannels
+ Future<void> setVoltage(String, double)
+ Future<void> setCurrent(double)
+ Future<void> setPV1(double)
+ Future<void> setPV2(double)
+ Future<void> setPV3(double)
+ Future<void> setPCS(double)
}
class DACChannel {
+ String name
+ int channum
+ int offset
+ List<double> range
+ double slope
+ double intercept
+ Polynomial vToCode
+ Polynomial codeToV
+ int channelCode
+ String calibrationEnabled
+ DACChannel(name, span, channum, channelCode)
}
PowerSourceStateProvider --> ScienceLab
ScienceLab --> DACChannel
File-Level Changes
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/16448905220/artifacts/3588978508 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @AsCress - I've reviewed your changes - here's some feedback:
- Consider refactoring the nearly identical Card+Row UI blocks for PV1/PV2/PV3/PCS into a reusable widget to reduce duplication and simplify future updates.
- Avoid instantiating a new TextEditingController inside build—either lift the controller to stateful scope or use TextFormField's initialValue to prevent unnecessary rebuilds and state resets.
- Replace the magic numeric case values (e.g. case 5 for power source navigation) with named constants or an enum to make the routing logic clearer and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider refactoring the nearly identical Card+Row UI blocks for PV1/PV2/PV3/PCS into a reusable widget to reduce duplication and simplify future updates.
- Avoid instantiating a new TextEditingController inside build—either lift the controller to stateful scope or use TextFormField's initialValue to prevent unnecessary rebuilds and state resets.
- Replace the magic numeric case values (e.g. case 5 for power source navigation) with named constants or an enum to make the routing logic clearer and less error-prone.
## Individual Comments
### Comment 1
<location> `lib/view/power_source_screen.dart:57` </location>
<code_context>
+ ),
+ textAlign: TextAlign.center,
+ onSubmitted: (value) async {
+ String powerValue =
+ value.replaceAll("V", "").trim();
+ double parsedValue =
+ double.tryParse(powerValue) ?? 0.0;
+ await provider.setPV1(parsedValue);
</code_context>
<issue_to_address>
Parsing logic for user input does not handle non-numeric or malformed input robustly.
If parsing fails, 0.0 is set, which may be unintended. Add input validation or user feedback to avoid this.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
onSubmitted: (value) async {
String powerValue =
value.replaceAll("V", "").trim();
double parsedValue =
double.tryParse(powerValue) ?? 0.0;
await provider.setPV1(parsedValue);
},
=======
onSubmitted: (value) async {
String powerValue =
value.replaceAll("V", "").trim();
double? parsedValue =
double.tryParse(powerValue);
if (parsedValue == null) {
ScaffoldMessenger.of(context).showSnackBar(
SnackBar(
content: Text('Please enter a valid number for voltage.'),
backgroundColor: Colors.red,
),
);
return;
}
await provider.setPV1(parsedValue);
},
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `lib/providers/power_source_state_provider.dart:96` </location>
<code_context>
+ Future<void> setPV1(double value) async {
+ final clampedValue = value.clamp(rangePV1[0], rangePV1[1]);
+ voltagePV1 = clampedValue;
+ voltagePV3 = (3.3 / 2) * (1 + (voltagePV1 / 5.0));
+ await _scienceLab.setPV1(voltagePV1);
+ notifyListeners();
</code_context>
<issue_to_address>
Setting voltagePV3 in setPV1 may cause unexpected side effects or circular updates.
Since setPV1 updates voltagePV3 and setPV3 updates voltagePV1, this may result in inconsistent state or circular updates. Please clarify the relationship and prevent potential conflicts.
</issue_to_address>
### Comment 3
<location> `lib/providers/power_source_state_provider.dart:104` </location>
<code_context>
+ Future<void> setPV2(double value) async {
+ final clampedValue = value.clamp(rangePV2[0], rangePV2[1]);
+ voltagePV2 = clampedValue;
+ currentPCS = (3.3 - voltagePV2) / 2;
+ await _scienceLab.setPV2(voltagePV2);
+ notifyListeners();
</code_context>
<issue_to_address>
The formula for currentPCS in setPV2 may not match the expected physical relationship.
Please verify that the formula accurately reflects the hardware's behavior and maintains unit consistency. Also, ensure currentPCS remains within valid limits, particularly when voltagePV2 is at its extremes.
Suggested implementation:
```
Future<void> setPV2(double value) async {
final clampedValue = value.clamp(rangePV2[0], rangePV2[1]);
voltagePV2 = clampedValue;
// The formula below is based on the hardware specification:
// currentPCS = (3.3 - voltagePV2) / 2
// Please update this formula if the hardware relationship changes.
double calculatedCurrent = (3.3 - voltagePV2) / 2;
// Clamp currentPCS to its valid range to ensure physical plausibility
currentPCS = calculatedCurrent.clamp(rangeCurrentPCS[0], rangeCurrentPCS[1]);
await _scienceLab.setPV2(voltagePV2);
notifyListeners();
}
```
- If `rangeCurrentPCS` is not already defined in your provider, you should define it as a `List<double>` or similar, representing the valid min/max for `currentPCS`.
- If the formula for `currentPCS` is different according to your hardware documentation, update it accordingly in the comment and code.
</issue_to_address>
### Comment 4
<location> `lib/communication/science_lab.dart:1057` </location>
<code_context>
+
+ Future<void> setCurrent(double current) async {
+ DACChannel dacChannel = dacChannels['PCS']!;
+ int v = (3300 - dacChannel.vToCode(current)).toInt();
+ try {
+ mPacketHandler.sendByte(mCommandsProto.dac);
</code_context>
<issue_to_address>
The calculation for 'v' in setCurrent may not be robust for all input ranges.
If dacChannel.vToCode(current) exceeds 3300, 'v' becomes negative, which may be invalid for the hardware. Please clamp or validate 'v' to ensure it remains within a valid range.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
Future<void> setCurrent(double current) async {
DACChannel dacChannel = dacChannels['PCS']!;
+ int v = (3300 - dacChannel.vToCode(current)).toInt();
=======
Future<void> setCurrent(double current) async {
DACChannel dacChannel = dacChannels['PCS']!;
int v = (3300 - dacChannel.vToCode(current)).toInt();
v = v.clamp(0, 3300);
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `lib/communication/peripherals/dac_channel.dart:24` </location>
<code_context>
+ DataType.float, [3300.0 / slope, -3300.0 * intercept / slope]);
+ codeToV = Polynomial.fromCoefficients(
+ DataType.float, [slope / 3300.0, intercept]);
+ calibrationEnabled = "false";
+ offset = 0;
+ }
</code_context>
<issue_to_address>
calibrationEnabled is a String instead of a boolean.
Consider changing calibrationEnabled to a boolean to avoid confusion and potential bugs.
Suggested implementation:
```
late bool calibrationEnabled;
```
```
calibrationEnabled = false;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
AsCress
force-pushed
the
power_source
branch
3 times, most recently
from
7815a48 to
567a899
Compare
CloudyPadmal
approved these changes
Jul 23, 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.
Adds the Power Source screen.
Screenshots/Recordings
Screen_recording_20250720_201843.mp4
Summary by Sourcery
Introduce a new Power Source screen and update navigation to support access to it from the instruments screen.
New Features:
Enhancements:
Summary by Sourcery
Add a new Power Source feature that provides a dedicated screen with interactive controls for adjusting power rails, backed by a new state provider, DACChannel abstraction, and extended ScienceLab communication commands.
New Features:
Enhancements: