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

Voltage colors #3412

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Voltage colors #3412

wants to merge 17 commits into from

Conversation

Muxite
Copy link

@Muxite Muxite commented Nov 30, 2024

Description

Color of battery indicator in thunderscope diagnostics has been changed to a gradient from red to yellow to green. Red is at the minimum voltage and below, yellow at the center, and green at max voltage or above. Fixed the self.percentage variable, as the method was using int, causing the number to be 0 or 1 instead of a float.

Testing Done

Testing was done by connecting to a robot in the mezz, and changing out the batteries. Continued testing by plugging in different percentage values to cause color changes.

Resolved Issues

#3395

Length Justification and Key Files to Review

common_widgets.py

Review Checklist

  • [ x] Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
  • Remove all commented out code
  • Remove extra print statements: for example, those just used for testing
  • [x ] Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

@potatoisagender potatoisagender self-requested a review December 1, 2024 07:51
potatoisagender
potatoisagender previously approved these changes Dec 1, 2024
Copy link
Contributor

@potatoisagender potatoisagender left a comment

Choose a reason for hiding this comment

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

If we really wanted to, we could move the entire selector as a helper function to be used everywhere. I used it in #3413, which means duplicated code that could be unhealthy.

@@ -147,23 +148,34 @@ def setValue(self, value: float) -> None:
"""
super(ColorProgressBar, self).setValue(int(value * self.decimals))

# clamp percent to make sure it's between 0% and 100%
percent = self.getPercentage()
def sigmoid_interpolate(val, x1, y1, x2, y2):
Copy link
Contributor

Choose a reason for hiding this comment

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

you could use this function from C++ for this:

double sigmoid(const double& v, const double& offset, const double& sig_width);

you would need to pybind it via python_bindings.cpp https://github.com/UBC-Thunderbots/Software/blob/81ec0830e11d0a53002aa517a2cc190de75a9721/src/software/python_bindings.cpp

The advantage of doing this is that the C++ one is unit tested while this one isn't (also less code duplication)

Copy link
Contributor

@itsarune itsarune left a comment

Choose a reason for hiding this comment

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

awesome work, just got a nit for you

Comment on lines 165 to 174
percent = self.getPercentage()
if 0 < percent < 0.5:
r = 200
g = sigmoid_interpolate(percent, 0, 0, 0.5, 200)
elif percent < 0:
r = 200
g = 0
else:
super(ColorProgressBar, self).setStyleSheet(
"QProgressBar::chunk"
"{"
f"background: rgb({255 * 2 * (1 - percent)}, 255, 0)"
"}"
)
r = sigmoid_interpolate(percent, 0.5, 200, 1, 0)
g = 200
Copy link
Contributor

@itsarune itsarune Dec 11, 2024

Choose a reason for hiding this comment

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

I suggest moving this helper to util.py because it seems like #3413 would benefit from it

Copy link
Contributor

Choose a reason for hiding this comment

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

Muk, I think I can handle the implementation for this. I'll get to it either tonight or tomorrow, and it should also use the cpp sigmoid interpolate, so I'll be killing two birds with one stone. I'll shoot you a text when I finish the implementation so you can use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok if my pr gets merged you can use the function called color_from_gradient to achieve the same thing.

The way it works is you define threshold values for each gradient stop point, and then define the rgb values for each one of those stop points. If you want to make a hard change like in my pr then you list the stop point twice. See my pr.

Copy link
Author

Choose a reason for hiding this comment

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

ok excellent

@itsarune itsarune mentioned this pull request Dec 11, 2024
4 tasks
@potatoisagender
Copy link
Contributor

You need to change the dependencies in the BUILD file to include util.py, so that your CL tests pass.

Copy link
Contributor

@potatoisagender potatoisagender left a comment

Choose a reason for hiding this comment

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

Looks good.

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.

4 participants