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

fix: Crash in XY plot of Oscilloscope screen #2580 #2582

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

Surajkumar5050
Copy link

@Surajkumar5050 Surajkumar5050 commented Dec 9, 2024

Fixed issue - #2580

Fixes #2580

Changes

  • Changes in fetchdata() method in ScienceLab.java class
  • An additional check was added to ensure that the second byte of data is within bounds. This prevents errors when the list has an odd number of elements.
  • An error message is logged if the index for accessing the second byte is out of bounds, improving the debugging process.
  • The buffer assignment now occurs only if the data index is valid, preventing issues such as accessing invalid memory or creating incorrect buffer values.
  • These updates make the code more robust, particularly when dealing with cases where the number of elements in the data set is odd, preventing crashes or incorrect data handling.

Screenshots / Recordings

N/A

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

Fix index out of bounds error in the fetchData method and improve code readability by adding braces to control structures.

Bug Fixes:

  • Fix index out of bounds error in the fetchData method by adding a check before accessing listData.

Enhancements:

  • Improve code readability and maintainability in the fetchData method by adding braces to control structures.

Copy link

sourcery-ai bot commented Dec 9, 2024

Reviewer's Guide by Sourcery

The changes focus on improving the data handling and error checking in the oscilloscope's fetchData method. The implementation adds bounds checking when processing buffer data and improves code formatting for better readability.

Class diagram for ScienceLab class changes

classDiagram
    class ScienceLab {
        - int[] buffer
        - List<Channel> aChannels
        - int channelsInBuffer
        - int dataSplitting
        + boolean fetchData(int channelNumber)
    }
    class Channel {
        - int length
        - int bufferIndex
        + int[] yAxis
        + int[] fixValue(int[] data)
    }
    ScienceLab --> Channel
    note for ScienceLab "Improved data handling and error checking in fetchData method"
Loading

File-Level Changes

Change Details Files
Added array bounds checking in buffer data processing
  • Added conditional check before accessing array indices
  • Added error logging for index out of bounds scenarios
  • Maintained original functionality while adding safety checks
app/src/main/java/io/pslab/communication/ScienceLab.java
Improved code formatting and structure
  • Fixed indentation in the fetchData method
  • Added consistent bracing style for loop and conditional blocks
  • Reorganized code blocks for better readability
app/src/main/java/io/pslab/communication/ScienceLab.java

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

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 @Surajkumar5050 - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please fill out the PR description with details about what issue Crash in XY plot of Oscilloscope screen #2580 is and what specific changes were made to address it. This helps reviewers understand the context and purpose of the changes.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 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 and I'll use the feedback to improve your reviews.

Copy link

github-actions bot commented Dec 9, 2024

@Surajkumar5050 Surajkumar5050 changed the title #2580 Fix: Crash in XY plot of Oscilloscope screen #2580 Dec 9, 2024
@Surajkumar5050 Surajkumar5050 changed the title Fix: Crash in XY plot of Oscilloscope screen #2580 fix: Crash in XY plot of Oscilloscope screen #2580 Dec 9, 2024
@AsCress
Copy link
Contributor

AsCress commented Dec 10, 2024

@Surajkumar5050 Thank you very much for your work on this issue!
Just had a look at the issue and your pull request. This issue isn't related to the amount of data that we fetch from the device but rather the index of the channel that we fetch.
Some general points about this:

  1. This issue isn't related to the In-Built MIC. The app crashes irrespective of the state of the In-Built MIC.
  2. We have a HashMap channelIndexMap, where we store the indices of the respective channels.
  3. This issue arises due to a very small bug while accessing the channels from that map. We already increment the channel number by one in the map, i.e., CH1 is 1, CH2 is 2 and so on. However, in line 1309 and 1311 of OscilloscopeActivity.java(exactly where the log points to), we are incrementing the channel number once again, which leads to an index overflow. We remove those +1s and we are good to go.

I'll add all the necessary changes in a review to make it easy for you.

Copy link
Contributor

@AsCress AsCress left a comment

Choose a reason for hiding this comment

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

That's everything for ScienceLab.java, we don't need any changes in here.
In order to fix the issue, you can make the required changes in OscilloscopeActivity.java, as mentioned above.

@@ -446,52 +446,58 @@ public int[] oscilloscopeProgress() {
}

private boolean fetchData(int channelNumber) {
int samples = this.aChannels.get(channelNumber - 1).length;
Copy link
Contributor

Choose a reason for hiding this comment

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

Some formatting issues there.
It'd be great if you could format your files after you finish work and before you commit them.
Just select the whole file (Ctrl+A) and then run reformat (Ctrl+Alt+L).


if ((samples % this.dataSplitting) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for here. Formatting issues.

}

for (int i = 0; i < listData.size() / 2; i++) {
if (i * 2 + 1 < listData.size()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this. We already have checks in place if the amount of data required isn't read properly from the PSLab device.

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.

Crash in XY plot of Oscilloscope screen
2 participants