pslab-android icon indicating copy to clipboard operation
pslab-android copied to clipboard

refactor range select in oscilloscope

Open marcnause opened this issue 1 year ago • 9 comments

Fixes #2099

Changes

  • refactoring of scale handling: moved scale values to extra class
  • only set scale when required, not on every new data frame

Checklist:

  • [ ] No hard coding: I have used resources from strings.xml, dimens.xml and colors.xml without hard coding any value.
  • [x] No end of file edits: No modifications done at end of resource files strings.xml, dimens.xml or colors.xml.
  • [x] Code reformatting: I have reformatted code and fixed indentation in every file included in this pull request.
  • [x] No extra space: My code does not contain any extra lines or extra spaces than the ones that are necessary.

Summary by Sourcery

Refactor the oscilloscope's range selection by encapsulating scale values in a new class and optimizing scale updates to occur only when necessary. This improves code structure and performance.

Enhancements:

  • Refactor the oscilloscope's scale handling by moving scale values to a dedicated class, improving code organization and maintainability.
  • Introduce a mechanism to set axis scales only when necessary, reducing unnecessary updates and improving performance.
  • Replace anonymous inner classes with lambda expressions for cleaner and more concise code.

marcnause avatar Sep 22 '24 21:09 marcnause

Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/11670470293/artifacts/2142682113

github-actions[bot] avatar Sep 22 '24 21:09 github-actions[bot]

@AsCress: In https://github.com/fossasia/pslab-android/pull/2551 you mentioned, that the auto-scale-feature may have problems too. I think it only changes the scale for the left Y-axis. Is that correct?

I have added two comments which contain TODO and your name. I am not sure if what I have done there is correct and it would be great if you could take an extra close look there.

Static code analysis fails due to high complexity in one of the Fragments.

marcnause avatar Sep 22 '24 21:09 marcnause

Reviewer's Guide by Sourcery

This pull request refactors the range select functionality in the oscilloscope feature. The main changes involve introducing a new OscilloscopeAxisScale class to encapsulate axis scale values and their management, and updating related classes to use this new abstraction. This refactoring aims to improve code organization, reduce duplication, and enhance maintainability.

File-Level Changes

Change Details Files
Introduction of OscilloscopeAxisScale class
  • Created new class to encapsulate axis scale values
  • Implemented methods for getting and setting axis scales
  • Added listener interface for scale change notifications
app/src/main/java/io/pslab/others/OscilloscopeAxisScale.java
Refactoring OscilloscopeActivity to use OscilloscopeAxisScale
  • Removed direct management of axis scales
  • Added OscilloscopeAxisScale instance
  • Updated methods to use OscilloscopeAxisScale for scale operations
  • Implemented AxisScaleSetListener interface
app/src/main/java/io/pslab/activity/OscilloscopeActivity.java
Updating fragments to use OscilloscopeAxisScale
  • Modified ChannelParametersFragment to use OscilloscopeAxisScale
  • Updated DataAnalysisFragment to use OscilloscopeAxisScale
  • Refactored TimebaseTriggerFragment to use OscilloscopeAxisScale
  • Changed XYPlotFragment to use OscilloscopeAxisScale
app/src/main/java/io/pslab/fragment/ChannelParametersFragment.java
app/src/main/java/io/pslab/fragment/DataAnalysisFragment.java
app/src/main/java/io/pslab/fragment/TimebaseTriggerFragment.java
app/src/main/java/io/pslab/fragment/XYPlotFragment.java
Code cleanup and modernization
  • Replaced anonymous inner classes with lambda expressions
  • Removed unused imports
  • Simplified some conditional statements
app/src/main/java/io/pslab/activity/OscilloscopeActivity.java
app/src/main/java/io/pslab/fragment/ChannelParametersFragment.java
app/src/main/java/io/pslab/fragment/DataAnalysisFragment.java
app/src/main/java/io/pslab/fragment/TimebaseTriggerFragment.java
app/src/main/java/io/pslab/fragment/XYPlotFragment.java

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

sourcery-ai[bot] avatar Sep 22 '24 21:09 sourcery-ai[bot]

This PR breaks Fourier Transform in Data Analysis screen of the Oscilloscope. Therefore I will change it to WIP.

marcnause avatar Sep 30 '24 19:09 marcnause

Fourier Transform works again!

@AsCress, could you please check if the oscilloscope still works as expected for you?

marcnause avatar Sep 30 '24 20:09 marcnause

@marcnause This is a fantastic PR ! The idea to clean the code by refactoring it into a different file is an excellent one ! I've fixed some general issues regarding the mV (milli volt) scale and removed the TODOs after reviewing them. We're good to go with this PR. This only issue left to tackle is the rightYAxisScale; changing it has no affect on the graph. We'd have to fix this by plotting the channel 2 output according to the Right Y Axis, however, that I think should be done in a different PR :))

AsCress avatar Oct 01 '24 15:10 AsCress

@AsCress thank you for your review! Unfortunately, you don't seem to have write access yet and I am not allowed to review your changes since I started the PR. I guess we need either @CloudyPadmal or @mariobehling to approve. 👀

Note: Static code analysis fails due to high complexity in a method I touched, but complexity has been high before that already.

marcnause avatar Oct 01 '24 17:10 marcnause

@marcnause Yes that's correct :))

AsCress avatar Oct 02 '24 16:10 AsCress

I noticed that the automatic measurements show strange values when the trigger is moved to a value out of the range of the current signal. (As discussed in the call today.)

Bildschirmfoto vom 2024-10-14 17-43-19

marcnause avatar Oct 14 '24 15:10 marcnause

@marcnause Can you confirm if this issue still exists? Strangely, I wasn't able to reproduce this on my device today 😕.

AsCress avatar Oct 31 '24 16:10 AsCress

The issue still exists, I was able to reproduce it with the APK from this PR and 2 PSLab boards.

marcnause avatar Nov 03 '24 22:11 marcnause

@marcnause I see. This issue seems to be more device-specific as I can reproduce this on some devices that I have and not on others. Also, this issue doesn't arise due to this PR, but due to something else. In any case, we can solve it here and I'll push to this PR very soon solving this issue.

AsCress avatar Nov 04 '24 06:11 AsCress

I have created a new issue (https://github.com/fossasia/pslab-android/issues/2567) and will merge the code in this PR.

marcnause avatar Nov 04 '24 18:11 marcnause