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: fixed errors in Power Source values #2559

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

Conversation

AsCress
Copy link
Contributor

@AsCress AsCress commented Oct 31, 2024

Fixes #2549

Changes

  • Errors in setting values using the Power Source were due to duplicate commands being sent to the PSLab for each value change, as the controllers were synced.
  • This issue is now resolved: a single command is sent to the PSLab for each user-set value.

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.

@marcnause Could you please check and confirm if the values being set now are accurate ?

Summary by Sourcery

Bug Fixes:

  • Resolve issue with duplicate commands being sent to the PSLab for each value change by ensuring a single command is sent for each user-set value.

Copy link

sourcery-ai bot commented Oct 31, 2024

Reviewer's Guide by Sourcery

The changes fix duplicate command issues in the Power Source functionality by restructuring when power values are updated and sent to the PSLab device. The implementation separates display updates from power setting commands and moves the power setting logic to occur only when user interaction is complete.

Sequence diagram for Power Source value update process

sequenceDiagram
    actor User
    participant App
    participant PSLab

    User->>App: Change Power Source value
    App->>App: Update display with new value
    App->>PSLab: Send command to set power value
    PSLab-->>App: Acknowledge command
    App-->>User: Display updated value
Loading

Updated class diagram for PowerSourceActivity

classDiagram
    class PowerSourceActivity {
        - TextView displayPV1
        - TextView displayPV2
        - TextView displayPV3
        - TextView displayPCS
        - Croller controllerPV1
        - Croller controllerPV2
        - Croller controllerPV3
        - Croller controllerPCS
        - float voltagePV1
        - float voltagePV2
        - float voltagePV3
        - float currentPCS
        + updatePower(TextView display, float value, Pin pin)
        + updateDisplay(TextView display, float value, Pin pin)
        + setMappedPower(Pin pin, int progress)
    }
    note for PowerSourceActivity "updatePower method now handles both display and power setting"
Loading

File-Level Changes

Change Details Files
Refactored power value update mechanism to prevent duplicate commands
  • Split updateDisplay method into updateDisplay and updatePower to separate display updates from power setting commands
  • Moved power setting logic from onProgressChanged to onStopTrackingTouch to trigger only when user finishes adjusting
  • Added display updates for paired channels when values change
app/src/main/java/io/pslab/activity/PowerSourceActivity.java
Updated controller monitoring implementation
  • Removed immediate power updates during progress changes
  • Consolidated power setting to occur only on touch release
  • Updated increment/decrement methods to use new updatePower method
app/src/main/java/io/pslab/activity/PowerSourceActivity.java

Assessment against linked issues

Issue Objective Addressed Explanation
#2549 Reduce the voltage/current offset error when setting values in the Power Supply screen to be comparable with Python library accuracy

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

@AsCress AsCress added Instrument: Power Source Fix Solution to an existing issue in app labels Oct 31, 2024
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 @AsCress - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 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 and I'll use the feedback to improve your reviews.

Copy link

github-actions bot commented Oct 31, 2024

@marcnause
Copy link
Contributor

Works much better now!

I noticed that the value in the TextView is only updated when the voltage is changed via the buttons or when the user stops touching the controller (Croller). Is that intentional?

@AsCress
Copy link
Contributor Author

AsCress commented Nov 4, 2024

@marcnause Yes that has been kept intentionally by me. What are your views on this?

@marcnause
Copy link
Contributor

I find it a tad confusing if the TextViews are not updated when the controller is changed. I guess the reason for the change was to avoid sending too many commands, right?

@AsCress
Copy link
Contributor Author

AsCress commented Nov 5, 2024

@marcnause I think yes you are right. Probably, I should think about another fix for this issue rather than updating the TextViews when the user stops touching the Croller.
It is indeed confusing. I'll push to this branch soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Solution to an existing issue in app Instrument: Power Source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Power Supply: Set values have a relatively high offset
3 participants