Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor range select in oscilloscope #2552

Merged
merged 4 commits into from
Nov 4, 2024

Conversation

marcnause
Copy link
Contributor

@marcnause marcnause commented Sep 22, 2024

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.
  • No end of file edits: No modifications done at end of resource files strings.xml, dimens.xml or colors.xml.
  • Code reformatting: I have reformatted code and fixed indentation in every file included in this pull request.
  • 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.

Copy link

github-actions bot commented Sep 22, 2024

@marcnause
Copy link
Contributor Author

marcnause commented Sep 22, 2024

@AsCress: In #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 marcnause marked this pull request as ready for review September 22, 2024 21:50
Copy link

sourcery-ai bot commented Sep 22, 2024

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.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @marcnause - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@marcnause
Copy link
Contributor Author

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

@marcnause marcnause changed the title refactor range select in oscilloscope WIP: refactor range select in oscilloscope Sep 30, 2024
@marcnause marcnause changed the title WIP: refactor range select in oscilloscope refactor range select in oscilloscope Sep 30, 2024
@marcnause
Copy link
Contributor Author

Fourier Transform works again!

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

@AsCress
Copy link
Contributor

AsCress commented Oct 1, 2024

@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 :))

@marcnause
Copy link
Contributor Author

marcnause commented Oct 1, 2024

@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.

@AsCress
Copy link
Contributor

AsCress commented Oct 2, 2024

@marcnause Yes that's correct :))

@marcnause
Copy link
Contributor Author

marcnause commented Oct 14, 2024

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

@AsCress
Copy link
Contributor

AsCress commented Oct 31, 2024

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

@marcnause marcnause force-pushed the refactorScale branch 2 times, most recently from 5bdc858 to 4dcb2b5 Compare November 3, 2024 21:40
@marcnause
Copy link
Contributor Author

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

@AsCress
Copy link
Contributor

AsCress commented Nov 4, 2024

@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.

@marcnause
Copy link
Contributor Author

I have created a new issue (#2567) and will merge the code in this PR.

@marcnause marcnause merged commit 4b59ea5 into fossasia:development Nov 4, 2024
1 of 2 checks passed
@marcnause marcnause deleted the refactorScale branch November 4, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Oscilloscope: Range does not change, when option is chosen
3 participants